From a382620879519ba16ca46328157ffe76896fe937 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 13:02:17 -0700 Subject: [PATCH 01/11] test(nvsnap): refuse to measure a cold start as a restore The restore step never checked that the pod it launched was admitted as a restore. When it was not, the pod cold-started, served normally, passed every downstream check, and the harness printed restore timings that were really cold-start timings. Rootfs/cachedir restore manifests carry nvsnap.io/restore-from: "__CAPTURE_HASH__". Applying such a template without substituting leaves the webhook nothing to resolve, so it injects no cache mount and no cache env, and the workload fetches its model again. A 70B TP=4 "restore" measured this way spent 8m47s downloading weights; the pod had no /opt/nvsnap mount and HF_HOME still pointed at /root/.cache/huggingface. The number looked plausible and was used to reason about restore performance before the mistake surfaced. Add two guards: - refuse to apply a restore manifest that still contains __PLACEHOLDER__ - after creating the restore pod, assert the captured cache is mounted and the cache env points into it; fail loudly when it is not Both were checked against the saved spec of the pod that produced the bad number, and both fire on it. A restore benchmark that silently degrades to a cold start is worse than one that fails, because the result is quotable. Closes #964 Co-Authored-By: Balaji Ganesan --- .../nvsnap/scripts/test-e2e.sh | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh index d343134b64..4e8f31a6a2 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh @@ -793,6 +793,18 @@ else "$RESTORE_MANIFEST_TEMPLATE" > "$RESTORE_MANIFEST" fi +# A template placeholder that survives substitution is not a cosmetic problem. +# nvsnap.io/restore-from: "__CAPTURE_HASH__" gives the webhook nothing to +# resolve, so it injects no storage and no cache env -- and the pod cold-starts +# while looking exactly like a slow restore, model download included. Every +# timing measured from that point is a cold-start number wearing a restore +# label. Refuse to launch instead. +if grep -nE '__[A-Z_]+__' "$RESTORE_MANIFEST"; then + log_error "Unsubstituted placeholder(s) above in $RESTORE_MANIFEST." + log_error "The webhook would ignore this pod and it would COLD START, not restore." + exit 1 +fi + # Phase 5d: restore goes back to the simple hostPath mount on the # capture-source node (or the agent's EnsureLocal cascade materializes # it locally on a different target node before the placeholder reads @@ -830,6 +842,54 @@ if [ "$CAPTURE_PATH" = "criu-v2" ]; then log_info "criu-v2: agent restore returned: $RESTORE_RESP" fi +# Prove the webhook admitted this pod AS A RESTORE before timing anything. +# +# A pod the webhook declined still starts, still serves, and still passes every +# check below -- it just cold-starts, downloading the model again. The timings +# then describe a cold start wearing a restore label, and nothing in the run +# says so. (Measured: a 70B "restore" that spent 8m47s downloading weights, +# because the pod carried no injected cache at all.) +# +# The observable signature of a real restore on the rootfs/cachedir path is the +# injected cache mount plus a cache env that points into it. +if [ "$CAPTURE_PATH" = "rootfs" ]; then + log_info "Verifying the restore pod was admitted as a restore..." + for _ in $(seq 1 30); do + kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" -o json >/tmp/nvsnap-restore-pod.json 2>/dev/null && break + sleep 2 + done + if ! python3 - /tmp/nvsnap-restore-pod.json "$RESTORE_CONTAINER_NAME" <<'PY' +import json, sys +pod = json.load(open(sys.argv[1])) +want = sys.argv[2] +containers = pod["spec"]["containers"] +c = next((x for x in containers if x["name"] == want), containers[0]) +env = {e["name"]: e.get("value", "") for e in (c.get("env") or [])} +mounts = [m["mountPath"] for m in (c.get("volumeMounts") or [])] + +problems = [] +if not [m for m in mounts if m.startswith("/opt/nvsnap")]: + problems.append(f"no captured-cache mount injected (mounts: {mounts})") +for var in ("HF_HOME", "NIM_CACHE_PATH"): + val = env.get(var) + if val and not val.startswith("/opt/nvsnap"): + problems.append(f"{var}={val!r} points outside the restored cache") +if problems: + for p in problems: + print(f" {p}", file=sys.stderr) + sys.exit(1) +PY + then + log_error "Restore pod was NOT decorated by the webhook — it will COLD START." + log_error "Any timing from this run would be a cold start labelled as a restore." + kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" \ + -o jsonpath='{.metadata.annotations.nvsnap\.io/restore-from}{"\n"}' 2>/dev/null \ + | sed 's/^/ restore-from: /' + fail "Restore pod not admitted as a restore" + fi + log_info " verified: captured cache is mounted and the cache env points into it" +fi + log_info "Waiting for restore pod ready (up to ${RESTORE_READY_TIMEOUT}s)..." log_info " (readiness probe polls /v1/models — succeeds only when serving)" if kubectl wait --for=condition=ready pod/$RESTORE_POD_NAME -n $NAMESPACE --timeout=${RESTORE_READY_TIMEOUT}s; then From 69cc273819241907658c3c2947bd4d5410710554 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 13:07:49 -0700 Subject: [PATCH 02/11] fix(nvsnap): declare cachedir, not rootfs, on every multi-GPU manifest Four manifests declared nvsnap.io/path: "rootfs" while the agent, running with cachedir mode on, captured only the pod's cache mount. The label described a path that was not running. That mismatch is not cosmetic. Reading the annotation is the natural way to answer "which path did this use", and it gives the wrong answer, so analysis built on it is wrong from the start -- including a benchmark comparison this week that attributed a difference to capture path when both runs used the same one. Switch nim-qwen3-32b, vllm-tp2, vllm-70b and gpt-oss-120b to "cachedir", add that value alongside criu, and mark rootfs deprecated: no workload uses it. Also correct the descriptions that advertised whole-rootfs behaviour -- vllm-70b claimed to capture the overlay upperdir, which cachedir does not do. The annotation feeds the demo catalog and the criu conformance check; neither switches on "rootfs", so adding a value is safe. Co-Authored-By: Balaji Ganesan --- .../k8s/benchmarks/gpt-oss-120b-restore.yaml | 2 +- .../deploy/k8s/benchmarks/gpt-oss-120b.yaml | 6 +++--- .../deploy/k8s/workloads/nim-qwen3-32b.yaml | 6 +++--- .../nvsnap/deploy/k8s/workloads/vllm-70b.yaml | 12 ++++++------ .../nvsnap/deploy/k8s/workloads/vllm-tp2.yaml | 2 +- .../nvsnap/internal/server/manifests.go | 15 ++++++++++++++- 6 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b-restore.yaml index d15d614ee7..75e4427f18 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b-restore.yaml @@ -1,7 +1,7 @@ # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -# Restore pod for PDF-bench gpt-oss-120b (TP=4, rootfs path). +# Restore pod for PDF-bench gpt-oss-120b (TP=4, cachedir path). # # Customer-shape: no CRIU init ladder, no nodeName pin. The nvsnap # webhook injects PVC mounts and (for Local backend) nodeAffinity diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b.yaml index d4cb1120ac..6c3a462bf8 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gpt-oss-120b.yaml @@ -20,7 +20,7 @@ # --no-enable-prefix-caching # Hardware : 4x H100 (one node, TP=4) # -# Multi-GPU → rootfs capture path per CLAUDE.md rule 20. No CRIU init +# Multi-GPU → cachedir capture path per CLAUDE.md rule 20. No CRIU init # container ladder; just the workload + nvsnap.io/capture=true label. # The agent's rootfsonly.Watcher snapshots the overlay upperdir after # the readiness probe passes + 60s warmup. @@ -33,10 +33,10 @@ metadata: app: bench-gpt-oss-120b nvsnap.io/bench: "pdf-matrix" nvsnap.io/bench-row: "llm-medium" - nvsnap.io/capture: "true" # opt-in to rootfs capture (multi-GPU only) + nvsnap.io/capture: "true" # opt-in to cachedir capture (multi-GPU only) annotations: nvsnap.io/desc: "PDF bench: openai/gpt-oss-120b TP=4 on vllm:v0.20.0" - nvsnap.io/path: "rootfs" + nvsnap.io/path: "cachedir" spec: tolerations: - key: "nvidia.com/gpu" diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-qwen3-32b.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-qwen3-32b.yaml index aad2801305..7fccccc95f 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-qwen3-32b.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-qwen3-32b.yaml @@ -15,14 +15,14 @@ metadata: labels: app: nim-qwen3-32b nvsnap.io/demo: "true" - nvsnap.io/capture: "true" # opt-in to rootfs capture (multi-GPU only) + nvsnap.io/capture: "true" # opt-in to cachedir capture (multi-GPU only) annotations: nvsnap.io/demo-name: "NIM" - nvsnap.io/desc: "Qwen3-32B multi-GPU TP=2 (rootfs)" + nvsnap.io/desc: "Qwen3-32B multi-GPU TP=2 (cachedir)" nvsnap.io/model: "qwen/qwen3-32b" nvsnap.io/port: "8000" nvsnap.io/gpus: "2" - nvsnap.io/path: "rootfs" + nvsnap.io/path: "cachedir" nvsnap.io/ckpt-size: "61 GB" spec: tolerations: diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-70b.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-70b.yaml index d769f074bf..646df7b2c6 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-70b.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-70b.yaml @@ -6,9 +6,9 @@ # nvsnap agent's rootfs watcher captures this pod after the warmup window. # # Customer-shape minimal yaml — no CRIU init container ladder. The -# rootfs path captures the pod's overlay upperdir + Hugging Face cache -# + compiled engine caches and replays them via webhook-injected bind -# mounts on the corresponding `vllm-70b-fresh` pod. +# cachedir path captures ONLY the pod's cache mount (Hugging Face model +# + compiled engine caches), not the whole container rootfs, and replays +# it via webhook-injected mounts on the corresponding restore pod. apiVersion: v1 kind: Pod metadata: @@ -17,14 +17,14 @@ metadata: labels: app: vllm-70b nvsnap.io/demo: "true" - nvsnap.io/capture: "true" # opt-in to rootfs capture (multi-GPU only) + nvsnap.io/capture: "true" # opt-in to cachedir capture (multi-GPU only) annotations: nvsnap.io/demo-name: "vLLM" - nvsnap.io/desc: "Llama-3.1-70B multi-GPU tensor-parallel (TP=4, rootfs)" + nvsnap.io/desc: "Llama-3.1-70B multi-GPU tensor-parallel (TP=4, cachedir)" nvsnap.io/model: "meta-llama/Llama-3.1-70B-Instruct" nvsnap.io/port: "8000" nvsnap.io/gpus: "4" - nvsnap.io/path: "rootfs" + nvsnap.io/path: "cachedir" nvsnap.io/ckpt-size: "132 GB" spec: tolerations: diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml index 7f44d325ea..99b5589c38 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml @@ -33,7 +33,7 @@ metadata: # anything requesting >= 2 GPUs here. Declaring "criu" made the manifest # generator emit a criu-v2 restore placeholder that nothing ever drives, # so the restore pod idled until the readiness timeout. - nvsnap.io/path: "rootfs" + nvsnap.io/path: "cachedir" spec: automountServiceAccountToken: false tolerations: diff --git a/src/compute-plane-services/nvsnap/internal/server/manifests.go b/src/compute-plane-services/nvsnap/internal/server/manifests.go index 1785fbc834..29024ba7e3 100644 --- a/src/compute-plane-services/nvsnap/internal/server/manifests.go +++ b/src/compute-plane-services/nvsnap/internal/server/manifests.go @@ -84,7 +84,20 @@ type CapturePath string // Capture path identifiers. const ( - CapturePathCRIU CapturePath = "criu" + CapturePathCRIU CapturePath = "criu" + + // CapturePathCacheDir captures only the pod's cache mount (model + + // compile caches) rather than the whole container rootfs. This is what + // multi-GPU workloads use. + CapturePathCacheDir CapturePath = "cachedir" + + // CapturePathRootfs captures the whole container rootfs. + // + // Deprecated: not used by any workload. It was the original multi-GPU + // path, and manifests kept declaring "rootfs" long after the agent's + // cachedir mode meant they were really capturing only the cache dir -- + // so the label described a path that was not running. That mismatch + // cost real debugging time. Kept only so an older manifest still parses. CapturePathRootfs CapturePath = "rootfs" ) From acf41fe67cd373054aeb83fdbb8de6ca4759a74f Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 13:10:58 -0700 Subject: [PATCH 03/11] fix(nvsnap): refuse whole-rootfs capture unless explicitly allowed Capture ran whole-rootfs whenever --pod-cache-dir was unset. That path succeeds quietly: it produces a capture, restores work, and nothing looks wrong -- so a cluster that lost its cachedir setting keeps running while diverging from every workload and benchmark that assumes cachedir. The difference only surfaces later as restores that behave unlike the ones that were measured, which reads as a performance mystery rather than a misconfiguration. Refuse at startup instead, where an operator sees it, and say how to fix it: set --pod-cache-dir, or pass --allow-whole-rootfs to run that path deliberately. The override exists so this is a guard rather than a removal; a deployment that genuinely needs whole-rootfs is not blocked from a code change. Tests cover all three states: refusal without the flag, the opt-in getting past the guard, and capture-disabled staying a clean no-op so the guard cannot turn "no capture" into a startup failure. Co-Authored-By: Balaji Ganesan --- .../nvsnap/cmd/agent/main.go | 2 + .../nvsnap/internal/agent/BUILD.bazel | 1 + .../agent/rootfs_wholerootfs_guard_test.go | 67 +++++++++++++++++++ .../internal/agent/rootfsonly_integration.go | 23 +++++++ 4 files changed, 93 insertions(+) create mode 100644 src/compute-plane-services/nvsnap/internal/agent/rootfs_wholerootfs_guard_test.go diff --git a/src/compute-plane-services/nvsnap/cmd/agent/main.go b/src/compute-plane-services/nvsnap/cmd/agent/main.go index 55b85a5872..86317c0212 100644 --- a/src/compute-plane-services/nvsnap/cmd/agent/main.go +++ b/src/compute-plane-services/nvsnap/cmd/agent/main.go @@ -116,6 +116,8 @@ func main() { "Directory the Local backend writes captures into") flag.StringVar(&config.RootfsCapture.PodCacheDir, "pod-cache-dir", os.Getenv("NVSNAP_POD_CACHE_DIR"), "In-pod cache mount path (e.g. /opt/nvsnap) for cachedir mode: capture ONLY this dir as the PVC root, restore RO-mounts the rox here (no overlayfs). Empty = standard whole-rootfs capture. Must match the webhook's cacheDir.") + flag.BoolVar(&config.RootfsCapture.AllowWholeRootfs, "allow-whole-rootfs", os.Getenv("NVSNAP_ALLOW_WHOLE_ROOTFS") == "1", + "Permit capture without --pod-cache-dir, i.e. capture the whole container rootfs. Off by default: whole-rootfs capture succeeds silently and only diverges later, at restore, from the cachedir behaviour every workload and benchmark assumes. Set only to run that path deliberately.") flag.StringVar(&config.RootfsCapture.PodCacheEnvFile, "cachedir-env-file", os.Getenv("NVSNAP_CACHEDIR_ENV_FILE"), "Path to a mounted ConfigMap file with the cachedir env template (NAME=value lines; {root}/{cache}/{model} placeholders). Read on capture inject only — edit the ConfigMap to add/remove cache env vars without an agent rebuild. Empty/unreadable = built-in default. Restore replays the env stamped in the manifest.") flag.StringVar(&config.OverlayRoot, "overlay-root", "/var/lib/nvsnap/overlays", diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index 4fcfefc3e0..c94ea2a02d 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -82,6 +82,7 @@ go_library( go_test( name = "agent_test", srcs = [ + "rootfs_wholerootfs_guard_test.go", "advertise_test.go", "auth_test.go", "blob_uploader_test.go", diff --git a/src/compute-plane-services/nvsnap/internal/agent/rootfs_wholerootfs_guard_test.go b/src/compute-plane-services/nvsnap/internal/agent/rootfs_wholerootfs_guard_test.go new file mode 100644 index 0000000000..4078439646 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/rootfs_wholerootfs_guard_test.go @@ -0,0 +1,67 @@ +/* +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 ( + "context" + "strings" + "testing" +) + +// Whole-rootfs capture must not start by accident. It succeeds quietly -- +// producing a capture that restores -- so a cluster whose cachedir setting was +// dropped keeps running while diverging from every workload and benchmark that +// assumes cachedir. The agent refuses at startup instead. +func TestStartRootfsCaptureRefusesWholeRootfs(t *testing.T) { + a := &Agent{} + _, err := a.startRootfsCapture(context.Background(), RootfsCaptureConfig{ + Enabled: true, // no PodCacheDir, no override + }) + if err == nil { + t.Fatal("expected refusal when --pod-cache-dir is unset; whole-rootfs must be opt-in") + } + for _, want := range []string{"pod-cache-dir", "allow-whole-rootfs"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error should tell the operator about %q, got: %v", want, err) + } + } +} + +// The override exists so an operator can still run that path deliberately. +// It must get past the guard -- failing later for an unrelated reason (no +// kube client in a unit test) is fine; failing *at the guard* is not. +func TestStartRootfsCaptureAllowsExplicitOptIn(t *testing.T) { + a := &Agent{} + _, err := a.startRootfsCapture(context.Background(), RootfsCaptureConfig{ + Enabled: true, + AllowWholeRootfs: true, + }) + if err != nil && strings.Contains(err.Error(), "whole-rootfs capture is not supported") { + t.Fatalf("explicit opt-in must pass the guard, got: %v", err) + } +} + +// Disabled stays a clean no-op: the guard must not turn "capture off" into an +// error for every agent that does not run capture at all. +func TestStartRootfsCaptureDisabledIsNoop(t *testing.T) { + a := &Agent{} + b, err := a.startRootfsCapture(context.Background(), RootfsCaptureConfig{Enabled: false}) + if err != nil || b != nil { + t.Fatalf("disabled capture should be a no-op, got backend=%v err=%v", b, err) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration.go b/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration.go index b2800d7db7..cb92f32e37 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration.go +++ b/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration.go @@ -57,6 +57,16 @@ type RootfsCaptureConfig struct { // replays the env stamped into the manifest. PodCacheEnvFile string + // AllowWholeRootfs permits capture to run with PodCacheDir empty, i.e. + // capturing the whole container rootfs instead of just the cache mount. + // + // Off by default, and the agent refuses to start capture without it, + // because the failure mode is silent: whole-rootfs looks like a working + // capture, and the difference only surfaces later as restores that + // behave unlike the ones that were benchmarked. Requiring an explicit + // opt-in makes running it a decision rather than an oversight. + AllowWholeRootfs bool + // CMNamespace is the K8s namespace ConfigMaps are written to so // any node's webhook can resolve a hash. Default "nvsnap-system". CMNamespace string @@ -84,6 +94,19 @@ func (a *Agent) startRootfsCapture(ctx context.Context, cfg RootfsCaptureConfig) if !cfg.Enabled { return nil, nil } + // Refuse whole-rootfs capture unless explicitly allowed. Capturing the + // entire container rootfs still "works" -- it produces a capture, restores + // succeed, and nothing looks wrong -- so a cluster that lost its cachedir + // setting would keep running and silently diverge from every workload and + // benchmark that assumes cachedir. Fail at startup, where an operator sees + // it, rather than at restore time, where it looks like a performance + // mystery. + if cfg.PodCacheDir == "" && !cfg.AllowWholeRootfs { + return nil, fmt.Errorf("rootfs capture is enabled without --pod-cache-dir: " + + "whole-rootfs capture is not supported for normal use. Set --pod-cache-dir " + + "(e.g. /opt/nvsnap) to capture the cache mount, or pass --allow-whole-rootfs " + + "to override deliberately") + } if cfg.CacheDir == "" { cfg.CacheDir = "/var/lib/nvsnap/cache" } From a55553211e048197bfdcd36b86e957cb3cfa0b31 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 13:44:49 -0700 Subject: [PATCH 04/11] fix(nvsnap): harden the restore-verification guard Review found the guard could pass while the workload cold-started. It fell back to containers[0] when the named container was absent, so a decorated sidecar could vouch for a workload that was not restoring. It prefix-matched the cache path, so /opt/nvsnap-other satisfied a check for /opt/nvsnap. It treated missing cache env as acceptable, though a pod that inherited none of the stamped env is not restoring from anything. And it hardcoded /opt/nvsnap rather than reading the agent's configured --pod-cache-dir, so it asserted a default instead of the cluster's setup. Now: require the exact named container, require the configured cache dir to be mounted, require at least one cache variable, and accept a cache path only when it equals that dir or lies beneath it. Also use mktemp with an exit trap instead of a fixed /tmp path, which was open to a symlink swap, and drop a non-ASCII character from a log line. TestStartRootfsCapture_EnabledFailsWithoutKubeConfig was passing on the new whole-rootfs guard rather than the kube client it is named for. Give it a PodCacheDir so it reaches the kube client again, and assert it did not stop at the guard, so the coverage cannot vanish silently a second time. Verified against the saved spec of the pod that produced the bad measurement: still aborts, now for both the missing mount and the env pointing outside the cache dir. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/BUILD.bazel | 2 +- .../agent/rootfsonly_integration_test.go | 12 +++- .../nvsnap/scripts/test-e2e.sh | 61 +++++++++++++++---- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index c94ea2a02d..48881a4dca 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -82,7 +82,6 @@ go_library( go_test( name = "agent_test", srcs = [ - "rootfs_wholerootfs_guard_test.go", "advertise_test.go", "auth_test.go", "blob_uploader_test.go", @@ -108,6 +107,7 @@ go_test( "restoreoverlay_integration_test.go", "restoreoverlay_test.go", "rootfs_diff_test.go", + "rootfs_wholerootfs_guard_test.go", "rootfsonly_integration_test.go", ], embed = [":agent"], diff --git a/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go b/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go index e0e76957c3..188d70e4d4 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go @@ -19,6 +19,7 @@ package agent import ( "context" + "strings" "testing" "github.com/sirupsen/logrus" @@ -38,8 +39,17 @@ func TestStartRootfsCapture_EnabledFailsWithoutKubeConfig(t *testing.T) { t.Setenv("KUBECONFIG", "/nonexistent/kubeconfig") t.Setenv("HOME", t.TempDir()) // hide any ~/.kube/config the test runner has a := &Agent{config: Config{}, log: logrus.New()} - _, err := a.startRootfsCapture(context.Background(), RootfsCaptureConfig{Enabled: true}) + // PodCacheDir is set so the whole-rootfs guard does not answer first. + // Without it this test would still fail -- but on the guard, not on the + // kube client it is named for, and the coverage would be gone silently. + _, err := a.startRootfsCapture(context.Background(), RootfsCaptureConfig{ + Enabled: true, + PodCacheDir: "/opt/nvsnap", + }) if err == nil { t.Fatal("expected kube client construction error when no config available") } + if strings.Contains(err.Error(), "whole-rootfs") { + t.Fatalf("guard fired instead of the kube client path; this test no longer covers what it claims: %v", err) + } } diff --git a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh index 4e8f31a6a2..03d08ee4d5 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh @@ -854,40 +854,75 @@ fi # injected cache mount plus a cache env that points into it. if [ "$CAPTURE_PATH" = "rootfs" ]; then log_info "Verifying the restore pod was admitted as a restore..." + + # The cache path is the agent's, not ours to guess: read the deployed + # --pod-cache-dir so this check follows a cluster that was configured + # differently instead of asserting a hardcoded default. + POD_CACHE_DIR=$(kubectl get ds nvsnap-agent -n nvsnap-system \ + -o jsonpath='{.spec.template.spec.containers[0].args}' 2>/dev/null \ + | tr ',' '\n' | sed -n 's|.*--pod-cache-dir=\([^"]*\).*|\1|p' | head -1) + if [ -z "$POD_CACHE_DIR" ]; then + log_error "Agent has no --pod-cache-dir; cachedir capture is not configured on this cluster." + fail "Restore verification cannot run without a configured cache dir" + fi + + RESTORE_POD_JSON=$(mktemp -t nvsnap-restore-pod.XXXXXX.json) + trap 'rm -f "$RESTORE_POD_JSON"' EXIT for _ in $(seq 1 30); do - kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" -o json >/tmp/nvsnap-restore-pod.json 2>/dev/null && break + kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" -o json >"$RESTORE_POD_JSON" 2>/dev/null && break sleep 2 done - if ! python3 - /tmp/nvsnap-restore-pod.json "$RESTORE_CONTAINER_NAME" <<'PY' -import json, sys -pod = json.load(open(sys.argv[1])) -want = sys.argv[2] + if ! python3 - "$RESTORE_POD_JSON" "$RESTORE_CONTAINER_NAME" "$POD_CACHE_DIR" <<'PY' +import json, sys, posixpath + +pod_json, want, cache_dir = sys.argv[1], sys.argv[2], sys.argv[3].rstrip("/") +pod = json.load(open(pod_json)) containers = pod["spec"]["containers"] -c = next((x for x in containers if x["name"] == want), containers[0]) + +# Fail closed on the container: falling back to containers[0] would let a +# decorated sidecar vouch for a workload that is cold-starting. +c = next((x for x in containers if x["name"] == want), None) +if c is None: + print(f" container {want!r} not found (have: {[x['name'] for x in containers]})", file=sys.stderr) + sys.exit(1) + env = {e["name"]: e.get("value", "") for e in (c.get("env") or [])} -mounts = [m["mountPath"] for m in (c.get("volumeMounts") or [])] +mounts = {m["mountPath"].rstrip("/"): m.get("name", "") for m in (c.get("volumeMounts") or [])} + +def at_or_under(path, root): + # Exact match or a genuine child. Prefix matching alone would accept + # "/opt/nvsnap-other" for root "/opt/nvsnap". + path = path.rstrip("/") + return path == root or path.startswith(root + posixpath.sep) problems = [] -if not [m for m in mounts if m.startswith("/opt/nvsnap")]: - problems.append(f"no captured-cache mount injected (mounts: {mounts})") +if cache_dir not in mounts: + problems.append(f"cache dir {cache_dir} is not mounted (mounts: {sorted(mounts)})") + +# The cache env is what makes the engine reuse the capture instead of +# fetching. Absent counts as a failure: a restore pod that inherited none +# of the stamped env is not restoring from anything. +if not any(v in env for v in ("HF_HOME", "NIM_CACHE_PATH")): + problems.append("no cache env (HF_HOME / NIM_CACHE_PATH) injected") for var in ("HF_HOME", "NIM_CACHE_PATH"): val = env.get(var) - if val and not val.startswith("/opt/nvsnap"): - problems.append(f"{var}={val!r} points outside the restored cache") + if val and not at_or_under(val, cache_dir): + problems.append(f"{var}={val!r} points outside {cache_dir}") + if problems: for p in problems: print(f" {p}", file=sys.stderr) sys.exit(1) PY then - log_error "Restore pod was NOT decorated by the webhook — it will COLD START." + log_error "Restore pod was NOT decorated by the webhook - it will COLD START." log_error "Any timing from this run would be a cold start labelled as a restore." kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" \ -o jsonpath='{.metadata.annotations.nvsnap\.io/restore-from}{"\n"}' 2>/dev/null \ | sed 's/^/ restore-from: /' fail "Restore pod not admitted as a restore" fi - log_info " verified: captured cache is mounted and the cache env points into it" + log_info " verified: $POD_CACHE_DIR is mounted and the cache env points into it" fi log_info "Waiting for restore pod ready (up to ${RESTORE_READY_TIMEOUT}s)..." From 0469cdaa757804f42173363da033aa3d4eadc14f Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 14:00:54 -0700 Subject: [PATCH 05/11] test(nvsnap): share the restore guard with test-bench.sh test-bench.sh had the same blind spot as test-e2e.sh: it substituted the capture hash, applied the manifest, and measured whatever started. A pod the webhook declined cold-starts, serves, and passes every check, so the run produces restore timings for a cold start. It matters more here. test-bench.sh appends its numbers to docs/PDF-BENCH-RESULTS.md, including a "Restore: Model DL" column -- so a cold start does not just mislead the operator, it gets published as a benchmark row and quoted later. Move the checks into scripts/lib/restore-guard.sh and source it from both scripts, so the contract has one implementation rather than two that can drift: - assert_no_placeholders: refuse a manifest still carrying __PLACEHOLDER__ - agent_pod_cache_dir: read the deployed --pod-cache-dir instead of assuming a default - assert_restore_admitted: require the exact named container, the configured cache dir mounted, and cache env pointing at or beneath it Verified against the saved spec of the pod that produced the bad measurement: refuses for the missing mount and for the env pointing outside, and refuses on an unconfigured cache dir or a wrong container name. Co-Authored-By: Balaji Ganesan --- .../nvsnap/scripts/lib/restore-guard.sh | 111 ++++++++++++++++++ .../nvsnap/scripts/test-bench.sh | 10 ++ .../nvsnap/scripts/test-e2e.sh | 70 +---------- 3 files changed, 125 insertions(+), 66 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh diff --git a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh new file mode 100644 index 0000000000..45edbd3b03 --- /dev/null +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh @@ -0,0 +1,111 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Guards that stop a restore test from measuring a cold start. +# +# A pod the webhook declined still starts, still serves, and still passes every +# functional check -- it just fetches its model again. The timings then describe +# a cold start wearing a restore label, and nothing in the run says so. That is +# worse than a failure, because the number is plausible and gets quoted: in +# test-bench.sh it is written straight into the published results table. +# +# Sourced by test-e2e.sh and test-bench.sh so both enforce the same contract. + +# assert_no_placeholders +# +# Restore templates carry nvsnap.io/restore-from: "__CAPTURE_HASH__". If a +# placeholder survives substitution the webhook has nothing to resolve, injects +# nothing, and the pod cold-starts. +assert_no_placeholders() { + local manifest="$1" + if grep -nE '__[A-Z_]+__' "$manifest"; then + echo "ERROR: unsubstituted placeholder(s) above in $manifest" >&2 + echo "ERROR: the webhook would ignore this pod and it would COLD START, not restore" >&2 + return 1 + fi + return 0 +} + +# agent_pod_cache_dir +# +# The cache path is the agent's, not ours to guess. Echoes the deployed +# --pod-cache-dir so callers follow a cluster configured differently instead of +# asserting a hardcoded default. Empty output means cachedir capture is not +# configured, which callers should treat as fatal for a restore test. +agent_pod_cache_dir() { + kubectl get ds nvsnap-agent -n nvsnap-system \ + -o jsonpath='{.spec.template.spec.containers[0].args}' 2>/dev/null \ + | tr ',' '\n' | sed -n 's|.*--pod-cache-dir=\([^"]*\).*|\1|p' | head -1 +} + +# assert_restore_admitted +# +# Proves the webhook decorated the pod as a restore: the configured cache dir is +# mounted, and the cache env points into it. Returns non-zero with the reasons +# on stderr otherwise. +assert_restore_admitted() { + local pod="$1" ns="$2" container="$3" cache_dir="$4" + local json rc + + if [ -z "$cache_dir" ]; then + echo "ERROR: agent has no --pod-cache-dir; cachedir capture is not configured" >&2 + return 1 + fi + + json=$(mktemp -t nvsnap-restore-pod.XXXXXX.json) || return 1 + local i + for i in $(seq 1 30); do + kubectl get pod "$pod" -n "$ns" -o json >"$json" 2>/dev/null && break + sleep 2 + done + + python3 - "$json" "$container" "$cache_dir" <<'PY' +import json, sys, posixpath + +pod_json, want, cache_dir = sys.argv[1], sys.argv[2], sys.argv[3].rstrip("/") +pod = json.load(open(pod_json)) +containers = pod["spec"]["containers"] + +# Fail closed on the container: falling back to containers[0] would let a +# decorated sidecar vouch for a workload that is cold-starting. +c = next((x for x in containers if x["name"] == want), None) +if c is None: + print(f" container {want!r} not found (have: {[x['name'] for x in containers]})", file=sys.stderr) + sys.exit(1) + +env = {e["name"]: e.get("value", "") for e in (c.get("env") or [])} +mounts = {m["mountPath"].rstrip("/") for m in (c.get("volumeMounts") or [])} + +def at_or_under(path, root): + # Exact match or a genuine child. Prefix matching alone would accept + # "/opt/nvsnap-other" for root "/opt/nvsnap". + path = path.rstrip("/") + return path == root or path.startswith(root + posixpath.sep) + +problems = [] +if cache_dir not in mounts: + problems.append(f"cache dir {cache_dir} is not mounted (mounts: {sorted(mounts)})") +# A restore pod that inherited none of the stamped cache env is not restoring +# from anything, so absent counts as a failure rather than "nothing to check". +if not any(v in env for v in ("HF_HOME", "NIM_CACHE_PATH")): + problems.append("no cache env (HF_HOME / NIM_CACHE_PATH) injected") +for var in ("HF_HOME", "NIM_CACHE_PATH"): + val = env.get(var) + if val and not at_or_under(val, cache_dir): + problems.append(f"{var}={val!r} points outside {cache_dir}") + +for p in problems: + print(f" {p}", file=sys.stderr) +sys.exit(1 if problems else 0) +PY + rc=$? + rm -f "$json" + if [ $rc -ne 0 ]; then + echo "ERROR: restore pod was NOT decorated by the webhook - it will COLD START" >&2 + echo "ERROR: any timing from this run would be a cold start labelled as a restore" >&2 + kubectl get pod "$pod" -n "$ns" \ + -o jsonpath='{.metadata.annotations.nvsnap\.io/restore-from}{"\n"}' 2>/dev/null \ + | sed 's/^/ restore-from: /' >&2 + fi + return $rc +} diff --git a/src/compute-plane-services/nvsnap/scripts/test-bench.sh b/src/compute-plane-services/nvsnap/scripts/test-bench.sh index 88c397ea12..2f2e4b1927 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-bench.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-bench.sh @@ -32,6 +32,7 @@ set -euo pipefail SCRIPT_DIR="$(dirname "$(readlink -f "$0")")" PROJECT_ROOT="$(dirname "$SCRIPT_DIR")" +source "$SCRIPT_DIR/lib/restore-guard.sh" source "$SCRIPT_DIR/versions.sh" # ─── Global temp-file cleanup ──────────────────────────────────────────────── @@ -679,7 +680,16 @@ if [ "$SKIP_RESTORE" -eq 0 ] && [ -n "$CHECKPOINT_ID" ]; then -e "s|nodeName: __NODE_NAME__|nodeName: $NODE|" \ "$RESTORE_MANIFEST_TEMPLATE" > "$R_MANIFEST" fi + # A cold start measured here is published into PDF-BENCH-RESULTS.md as a + # restore row, so verify before spending the run rather than after. + assert_no_placeholders "$R_MANIFEST" || exit 1 kubectl apply -f "$R_MANIFEST" >/dev/null + if [ "$CAPTURE_PATH" = "rootfs" ]; then + POD_CACHE_DIR=$(agent_pod_cache_dir) + assert_restore_admitted "$RESTORE_POD_NAME" "$NAMESPACE" \ + "$RESTORE_CONTAINER_NAME" "$POD_CACHE_DIR" || exit 1 + log_info " verified: $POD_CACHE_DIR mounted, cache env points into it" + fi wait_ready "$RESTORE_POD_NAME" "$POD_READY_TIMEOUT" || { log_error "restore pod didn't ready"; exit 1; } verify_infer "$RESTORE_POD_NAME" "$RESTORE_CONTAINER_NAME" || log_warn " post-restore inference probe failed (continuing)" IFS=':' read -r RESTORE_CDL RESTORE_MDL RESTORE_INIT RESTORE_TOTAL <<<"$(measure_phase "$RESTORE_POD_NAME" "$RESTORE_CONTAINER_NAME" restore)" diff --git a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh index 03d08ee4d5..39963eac9f 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh @@ -32,6 +32,7 @@ fi # Verify deployed agent matches expected version source "$SCRIPT_DIR/versions.sh" source "$SCRIPT_DIR/lib/agent-auth.sh" +source "$SCRIPT_DIR/lib/restore-guard.sh" DEPLOYED=$(kubectl get ds nvsnap-agent -n nvsnap-system -o jsonpath='{.spec.template.spec.containers[0].image}' 2>/dev/null) EXPECTED="${NVSNAP_REGISTRY}/nvsnap-agent:${NVSNAP_APP_VERSION}" if [ "$DEPLOYED" != "$EXPECTED" ]; then @@ -854,72 +855,9 @@ fi # injected cache mount plus a cache env that points into it. if [ "$CAPTURE_PATH" = "rootfs" ]; then log_info "Verifying the restore pod was admitted as a restore..." - - # The cache path is the agent's, not ours to guess: read the deployed - # --pod-cache-dir so this check follows a cluster that was configured - # differently instead of asserting a hardcoded default. - POD_CACHE_DIR=$(kubectl get ds nvsnap-agent -n nvsnap-system \ - -o jsonpath='{.spec.template.spec.containers[0].args}' 2>/dev/null \ - | tr ',' '\n' | sed -n 's|.*--pod-cache-dir=\([^"]*\).*|\1|p' | head -1) - if [ -z "$POD_CACHE_DIR" ]; then - log_error "Agent has no --pod-cache-dir; cachedir capture is not configured on this cluster." - fail "Restore verification cannot run without a configured cache dir" - fi - - RESTORE_POD_JSON=$(mktemp -t nvsnap-restore-pod.XXXXXX.json) - trap 'rm -f "$RESTORE_POD_JSON"' EXIT - for _ in $(seq 1 30); do - kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" -o json >"$RESTORE_POD_JSON" 2>/dev/null && break - sleep 2 - done - if ! python3 - "$RESTORE_POD_JSON" "$RESTORE_CONTAINER_NAME" "$POD_CACHE_DIR" <<'PY' -import json, sys, posixpath - -pod_json, want, cache_dir = sys.argv[1], sys.argv[2], sys.argv[3].rstrip("/") -pod = json.load(open(pod_json)) -containers = pod["spec"]["containers"] - -# Fail closed on the container: falling back to containers[0] would let a -# decorated sidecar vouch for a workload that is cold-starting. -c = next((x for x in containers if x["name"] == want), None) -if c is None: - print(f" container {want!r} not found (have: {[x['name'] for x in containers]})", file=sys.stderr) - sys.exit(1) - -env = {e["name"]: e.get("value", "") for e in (c.get("env") or [])} -mounts = {m["mountPath"].rstrip("/"): m.get("name", "") for m in (c.get("volumeMounts") or [])} - -def at_or_under(path, root): - # Exact match or a genuine child. Prefix matching alone would accept - # "/opt/nvsnap-other" for root "/opt/nvsnap". - path = path.rstrip("/") - return path == root or path.startswith(root + posixpath.sep) - -problems = [] -if cache_dir not in mounts: - problems.append(f"cache dir {cache_dir} is not mounted (mounts: {sorted(mounts)})") - -# The cache env is what makes the engine reuse the capture instead of -# fetching. Absent counts as a failure: a restore pod that inherited none -# of the stamped env is not restoring from anything. -if not any(v in env for v in ("HF_HOME", "NIM_CACHE_PATH")): - problems.append("no cache env (HF_HOME / NIM_CACHE_PATH) injected") -for var in ("HF_HOME", "NIM_CACHE_PATH"): - val = env.get(var) - if val and not at_or_under(val, cache_dir): - problems.append(f"{var}={val!r} points outside {cache_dir}") - -if problems: - for p in problems: - print(f" {p}", file=sys.stderr) - sys.exit(1) -PY - then - log_error "Restore pod was NOT decorated by the webhook - it will COLD START." - log_error "Any timing from this run would be a cold start labelled as a restore." - kubectl get pod "$RESTORE_POD_NAME" -n "$NAMESPACE" \ - -o jsonpath='{.metadata.annotations.nvsnap\.io/restore-from}{"\n"}' 2>/dev/null \ - | sed 's/^/ restore-from: /' + POD_CACHE_DIR=$(agent_pod_cache_dir) + if ! assert_restore_admitted "$RESTORE_POD_NAME" "$NAMESPACE" \ + "$RESTORE_CONTAINER_NAME" "$POD_CACHE_DIR"; then fail "Restore pod not admitted as a restore" fi log_info " verified: $POD_CACHE_DIR is mounted and the cache env points into it" From ea340ebcb7263ba7d8ecd44cbc230cc14ef0ce2d Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 14:11:49 -0700 Subject: [PATCH 06/11] docs(nvsnap): add a README for the test suite Someone handed this suite had to read both scripts to learn which one to run, what the capture paths mean, why a run refuses to start, and how to force a re-capture. Write it down. The guards table is the part worth having: each entry exists because that check silently produced a wrong result before, so a guard firing is information rather than an obstacle. Documents only what this branch carries -- the capture-timeout guard belongs to a separate change and is left out until it lands. Also finish the refactor: test-e2e.sh still had its own copy of the placeholder check rather than calling the shared one, which was the duplication the shared lib was meant to remove. Co-Authored-By: Balaji Ganesan --- .../nvsnap/scripts/README.md | 159 ++++++++++++++---- .../nvsnap/scripts/test-e2e.sh | 14 +- 2 files changed, 125 insertions(+), 48 deletions(-) diff --git a/src/compute-plane-services/nvsnap/scripts/README.md b/src/compute-plane-services/nvsnap/scripts/README.md index 4ccbc5d809..970e855557 100644 --- a/src/compute-plane-services/nvsnap/scripts/README.md +++ b/src/compute-plane-services/nvsnap/scripts/README.md @@ -2,40 +2,125 @@ SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. SPDX-License-Identifier: Apache-2.0 --> -# scripts/ - -Build, deploy, and test automation. Prefer these over ad-hoc `kubectl`/`docker` -commands — they encode setup that's easy to get wrong by hand. Key entry points -(grouped by purpose): - -## Versions -- [`versions.sh`](versions.sh) — **single source of truth** for image tags, - registry, and fork repos/refs. Sourced by every build/deploy script. -- [`sync-versions.sh`](sync-versions.sh) — stamp the current tags into the K8s - manifests. - -## Build -- [`build-agent.sh`](build-agent.sh) — agent base/app images (`base`, `app`, - `push-*`, `deploy`). -- [`build-deps.sh`](build-deps.sh), `build-libzmq-image.sh`, - `build-uvloop-wheel.sh`, `build-pyzmq-wheel.sh` — patched dependency images. -- CI builds every image via [`../ci/build-image.sh`](../ci/build-image.sh). - -## Deploy -- [`install-nvsnap.sh`](install-nvsnap.sh) — one-command cluster bootstrap - (namespace + pull secret + helm; `--without-webhook` to skip cert-manager). - -## Test / validate -- [`test-e2e.sh`](test-e2e.sh) `` — deploy → warm → capture → restore → - verify inference. The merge gate for capture/restore changes (`CLAUDE.md` - rule 10). `CAPTURE_PATH=rootfs` forces the rootfs/cachedir path. -- [`test-bench.sh`](test-bench.sh) `` — same flow with cold/capture/ - restore timings for the benchmark matrix. -- [`checkpoint.sh`](checkpoint.sh) — checkpoint-creation helper (exit 42 on a - rootfs redirect). - -## Rules - -Scripts live on disk and are version-controlled — never type build commands -ad-hoc in a terminal (`CLAUDE.md` rule 13). Keep `DOCKER_HOST` unset; never -`sudo` docker. + +# nvsnap test suite + +Two entry points: + +| Script | Answers | Writes | +|---|---|---| +| `test-e2e.sh ` | does capture/restore work for this workload | pass/fail + step timings to stdout | +| `test-bench.sh ` | how long does cold vs warm vs restore take | a row in `docs/PDF-BENCH-RESULTS.md` | + +Use `test-e2e.sh` to check correctness, `test-bench.sh` to produce numbers. + +## Before you run anything + +```sh +export KUBECONFIG= +kubectl get nodes # must work; expired credentials are the #1 cause of confusing failures +./scripts/test-e2e.sh # no args: prints the workload list +``` + +The suite refuses to start unless the deployed agent matches +`scripts/versions.sh`. That is deliberate: a run against a different build +produces numbers attributed to the wrong version. To test a build that is +already deployed, override rather than editing the file: + +```sh +NVSNAP_APP_VERSION=v0.2.46 ./scripts/test-e2e.sh vllm-small +``` + +## Running + +```sh +./scripts/test-e2e.sh vllm-small # single GPU, CRIU path, ~5 min +./scripts/test-e2e.sh vllm-70b # 4 GPUs, cachedir path, ~30 min +./scripts/test-bench.sh gpt-oss-120b # benchmark row instead of pass/fail +``` + +Both leave the source and restored pods in place on failure so you can inspect +them. On success they clean up. + +## Capture paths + +Which path runs is decided by GPU count, not by the manifest: + +- **1 GPU** -> `criu-v2`: CRIU + cuda-checkpoint of the live process, GPU state + included. +- **2+ GPUs** -> `cachedir`: capture the pod's cache mount (model weights, + compiled kernels). No process state. Multi-GPU CRIU does not work. + +Override with `CAPTURE_PATH=criu-v2` or `CAPTURE_PATH=rootfs` when you need the +other one. + +The `nvsnap.io/path` annotation in a workload manifest documents the intent; it +does not select the path. The agent's `--pod-cache-dir` flag is what decides +whether a `rootfs`-family capture is really cachedir. If those two disagree, +believe the flag. + +## Guards, and why a run may refuse to start + +These exist because each one has silently produced a wrong result before. +If a guard fires, it is telling you the run would have measured something other +than what you asked for. + +| Guard | Refuses when | Why | +|---|---|---| +| agent version | deployed image != `versions.sh` | numbers would be attributed to the wrong build | +| image exists | tag missing from the registry | catches a failed push before a 30 min run | +| placeholder | `__CAPTURE_HASH__` survived substitution | the webhook ignores the pod and it cold-starts | +| restore admitted | cache dir not mounted, or cache env points outside it | the pod cold-starts while looking like a restore | + +The restore-admitted guard is the one worth understanding. A pod the webhook +declined still starts, still serves, and passes every functional check -- it +just fetches its model again. Without the guard the run reports a restore time +that is really a cold start, and in `test-bench.sh` that number is published. + +Shared implementation: `scripts/lib/restore-guard.sh`, sourced by both scripts +so the contract cannot drift between them. + +## When a run fails + +```sh +kubectl get pods -n nvsnap-system # both pods are left behind +kubectl logs -n nvsnap-system --tail=100 +kubectl logs -n nvsnap-system -l app=nvsnap-agent -c agent --since=30m | grep -i capture +``` + +Capture and restore logs land next to the checkpoint on the node: + +```sh +AGENT=$(kubectl get pods -n nvsnap-system -l app=nvsnap-agent -o name | head -1) +kubectl exec -n nvsnap-system ${AGENT#pod/} -c agent -- \ + sh -c 'ls -1dt /var/lib/containerd/nvsnap-cache/*/ | head -3' +``` + +Copy anything you need out before re-running: a second run may reuse or replace +the capture, and the evidence goes with it. + +## Re-capturing + +Captures are content-addressed. A second run with the same pod identity reuses +the existing capture rather than making a new one -- normally what you want, and +confusing when you are trying to test the capture path itself. + +To force a fresh capture, remove what claims the hash: + +```sh +kubectl delete cm -n nvsnap-system nvsnap-capture- # manifest tier +kubectl delete pvc -n nvsnap-system rox- # L2 tier +``` + +Deleting only one tier is not enough: the agent skips the capture if any tier +claims the hash. A schema change bumps `CaptureFormatVersion`, which changes the +hash and re-captures everything automatically. + +## Adding a workload + +1. `deploy/k8s/workloads/.yaml` plus `-restore.yaml`. +2. Restore manifest carries `nvsnap.io/restore-from: "__CAPTURE_HASH__"`; the + scripts substitute it. +3. Add a `case` arm in both scripts with the pod names, port, model, and + inference payloads. +4. Annotate `nvsnap.io/gpus` accurately -- it selects the capture path. diff --git a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh index 39963eac9f..aa8bc01e58 100755 --- a/src/compute-plane-services/nvsnap/scripts/test-e2e.sh +++ b/src/compute-plane-services/nvsnap/scripts/test-e2e.sh @@ -794,17 +794,9 @@ else "$RESTORE_MANIFEST_TEMPLATE" > "$RESTORE_MANIFEST" fi -# A template placeholder that survives substitution is not a cosmetic problem. -# nvsnap.io/restore-from: "__CAPTURE_HASH__" gives the webhook nothing to -# resolve, so it injects no storage and no cache env -- and the pod cold-starts -# while looking exactly like a slow restore, model download included. Every -# timing measured from that point is a cold-start number wearing a restore -# label. Refuse to launch instead. -if grep -nE '__[A-Z_]+__' "$RESTORE_MANIFEST"; then - log_error "Unsubstituted placeholder(s) above in $RESTORE_MANIFEST." - log_error "The webhook would ignore this pod and it would COLD START, not restore." - exit 1 -fi +# See scripts/lib/restore-guard.sh: a surviving placeholder leaves the webhook +# nothing to resolve, so the pod cold-starts while looking like a slow restore. +assert_no_placeholders "$RESTORE_MANIFEST" || exit 1 # Phase 5d: restore goes back to the simple hostPath mount on the # capture-source node (or the agent's EnsureLocal cascade materializes From f72f3f77428a231f17136ef9e54e4798d73f1299 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 14:39:53 -0700 Subject: [PATCH 07/11] fix(nvsnap): do not treat a placeholder named in a comment as unsubstituted The placeholder guard scanned the whole manifest, so a template that names its own placeholder in an explanatory comment failed even when every value was substituted correctly: 25: # test-e2e.sh substitutes __NODE_NAME__ from the source pod's status. ERROR: unsubstituted placeholder(s) above That rejected a correct vllm-small restore and failed a run that would otherwise have passed. A guard that blocks good runs is worse than the problem it was added for, and this one was caught by the first suite run rather than by me. Strip comments before matching. Verified both directions: a placeholder named only in a comment passes, a real unsubstituted value still fails, including when a comment on the same line mentions one. Co-Authored-By: Balaji Ganesan --- .../nvsnap/scripts/lib/restore-guard.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh index 45edbd3b03..c6a0855850 100644 --- a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh @@ -17,8 +17,15 @@ # placeholder survives substitution the webhook has nothing to resolve, injects # nothing, and the pod cold-starts. assert_no_placeholders() { - local manifest="$1" - if grep -nE '__[A-Z_]+__' "$manifest"; then + local manifest="$1" hits + # Comments are excluded deliberately. Templates name their own placeholders + # in explanatory comments ("test-e2e.sh substitutes __NODE_NAME__ from ..."), + # and matching those fails a correctly substituted manifest -- a guard that + # blocks good runs is worse than the problem it was added for. sed keeps the + # line count, so grep -n still reports true line numbers. + hits=$(sed 's/#.*//' "$manifest" | grep -nE '__[A-Z_]+__') + if [ -n "$hits" ]; then + printf '%s\n' "$hits" >&2 echo "ERROR: unsubstituted placeholder(s) above in $manifest" >&2 echo "ERROR: the webhook would ignore this pod and it would COLD START, not restore" >&2 return 1 From 58f2855c792e101fc2b9f01efe1c1f6e819bdecb Mon Sep 17 00:00:00 2001 From: balaji Date: Fri, 21 Aug 2026 10:13:01 -0700 Subject: [PATCH 08/11] fix(nvsnap): address review on the restore guards agent_pod_cache_dir parsed the wrong thing. The jsonpath rendered the whole args array, which prints space-separated rather than comma-separated, so the split on commas did nothing and the match ran past the value into the next argument: "[--pod-cache-dir=/opt/nvsnap --other=2]" yielded "/opt/nvsnap --other=2]". Emit one argument per line and match the flag exactly. Add fixture tests for the guards. They decide whether a run means anything -- they are what stops a cold start being published as a restore time -- so a guard that silently stops guarding is worse than no guard, because the green result is still believed. Eight cases covering unresolved tokens, tokens named only in comments, the argument boundary above, an absent flag, and a non-default cache dir. Assert the kube-client failure positively in the rootfs capture test. It only checked that some error occurred, so any new early return would satisfy it while leaving buildKubeClient uncovered, under a test name that no longer described it. Document what the placeholder guard actually rejects: any unresolved __[A-Z_]+__ token, not only __CAPTURE_HASH__. Make the log-location snippet read the cache dir from the deployed agent and target the node the workload ran on, rather than hardcoding a path the guard itself is careful never to assume. Remove markdown bold from scripts/README.md per the repository documentation rule. Co-Authored-By: Balaji Ganesan --- .../agent/rootfsonly_integration_test.go | 8 ++ .../nvsnap/scripts/README.md | 26 +++-- .../nvsnap/scripts/lib/restore-guard-test.sh | 95 +++++++++++++++++++ .../nvsnap/scripts/lib/restore-guard.sh | 13 ++- 4 files changed, 134 insertions(+), 8 deletions(-) create mode 100755 src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh diff --git a/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go b/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go index 188d70e4d4..517f4af45a 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/rootfsonly_integration_test.go @@ -52,4 +52,12 @@ func TestStartRootfsCapture_EnabledFailsWithoutKubeConfig(t *testing.T) { if strings.Contains(err.Error(), "whole-rootfs") { t.Fatalf("guard fired instead of the kube client path; this test no longer covers what it claims: %v", err) } + // Assert positively, not just that some error occurred. Any new early + // return -- another guard, a validation, a typo'd default -- would satisfy + // "err != nil" while leaving buildKubeClient uncovered, and the test would + // keep passing under a name that no longer describes it. + msg := strings.ToLower(err.Error()) + if !strings.Contains(msg, "kube") && !strings.Contains(msg, "cluster") && !strings.Contains(msg, "config") { + t.Fatalf("expected a kube client construction failure, got something else: %v", err) + } } diff --git a/src/compute-plane-services/nvsnap/scripts/README.md b/src/compute-plane-services/nvsnap/scripts/README.md index 970e855557..4774d701cd 100644 --- a/src/compute-plane-services/nvsnap/scripts/README.md +++ b/src/compute-plane-services/nvsnap/scripts/README.md @@ -46,9 +46,9 @@ them. On success they clean up. Which path runs is decided by GPU count, not by the manifest: -- **1 GPU** -> `criu-v2`: CRIU + cuda-checkpoint of the live process, GPU state +- 1 GPU -> `criu-v2`: CRIU + cuda-checkpoint of the live process, GPU state included. -- **2+ GPUs** -> `cachedir`: capture the pod's cache mount (model weights, +- 2+ GPUs -> `cachedir`: capture the pod's cache mount (model weights, compiled kernels). No process state. Multi-GPU CRIU does not work. Override with `CAPTURE_PATH=criu-v2` or `CAPTURE_PATH=rootfs` when you need the @@ -69,9 +69,16 @@ than what you asked for. |---|---|---| | agent version | deployed image != `versions.sh` | numbers would be attributed to the wrong build | | image exists | tag missing from the registry | catches a failed push before a 30 min run | -| placeholder | `__CAPTURE_HASH__` survived substitution | the webhook ignores the pod and it cold-starts | +| placeholder | any `__NAME__` token survived substitution, not just `__CAPTURE_HASH__` | the webhook ignores the pod and it cold-starts | | restore admitted | cache dir not mounted, or cache env points outside it | the pod cold-starts while looking like a restore | +The placeholder guard rejects any unresolved `__[A-Z_]+__` token, not only +`__CAPTURE_HASH__`, because a manifest that still carries `__NODE_NAME__` or +`__CHECKPOINT_ID__` is just as unusable. Tokens named inside comments are +ignored deliberately: templates explain their own placeholders, and a guard +that fails a correctly substituted manifest is worse than the problem it +was added for. + The restore-admitted guard is the one worth understanding. A pod the webhook declined still starts, still serves, and passes every functional check -- it just fetches its model again. Without the guard the run reports a restore time @@ -91,9 +98,16 @@ kubectl logs -n nvsnap-system -l app=nvsnap-agent -c agent --since=30m | grep -i Capture and restore logs land next to the checkpoint on the node: ```sh -AGENT=$(kubectl get pods -n nvsnap-system -l app=nvsnap-agent -o name | head -1) -kubectl exec -n nvsnap-system ${AGENT#pod/} -c agent -- \ - sh -c 'ls -1dt /var/lib/containerd/nvsnap-cache/*/ | head -3' +# Read the cache dir from the deployed agent rather than assuming it, and use +# the node the workload actually ran on -- a different agent pod sees a +# different disk. +. scripts/lib/restore-guard.sh +CACHE=$(agent_pod_cache_dir) +NODE=$(kubectl get pod -n nvsnap-system -o jsonpath='{.spec.nodeName}') +AGENT=$(kubectl get pods -n nvsnap-system -l app=nvsnap-agent -o wide \ + | awk -v n="$NODE" '$7==n {print $1}' | head -1) +kubectl exec -n nvsnap-system "$AGENT" -c agent -- \ + sh -c "ls -1dt $CACHE/*/ | head -3" ``` Copy anything you need out before re-running: a second run may reuse or replace diff --git a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh new file mode 100755 index 0000000000..867dd6e278 --- /dev/null +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Fixture tests for restore-guard.sh. +# +# These guards decide whether a test run means anything: they are what stops a +# cold start being published as a restore time. A guard that silently stops +# guarding is worse than no guard, because the green result is still believed. +# +# Run: scripts/lib/restore-guard-test.sh + +set -uo pipefail +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=/dev/null +. "$HERE/restore-guard.sh" + +PASS=0 +FAIL=0 +ok() { PASS=$((PASS+1)); printf ' ok %s\n' "$1"; } +bad() { FAIL=$((FAIL+1)); printf ' FAIL %s\n' "$1"; } +check(){ # check + if [ "$2" = "$3" ]; then ok "$1"; else bad "$1 (want rc=$2, got rc=$3)"; fi +} + +TMP=$(mktemp -d); trap 'rm -rf "$TMP" "$TMP/bin" 2>/dev/null' EXIT +mkdir -p "$TMP/bin" + +echo "assert_no_placeholders" + +cat > "$TMP/clean.yaml" <<'YAML' +apiVersion: v1 +kind: Pod +metadata: + name: vllm-small-restored + annotations: + nvsnap.io/restore-from: "abc123" +YAML +assert_no_placeholders "$TMP/clean.yaml" >/dev/null 2>&1 +check "accepts a fully substituted manifest" 0 $? + +cat > "$TMP/unsub.yaml" <<'YAML' +metadata: + annotations: + nvsnap.io/restore-from: "__CAPTURE_HASH__" +YAML +assert_no_placeholders "$TMP/unsub.yaml" >/dev/null 2>&1 +check "rejects an unsubstituted __CAPTURE_HASH__" 1 $? + +# The guard must reject ANY unresolved token, not only the one it was written +# for. A manifest carrying __NODE_NAME__ is equally unusable. +cat > "$TMP/othertoken.yaml" <<'YAML' +spec: + nodeName: __NODE_NAME__ +YAML +assert_no_placeholders "$TMP/othertoken.yaml" >/dev/null 2>&1 +check "rejects any unresolved token, not just the hash" 1 $? + +# Templates name their own placeholders in comments. Matching those fails a +# correctly substituted manifest, which is worse than the bug it guards. +cat > "$TMP/comment.yaml" <<'YAML' +# test-e2e.sh substitutes __NODE_NAME__ from the source pod's status. +spec: + nodeName: ip-10-0-0-1 +YAML +assert_no_placeholders "$TMP/comment.yaml" >/dev/null 2>&1 +check "ignores a placeholder named only in a comment" 0 $? + +echo "agent_pod_cache_dir" + +mk_kubectl() { printf '#!/bin/sh\n%s\n' "$1" > "$TMP/bin/kubectl"; chmod +x "$TMP/bin/kubectl"; } + +mk_kubectl "printf -- '--foo=1\n--pod-cache-dir=/opt/nvsnap\n--bar=2\n'" +got=$(PATH="$TMP/bin:$PATH" agent_pod_cache_dir) +if [ "$got" = "/opt/nvsnap" ]; then ok "extracts the value"; else bad "extracts the value (got '$got')"; fi + +# The bug this replaced: a greedy match ran past the value into the next +# argument, yielding "/opt/nvsnap --other=2". +mk_kubectl "printf -- '--pod-cache-dir=/opt/nvsnap\n--other=2\n'" +got=$(PATH="$TMP/bin:$PATH" agent_pod_cache_dir) +if [ "$got" = "/opt/nvsnap" ]; then ok "stops at the argument boundary"; else bad "stops at the argument boundary (got '$got')"; fi + +mk_kubectl "printf -- '--foo=1\n--bar=2\n'" +got=$(PATH="$TMP/bin:$PATH" agent_pod_cache_dir) +if [ -z "$got" ]; then ok "empty when the flag is absent"; else bad "empty when the flag is absent (got '$got')"; fi + +# A cluster configured with a different cache dir must be followed, not +# second-guessed; that is the whole reason this reads the deployed value. +mk_kubectl "printf -- '--pod-cache-dir=/var/lib/containerd/nvsnap-cache\n'" +got=$(PATH="$TMP/bin:$PATH" agent_pod_cache_dir) +if [ "$got" = "/var/lib/containerd/nvsnap-cache" ]; then ok "follows a non-default cache dir"; else bad "follows a non-default cache dir (got '$got')"; fi + +echo +echo "$PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ] diff --git a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh index c6a0855850..7df17362e7 100644 --- a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh @@ -40,9 +40,18 @@ assert_no_placeholders() { # asserting a hardcoded default. Empty output means cachedir capture is not # configured, which callers should treat as fatal for a restore test. agent_pod_cache_dir() { + # Emit one arg per line rather than rendering the whole array. The array + # form prints as "[--a --pod-cache-dir=/x --b]", which is space-separated + # rather than comma-separated, so splitting on commas does nothing and a + # greedy match runs past the value into the following arguments -- for + # "[--pod-cache-dir=/var/lib/x --other]" it would yield "/var/lib/x --other]". kubectl get ds nvsnap-agent -n nvsnap-system \ - -o jsonpath='{.spec.template.spec.containers[0].args}' 2>/dev/null \ - | tr ',' '\n' | sed -n 's|.*--pod-cache-dir=\([^"]*\).*|\1|p' | head -1 + -o jsonpath='{range .spec.template.spec.containers[0].args[*]}{@}{"\n"}{end}' 2>/dev/null \ + | while IFS= read -r arg; do + case "$arg" in + --pod-cache-dir=*) printf '%s\n' "${arg#--pod-cache-dir=}"; break ;; + esac + done } # assert_restore_admitted From 3ab37dda89b17a747af7c6112955775ae1445dc7 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Fri, 21 Aug 2026 13:12:11 -0700 Subject: [PATCH 09/11] 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) with: Error (criu/cr-restore.c:1242): Can't fork for 363: File exists CRIU recreates a 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 the write returns EPERM inside a container, and replaced with 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, next child at pid 100003). Without it the login shell forks a few hundred times sourcing profile.d before the manifest's tail -F starts, parking a long-lived process inside the restored range. The exact landing pid varies per run, which is why it read as flakiness rather than breakage. Restores the bump on the criu-v2 restore manifests and in restore-entrypoint, so production restores are covered whatever the tenant's command is. The agent now refuses to restore into a placeholder whose pid range was never pushed up, waiting for the reservation rather than sampling once. NIM's placeholder runs as uid 0 because its image defaults to uid 1000 and privileged does not confer root. Verified: 14/14 on the single-GPU suite (7 workloads x 2 passes), each including a 75s soak plus a serving check, up from 3/14. Refs #925 --- .../nvsnap/cmd/restore-entrypoint/BUILD.bazel | 5 +- .../nvsnap/cmd/restore-entrypoint/main.go | 58 +++++ .../restore-entrypoint/pid_reserve_test.go | 66 +++++ .../k8s/workloads/e5-mistral-restore.yaml | 23 +- .../k8s/workloads/gemma-sglang-restore.yaml | 23 +- .../k8s/workloads/nim-llama-8b-restore.yaml | 30 ++- .../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 +- .../docs/proposals/dynamo-cr-validation.md | 220 +++++++++++++++++ .../nvsnap/docs/proposals/pidns-capture.md | 148 ++++++++++++ .../nvsnap/internal/agent/BUILD.bazel | 1 + .../nvsnap/internal/agent/checkpoint_v2.go | 23 +- .../nvsnap/internal/agent/restore_v2.go | 154 ++++++++++++ .../agent/restore_v2_pidguard_test.go | 227 ++++++++++++++++++ .../nvsnap/internal/checkpointstore/store.go | 8 +- 20 files changed, 1082 insertions(+), 65 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go create mode 100644 src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md create mode 100644 src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md create mode 100644 src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_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 7e9ed99347..1255ecbd79 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 979635cb59..f6da1882cc 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 0000000000..c1bdef3592 --- /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/deploy/k8s/workloads/e5-mistral-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml index c02fcccc08..424da65aa9 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 b5417d9f79..34c91892ab 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 4780143902..4ff35a14b7 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. # @@ -74,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" 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 81bcd17704..9d71654dc5 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 bd5b8476d9..a0f913f088 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 da70f66696..b76dd4013f 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 c782d19ff6..de94b8dc18 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 598f1bec40..50c5eaa12c 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 262f5fbd51..c11fb7ccfe 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 a52402f849..bfaea1248c 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/docs/proposals/dynamo-cr-validation.md b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md new file mode 100644 index 0000000000..ea02ef6551 --- /dev/null +++ b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md @@ -0,0 +1,220 @@ + + +# 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 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 + +- 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). 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. 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 + +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 + +Use request pinning, established above. For each rung: + +- 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. + +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 + +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. 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 0000000000..22af2da518 --- /dev/null +++ b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md @@ -0,0 +1,148 @@ + + +# Capture the PID namespace, so restore can create a fresh one + +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 + +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. + +## 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.** 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 +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 + +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: + +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/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index 48881a4dca..c700911aa0 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", 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 d95f03f45f..51f50a690e 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go +++ b/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go @@ -167,9 +167,25 @@ 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 sid, err := sessionID(procBase, gpuPIDs[0]); err == nil && sid > 1 { @@ -487,3 +503,4 @@ func tailOfFile(path string, n int) string { } return strings.Join(lines, " | ") } + 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 ec5cc9f09e..604e648648 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,28 @@ 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. + // + // 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{ "placeholderPID": hostPID, "imagesDir": imgsInContainer, @@ -222,3 +244,135 @@ 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) +} + +// 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 new file mode 100644 index 0000000000..a2b8d0e963 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go @@ -0,0 +1,227 @@ +/* +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 ( + "io" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/sirupsen/logrus" +) + +// 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) + } +} + +// 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) + } + }) +} diff --git a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go index e0f6ec0c08..73fb459182 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 a5ee7cac914688d300c769807f334a5a77df3c03 Mon Sep 17 00:00:00 2001 From: balaji Date: Fri, 21 Aug 2026 13:52:58 -0700 Subject: [PATCH 10/11] test(nvsnap): cover the restore-admission guard with fixtures The placeholder half of restore-guard.sh had fixture tests; the restore-admission half had none, so the check that actually decides whether a timing is a restore or a cold start was itself unverified. Adds nine cases driven by a stubbed kubectl, needing no cluster: a decorated pod, an unconfigured cache dir, a missing container, an unmounted cache dir, absent cache env, env pointing outside the cache dir, a sibling directory sharing the cache dir's prefix, NIM_CACHE_PATH in place of HF_HOME, and a trailing slash. Three of these were checked by mutation rather than assumed. Letting a missing container fall back to containers[0], replacing at_or_under with a plain startswith, and dropping the absent-env check each turn exactly one case red. That matters here because a guard test that passes against a broken guard is the failure mode being defended against. Also completes the guard table in the scripts README, which listed only two of the five conditions restore admission can fail on. Co-Authored-By: Balaji Ganesan --- .../nvsnap/scripts/README.md | 15 ++++- .../nvsnap/scripts/lib/restore-guard-test.sh | 67 +++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/compute-plane-services/nvsnap/scripts/README.md b/src/compute-plane-services/nvsnap/scripts/README.md index 4774d701cd..abe2fce454 100644 --- a/src/compute-plane-services/nvsnap/scripts/README.md +++ b/src/compute-plane-services/nvsnap/scripts/README.md @@ -70,7 +70,7 @@ than what you asked for. | agent version | deployed image != `versions.sh` | numbers would be attributed to the wrong build | | image exists | tag missing from the registry | catches a failed push before a 30 min run | | placeholder | any `__NAME__` token survived substitution, not just `__CAPTURE_HASH__` | the webhook ignores the pod and it cold-starts | -| restore admitted | cache dir not mounted, or cache env points outside it | the pod cold-starts while looking like a restore | +| restore admitted | the agent has no `--pod-cache-dir`, the named container is absent, the cache dir is not mounted, neither `HF_HOME` nor `NIM_CACHE_PATH` is set, or one of them points outside the cache dir | the pod cold-starts while looking like a restore | The placeholder guard rejects any unresolved `__[A-Z_]+__` token, not only `__CAPTURE_HASH__`, because a manifest that still carries `__NODE_NAME__` or @@ -84,8 +84,19 @@ declined still starts, still serves, and passes every functional check -- it just fetches its model again. Without the guard the run reports a restore time that is really a cold start, and in `test-bench.sh` that number is published. +Two of its conditions are easy to misread as "nothing to check". Absent cache +env is a failure, not a pass: a restore pod that inherited none of the stamped +variables is not restoring from anything. And the named container must exist -- +falling back to the first container would let a decorated sidecar vouch for a +workload that is cold-starting. Both fail closed. + Shared implementation: `scripts/lib/restore-guard.sh`, sourced by both scripts -so the contract cannot drift between them. +so the contract cannot drift between them. `scripts/lib/restore-guard-test.sh` +covers it with fixtures and needs no cluster: + +```sh +scripts/lib/restore-guard-test.sh +``` ## When a run fails diff --git a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh index 867dd6e278..cc532a8da3 100755 --- a/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh @@ -90,6 +90,73 @@ mk_kubectl "printf -- '--pod-cache-dir=/var/lib/containerd/nvsnap-cache\n'" got=$(PATH="$TMP/bin:$PATH" agent_pod_cache_dir) if [ "$got" = "/var/lib/containerd/nvsnap-cache" ]; then ok "follows a non-default cache dir"; else bad "follows a non-default cache dir (got '$got')"; fi +echo "assert_restore_admitted" + +# Stubs kubectl to return a fixed pod. The guard reads the pod as JSON, so the +# fixture is the whole input -- no cluster is involved. +mk_pod() { printf '#!/bin/sh\ncat <<'"'"'JSON'"'"'\n%s\nJSON\n' "$1" > "$TMP/bin/kubectl"; chmod +x "$TMP/bin/kubectl"; } +admitted() { PATH="$TMP/bin:$PATH" assert_restore_admitted vllm-restored default "$1" "$2" >/dev/null 2>&1; } + +VALID='{"spec":{"containers":[{"name":"vllm", + "env":[{"name":"HF_HOME","value":"/opt/nvsnap/hf"}], + "volumeMounts":[{"mountPath":"/opt/nvsnap"}]}]}}' + +mk_pod "$VALID" +admitted vllm /opt/nvsnap +check "accepts a decorated restore pod" 0 $? + +# An agent with no --pod-cache-dir means cachedir capture is not configured at +# all, so there is nothing a restore could have come from. +mk_pod "$VALID" +admitted vllm "" +check "rejects an empty cache dir" 1 $? + +# Falling back to containers[0] would let a decorated sidecar vouch for a +# workload that is cold-starting, so a missing container fails closed. +mk_pod "$VALID" +admitted engine /opt/nvsnap +check "rejects a missing restore container" 1 $? + +mk_pod '{"spec":{"containers":[{"name":"vllm", + "env":[{"name":"HF_HOME","value":"/opt/nvsnap/hf"}], + "volumeMounts":[{"mountPath":"/var/tmp"}]}]}}' +admitted vllm /opt/nvsnap +check "rejects an unmounted cache dir" 1 $? + +# Absent cache env is a failure, not "nothing to check": a restore pod that +# inherited none of the stamped variables is not restoring from anything. +mk_pod '{"spec":{"containers":[{"name":"vllm", + "env":[{"name":"PATH","value":"/usr/bin"}], + "volumeMounts":[{"mountPath":"/opt/nvsnap"}]}]}}' +admitted vllm /opt/nvsnap +check "rejects a pod with no cache env" 1 $? + +mk_pod '{"spec":{"containers":[{"name":"vllm", + "env":[{"name":"HF_HOME","value":"/root/.cache/huggingface"}], + "volumeMounts":[{"mountPath":"/opt/nvsnap"}]}]}}' +admitted vllm /opt/nvsnap +check "rejects cache env pointing outside the cache dir" 1 $? + +# Prefix matching alone would accept a sibling directory whose name merely +# starts with the cache dir. +mk_pod '{"spec":{"containers":[{"name":"vllm", + "env":[{"name":"HF_HOME","value":"/opt/nvsnap-other/hf"}], + "volumeMounts":[{"mountPath":"/opt/nvsnap"}]}]}}' +admitted vllm /opt/nvsnap +check "rejects a sibling dir that shares the cache dir prefix" 1 $? + +# NIM images stamp NIM_CACHE_PATH instead of HF_HOME; either satisfies the guard. +mk_pod '{"spec":{"containers":[{"name":"nim", + "env":[{"name":"NIM_CACHE_PATH","value":"/opt/nvsnap/nim"}], + "volumeMounts":[{"mountPath":"/opt/nvsnap"}]}]}}' +admitted nim /opt/nvsnap +check "accepts NIM_CACHE_PATH in place of HF_HOME" 0 $? + +# A trailing slash is a spelling of the same path, not a different one. +mk_pod "$VALID" +admitted vllm /opt/nvsnap/ +check "treats a trailing slash as the same cache dir" 0 $? + echo echo "$PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ] From f795ea5bfa0d17fbb321334605f4684f982d1697 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 23 Sep 2026 21:24:54 -0700 Subject: [PATCH 11/11] docs(nvsnap): tag the two fenced blocks in the pidns capture proposal The markdown lint job became blocking in #1751, after this branch was written. It lints every tracked file, so these two untagged fences (MD040) failed the job on a file the rest of the branch only added. Co-Authored-By: Balaji Ganesan Co-Authored-By: Claude Opus 5 --- .../nvsnap/docs/proposals/pidns-capture.md | 4 ++-- 1 file changed, 2 insertions(+), 2 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 22af2da518..401321e5a9 100644 --- a/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md +++ b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md @@ -13,7 +13,7 @@ been fixed another way (the placeholder pid reservation). CRIU restore dies with one of: -``` +```text 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 ``` @@ -77,7 +77,7 @@ what a container checkpoint normally contains. Measured, not theorised. With the gate on, the dump ran as: -``` +```sh nsenter -t -m -p -n -i -u -r -w -- criu dump -t 1 ... ```