[
https://issues.apache.org/jira/browse/YUNIKORN-3405?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andy Huang updated YUNIKORN-3405:
---------------------------------
Description:
The Kubernetes shim dispatcher lifecycle implementation uses separate checks
and state updates that are not atomic across Start() and Stop().
Dispatcher.Start() checks isRunning(), replaces stopChan, updates a WaitGroup,
starts a goroutine, and only then marks the dispatcher as running. Concurrent
Start() calls can both observe running=false and proceed. This can result in:
* multiple dispatcher goroutines being started;
* stopChan being replaced while another goroutine still references it;
* a dispatcher goroutine becoming unreachable by Stop();
* inconsistent WaitGroup accounting;
* leaked goroutines.
Dispatcher.Stop() first checks whether stopChan is closed and later closes it
as a separate operation. Concurrent Stop() calls can both observe an open
channel and attempt to close it, causing a close-of-closed-channel panic.
Start() and Stop() may also race while stopChan and the running state are
updated independently.
There is an additional timeout recovery problem. Stop() waits up to five
seconds for the dispatcher loop to terminate. If that timeout is reached,
running remains true. A subsequent Start() then no-ops because it sees
running=true. When the stale dispatcher loop eventually exits and clears
running, no event-processing loop is started for the remainder of the process.
This primarily affects in-process test reuse, but represents an inconsistent
lifecycle state.
The dispatcher lifecycle should be made thread-safe, idempotent, and explicit.
State transitions involving running, stopChan, and the shutdown WaitGroup must
be coordinated as one operation.
Expected behavior:
* Only one dispatcher event-processing goroutine can run.
* Concurrent Start() calls do not create duplicate or unreachable goroutines.
* Stop() is safe and idempotent.
* Concurrent Stop() calls do not panic.
* Start() and Stop() cannot leave stopChan, the running state, or WaitGroup
accounting inconsistent.
* A timed-out Stop() cannot prevent a later Start() from creating an active
event-processing loop.
* Stop() waits for the active dispatcher goroutine to terminate, subject to the
existing shutdown timeout.
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 dispatcher lifecycle implementation uses separate checks
and state updates that are not atomic across the complete Start() and Stop()
operations.
Dispatcher.Start() currently checks isRunning(), replaces stopChan, updates a
WaitGroup, starts a goroutine, and only then marks the dispatcher as running.
Concurrent Start() calls can both observe running=false and proceed. This can
result in:
* multiple dispatcher goroutines being started
* stopChan being replaced while another goroutine still references it
* a dispatcher goroutine becoming unreachable by Stop()
* inconsistent WaitGroup accounting
* leaked goroutines
Dispatcher.Stop() first checks whether stopChan is closed and later closes it
as a separate operation. Concurrent Stop() calls can both observe an open
channel and attempt to close it, potentially causing a close-of-closed-channel
panic.
Start() and Stop() may also race with each other while stopChan and the running
state are being updated independently.
The dispatcher lifecycle should be made thread-safe, idempotent, and explicit.
All state transitions involving running, stopChan, and the shutdown WaitGroup
should be coordinated as one operation.
This was identified while reviewing the KubernetesShim shutdown fixes in:
* YUNIKORN-3367
* https://github.com/apache/yunikorn-k8shim/pull/1074
Relevant code:
* pkg/dispatcher/dispatcher.go
* dispatcher.Start()
* dispatcher.Stop()
Expected behavior
* Only one dispatcher event-processing goroutine can run.
* Concurrent Start() calls do not create duplicate or unreachable goroutines.
* Stop() is safe and idempotent.
* Concurrent Stop() calls do not panic.
* Start() and Stop() cannot leave stopChan, the running state, or WaitGroup
accounting inconsistent.
* Stop() waits for the active dispatcher goroutine to terminate, subject to the
existing shutdown timeout.
> Dispatcher Start and Stop operations have race conditions
> ---------------------------------------------------------
>
> Key: YUNIKORN-3405
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3405
> Project: Apache YuniKorn
> Issue Type: Bug
> Components: shim - kubernetes
> Reporter: Andy Huang
> Priority: Major
>
> The Kubernetes shim dispatcher lifecycle implementation uses separate checks
> and state updates that are not atomic across Start() and Stop().
> Dispatcher.Start() checks isRunning(), replaces stopChan, updates a
> WaitGroup, starts a goroutine, and only then marks the dispatcher as running.
> Concurrent Start() calls can both observe running=false and proceed. This can
> result in:
> * multiple dispatcher goroutines being started;
> * stopChan being replaced while another goroutine still references it;
> * a dispatcher goroutine becoming unreachable by Stop();
> * inconsistent WaitGroup accounting;
> * leaked goroutines.
> Dispatcher.Stop() first checks whether stopChan is closed and later closes it
> as a separate operation. Concurrent Stop() calls can both observe an open
> channel and attempt to close it, causing a close-of-closed-channel panic.
> Start() and Stop() may also race while stopChan and the running state are
> updated independently.
> There is an additional timeout recovery problem. Stop() waits up to five
> seconds for the dispatcher loop to terminate. If that timeout is reached,
> running remains true. A subsequent Start() then no-ops because it sees
> running=true. When the stale dispatcher loop eventually exits and clears
> running, no event-processing loop is started for the remainder of the
> process. This primarily affects in-process test reuse, but represents an
> inconsistent lifecycle state.
> The dispatcher lifecycle should be made thread-safe, idempotent, and
> explicit. State transitions involving running, stopChan, and the shutdown
> WaitGroup must be coordinated as one operation.
> Expected behavior:
> * Only one dispatcher event-processing goroutine can run.
> * Concurrent Start() calls do not create duplicate or unreachable goroutines.
> * Stop() is safe and idempotent.
> * Concurrent Stop() calls do not panic.
> * Start() and Stop() cannot leave stopChan, the running state, or WaitGroup
> accounting inconsistent.
> * A timed-out Stop() cannot prevent a later Start() from creating an active
> event-processing loop.
> * Stop() waits for the active dispatcher goroutine to terminate, subject to
> the existing shutdown timeout.
> 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]
