fix: honor context cancellation in node health/exit wait loops - #3257
Open
mvanhorn wants to merge 5 commits into
Open
fix: honor context cancellation in node health/exit wait loops#3257mvanhorn wants to merge 5 commits into
mvanhorn wants to merge 5 commits into
Conversation
The per-node wait loops in scheduleNodeWorkerF polled node health and container exit status with a bare time.Sleep(time.Second), so a deploy that was interrupted (Ctrl-C only cancels the context) kept spinning when a dependency never reached the awaited state (e.g. a crashed or removed container). The worker goroutine could not unwind, leaving containerlab deploy hanging until SIGQUIT. Replace the bare sleeps in the WaitForHealthy and WaitForExit loops, and the StartupDelay sleep, with a select on ctx.Done() vs time.After so the worker returns promptly once the deploy context is cancelled. Happy-path polling cadence is unchanged. Fixes srl-labs#3162 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQ7G74DcZPFPEgDJMTEcCR
Addresses review feedback: cancelling the deploy context unwinds the polling worker, but a node already blocked in EnterStage waiting on a dependency stage (a bare sync.WaitGroup.Wait) stayed parked, so nodesWg.Wait could still hang after Ctrl-C. Wait on the stage waitgroup in a goroutine and select against ctx.Done so EnterStage returns when the deploy is cancelled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQ7G74DcZPFPEgDJMTEcCR
Addresses review feedback: - Initialize CLab.Config in the worker test so scheduleNodeWorkerF does not panic dereferencing Config.Name before reaching the cancellation path. - After each EnterStage call the worker now checks ctx.Err() and returns if the deploy was cancelled, so a cancelled stage wait no longer falls through into PostDeploy / the health and exit polls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQ7G74DcZPFPEgDJMTEcCR
Addresses review feedback: the scheduler goroutine called EnterStage(ctx, WaitForCreate) and then sent the node on the worker channel unconditionally. After Ctrl-C the workers have already returned, so the send blocked forever and close(concurrentChan) never ran, leaving the deploy hanging. Bail out when ctx is cancelled after the create wait, and select the worker-channel send against ctx.Done so it can never block once the workers have unwound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQ7G74DcZPFPEgDJMTEcCR
Addresses review feedback: after the node workers unwind on Ctrl-C, Deploy treated their completion as success and continued into host endpoint deployment, vxlan stitching, export and inventory generation for a partially deployed lab. Check ctx.Err() right after NodesWg.Wait() and return the cancellation error so a cancelled deploy stops promptly instead of doing post-deploy work on an incomplete topology. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQ7G74DcZPFPEgDJMTEcCR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make the per-node deploy wait loops honor context cancellation so a stuck
containerlab deployunwinds on Ctrl-C instead of hanging until SIGQUIT.Why this matters
When a node container crashes or disappears mid-deploy, a dependent worker in
scheduleNodeWorkerF(core/clab.go) spins ontime.Sleep(time.Second)waiting for a state that never arrives. Ctrl-C only cancels the deploy context, so these bare sleeps and the blocking stage waits never observe the cancellation and the process hangs with open sockets until SIGQUIT (issue #3162; the current workaround is--skip-post-deploy).The fix closes the hang at every layer it could occur:
time.Sleepcalls in theWaitForHealthyandWaitForExitpoll loops, and theStartupDelaysleep, withselect { case <-ctx.Done(): return; case <-time.After(...): }(core/clab.go).DependencyNode.EnterStagewait on its stage waitgroup through aselectonctx.Done()so a node blocked on an unmet dependency returns on cancellation instead of blocking forever (core/dependency_manager/dependency_node.go). This is the single blocking wait path for all stage dependencies, so making it cancellation-aware unwinds dependent workers too.EnterStagecall in the worker, return early whenctx.Err() != nil. InscheduleNodes, movewfcwg.Done()into adefer, bail on cancellation after the create-stage wait, and make theworkerChansend aselectonctx.Done()so the channel always closes and the send cannot block once workers have unwound.Deploy, checkctx.Err()right afterNodesWg.Wait()and return the cancellation error instead of running host endpoint deployment, vxlan stitching, exports, and inventories on a partially deployed lab (core/deploy.go).Happy-path behavior is unchanged: the loops still poll every second and break when a node turns healthy or exits, and post-deploy ordering is untouched.
Testing
Added focused unit tests:
core/clab_test.godrivesscheduleNodeWorkerFinto the health wait loop with a context cancelled during the health check and asserts the worker returns rather than blocking past a short deadline;core/dependency_manager/dependency_node_test.goblocksEnterStageon an unmet dependency, cancels the context, and asserts it returns.gofmtis clean andcore/pluscore/dependency_manager/(and their test binaries) build and vet underGOOS=linux. Fullgo testexecution requires a Linux host because containerlab pulls Linux-only dependencies.Fixes #3162