From 5a5f1e676aeeab9e4e7afcb2cf0277433ca07847 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 16:36:38 -0700 Subject: [PATCH 1/9] feat(nvsnap): allow capture to dump the pid namespace root 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 --- .../nvsnap/docs/proposals/pidns-capture.md | 106 ++++++++++++++++++ .../nvsnap/internal/agent/checkpoint_v2.go | 43 ++++++- 2 files changed, 145 insertions(+), 4 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md diff --git a/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md new file mode 100644 index 000000000..f93ba305b --- /dev/null +++ b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md @@ -0,0 +1,106 @@ + + +# Capture the PID namespace, so restore can create a fresh one + +Status: proposed, implementation behind `NVSNAP_DUMP_PIDNS_ROOT=1` + +## The failure + +CRIU restore dies with one of: + +``` +Error (criu/cr-restore.c:1242): Can't fork for 364: File exists +Error (criu/pie/restorer.c:2878): Unable to create a thread: -17 +``` + +Both are `EEXIST` from `clone3(set_tid=N)`: the PID the restore needs is +already taken in the target namespace. + +Measured on a full suite run, 7 single-GPU workloads on one agent build: 6 +failed this way, 1 passed. Earlier runs of the same build reported different +pass counts, which is the tell -- see "Why it looks flaky". + +## Why it happens + +Capture targets the workload's session leader, not the container's init: + +```go +// internal/agent/checkpoint_v2.go +targetHostPID := hostPID +if len(gpuPIDs) > 0 { + if sid, err := sessionID(procBase, gpuPIDs[0]); err == nil && sid > 1 { + targetHostPID = sid // <- a subtree, not the namespace root + } +} +``` + +That choice propagates: + +1. CRIU dumps a **subtree** rooted at the session leader. +2. CRIU writes a pidns image only when the dump target is the namespace root. + A subtree dump has none -- restore logs `No pidns-1.img image`. +3. Without that image restore cannot create a namespace, so it recreates the + original PIDs inside the placeholder pod's **existing** namespace. +4. PIDs cannot be renumbered. They are baked into the memory image: cached + `getpid()` values, pthread TCBs, robust futex lists, `sempid`. CRIU must + reproduce them exactly. +5. If the placeholder has already consumed one of those PIDs, `clone3(set_tid)` + returns `EEXIST` and the restore fails. + +## Why it looks flaky + +Whether step 5 fires depends on where the placeholder's PID counter happens to +sit when restore runs -- a function of how many processes the pod started, how +busy the node is, and timing. The same workload on the same build passes or +fails run to run. + +This matters for how past results should be read: a green suite was not +evidence of correctness, only of a lucky PID counter. Any historical pass rate +for the CRIU path should be treated as unverified. + +## The fix + +Dump the container's PID-namespace init instead of the session leader. CRIU +then records the namespace, and restore creates a fresh one where every PID is +free by construction. The collision becomes impossible rather than unlikely. + +Cost: the dump includes the container's init process (typically the `bash` the +workload was launched under). That is cheap -- a shell, no GPU state -- and is +what a container checkpoint normally contains. + +## Alternatives rejected + +**Bump `ns_last_pid` before restore.** Tried and reverted. The in-pod write is +`EPERM` even privileged; the agent-side write works but is a race -- nothing +stops another process consuming the PID between the bump and the clone. It +lowers the failure rate without removing the failure. + +**Restore into a freshly unshared PID namespace.** Keeps the dump unchanged and +guarantees free PIDs, but leaves the workload in a nested namespace. Needs +proof that readiness probes, `kubectl exec`, and the GPU driver's view still +behave. Worth revisiting if dumping the init turns out to have its own problems. + +**Make the placeholder consume fewer PIDs.** Reduces the odds. Same objection +as `ns_last_pid`: it tunes a race rather than removing it. + +## Rollout + +Off by default (`NVSNAP_DUMP_PIDNS_ROOT=1` to enable). It changes what a capture +contains, so it must not switch silently under a running deployment. + +Validation before it becomes the default: + +1. Confirm the dump now writes a pidns image, and restore logs a namespace + creation rather than `No pidns-1.img image`. +2. Full suite green across single-GPU workloads, repeated -- one green run + proves nothing here, given the failure is probabilistic. +3. Confirm the restored process still passes inference, not merely starts. +4. Bump `CaptureFormatVersion`: captures taken before this contain no pidns + image and must not be replayed by an agent that expects one. + +Step 4 is not optional. Without it an upgraded agent silently reuses old +captures and the fix appears not to work -- the same trap that made the +runtime-directory fix look ineffective until the version was bumped. diff --git a/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go b/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go index d95f03f45..4cfaa254e 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go +++ b/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go @@ -167,11 +167,31 @@ func (a *Agent) dumpV2(ctx context.Context, containerInfo *containerd.ContainerI } // 4. Dump target: CRIU's -t is resolved in the entered pid namespace. - // Use the GPU leader's session leader when available (PoC convention: - // workload launched via setsid, tree root != container init); fall back - // to the container init's in-namespace pid. + // + // Two choices, and they decide whether restore can ever be reliable: + // + // session leader (default today): dumps a SUBTREE. CRIU only writes a + // pidns image when the target is the namespace root, so a subtree dump + // has none. Restore then cannot create a namespace -- it must recreate + // the original PIDs inside the placeholder's existing one, because PIDs + // are baked into the memory image (cached getpid, pthread TCBs, robust + // futexes). If the placeholder already used one of them, clone3(set_tid) + // fails with EEXIST and the restore dies. Whether that happens depends + // on where the placeholder's PID counter sits, which is why this looks + // like flakiness rather than a bug. + // + // namespace init (this option): dumps the whole container tree, so CRIU + // records the pid namespace and restore recreates it fresh. Every PID + // is free by construction and the collision cannot occur. + // + // Off by default until validated end to end on every workload: it changes + // what a capture contains, so it must not switch silently under anyone. targetHostPID := hostPID - if len(gpuPIDs) > 0 { + if dumpNamespaceRoot() { + // hostPID is the container init (NSpid 1) -- the namespace root. + log.WithField("dump_target", "namespace-init"). + Info("dumping the container's pid namespace root (pidns image expected)") + } else if len(gpuPIDs) > 0 { if sid, err := sessionID(procBase, gpuPIDs[0]); err == nil && sid > 1 { targetHostPID = sid } @@ -487,3 +507,18 @@ func tailOfFile(path string, n int) string { } return strings.Join(lines, " | ") } + +// dumpNamespaceRoot reports whether capture should target the container's pid +// namespace root rather than the workload's session leader. +// +// Dumping the namespace root is what lets CRIU record a pid namespace, which +// in turn lets restore create a fresh one instead of recreating exact PIDs in +// the placeholder's namespace. It is the structural fix for the clone3 EEXIST +// restore failures; see docs/proposals/pidns-capture.md. +// +// Env-gated rather than a flag so it can be flipped per-run during validation +// without a chart change, and defaults off so an upgrade never silently alters +// what captures contain. +func dumpNamespaceRoot() bool { + return os.Getenv("NVSNAP_DUMP_PIDNS_ROOT") == "1" +} From a4cbabb604fa3824f67eaa23669741f7b7cc0dfe Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 19:39:20 -0700 Subject: [PATCH 2/9] docs(nvsnap): pid namespace dump hangs, and why 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 --- .../nvsnap/docs/proposals/pidns-capture.md | 26 +++++++++++++++++++ .../nvsnap/internal/checkpointstore/store.go | 8 +++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md index f93ba305b..5b5854a7e 100644 --- a/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md +++ b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md @@ -71,6 +71,32 @@ Cost: the dump includes the container's init process (typically the `bash` the workload was launched under). That is cheap -- a shell, no GPU state -- and is what a container checkpoint normally contains. +## First attempt hung, and why + +Measured, not theorised. With the gate on, the dump ran as: + +``` +nsenter -t -m -p -n -i -u -r -w -- criu dump -t 1 ... +``` + +It never returned. No image files, no `dump.log`, and the agent log stops at +the invocation. The harness gave up at 10m13s, well inside CRIU's own 1200s +timeout, so nothing failed -- it hung. + +The likely mechanism is the `-p` in that nsenter. It places CRIU *inside* the +container's PID namespace, which 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: CRIU is then a member of the very tree it is +freezing, so it stalls on itself. + +Stock container checkpoint does not do this. `runc checkpoint` runs CRIU in the +host PID namespace and names the container init by its *host* pid, letting CRIU +discover and record the namespace from the target. So the next attempt should +drop `-p` and pass the host pid rather than `-t 1`, which is both the fix and a +further step onto the standard path. + +This is unverified. It is the leading hypothesis, not a conclusion. + ## Alternatives rejected **Bump `ns_last_pid` before restore.** Tried and reverted. The in-pod write is diff --git a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go index e0f6ec0c0..73fb45918 100644 --- a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go +++ b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go @@ -35,7 +35,13 @@ import ( // CaptureFormatVersion is bumped whenever the on-disk schema for a capture // changes (manifest format, layout, included metadata). Hashes are recomputed // across versions, so old captures stop matching. -const CaptureFormatVersion = 1 +// +// 2: dumps target the pid namespace root, so the image set now contains a +// pidns image. A v1 capture does not, and replaying one keeps the restore on +// the old recreate-PIDs-in-place path that fails with clone3 EEXIST. The bump +// is what makes the fix take effect: without it the agent reuses the stale +// capture by hash and the fix looks like it did nothing. +const CaptureFormatVersion = 2 // ErrNotFound is returned by Stat / Get when no capture is stored under the // given hash. From 89da58228529a2e3c3d5a13c48417ab7993074a9 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 08:11:41 -0700 Subject: [PATCH 3/9] fix(nvsnap): restore the placeholder pid reservation and enforce it 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 --- .../k8s/workloads/e5-mistral-restore.yaml | 23 ++- .../k8s/workloads/gemma-sglang-restore.yaml | 23 ++- .../k8s/workloads/nim-llama-8b-restore.yaml | 23 ++- .../k8s/workloads/sglang-8b-restore.yaml | 23 ++- .../k8s/workloads/sglang-small-restore.yaml | 23 ++- .../k8s/workloads/trtllm-small-restore.yaml | 23 ++- .../deploy/k8s/workloads/vllm-8b-restore.yaml | 23 ++- .../deploy/k8s/workloads/vllm-mp-restore.yaml | 23 ++- .../k8s/workloads/vllm-qwen32b-restore.yaml | 23 ++- .../k8s/workloads/vllm-small-restore.yaml | 23 ++- .../nvsnap/internal/agent/restore_v2.go | 90 ++++++++++ .../agent/restore_v2_pidguard_test.go | 168 ++++++++++++++++++ 12 files changed, 428 insertions(+), 60 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml index c02fcccc0..424da65aa 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml index b5417d9f7..34c91892a 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /sglang.out (the # source manifest's setsid convention); surface it via kubelet. touch /sglang.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml index 478014390..742c5e167 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml @@ -37,12 +37,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /tmp/nim.out (the # source manifest's setsid convention); surface it via kubelet. # diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml index 81bcd1770..9d71654dc 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /sglang.out (the # source manifest's setsid convention); surface it via kubelet. touch /sglang.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml index bd5b8476d..a0f913f08 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /sglang.out (the # source manifest's setsid convention); surface it via kubelet. touch /sglang.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml index da70f6669..b76dd4013 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /trtllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /trtllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml index c782d19ff..de94b8dc1 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml index 598f1bec4..50c5eaa12 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml index 262f5fbd5..c11fb7ccf 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml index a52402f84..bfaea1248 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go b/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go index ec5cc9f09..29580e38a 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go +++ b/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go @@ -101,6 +101,26 @@ func (a *Agent) restoreV2(ctx context.Context, metadata *CheckpointMetadata, che } } + // Refuse to restore into a placeholder that never pushed its own pid + // allocations clear of the dumped range. CRIU recreates the dumped tree at + // its exact original pids, so any long-lived process the placeholder parked + // in that range makes the restore fail with + // + // Error (criu/cr-restore.c:1242): Can't fork for 363: File exists + // + // The placeholder bumps ns_last_pid for exactly this reason. When that line + // went missing the failure rate was 79% across the single-GPU suite, and it + // read as flakiness because whether it fires depends on where the shell's + // own forks happened to land. Failing here names the cause instead. + if maxPID, perr := placeholderMaxNSPID(procBase, hostPID); perr != nil { + log.WithError(perr).Warn("criu-v2: could not verify the placeholder reserved its pid range; continuing") + } else if maxPID < reservedPIDFloor { + return nil, fmt.Errorf( + "criu-v2: placeholder did not reserve its pid range (highest pid %d < %d): "+ + "the ns_last_pid bump in the restore manifest is missing or failed, and CRIU's "+ + "exact-pid forks will collide with this pod's own processes", maxPID, reservedPIDFloor) + } + log.WithFields(logrus.Fields{ "placeholderPID": hostPID, "imagesDir": imgsInContainer, @@ -222,3 +242,73 @@ func (a *Agent) gpuProcessInSamePidNS(ctx context.Context, procBase string, cont } return 0, nil } + +// reservedPIDFloor is the lowest highest-pid we accept in a placeholder before +// restoring into it. The manifest bumps ns_last_pid to 100000, so a correctly +// prepared placeholder sits just above that; a placeholder that skipped the +// bump sits in the hundreds. Anything in between is not a case we produce, so +// the floor is set well clear of both rather than tuned. +const reservedPIDFloor = 50000 + +// placeholderMaxNSPID returns the highest in-container pid currently live in +// the placeholder's pid namespace. +// +// Read from the host rather than by exec'ing into the pod: entering the +// namespace to measure it would itself allocate a pid there, which is the very +// resource under test. +func placeholderMaxNSPID(procBase string, hostPID int) (int, error) { + want, err := os.Readlink(filepath.Join(procBase, strconv.Itoa(hostPID), "ns", "pid")) + if err != nil { + return 0, fmt.Errorf("read placeholder pid namespace: %w", err) + } + + entries, err := os.ReadDir(procBase) + if err != nil { + return 0, fmt.Errorf("read %s: %w", procBase, err) + } + + max := 0 + for _, e := range entries { + pid, aerr := strconv.Atoi(e.Name()) + if aerr != nil { + continue // not a pid directory + } + // Processes come and go while we walk; a vanished one is not an error. + ns, rerr := os.Readlink(filepath.Join(procBase, e.Name(), "ns", "pid")) + if rerr != nil || ns != want { + continue + } + nspid, nerr := nsPIDOf(procBase, pid) + if nerr != nil { + continue + } + if nspid > max { + max = nspid + } + } + if max == 0 { + return 0, fmt.Errorf("no processes found in the placeholder's pid namespace") + } + return max, nil +} + +// nsPIDOf returns a process's pid as seen from the innermost namespace it +// belongs to -- the last field of NSpid in /proc//status. +func nsPIDOf(procBase string, pid int) (int, error) { + b, err := os.ReadFile(filepath.Join(procBase, strconv.Itoa(pid), "status")) + if err != nil { + return 0, err + } + for _, line := range strings.Split(string(b), "\n") { + rest, ok := strings.CutPrefix(line, "NSpid:") + if !ok { + continue + } + fields := strings.Fields(rest) + if len(fields) == 0 { + return 0, fmt.Errorf("empty NSpid for %d", pid) + } + return strconv.Atoi(fields[len(fields)-1]) + } + return 0, fmt.Errorf("no NSpid line for %d", pid) +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go b/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go new file mode 100644 index 000000000..afc312894 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go @@ -0,0 +1,168 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package agent + +import ( + "os" + "path/filepath" + "strconv" + "testing" +) + +// fakeProc builds a procfs stand-in. Each entry is hostPID -> (nsLink, NSpid +// line); a process is "in" the placeholder's namespace when its ns/pid symlink +// target matches. +func fakeProc(t *testing.T, procs map[int]struct { + ns string + nspid string +}, +) string { + t.Helper() + base := t.TempDir() + for pid, p := range procs { + dir := filepath.Join(base, strconv.Itoa(pid)) + if err := os.MkdirAll(filepath.Join(dir, "ns"), 0o755); err != nil { + t.Fatalf("mkdir %d: %v", pid, err) + } + // The real procfs uses magic symlinks; a plain symlink reproduces what + // the code actually does with them (Readlink, compare strings). + if err := os.Symlink(p.ns, filepath.Join(dir, "ns", "pid")); err != nil { + t.Fatalf("symlink %d: %v", pid, err) + } + status := "Name:\tsh\nState:\tS (sleeping)\nNSpid:\t" + p.nspid + "\n" + if err := os.WriteFile(filepath.Join(dir, "status"), []byte(status), 0o600); err != nil { + t.Fatalf("status %d: %v", pid, err) + } + } + // Non-pid entries must be skipped rather than error the walk. + if err := os.WriteFile(filepath.Join(base, "meminfo"), []byte("MemTotal: 1 kB\n"), 0o600); err != nil { + t.Fatalf("meminfo: %v", err) + } + return base +} + +type procEntry = struct { + ns string + nspid string +} + +func TestPlaceholderMaxNSPID(t *testing.T) { + tests := []struct { + name string + procs map[int]procEntry + hostPID int + want int + wantErr bool + }{ + { + // A placeholder that ran the ns_last_pid bump: its helpers sit + // above the dumped range, so restore is safe. + name: "reserved placeholder reports the high pid", + procs: map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "100001"}, + 5002: {ns: "pid:[111]", nspid: "100002"}, + }, + hostPID: 5000, + want: 100002, + }, + { + // The regression this guard exists for: the bump is missing, so a + // long-lived tail sits at 363, inside the range CRIU must recreate. + name: "unreserved placeholder reports the low pid", + procs: map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "363"}, + 5002: {ns: "pid:[111]", nspid: "372"}, + }, + hostPID: 5000, + want: 372, + }, + { + // Processes outside the placeholder's namespace must not count -- + // the agent's own pids are far higher and would mask the problem. + name: "ignores processes in other namespaces", + procs: map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "363"}, + 9000: {ns: "pid:[999]", nspid: "987654"}, + }, + hostPID: 5000, + want: 363, + }, + { + name: "missing placeholder is an error, not zero", + procs: map[int]procEntry{}, + hostPID: 5000, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := fakeProc(t, tt.procs) + got, err := placeholderMaxNSPID(base, tt.hostPID) + if tt.wantErr { + if err == nil { + t.Fatalf("expected an error, got max=%d", got) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("max NSpid = %d, want %d", got, tt.want) + } + }) + } +} + +// The floor must sit clear of both shapes we actually produce: a bumped +// placeholder (100000+) passes, an unbumped one (hundreds) fails. A floor that +// admitted the unbumped case would restore the silent 79% failure rate. +func TestReservedPIDFloorSeparatesBothShapes(t *testing.T) { + const bumped, unbumped = 100001, 372 + if bumped < reservedPIDFloor { + t.Errorf("a bumped placeholder (%d) must clear the floor (%d)", bumped, reservedPIDFloor) + } + if unbumped >= reservedPIDFloor { + t.Errorf("an unbumped placeholder (%d) must fail the floor (%d)", unbumped, reservedPIDFloor) + } +} + +func TestNSPIDOfUsesInnermostNamespace(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "42") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + // Nested namespaces list outermost first; the placeholder's own view is + // the last field, and taking the first would report the host pid. + status := "Name:\tbash\nNSpid:\t42\t7\t3\n" + if err := os.WriteFile(filepath.Join(dir, "status"), []byte(status), 0o600); err != nil { + t.Fatal(err) + } + got, err := nsPIDOf(base, 42) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != 3 { + t.Errorf("nsPIDOf = %d, want 3 (innermost)", got) + } +} From 0c7bdd74bde0442f6dc77866b2834dbe1c54ee01 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 13:05:34 -0700 Subject: [PATCH 4/9] fix(nvsnap): reserve the pid range for production restores too 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 --- .../nvsnap/cmd/restore-entrypoint/BUILD.bazel | 5 +- .../nvsnap/cmd/restore-entrypoint/main.go | 58 ++++++++++++++++ .../restore-entrypoint/pid_reserve_test.go | 66 +++++++++++++++++++ .../nvsnap/internal/agent/BUILD.bazel | 1 + 4 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go diff --git a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel index 7e9ed9934..1255ecbd7 100644 --- a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel @@ -26,6 +26,9 @@ go_binary( go_test( name = "restore-entrypoint_test", - srcs = ["cold_start_fallback_test.go"], + srcs = [ + "cold_start_fallback_test.go", + "pid_reserve_test.go", + ], embed = [":restore-entrypoint_lib"], ) diff --git a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go index 979635cb5..f6da1882c 100644 --- a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go +++ b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go @@ -441,6 +441,10 @@ func main() { } fmt.Println("=== NVSNAP Restore Entrypoint (go-criu) ===") + // Before anything else forks: push this pod's pid allocations clear of the + // range the checkpoint captured. + reservePIDRange() + // OpenTelemetry. No-op when OTEL_EXPORTER_OTLP_ENDPOINT is unset. // When the agent sets OTEL_TRACE_PARENT in the placeholder pod env // (see internal/agent/restore.go), our spans nest under the agent's @@ -3547,3 +3551,57 @@ func discoverNvidiaMajors() map[uint32]bool { } return majors } + +// pidReserveFloor is the value written to ns_last_pid so this pod's own +// processes allocate above anything a checkpoint is likely to contain. It must +// stay above the agent's acceptance floor (reservedPIDFloor in +// internal/agent/restore_v2.go), which refuses to restore into a pod that +// skipped this step. +const pidReserveFloor = "100000" + +// reservePIDRange pushes this pid namespace's next allocation clear of the +// range a checkpoint captured. +// +// CRIU recreates a dumped tree at its exact original pids -- they are baked +// into the image (cached getpid, pthread TCBs, robust futex lists, file lock +// owners), so it cannot renumber them. Any long-lived process this pod starts +// before the restore therefore has to land somewhere the checkpoint does not +// need, or the restore dies with: +// +// Error (criu/cr-restore.c:1242): Can't fork for 363: File exists +// +// Doing it here rather than in the pod manifest is what makes it work for +// production restores: the webhook rewrites the container command to this +// binary (see internal/webhook/restore_entrypoint.go), so this runs as pid 1 +// on every restore pod regardless of what the tenant's own command is. A +// manifest-level bump only ever covered pods whose manifest we wrote. +// +// Ordering is the whole point -- this must run before anything forks, which is +// why it sits at the top of main rather than alongside the restore logic. +// +// Deliberately not fatal. A restore pod that cannot reserve still has the +// cold-start fallback path, and crash-looping here would turn a degraded +// restore into no workload at all. The agent enforces instead: it refuses to +// restore into a pod whose pid range was never pushed up, so a silent failure +// here surfaces as a named error there rather than as the intermittent +// "flakiness" this cost us before. +func reservePIDRange() { + reservePIDRangeAt(nsLastPIDPath) +} + +// nsLastPIDPath is the real control file; tests point reservePIDRangeAt at a +// temp file instead. +const nsLastPIDPath = "/proc/sys/kernel/ns_last_pid" + +func reservePIDRangeAt(path string) { + if err := os.WriteFile(path, []byte(pidReserveFloor), 0o644); err != nil { + // Loud on purpose. The previous shell version ended in `|| echo`, and + // that swallowed failure is what let a 79% restore failure rate look + // like flakiness for days. + fmt.Printf("ERROR: could not reserve pid range: write %s: %v\n", path, err) + fmt.Println("ERROR: CRIU's exact-pid forks may collide with this pod's own processes;") + fmt.Println("ERROR: the agent will refuse the restore rather than fail in the middle of it.") + return + } + fmt.Printf("Reserved pid range: ns_last_pid=%s (own processes allocate above it)\n", pidReserveFloor) +} diff --git a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go new file mode 100644 index 000000000..c1bdef359 --- /dev/null +++ b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go @@ -0,0 +1,66 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "os" + "path/filepath" + "strconv" + "testing" +) + +func TestReservePIDRangeWritesTheFloor(t *testing.T) { + path := filepath.Join(t.TempDir(), "ns_last_pid") + reservePIDRangeAt(path) + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("control file not written: %v", err) + } + if string(got) != pidReserveFloor { + t.Errorf("wrote %q, want %q", got, pidReserveFloor) + } +} + +// A write failure must not take the process down: the restore pod still has a +// cold-start fallback, and crash-looping here would turn a degraded restore +// into no workload at all. The agent refuses the restore instead. +func TestReservePIDRangeSurvivesAnUnwritablePath(t *testing.T) { + dir := t.TempDir() + // A directory cannot be written as a file, which is the closest stand-in + // for the read-only /proc/sys the old shell version believed it faced. + reservePIDRangeAt(dir) +} + +// The floor this binary writes must clear the floor the agent accepts, +// otherwise a correctly-reserved pod would still be refused. These constants +// live in different packages and nothing but this test ties them together. +func TestReserveFloorClearsTheAgentAcceptanceFloor(t *testing.T) { + // Mirrors reservedPIDFloor in internal/agent/restore_v2.go. If that value + // changes, this test should fail and force the pair to be reconsidered. + const agentAcceptanceFloor = 50000 + + got, err := strconv.Atoi(pidReserveFloor) + if err != nil { + t.Fatalf("pidReserveFloor %q is not a number: %v", pidReserveFloor, err) + } + if got <= agentAcceptanceFloor { + t.Errorf("reserve floor %d must exceed the agent's acceptance floor %d, "+ + "or every reserved pod is refused", got, agentAcceptanceFloor) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index 48881a4dc..c700911aa 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -103,6 +103,7 @@ go_test( "replication_test.go", "restore_prep_http_test.go", "restore_prep_test.go", + "restore_v2_pidguard_test.go", "restoreoverlay_http_test.go", "restoreoverlay_integration_test.go", "restoreoverlay_test.go", From 625f556ac978a18d4ca84fad70a3324471ee752d Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 16:33:33 -0700 Subject: [PATCH 5/9] refactor(nvsnap): drop the pid-namespace dump env switch 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 --- .../nvsnap/docs/proposals/pidns-capture.md | 30 ++++++++++++++----- .../nvsnap/internal/agent/checkpoint_v2.go | 20 +------------ 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md index 5b5854a7e..22af2da51 100644 --- a/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md +++ b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md @@ -5,7 +5,9 @@ SPDX-License-Identifier: Apache-2.0 # Capture the PID namespace, so restore can create a fresh one -Status: proposed, implementation behind `NVSNAP_DUMP_PIDNS_ROOT=1` +Status: proposed. Not implemented -- the trial implementation was removed +(see "First attempt hung" below), and the immediate failure it targeted has since +been fixed another way (the placeholder pid reservation). ## The failure @@ -99,10 +101,21 @@ This is unverified. It is the leading hypothesis, not a conclusion. ## Alternatives rejected -**Bump `ns_last_pid` before restore.** Tried and reverted. The in-pod write is -`EPERM` even privileged; the agent-side write works but is a race -- nothing -stops another process consuming the PID between the bump and the clone. It -lowers the failure rate without removing the failure. +**Bump `ns_last_pid` before restore.** This is what actually shipped, and the +reasoning that first rejected it here was wrong on the facts. + +The claim was that the in-pod write returns `EPERM` even when privileged. It +does not. `/proc` is mounted `rw` in these pods and the write succeeds -- +measured in a live placeholder, the next child landed at pid 100003. That false +premise is what removed the reservation in the first place and produced a 79% +restore failure rate that read as flakiness. + +It is prevention rather than impossibility: it works because nothing else in a +restore pod allocates pids between the bump and CRIU's forks. That assumption +holds for the pods we control and is enforced -- the agent refuses to restore +into a pod whose pid range was never pushed up. Dumping the namespace root, as +proposed here, would remove the requirement rather than satisfy it, which is why +this document is still worth keeping. **Restore into a freshly unshared PID namespace.** Keeps the dump unchanged and guarantees free PIDs, but leaves the workload in a nested namespace. Needs @@ -114,8 +127,11 @@ as `ns_last_pid`: it tunes a race rather than removing it. ## Rollout -Off by default (`NVSNAP_DUMP_PIDNS_ROOT=1` to enable). It changes what a capture -contains, so it must not switch silently under a running deployment. +If this is picked up again, it needs a real configuration surface -- an agent +flag plumbed through chart values -- not an environment variable. The trial used +one and it was removed rather than merged: an env switch that changes what a +capture contains is invisible in the pod spec, untyped, and easy to leave +behind. Validation before it becomes the default: diff --git a/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go b/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go index 4cfaa254e..51f50a690 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go +++ b/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go @@ -187,11 +187,7 @@ func (a *Agent) dumpV2(ctx context.Context, containerInfo *containerd.ContainerI // Off by default until validated end to end on every workload: it changes // what a capture contains, so it must not switch silently under anyone. targetHostPID := hostPID - if dumpNamespaceRoot() { - // hostPID is the container init (NSpid 1) -- the namespace root. - log.WithField("dump_target", "namespace-init"). - Info("dumping the container's pid namespace root (pidns image expected)") - } else if len(gpuPIDs) > 0 { + if len(gpuPIDs) > 0 { if sid, err := sessionID(procBase, gpuPIDs[0]); err == nil && sid > 1 { targetHostPID = sid } @@ -508,17 +504,3 @@ func tailOfFile(path string, n int) string { return strings.Join(lines, " | ") } -// dumpNamespaceRoot reports whether capture should target the container's pid -// namespace root rather than the workload's session leader. -// -// Dumping the namespace root is what lets CRIU record a pid namespace, which -// in turn lets restore create a fresh one instead of recreating exact PIDs in -// the placeholder's namespace. It is the structural fix for the clone3 EEXIST -// restore failures; see docs/proposals/pidns-capture.md. -// -// Env-gated rather than a flag so it can be flipped per-run during validation -// without a chart change, and defaults off so an upgrade never silently alters -// what captures contain. -func dumpNamespaceRoot() bool { - return os.Getenv("NVSNAP_DUMP_PIDNS_ROOT") == "1" -} From 5302a31422ee7b9e30f47119c50941cb4bb07719 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 16:55:38 -0700 Subject: [PATCH 6/9] fix(nvsnap): run the NIM restore placeholder as root so it can reserve 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 --- .../nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml index 742c5e167..4ff35a14b 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml @@ -85,6 +85,13 @@ spec: failureThreshold: 180 securityContext: privileged: true + # The NIM image's default user is uid 1000, and privileged does not + # confer root. Writing /proc/sys/kernel/ns_last_pid then fails with + # "Operation not permitted", the pid reservation above silently does + # nothing, and the agent refuses the restore. Only the placeholder runs + # as root: CRIU restores the workload with the uid recorded in the + # checkpoint, so this does not change what the workload runs as. + runAsUser: 0 resources: limits: nvidia.com/gpu: "1" From d51d715097a8fb7c7544fc34a6eca962ea50a3ee Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 18:32:24 -0700 Subject: [PATCH 7/9] fix(nvsnap): wait for the placeholder pid reservation instead of sampling 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 --- .../nvsnap/internal/agent/restore_v2.go | 78 +++++++++++++++++-- .../agent/restore_v2_pidguard_test.go | 59 ++++++++++++++ 2 files changed, 130 insertions(+), 7 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go b/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go index 29580e38a..604e64864 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go +++ b/src/compute-plane-services/nvsnap/internal/agent/restore_v2.go @@ -112,13 +112,15 @@ func (a *Agent) restoreV2(ctx context.Context, metadata *CheckpointMetadata, che // went missing the failure rate was 79% across the single-GPU suite, and it // read as flakiness because whether it fires depends on where the shell's // own forks happened to land. Failing here names the cause instead. - if maxPID, perr := placeholderMaxNSPID(procBase, hostPID); perr != nil { - log.WithError(perr).Warn("criu-v2: could not verify the placeholder reserved its pid range; continuing") - } else if maxPID < reservedPIDFloor { - return nil, fmt.Errorf( - "criu-v2: placeholder did not reserve its pid range (highest pid %d < %d): "+ - "the ns_last_pid bump in the restore manifest is missing or failed, and CRIU's "+ - "exact-pid forks will collide with this pod's own processes", maxPID, reservedPIDFloor) + // + // Wait rather than sample once: the pod is Running as soon as its shell + // starts, but the reservation only lands after that shell finishes sourcing + // its login profile, a few hundred forks in these images. Sampling once + // races that window and rejects a placeholder that was about to be fine. + if maxPID, perr := awaitPlaceholderPIDReservation(procBase, hostPID, log); perr != nil { + return nil, perr + } else if maxPID > 0 { + log.WithField("maxNSPID", maxPID).Info("criu-v2: placeholder reserved its pid range") } log.WithFields(logrus.Fields{ @@ -312,3 +314,65 @@ func nsPIDOf(procBase string, pid int) (int, error) { } return 0, fmt.Errorf("no NSpid line for %d", pid) } + +// pidReservationTimeout bounds how long we wait for the placeholder to push its +// pid range up. The reservation itself is one write; the wait is for the login +// shell ahead of it, which forks a few hundred times sourcing profile.d in +// these images. Generous on purpose: waiting a few extra seconds costs far less +// than rejecting a placeholder that was seconds from ready. +const pidReservationTimeout = 90 * time.Second + +// awaitPlaceholderPIDReservation blocks until the placeholder's pid allocations +// clear the dumped range, and returns the highest pid it saw. +// +// Returns an error only when the reservation never lands, which means the +// restore would fail partway through with a clone3 EEXIST that reads as +// flakiness. Failing here names the cause instead. +// +// A procfs read error is not fatal: the pid namespace may still be settling, +// and treating a transient read as a missing reservation would reintroduce +// exactly the false negative this function exists to avoid. +func awaitPlaceholderPIDReservation(procBase string, hostPID int, log *logrus.Entry) (int, error) { + return awaitPlaceholderPIDReservationFor(procBase, hostPID, pidReservationTimeout, log) +} + +// awaitPlaceholderPIDReservationFor is the body, with the wait injectable so +// tests can exercise the timeout path without waiting it out. +func awaitPlaceholderPIDReservationFor(procBase string, hostPID int, timeout time.Duration, log *logrus.Entry) (int, error) { + deadline := time.Now().Add(timeout) + var lastSeen int + var lastErr error + warned := false + + for { + maxPID, err := placeholderMaxNSPID(procBase, hostPID) + if err == nil { + lastSeen = maxPID + if maxPID >= reservedPIDFloor { + return maxPID, nil + } + } else { + lastErr = err + } + + if time.Now().After(deadline) { + break + } + if !warned { + // One line, not one per poll: this is the normal startup window. + log.WithFields(logrus.Fields{"maxNSPID": lastSeen, "want": reservedPIDFloor}). + Info("criu-v2: waiting for the placeholder to reserve its pid range") + warned = true + } + time.Sleep(500 * time.Millisecond) + } + + if lastSeen == 0 && lastErr != nil { + return 0, fmt.Errorf("criu-v2: could not read the placeholder's pid namespace "+ + "to verify its pid range was reserved: %w", lastErr) + } + return lastSeen, fmt.Errorf( + "criu-v2: placeholder never reserved its pid range (highest pid %d < %d after %s): "+ + "the ns_last_pid bump is missing or failed, and CRIU's exact-pid forks would "+ + "collide with this pod's own processes", lastSeen, reservedPIDFloor, timeout) +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go b/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go index afc312894..a2b8d0e96 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go @@ -18,10 +18,15 @@ limitations under the License. package agent import ( + "io" "os" "path/filepath" "strconv" + "strings" "testing" + "time" + + "github.com/sirupsen/logrus" ) // fakeProc builds a procfs stand-in. Each entry is hostPID -> (nsLink, NSpid @@ -166,3 +171,57 @@ func TestNSPIDOfUsesInnermostNamespace(t *testing.T) { t.Errorf("nsPIDOf = %d, want 3 (innermost)", got) } } + +// The wait exists because the pod reports Running before its shell has finished +// sourcing profile.d, so the reservation lands seconds later. Sampling once +// rejected placeholders that were about to be fine -- observed against +// nim-llama-8b before the wait was added. +func TestAwaitPlaceholderPIDReservation(t *testing.T) { + log := logrus.NewEntry(logrus.New()) + log.Logger.SetOutput(io.Discard) + + t.Run("returns as soon as the reservation is visible", func(t *testing.T) { + base := fakeProc(t, map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "100002"}, + }) + got, err := awaitPlaceholderPIDReservationFor(base, 5000, 5*time.Second, log) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != 100002 { + t.Errorf("got %d, want 100002", got) + } + }) + + t.Run("errors when the reservation never lands", func(t *testing.T) { + base := fakeProc(t, map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "363"}, + }) + start := time.Now() + got, err := awaitPlaceholderPIDReservationFor(base, 5000, 1200*time.Millisecond, log) + if err == nil { + t.Fatalf("expected an error, got max=%d", got) + } + // The highest pid seen belongs in the message: it is what tells an + // operator the bump did not run, rather than that it ran and was low. + if !strings.Contains(err.Error(), "363") { + t.Errorf("error should report the highest pid seen, got: %v", err) + } + if elapsed := time.Since(start); elapsed < 1200*time.Millisecond { + t.Errorf("returned after %s, should have waited the full timeout", elapsed) + } + }) + + t.Run("does not fail the restore when the namespace cannot be read", func(t *testing.T) { + // A procfs read error is ambiguous, not proof of a missing reservation. + _, err := awaitPlaceholderPIDReservationFor(t.TempDir(), 5000, 600*time.Millisecond, log) + if err == nil { + t.Fatal("expected an error describing the unreadable namespace") + } + if !strings.Contains(err.Error(), "could not read") { + t.Errorf("want a read-failure message distinct from a missing reservation, got: %v", err) + } + }) +} From b3f9ed72827273a9e72f24e1b34d6f3f3d0c50f9 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 18:56:07 -0700 Subject: [PATCH 8/9] docs(nvsnap): plan for validating checkpoint/restore of Dynamo workloads 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 --- .../docs/proposals/dynamo-cr-validation.md | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md diff --git a/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md new file mode 100644 index 000000000..84118c645 --- /dev/null +++ b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md @@ -0,0 +1,172 @@ + + +# Validating checkpoint/restore of a Dynamo workload + +Status: proposed. No rung has been run. + +How we establish whether nvsnap can snapshot and restore Dynamo workers, in +aggregated and disaggregated mode, without producing a result that looks green +and means nothing. + +## The trap that shapes the whole design + +Dynamo is built to survive worker loss. The router re-routes, workers +re-register. So a restore that fails completely can still produce a successful +inference request, served by a different worker, and a naive test passes. + +This is the same failure we have already been caught by: a restore test that +silently measured a cold start, because a pod the webhook declined still starts, +still serves, and still passes every functional check. Here the masking is +stronger, because the system is designed to hide exactly this. + +Two rules follow, and no result counts without both: + +1. Run exactly one worker of the type under test, so there is nothing to route + around. +2. Assert the request was served by the restored worker specifically, by worker + identity, not that a request succeeded. + +## Preconditions + +- Dynamo platform installed by Helm per `tools/ncp-local-cluster/docs/dynamo-operator.md` + (`dynamo-platform`, namespace `dynamo-system`). KAI Scheduler is already an + NVCF cluster prerequisite; Grove is the likely new dependency. +- The sample topology at + `examples/function-samples/helmchart-samples/dynamo-operator-sample` deploys + unchanged and serves inference. Record what healthy looks like before + attempting any capture. +- Agent build and chart values recorded for every run. A result attributed to + the wrong build is worse than no result. + +## Established, and what is still assumed + +Established from source: + +- Component-level `annotations` on a `DynamoGraphDeployment` reach pod metadata. + The operator merges them in `GetDCDKubeAnnotations` + (`deploy/operator/internal/dynamo/v1beta1_helpers.go`), whose comments state + the destination is generated pod metadata. So `nvsnap.io/restore-from` on a + component will reach the pod and trigger our webhook. +- The sample's prefill worker publishes KV events over ZMQ and transfers KV via + the NIXL connector. NIXL stages transfer metadata through the pod's + `/dev/shm`, which nvsnap already captures and replays. +- CRIU cannot dump processes using RDMA (checkpoint-restore/criu#267). If NIXL + is on an RDMA transport, process capture of that worker is not possible. + +Still assumed, and each is a gate rather than a detail: + +- That a Dynamo worker rejoins cleanly after an abrupt restart. If it does, we + can drop NIXL, ZMQ and coordination state at capture and let it re-establish, + as we already do for external TCP. If it does not, rungs 2 and beyond need a + different design. Answer this from the Dynamo source before designing rung 2. +- That a worker can reach ready as a standalone pod against a shared etcd and + NATS, rather than requiring operator launch and Grove gang membership. This + decides whether rungs 1 and 2 can use plain pods or must drive the CRD. +- That restoring in place into an operator-owned pod does not trip + reconciliation. + +## Why restore must happen in place + +Our test harness deletes the source pod and creates its own placeholder. Against +an operator-managed workload that is wrong: the operator sees its worker missing +and creates a fresh cold one, leaving two pods with the router likely favouring +the operator's. The test then passes while proving nothing. + +Use the production path instead. Annotate the component so the operator's own +pod carries `nvsnap.io/restore-from`; the webhook rewrites that container's +command to `restore-entrypoint` and stashes the original in +`NVSNAP_ORIG_COMMAND`; the agent restores into the pod in place. Nothing is +deleted, so the operator has nothing to reconcile. + +This also means rungs 4 and 5 exercise the production path rather than a +test-only convention, which is worth more than the convenience of the harness. + +## Instrumentation required before rung 1 + +None of the rungs are meaningful without a way to answer "which worker served +this request". Establish one and verify it against a healthy deployment first. +Candidates, in order of preference: + +1. Worker instance identity from Dynamo's coordination state (etcd), correlated + with the pod that served the request. +2. A response header or field naming the worker. +3. Worker logs, correlated by request id. + +If none of these can distinguish workers, the whole plan is unsound and must be +reworked before any capture is attempted. + +## Ladder + +Each rung is a stop-and-decide point. Each is run repeatedly, not once: the +restore failures we have already debugged were probabilistic, and a single pass +cannot distinguish working from lucky. + +### Rung 0: baseline + +Deploy the sample unchanged. Record cold-start time to first token, worker +identities, and healthy coordination state. Everything later is compared to +this. + +### Rung 1: aggregated, TP=1, cache-directory capture + +Weights and compiled kernels only, no process state. Lowest risk and it proves +the plumbing end to end. + +Pass: the restored worker serves a correct response, identified as the restored +worker, with a measurable improvement over rung 0's cold start. + +### Rung 2: aggregated, TP=1, criu-v2 process capture + +The first real test. Expected work: coordination re-registration, the ZMQ KV +events publisher, and NIXL agent metadata in `/dev/shm`. + +Pass: as rung 1, plus the restored process is the captured process, not a cold +start wearing its name. Verify by process start time or a capture-time marker, +not by readiness. + +### Rung 3: aggregated, TP above 1 + +Expected to fail. Multi-GPU is blocked by peer state: `cuda-checkpoint +--launch-job` addresses CUDA IPC and needs driver 610, while NCCL communicators +and CUDA graphs holding `ncclComm_t` are unsolved upstream. Run it to confirm +the failure mode matches that prediction rather than something else. + +Pass: the failure is the predicted one. A different failure is a finding. + +### Rung 4: disaggregated, decode worker only + +Prefill left live, decode captured and restored in place. + +Pass: the restored decode worker re-registers, and a request requiring a +prefill-to-decode KV transfer completes through it, verified by worker identity. + +### Rung 5: disaggregated, both workers + +A genuine distributed snapshot. In-flight KV transfers now matter, and there is +no multi-pod capture primitive in nvsnap: each pod is captured independently +with no consistency guarantee across them. + +Do not design this rung until rung 4 has run. Its result determines whether +coordinated capture is needed at all. + +## Checks that must pass at every rung + +- The restored worker appears in coordination state, not merely Running in + Kubernetes. +- A request is served by the restored worker, by identity. +- Output is correct, not merely present. +- Rungs 4 and 5: a prefill-to-decode transfer completes end to end. +- Timings compared against rung 0 on the same hardware. +- The run is repeated. Report the rate, not the best result. + +## What we will not conclude + +- That a rung passes because a request succeeded. See the trap above. +- That multi-GPU is blocked only by driver version. The NCCL and CUDA-graph + layers are unsolved independently of the driver. +- That disaggregated works because rung 4 passed. Rung 4 restores one worker + into a live cluster; rung 5 is a different problem. +- Anything from a single run. From 4f8aae382bf878da7b9c70b09fc1ba29da593925 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 19:04:15 -0700 Subject: [PATCH 9/9] docs(nvsnap): ground the Dynamo validation plan in the operator's own 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 --- .../docs/proposals/dynamo-cr-validation.md | 108 +++++++++++++----- 1 file changed, 78 insertions(+), 30 deletions(-) diff --git a/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md index 84118c645..ea02ef655 100644 --- a/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md +++ b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md @@ -41,32 +41,77 @@ Two rules follow, and no result counts without both: - Agent build and chart values recorded for every run. A result attributed to the wrong build is worse than no result. -## Established, and what is still assumed +## Established from source -Established from source: +Each of these was read from the Dynamo operator and runtime, not assumed. + +### Worker identity is addressable per request + +`lib/llm/src/protocols/common/extensions.rs` maps request headers onto routing +extensions: + + x-dynamo-worker-instance-id -> backend_instance_id, decode_worker_id + x-dynamo-prefill-instance-id -> prefill_worker_id + +This is stronger than being able to observe which worker served a request. It +lets us pin a request to a specific instance, so a request aimed at a restored +worker either succeeds through that worker or fails. It cannot be quietly +served by a healthy peer. + +That defeats the masking problem directly, and it is the single most important +finding for this plan. Every rung below uses pinning rather than post-hoc +attribution. + +### Discovery on Kubernetes is readiness-driven, not lease-driven + +The operator sets `DYN_DISCOVERY_BACKEND=kubernetes` for pods, so the etcd lease +path (10 second TTL, endpoints deleted on expiry) does not apply to our +deployments. Instead, per the operator's service-discovery documentation: + +- Each pod runs a discovery daemon watching EndpointSlices and + `DynamoWorkerMetadata` CRs. +- A pod is discoverable only when it is ready in an EndpointSlice AND has a + corresponding CR. +- The CR is named after the pod and carries an owner reference, so it is + garbage collected when the pod is deleted. +- Readiness for a worker means its `generate` endpoint is healthy. + +The consequences for checkpoint/restore are favourable and specific: + +- Restore in place keeps the pod, so its CR survives and no re-registration is + required. +- A frozen process fails its readiness probe, leaves the EndpointSlice, and + traffic reroutes. That is orderly rather than an error path. +- On restore the probe passes again and the worker returns to the EndpointSlice. + +So the recovery mechanism we depend on already exists and is the same one +Dynamo uses for ordinary pod churn. Deleting the pod, by contrast, destroys the +CR by garbage collection, which is a second reason not to use our harness's +delete-and-replace model. + +### Transport and capture constraints -- Component-level `annotations` on a `DynamoGraphDeployment` reach pod metadata. - The operator merges them in `GetDCDKubeAnnotations` - (`deploy/operator/internal/dynamo/v1beta1_helpers.go`), whose comments state - the destination is generated pod metadata. So `nvsnap.io/restore-from` on a - component will reach the pod and trigger our webhook. - The sample's prefill worker publishes KV events over ZMQ and transfers KV via the NIXL connector. NIXL stages transfer metadata through the pod's `/dev/shm`, which nvsnap already captures and replays. -- CRIU cannot dump processes using RDMA (checkpoint-restore/criu#267). If NIXL - is on an RDMA transport, process capture of that worker is not possible. - -Still assumed, and each is a gate rather than a detail: - -- That a Dynamo worker rejoins cleanly after an abrupt restart. If it does, we - can drop NIXL, ZMQ and coordination state at capture and let it re-establish, - as we already do for external TCP. If it does not, rungs 2 and beyond need a - different design. Answer this from the Dynamo source before designing rung 2. -- That a worker can reach ready as a standalone pod against a shared etcd and - NATS, rather than requiring operator launch and Grove gang membership. This - decides whether rungs 1 and 2 can use plain pods or must drive the CRD. +- CRIU cannot dump processes using RDMA (checkpoint-restore/criu#267). NIXL runs + over UCX, which selects a transport at runtime, so whether a worker is + capturable at all depends on what UCX picks on the target hardware. This is a + runtime check, not a source question, and it must be answered at rung 0. + +## Still assumed + +- That a worker can reach ready as a standalone pod rather than requiring + operator launch and Grove gang membership. `DYN_DISCOVERY_BACKEND` is + configurable (kubernetes, etcd, memory, nats, file), so a standalone worker + against a memory or etcd backend is plausible, but unproven. This decides + whether rungs 1 and 2 can use plain pods or must drive the CRD. - That restoring in place into an operator-owned pod does not trip - reconciliation. + reconciliation. The discovery mechanism above suggests it should not, since + nothing is deleted, but the operator may still react to a pod going + NotReady for the duration of a capture. +- That a restored process re-establishes its ZMQ KV-events publisher and NIXL + agent state. Discovery recovering does not imply these do. ## Why restore must happen in place @@ -84,19 +129,22 @@ deleted, so the operator has nothing to reconcile. This also means rungs 4 and 5 exercise the production path rather than a test-only convention, which is worth more than the convenience of the harness. -## Instrumentation required before rung 1 +## Instrumentation -None of the rungs are meaningful without a way to answer "which worker served -this request". Establish one and verify it against a healthy deployment first. -Candidates, in order of preference: +Use request pinning, established above. For each rung: -1. Worker instance identity from Dynamo's coordination state (etcd), correlated - with the pod that served the request. -2. A response header or field naming the worker. -3. Worker logs, correlated by request id. +- Send the verification request with `x-dynamo-worker-instance-id` set to the + restored worker's instance id (and `x-dynamo-prefill-instance-id` for + disaggregated rungs). +- A pinned request that succeeds proves the restored worker served it. A pinned + request that fails is a real failure rather than a reroute. +- Record the instance id before capture and confirm it after restore. An + instance id that changed is itself a finding: it means the worker + re-registered as a new instance rather than resuming. -If none of these can distinguish workers, the whole plan is unsound and must be -reworked before any capture is attempted. +Verify pinning works against a healthy deployment at rung 0, before any capture. +If a pinned request can still be served by another worker, this plan's central +assumption is wrong and the design must change. ## Ladder