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/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/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/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/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/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-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-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/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/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..401321e5a9 --- /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: + +```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 +``` + +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: + +```sh +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 4fcfefc3e0..c700911aa0 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -103,10 +103,12 @@ 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", "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/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/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 59fb39ee8a..6f030f8d44 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 == "" { // Under the containerd root, not /var/lib directly: on a typical GPU // node the latter is the boot volume while containerd sits on local 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..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 @@ -19,6 +19,7 @@ package agent import ( "context" + "strings" "testing" "github.com/sirupsen/logrus" @@ -38,8 +39,25 @@ 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) + } + // 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/internal/checkpointstore/store.go b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go index 3b9f9a8b59..c2e1be0426 100644 --- a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go +++ b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go @@ -35,12 +35,19 @@ 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. +// // 2: added EntryRuntimeDirs. A capture taken before this has no recorded // runtime directories, so restoring it cannot recreate them and workloads that // need one still fail. Without the bump those captures hash identically to new // ones and would be reused forever after an upgrade -- silently, since the // agent reports the reuse as a successful capture. -const CaptureFormatVersion = 2 +// +// 3: dumps target the pid namespace root, so the image set now contains a +// pidns image. A capture without 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 = 3 // ErrNotFound is returned by Stat / Get when no capture is stored under the // given hash. 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" ) diff --git a/src/compute-plane-services/nvsnap/scripts/README.md b/src/compute-plane-services/nvsnap/scripts/README.md index 926c19d925..abe2fce454 100644 --- a/src/compute-plane-services/nvsnap/scripts/README.md +++ b/src/compute-plane-services/nvsnap/scripts/README.md @@ -2,44 +2,150 @@ 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): +# nvsnap test suite -## Versions +Two entry points: -- [`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. +| 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` | -## Build +Use `test-e2e.sh` to check correctness, `test-bench.sh` to produce numbers. -- [`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). +## Before you run anything -## Deploy +```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 +``` -- [`install-nvsnap.sh`](install-nvsnap.sh) — one-command cluster bootstrap - (namespace + pull secret + helm; `--without-webhook` to skip cert-manager). +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: -## Test / validate +```sh +NVSNAP_APP_VERSION=v0.2.46 ./scripts/test-e2e.sh vllm-small +``` -- [`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). +## Running -## Rules +```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 +``` -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. +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 | any `__NAME__` token survived substitution, not just `__CAPTURE_HASH__` | the webhook ignores the pod and it cold-starts | +| 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 +`__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 +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. `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 + +```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 +# 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 +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/lib/restore-guard-test.sh b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh new file mode 100755 index 0000000000..cc532a8da3 --- /dev/null +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard-test.sh @@ -0,0 +1,162 @@ +#!/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 "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 ] 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..7df17362e7 --- /dev/null +++ b/src/compute-plane-services/nvsnap/scripts/lib/restore-guard.sh @@ -0,0 +1,127 @@ +# 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" 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 + 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() { + # 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='{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 +# +# 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 e5222e32f3..f6092dc010 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 @@ -815,6 +816,10 @@ else "$RESTORE_MANIFEST_TEMPLATE" > "$RESTORE_MANIFEST" 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 # it locally on a different target node before the placeholder reads @@ -852,6 +857,26 @@ 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..." + 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" +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