[
https://issues.apache.org/jira/browse/YUNIKORN-3404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108175#comment-18108175
]
Manikandan R commented on YUNIKORN-3404:
----------------------------------------
[~tigerquoll] has given comments on
[https://github.com/apache/yunikorn-k8shim/pull/1074
|https://github.com/apache/yunikorn-k8shim/pull/1074]related to this Jira and
the same has to be addressed.
> PlaceholderManager Start and Stop operations have race conditions
> -----------------------------------------------------------------
>
> Key: YUNIKORN-3404
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3404
> Project: Apache YuniKorn
> Issue Type: Bug
> Components: shim - kubernetes
> Reporter: Andy Huang
> Priority: Major
>
> The Kubernetes shim PlaceholderManager lifecycle implementation has several
> race conditions around its running state and stop channel.
> PlaceholderManager.Start() currently checks the running atomic and updates it
> in two separate operations:
> {code:go}
> if mgr.isRunning() {
> return
> }
> mgr.setRunning(true)
> go func() {
> // ...
> }()
> {code}
> Concurrent Start() calls can both observe running=false before either call
> stores true. This may start multiple cleanup goroutines for the same
> PlaceholderManager.
> PlaceholderManager.Stop() has a similar check-then-act pattern:
> {code:go}
> if !mgr.isRunning() {
> return
> }
> mgr.stopChan <- struct{}{}
> {code}
> Concurrent Stop() calls can both observe running=true. Only one send can be
> received before the cleanup goroutine exits, potentially leaving another
> caller blocked indefinitely on the unbuffered stop channel.
> The running state is changed asynchronously by the cleanup goroutine after
> receiving the stop signal. Stop() therefore does not guarantee that the
> manager is fully stopped when it returns, and lifecycle state can become
> inconsistent when Start() and Stop() are invoked concurrently.
> The lifecycle operations should be made atomic and idempotent. Possible
> approaches include CompareAndSwap for state transitions, closing a stop
> channel exactly once, and waiting for the cleanup goroutine to exit.
> This was identified while reviewing the KubernetesShim shutdown fixes in:
> * YUNIKORN-3367
> * [https://github.com/apache/yunikorn-k8shim/pull/1074]
> Relevant code:
> * pkg/cache/placeholder_manager.go
> * PlaceholderManager.Start()
> * PlaceholderManager.Stop()
> Expected behavior
> * At most one PlaceholderManager cleanup goroutine can run.
> * Concurrent Start() calls do not start duplicate goroutines.
> * Stop() is safe and idempotent.
> * Concurrent Stop() calls do not block or panic.
> * When Stop() returns, the manager has either fully stopped or exposes a
> reliable way to wait for shutdown completion.
> * Start() and Stop() do not leave the running state inconsistent.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
