[jira] [Commented] (YUNIKORN-2543) Investigate locking in RMProxy

["Wilfred Spiegelenburg (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YUNIKORN-2543?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17834766#comment-17834766 ] 

Wilfred Spiegelenburg commented on YUNIKORN-2543:
-------------------------------------------------

We have two messages for communication: request and response. The view is from the core. We should only process incoming or outgoing messages for RMs, or shims, we know. The rest we can just drop.

If a request comes in, RM -> core, it should be processed in the core, this adds nodes apps etc.  If a response is needed, core -> RM, we use the callback. This happens when we allocate on a node or trigger a placeholder removal etc. That means we need to find if the RM is registered or get the callback for a RM.  Both use the same check, the RM is in the map. This is true for incoming and outgoing messages.

Neither need to hold any locks as soon as we have established if we know the RM or have the callback. Processing should be unlocked and never hold a lock. Not sure why we do that but that is the issue. We should look at that locking as really simple. Retrieve under lock and then let go. We should never process *ANYTHING* while holding the RM lock. It is totally unnecessary.

A far simpler change that fixes the issue: follow the setup as in UpdateNode, UpdateApplication and UpdateAllocation in the RMProxy. Retrieve the proxy based on its ID, get the callback instance if needed, {{GetResourceManagerCallback}} already returns it. Fail immediately if callback does not exist. That means the RM is not registered so it handles both incoming as outgoing messages. If we get a callback process the event. Either using the callback that was retrieved if you need it or by just calling the core code. Nothing should be passing the rmID around in any calls. We only pass the callback around. The whole RMProxy becomes lock free except for the two calls:
 * {{GetResourceManagerCallback}}
 * {{RegisterResourceManager}}

The get call is short lived and does not include any processing.

Any change in the registration as proposed to introduce a factory in the SI is a backwards incompatible change.  That would require a 2.0 version for YuniKorn. We know that some people have build their own shims. We cannot even backport that kind of change. The simplification of the RMProxy we can backport.

> Investigate locking in RMProxy
> ------------------------------
>
>                 Key: YUNIKORN-2543
>                 URL: https://issues.apache.org/jira/browse/YUNIKORN-2543
>             Project: Apache YuniKorn
>          Issue Type: Sub-task
>          Components: core - scheduler
>            Reporter: Peter Bacsko
>            Assignee: Peter Bacsko
>            Priority: Major
>
> After merging YUNIKORN-2539, we already saw a potential issue with {{rmproxy.RMProxy}} and {{cache.Context}}:
> Gourutine 1:
> {noformat}
> github.com/apache/yunikorn-core@v0.0.0-20240405160823-c94a7d938c41/pkg/rmproxy/rmproxy.go:307 rmproxy.(*RMProxy).GetResourceManagerCallback ??? <<<<<
> github.com/apache/yunikorn-core@v0.0.0-20240405160823-c94a7d938c41/pkg/rmproxy/rmproxy.go:306 rmproxy.(*RMProxy).GetResourceManagerCallback ???
> github.com/apache/yunikorn-core@v0.0.0-20240405160823-c94a7d938c41/pkg/rmproxy/rmproxy.go:359 rmproxy.(*RMProxy).UpdateNode ???
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:1603 cache.(*Context).updateNodeResources ???
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:484 cache.(*Context).updateNodeOccupiedResources ???
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:392 cache.(*Context).updateForeignPod ???
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:286 cache.(*Context).UpdatePod ???
> {noformat}
> Goroutine 2:
> {noformat}
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:847 cache.(*Context).ForgetPod ??? <<<<<
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:846 cache.(*Context).ForgetPod ???
> github.com/apache/yunikorn-k8shim/pkg/cache/scheduler_callback.go:104 cache.(*AsyncRMCallback).UpdateAllocation ???
> github.com/apache/yunikorn-core@v0.0.0-20240405160823-c94a7d938c41/pkg/rmproxy/rmproxy.go:162 rmproxy.(*RMProxy).triggerUpdateAllocation ???
> github.com/apache/yunikorn-core@v0.0.0-20240405160823-c94a7d938c41/pkg/rmproxy/rmproxy.go:150 rmproxy.(*RMProxy).processRMReleaseAllocationEvent ???
> github.com/apache/yunikorn-core@v0.0.0-20240405160823-c94a7d938c41/pkg/rmproxy/rmproxy.go:234 rmproxy.(*RMProxy).handleRMEvents ???
> {noformat}
> Right now this seems to be safe because we only call {{RLock()}} in the {{RMProxy}} methods. However, should any of this change, we're in trouble due to lock ordering (Cache->RMProxy and RMProxy->Cache).
> We need to investigate why we use only {{RLock()}} and whether it's needed at all. If nothing is modified, then we can drop the mutex completely.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org