[
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 race
conditions and shutdown gaps around its running state and stop channel.
PlaceholderManager.Start() currently checks the running atomic and updates it
in 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, starting 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.
There is an additional shutdown problem. The cleanup goroutine alternates
between receiving from stopChan and cleanOrphanPlaceholders(). The latter holds
the manager lock and can issue Delete calls without a deadline. If shutdown
occurs while an orphan sweep is blocked on a slow API server, Stop() blocks on
its unbuffered send until the complete sweep returns.
The running state is changed asynchronously by the cleanup goroutine after
receiving the stop signal. Stop() therefore does not guarantee that the manager
has stopped when it returns, and Start() and Stop() can leave lifecycle state
inconsistent when invoked concurrently.
The lifecycle operations should be atomic, idempotent, and shutdown-safe. A
likely approach is to use CompareAndSwap or sync.Once for lifecycle
transitions, close a stop channel exactly once instead of sending on it, and
provide a reliable way to wait for the cleanup goroutine to exit.
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.
* Shutdown does not wait indefinitely for an orphan cleanup blocked on an API
call.
* 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 was identified while reviewing KubernetesShim shutdown fixes:
* https://issues.apache.org/jira/browse/YUNIKORN-3367
* [https://github.com/apache/yunikorn-k8shim/pull/1074]
was:
The Kubernetes shim PlaceholderManager lifecycle implementation has race
conditions and shutdown gaps around its running state and stop channel.
PlaceholderManager.Start() currently checks the running atomic and updates it
in 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, starting 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.
There is an additional shutdown problem. The cleanup goroutine alternates
between receiving from stopChan and cleanOrphanPlaceholders(). The latter holds
the manager lock and can issue Delete calls without a deadline. If shutdown
occurs while an orphan sweep is blocked on a slow API server, Stop() blocks on
its unbuffered send until the complete sweep returns.
The running state is changed asynchronously by the cleanup goroutine after
receiving the stop signal. Stop() therefore does not guarantee that the manager
has stopped when it returns, and Start() and Stop() can leave lifecycle state
inconsistent when invoked concurrently.
The lifecycle operations should be atomic, idempotent, and shutdown-safe. A
likely approach is to use CompareAndSwap or sync.Once for lifecycle
transitions, close a stop channel exactly once instead of sending on it, and
provide a reliable way to wait for the cleanup goroutine to exit.
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.
* Shutdown does not wait indefinitely for an orphan cleanup blocked on an API
call.
* 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 was identified while reviewing KubernetesShim shutdown fixes:
* https://issues.apache.org/jira/browse/YUNIKORN-3367
* https://issues.apache.org/jira/browse/YUNIKORN-3368
* https://github.com/apache/yunikorn-k8shim/pull/1074
> 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 race
> conditions and shutdown gaps around its running state and stop channel.
> PlaceholderManager.Start() currently checks the running atomic and updates it
> in 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, starting 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.
> There is an additional shutdown problem. The cleanup goroutine alternates
> between receiving from stopChan and cleanOrphanPlaceholders(). The latter
> holds the manager lock and can issue Delete calls without a deadline. If
> shutdown occurs while an orphan sweep is blocked on a slow API server, Stop()
> blocks on its unbuffered send until the complete sweep returns.
> The running state is changed asynchronously by the cleanup goroutine after
> receiving the stop signal. Stop() therefore does not guarantee that the
> manager has stopped when it returns, and Start() and Stop() can leave
> lifecycle state inconsistent when invoked concurrently.
> The lifecycle operations should be atomic, idempotent, and shutdown-safe. A
> likely approach is to use CompareAndSwap or sync.Once for lifecycle
> transitions, close a stop channel exactly once instead of sending on it, and
> provide a reliable way to wait for the cleanup goroutine to exit.
> 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.
> * Shutdown does not wait indefinitely for an orphan cleanup blocked on an
> API call.
> * 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 was identified while reviewing KubernetesShim shutdown fixes:
> * https://issues.apache.org/jira/browse/YUNIKORN-3367
> * [https://github.com/apache/yunikorn-k8shim/pull/1074]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
