[
https://issues.apache.org/jira/browse/YUNIKORN-3404?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andy Huang updated YUNIKORN-3404:
---------------------------------
Description:
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
* YUNIKORN-3368
* [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.
was:
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
* YUNIKORN-3368
* https://github.com/apache/yunikorn-k8shim/pull/1074
Relevant code:
* pkg/cache/placeholder_manager.go
* PlaceholderManager.Start()
* PlaceholderManager.Stop()
{code}
**Expected behavior**
```text
* 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.
> 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
> * YUNIKORN-3368
> * [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]
