Pin namespaces at attach time to fix race with container teardown - #181
Open
liayan wants to merge 1 commit into
Open
Pin namespaces at attach time to fix race with container teardown#181liayan wants to merge 1 commit into
liayan wants to merge 1 commit into
Conversation
user_init() checks whether a named container is already running and stores its PID in shm->ns_pid; task_init() later opens /proc/<ns_pid>/ns/* to join those namespaces. Nothing holds a reference to that PID in between, so if the step owning the container exits during that window, task_init() fails: pyxis: ignoring --container-mounts when attaching to a running container error: pyxis: unable to open user namespace file: No such file or directory error: pyxis: couldn't get container namespaces error: spank: required plugin spank_pyxis.so: task_init() failed with rc=-1 Easy to hit when several job steps attach to one named container concurrently, e.g. a setup step creates it and worker steps then attach to it. Only some of the attaching steps fail, since it comes down to timing. Also a PID-reuse hazard: if ns_pid gets recycled before task_init() runs, the open succeeds against the wrong namespaces. 5f3f3a6 originally got the namespace fds right in user_init(), as soon as it knew the PID was alive, for both the create and reuse cases. 41af3a0, three days later, moved container creation into task_init() to get it under the job's cgroup, and took the fd lookup for both paths along with it -- the create path needed that move, the reuse path didn't. Fix: open the namespace fds in user_init(), right after enroot_container_get() confirms the container is running, instead of waiting until task_init(). An open fd on /proc/<pid>/ns/<type> pins the namespace even after the owning process exits, and it's inherited across fork(2) into the task processes, so task_init() just skips the lookup when reuse_ns is already set. Only affects the attach path; container creation is unchanged since ns_pid there is pyxis's own freshly forked process. Fixes: 41af3a0 ("Move container create/start to slurm_spank_task_init") Signed-off-by: Liang Yan <lyan@coreweave.com>
Author
|
Hi @flx42, I’m wondering if you could help review this. We hit this race condition pretty often in multi-node inference runs, and currently have to add a ~3-minute sleep before each job as a workaround, which is quite annoying. Would really appreciate your help taking a look. |
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.
We hit this running large multi-node inference deployments: a setup step
creates a container on each node, several worker processes then attach to
it. Every so often a single worker on an otherwise healthy run would die
during startup, and since these are gang-scheduled distributed workers, that
one failure tore down the whole job — on an 18-node/72-worker run, 71 healthy
workers already registering with the control plane got killed because of one.
Problem
user_init() checks whether a named container is already running and stores
its PID in shm->ns_pid; task_init() later opens /proc/<ns_pid>/ns/* to join
those namespaces. Nothing holds a reference to that PID in between, so if the
step owning the container exits during that window, task_init() fails:
pyxis: ignoring --container-mounts when attaching to a running container error: pyxis: unable to open user namespace file: No such file or directory error: pyxis: couldn't get container namespaces error: spank: required plugin spank_pyxis.so: task_init() failed with rc=-1Easy to hit when several job steps attach to one named container
concurrently, e.g. a setup step creates it and worker steps then attach to
it. Only some of the attaching steps fail, since it comes down to timing.
Also a PID-reuse hazard: if ns_pid gets recycled before task_init() runs, the
open succeeds against the wrong namespaces.
5f3f3a6 originally got the namespace fds right in user_init(), as soon
as it knew the PID was alive, for both the create and reuse cases. 41af3a0
moved container creation into task_init() to get it under
the job's cgroup, and took the fd lookup for both paths along with it — the
create path needed that move, the reuse path didn't.
Fixes: 41af3a0 ("Move container create/start to slurm_spank_task_init")
Change
Open the namespace fds in user_init(), right after enroot_container_get()
confirms the container is running, instead of waiting until task_init(). An
open fd on /proc//ns/ pins the namespace even after the owning
process exits, and it's inherited across fork(2) into the task processes, so
task_init() just skips the lookup when reuse_ns is already set.
Only affects the attach path; container creation is unchanged since ns_pid
there is pyxis's own freshly forked process.
Testing
Reproduced against unpatched Pyxis on a live Slurm+Pyxis/Enroot cluster,
using ordinary job submissions only — no plugin swap or slurmd restart. One
step holds a named container and exits after a fixed sleep, while concurrent
steps attach to it with no --container-image:
672 / 3360 total attach attempts failed with the exact error chain above.
Failure rate peaking at an intermediate sleep duration and dropping to zero
at both extremes matches a timing window, not scheduling noise.
A/B'd baseline vs. this fix in an isolated single-node Slurm+munge+enroot
environment (same image, same parameters, only the plugin binary differs):