fix(executor): bound stop/delete's wait against a wedged podman - #94
Merged
Conversation
…dman Issue #88: DELETE on a GPU-attached pod hung for ~7 minutes -- podman pod stop was retried three times, a podman child parented by spark's own PID became a zombie (exited but never reaped), and an unrelated sudo podman pod ps hung host-wide for the same window. Root cause: two independent gaps stacked. StopPod/RemovePod ran on r.Context() straight through from the DELETE handler, which carries no deadline of its own -- only a client disconnect ends it. And even where a context does have a deadline, exec.CommandContext's cancellation only signals the direct child; if that child forks a subprocess (podman's own conmon/netavark helpers) that inherits the stdout/stderr pipe and is itself stuck on a storage/CDI lock, Wait()/CombinedOutput() keeps reading for EOF that never comes, even after the direct child has already exited to a zombie -- exactly what ps showed on the DGX. This is a documented os/exec gotcha (see Cmd.WaitDelay's own docs); the fix is also documented: set WaitDelay so Wait forcibly closes the pipes after a bound, instead of leaving it unset (the default). runPodmanBounded wraps StopPod/RemovePod's podman invocations with both: a 20s timeout layered on top of the caller's own context (never loosens an earlier deadline, only adds one where none existed), and a 5s WaitDelay so a wedged grandchild can no longer hold Wait() open forever. Verified with a real-subprocess repro (not a mock): a fake podman that backgrounds a long-lived grandchild inheriting its pipe reproduces the exact hang against the old code (confirmed via a manual red run: the unfixed StopPod took 30.1s tracking the grandchild's sleep, unbounded) and is bounded against the fix.
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
DELETE /api/v1/pods/{name}on a GPU-attached pod hung for ~7 minutes in live verification (self-resolved, not yet root-caused) --podman pod stopwas retried three times, apodmanchild parented by spark's own PID became a zombie (exited but never reaped), and an unrelated, unfilteredsudo podman pod pshung host-wide for the same window (fixes #88: "podman pod stop/rm on a GPU-attached pod hangs indefinitely, and the stuck child becomes a zombie under the spark PID"). This boundsStopPod/RemovePod's own wait so a wedged podman invocation can no longer block the calling goroutine (and the HTTP request) indefinitely.Root cause
Two gaps stacked:
internal/api/pods_mutate.go) passesr.Context()straight intoStopPod/RemovePod. That context carries no deadline -- it only ends if the client disconnects. A genuinely wedged podman invocation (blocked inside podman's own storage/CDI locking) has nothing to time it out.exec.CommandContext's cancellation only signals the direct child. If that child forks a subprocess (podman's ownconmon/netavark helpers) that inherits the stdout/stderr pipe and is itself stuck on a lock,Wait()/CombinedOutput()keeps reading for EOF that never arrives -- even after the direct child has already exited to a zombie. That's exactly whatpsshowed on the DGX during the incident:[podman] <defunct>parented by spark's own PID, while theCombinedOutput()call never returned. This is a documentedos/execgotcha; the stdlib's own fix for it isCmd.WaitDelay(Go 1.20+), which forcibly closes the pipes after a bound instead of waiting for EOF forever (the default whenWaitDelayis left unset, as every call site here did).Changes
internal/executor/podman.go: addedrunPodmanBounded, wrapping a podman invocation with (a) a 20s timeout layered on top of the caller's context (never loosens an earlier deadline -- only adds one where none existed) and (b) a 5sWaitDelaysoWait()can't be held open forever by an orphaned grandchild.StopPodandRemovePodnow go through it instead of callingexec.CommandContext(...).CombinedOutput()directly. Scoped to the stop/delete path per the issue's own suggested fix --PodStatus/ContainerStatuses/ListPodsetc. are unchanged.internal/executor/podman_wait_test.go(new): real-subprocess tests (no mocks, no podman/DGX dependency) reproducing the exact mechanism via a fakepodmanscript onPATHthat backgrounds a long-lived grandchild inheriting its pipe, then exits immediately -- the same shape as podman forking a stuck helper.Testing
go build ./...,go vet ./...,staticcheck ./...-- all clean.go test ./... -race -timeout 120s -count=1-- all packages pass, run twice back to back to check for flakiness (the executor package's new tests use real timing; the first draft had a race under parallel-raceload and was fixed by giving the pipe/EOF-only sub-test a generous timeout so scheduling jitter can't kill the test's own fixture prematurely).TestRunPodmanBounded_OrphanedGrandchildHangsWithoutWaitDelay/TestRunPodmanBounded_WaitDelayBoundsTheHang: isolate the pipe/EOF mechanism itself (waitDelay=0blocks for the grandchild's full sleep;waitDelay=300msbounds it).TestStopPod_BoundedDespiteOrphanedGrandchild/TestRemovePod_BoundedDespiteOrphanedGrandchild: exercise the real production methods end-to-end via a fakepodmanonPATH, using the actualpodmanStopTimeout/podmanWaitDelayconstants.podman.goto its pre-fix state and ranStopPodagainst the same wedged fake podman via a throwawaygo run. It took 30.1s, tracking the fake grandchild's sleep exactly, confirming the gap is real in the production code path and that the new tests would have failed against it.Live-verification recipe (for the coordinator -- HIGH RISK, do carefully)
This reproduces issue #88's own trigger conditions on
aitopatom-bfc8. Watch host-widepodman pod psresponsiveness throughout; if it stalls, that's expected per the issue (podman's own lock contention, not something this fix can prevent) -- what changed is that Spark's own DELETE call now can't be held open by it.nvidia.com/gpu: 1,--device nvidia.com/gpu=allvia CDI, imagepython:3.12-slim, commandsleep 300.DELETE /api/v1/pods/{name}and time the response.2 * (podmanStopTimeout + podmanWaitDelay)for stop then rm), not an indefinite hang. In the common case it should just succeed quickly, same as any other DELETE.timeout 10 sudo podman pod psa few times. If podman's own storage lock is genuinely contended during teardown, this may still stall during that window (that's podman's own locking, outside Spark's control) -- but it should not stay stuck for minutes, and Spark's own DELETE call must not still be blocked once this resolves.journalctl -u sparkthat at most one retry sequence ofpodman pod stop/pod rmappears (not the original three), and thatps -eo pid,ppid,stat,etime,cmdshows no lingering zombie[podman] <defunct>parented by spark's PID after the DELETE call returns.Risk & rollback
Low blast radius: the change only affects the stop/delete path's own internal timeout/WaitDelay handling, not podman's command arguments or lifecycle semantics otherwise. Worst case if the bounds are too tight for a legitimately slow GPU teardown: a DELETE that would have eventually succeeded on its own instead returns an error after ~25s per call (~50s total for stop+rm), which is still a large improvement over an unbounded hang and is a normal, retryable failure mode rather than a stuck goroutine. Revert is a straight
git revertof this commit if the live-verification recipe above surfaces a regression.Linked issues
Fixes #88 ("podman pod stop/rm on a GPU-attached pod hangs indefinitely, and the stuck child becomes a zombie under the spark PID") -- tracked as T2.9 in
docs/plan.md's E2 epic.