fix(nvsnap): restore the placeholder pid reservation and enforce it - #1027
Open
balajinvda wants to merge 9 commits into
Open
fix(nvsnap): restore the placeholder pid reservation and enforce it#1027balajinvda wants to merge 9 commits into
balajinvda wants to merge 9 commits into
Conversation
CRIU restore fails with clone3 EEXIST -- "Can't fork for N: File exists", "Unable to create a thread: -17" -- on most single-GPU workloads. A full suite run had 6 of 7 fail this way. Capture targets the workload's session leader, so CRIU dumps a subtree. CRIU writes a pidns image only when the target is the namespace root, so the dump has none, and restore logs "No pidns-1.img image". Without it restore cannot create a namespace: it must recreate the original PIDs in the placeholder's existing one, because PIDs are baked into the memory image (cached getpid, pthread TCBs, robust futexes, sempid). When the placeholder has already used one, the clone fails. That is also why this reads as flaky rather than broken: it depends on where the placeholder's PID counter happens to sit. A green run was luck, not correctness, so historical pass rates for this path are unverified. Add NVSNAP_DUMP_PIDNS_ROOT=1 to dump the container's namespace init instead, so CRIU records the namespace and restore creates a fresh one where every PID is free by construction -- removing the failure rather than making it less likely. Off by default: it changes what a capture contains and must not switch silently under a running deployment. docs/proposals/pidns-capture.md has the analysis, the rejected alternatives (ns_last_pid, nested namespace, smaller placeholder -- all tune the race rather than remove it), and the validation required before it becomes the default, including the CaptureFormatVersion bump without which old captures are silently reused. Refs #925 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
First run of the gated pid-namespace dump did not fail -- it hung. No image files, no dump.log, agent log stops at the criu invocation, and the harness gave up at 10m13s, well inside criu's own 1200s timeout. For comparison the subtree dump on the same workload completes in 1m08s. Leading hypothesis, recorded as unverified: the nsenter carries -p, which places criu inside the pid namespace it is dumping. That is harmless when the target is a subtree (criu is not a descendant of the session leader) and self-defeating when the target is the namespace root, because criu is then a member of the tree it freezes. Stock container checkpoint avoids this by running criu in the host pid namespace and naming the container init by host pid, so the next attempt should drop -p rather than abandon the approach. Worth noting for the pending go-criu removal: that path spawns criu swrk as a child of the agent and targets by pid with Root set, so it drives criu the way runc does and cannot hit this. It is currently the only in-tree caller doing so. Also bump CaptureFormatVersion to 2. Captures predating this change carry no pidns image, and replaying one silently keeps restore on the old path -- the bump is what makes any future fix here observable instead of masked by a cache hit. Refs #925 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
Restore into a criu-v2 placeholder failed 79% of the time across the
single-GPU suite (3/14 passed) with
Error (criu/cr-restore.c:1242): Can't fork for 363: File exists
CRIU recreates the dumped tree at its exact original pids. The placeholder
bumped ns_last_pid to 100000 so its own processes stayed clear of that
range. That line was removed on the belief that the write returns EPERM
inside a container, replaced by a comment pointing at an agent-side
reservePlaceholderPIDs that was never written.
The premise was wrong. /proc is mounted rw in these pods and the write
succeeds -- measured in a live placeholder, the next child landed at pid
100003. Without the bump the login shell forks a few hundred times
sourcing profile.d before the manifest's `tail -F` starts, so the tail
parks on a pid in the restored range: measured at 363, against observed
collisions at 292, 336, 340, 343, 363 and 365. It reads as flakiness
because the exact landing pid varies per run.
Restore the bump on the ten criu-v2 restore manifests, and have the agent
refuse to restore into a placeholder whose highest pid is still low. The
guard matters more than the line it protects: this failed silently for
days because `|| echo` swallowed the failure and the suite ran each
workload once, which cannot distinguish "broken" from "unlucky".
Refs #925
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
The pid reservation that keeps CRIU's exact-pid forks from colliding with
the restore pod's own processes only existed in the restore manifests we
hand-write for the test suite. Production restores do not use those: the
webhook stamps nvsnap.io/restore-from on a tenant pod and rewrites its
command to restore-entrypoint, and nothing along that path reserved
anything. A tenant entrypoint can hold far more pids than our three-process
placeholder, so the same collision applies with more room to go wrong:
Error (criu/cr-restore.c:1242): Can't fork for 363: File exists
Do it in restore-entrypoint instead, at the top of main before anything
forks. That covers every production restore pod whatever the tenant's own
command is, which a manifest-level bump never could.
Not fatal on failure: a restore pod that cannot reserve still has the
cold-start fallback, and crash-looping would turn a degraded restore into
no workload at all. The agent refuses the restore instead, so this surfaces
as a named error rather than as the intermittent flakiness it caused
before. Logged loudly either way -- the previous shell version ended in
`|| echo`, and that swallowed failure is what hid a 79% failure rate.
A test ties the value written here to the agent's acceptance floor; they
live in different packages and nothing else connects them.
Refs #925
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
NVSNAP_DUMP_PIDNS_ROOT gated an unvalidated capture change that hung on its only trial run. Shipping it would have left a 26th NVSNAP_* environment switch in the agent for a code path nobody can turn on safely. An environment variable is the wrong surface for this in particular: it changes what a capture contains, is invisible in the pod spec, is untyped, and survives long after the experiment that introduced it. If the approach is revisited it should arrive as an agent flag plumbed through chart values. Nothing is lost by removing it. The failure it targeted -- clone3 EEXIST during restore -- is fixed by the placeholder pid reservation, and the design plus the measured reason the trial hung stay in docs/proposals/pidns-capture.md. Also correct that document. It listed the ns_last_pid bump under rejected alternatives, on the grounds that the in-pod write returns EPERM even when privileged. That is false: /proc is mounted rw in these pods and the write succeeds, with the next child landing at pid 100003. The same wrong claim is what deleted the reservation originally, so leaving it in a design doc invites the regression a second time. Refs #925 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…e pids
The NIM image's default user is uid 1000, and privileged does not confer
root, so the placeholder's ns_last_pid write fails with
echo: write error: Operation not permitted
and the pid reservation silently does nothing. The restore then either
collides with the placeholder's own processes or, since the agent guard
landed, is refused outright. nim-llama-8b was the one workload of seven
still failing after the reservation was restored.
Only the placeholder runs as root. CRIU restores the workload with the uid
recorded in the checkpoint, so what the workload itself runs as is
unchanged.
This does not generalise to production, where the pod runs a tenant image
that is often non-root by design. Recorded on #925: reserving from inside
the pod cannot be the long-term answer there, which makes dumping the pid
namespace root load-bearing rather than a cleanup.
Refs #925
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…ling once The guard added alongside the pid reservation sampled the placeholder's pid range a single time and refused the restore if it was still low. That races the placeholder's own startup: the pod reports Running as soon as its shell starts, but the reservation only lands after that shell finishes sourcing profile.d, which is a few hundred forks in these images. The guard could therefore reject a placeholder that was seconds from correct. It did, on the first sweep after the guard landed: trtllm-small failed with "did not reserve its pid range (highest pid 356)", 356 being a transient profile.d fork rather than the tail the reservation was meant to move. Wait up to 90s instead, returning as soon as the reservation appears. This also matters beyond the test manifests: in production the reservation is done by restore-entrypoint, whose timing differs again. With this the single-GPU suite went from 3/14 to 13/13 across two passes of all seven workloads, including nim-llama-8b, which had failed every attempt until its placeholder was also given a uid that can write ns_last_pid. Refs #925 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
How we establish whether nvsnap can snapshot and restore Dynamo workers in aggregated and disaggregated mode, without producing a green result that means nothing. Dynamo is built to survive worker loss, so a restore that fails completely still yields a successful inference request served by another worker. That is the same masking that let a cold start be measured as a restore, except here the system is designed to hide it. The plan is shaped around defeating that: one worker of the type under test, and assert which worker served the request rather than that a request succeeded. Records what is established from source, notably that component-level annotations on a DynamoGraphDeployment reach pod metadata, so restore-from will trigger our webhook on operator-created pods. Separates that from the assumptions that still gate the design, chiefly whether workers rejoin after an abrupt restart. Also states why restore must happen in place: our harness deletes the source pod and creates a placeholder, which an operator will reconcile against, leaving two pods and a test that passes while proving nothing. Refs #1009 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
6 tasks
… behaviour Replaces assumptions with what the Dynamo runtime and operator actually do. Worker identity is addressable per request via x-dynamo-worker-instance-id and x-dynamo-prefill-instance-id, so a verification request can be pinned to the restored worker. Since Dynamo is built to survive worker loss, a failed restore would otherwise be served by a healthy peer and the test would pass while proving nothing. Pinning removes that. Discovery on Kubernetes is readiness-driven, not lease-driven: the operator sets DYN_DISCOVERY_BACKEND=kubernetes, and a pod is discoverable when it is ready in an EndpointSlice and has a pod-owned DynamoWorkerMetadata CR. So restoring in place preserves registration and recovery is automatic, while deleting the pod would garbage collect the CR. Refs #1009 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
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
2 participants
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.
Why
Restore into a criu-v2 placeholder was failing most of the time. A measured
sweep of the single-GPU suite, every workload twice, passed 3 of 14. The
failures all looked like this:
Both are
EEXISTfromclone3(set_tid=N). CRIU recreates a dumped tree at itsexact original pids -- they are baked into the image (cached getpid, pthread
TCBs, robust futex lists, file lock owners) and cannot be renumbered. So any
long-lived process the placeholder parks inside that range breaks the restore.
The placeholder used to push its own pid allocations clear of that range with
That line was removed on the grounds that the write returns EPERM inside a
container, and replaced with a comment pointing at an agent-side
reservePlaceholderPIDsthat was never written.The premise was wrong.
/procis mounted rw in these pods and the writesucceeds: measured in a live placeholder, the next child landed at pid 100003.
Without it the placeholder's login shell forks a few hundred times sourcing
profile.d before the manifest's
tail -Fstarts, so the tail parks at pid ~363,inside the restored range. Where it lands varies per run, which is why this
read as flakiness rather than breakage, and why a green suite was not evidence
of anything.
What changed
ns_last_pidreservation on the ten criu-v2 restore manifests.restore-entrypointas well, at the top of main before anythingforks. Production restore pods run a tenant image with the command rewritten
to that binary, so a manifest-level bump never covered them.
pushed up, and wait up to 90s for it rather than sampling once. Sampling
raced the placeholder's own startup and rejected placeholders that were
seconds from correct.
runAsUser: 0. That image defaults to uid1000 and
privilegeddoes not confer root, so its reservation silently didnothing.
NVSNAP_DUMP_PIDNS_ROOTenvironment switch. It gated an unvalidatedcapture change that hung on its only trial; the design and the measured
reason it hung are kept in
docs/proposals/pidns-capture.md.Customer Release Notes
Fixes intermittent GPU checkpoint restore failures. Restores that previously
failed with a process-creation error now succeed.
Plan Summary
Not applicable.
Usage
Not applicable. The reservation is automatic.
Testing
Single-GPU suite, every workload twice, on a QA cluster:
3/14 before, 13/13 after. nim-llama-8b was verified in a separate run after its
uid fix; restore-to-serving was 1m54s from a 99G checkpoint.
Repeat runs are the point. The failure is probabilistic, so one pass per
workload cannot distinguish working from lucky, which is how this survived for
as long as it did.
Unit tests cover the pid-namespace walk, the acceptance floor, the wait's
success and timeout paths, and that a procfs read failure is reported
differently from a missing reservation.
Notes
Reserving from inside the pod cannot be the long-term answer for production.
Restore pods run tenant images, non-root is normal practice there, and those
cannot write
ns_last_pidat all. The agent guard makes that a named refusalrather than a mid-restore failure, but it is a refusal. Dumping the pid
namespace root would remove the requirement instead of satisfying it; see
docs/proposals/pidns-capture.mdand the discussion on the issue.This branch is stacked on
nvsnap/e2e-restore-guardsand targets it rather thanmain, so the diff here is only the pid work.
Issues
Relates to #925
References
None
Related Pull Requests
Stacked on the branch for the restore-verification guards.
Dependencies
None