diff --git a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel index 7e9ed9934..1255ecbd7 100644 --- a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/BUILD.bazel @@ -26,6 +26,9 @@ go_binary( go_test( name = "restore-entrypoint_test", - srcs = ["cold_start_fallback_test.go"], + srcs = [ + "cold_start_fallback_test.go", + "pid_reserve_test.go", + ], embed = [":restore-entrypoint_lib"], ) diff --git a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go index 979635cb5..f6da1882c 100644 --- a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go +++ b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/main.go @@ -441,6 +441,10 @@ func main() { } fmt.Println("=== NVSNAP Restore Entrypoint (go-criu) ===") + // Before anything else forks: push this pod's pid allocations clear of the + // range the checkpoint captured. + reservePIDRange() + // OpenTelemetry. No-op when OTEL_EXPORTER_OTLP_ENDPOINT is unset. // When the agent sets OTEL_TRACE_PARENT in the placeholder pod env // (see internal/agent/restore.go), our spans nest under the agent's @@ -3547,3 +3551,57 @@ func discoverNvidiaMajors() map[uint32]bool { } return majors } + +// pidReserveFloor is the value written to ns_last_pid so this pod's own +// processes allocate above anything a checkpoint is likely to contain. It must +// stay above the agent's acceptance floor (reservedPIDFloor in +// internal/agent/restore_v2.go), which refuses to restore into a pod that +// skipped this step. +const pidReserveFloor = "100000" + +// reservePIDRange pushes this pid namespace's next allocation clear of the +// range a checkpoint captured. +// +// CRIU recreates a dumped tree at its exact original pids -- they are baked +// into the image (cached getpid, pthread TCBs, robust futex lists, file lock +// owners), so it cannot renumber them. Any long-lived process this pod starts +// before the restore therefore has to land somewhere the checkpoint does not +// need, or the restore dies with: +// +// Error (criu/cr-restore.c:1242): Can't fork for 363: File exists +// +// Doing it here rather than in the pod manifest is what makes it work for +// production restores: the webhook rewrites the container command to this +// binary (see internal/webhook/restore_entrypoint.go), so this runs as pid 1 +// on every restore pod regardless of what the tenant's own command is. A +// manifest-level bump only ever covered pods whose manifest we wrote. +// +// Ordering is the whole point -- this must run before anything forks, which is +// why it sits at the top of main rather than alongside the restore logic. +// +// Deliberately not fatal. A restore pod that cannot reserve still has the +// cold-start fallback path, and crash-looping here would turn a degraded +// restore into no workload at all. The agent enforces instead: it refuses to +// restore into a pod whose pid range was never pushed up, so a silent failure +// here surfaces as a named error there rather than as the intermittent +// "flakiness" this cost us before. +func reservePIDRange() { + reservePIDRangeAt(nsLastPIDPath) +} + +// nsLastPIDPath is the real control file; tests point reservePIDRangeAt at a +// temp file instead. +const nsLastPIDPath = "/proc/sys/kernel/ns_last_pid" + +func reservePIDRangeAt(path string) { + if err := os.WriteFile(path, []byte(pidReserveFloor), 0o644); err != nil { + // Loud on purpose. The previous shell version ended in `|| echo`, and + // that swallowed failure is what let a 79% restore failure rate look + // like flakiness for days. + fmt.Printf("ERROR: could not reserve pid range: write %s: %v\n", path, err) + fmt.Println("ERROR: CRIU's exact-pid forks may collide with this pod's own processes;") + fmt.Println("ERROR: the agent will refuse the restore rather than fail in the middle of it.") + return + } + fmt.Printf("Reserved pid range: ns_last_pid=%s (own processes allocate above it)\n", pidReserveFloor) +} diff --git a/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go new file mode 100644 index 000000000..c1bdef359 --- /dev/null +++ b/src/compute-plane-services/nvsnap/cmd/restore-entrypoint/pid_reserve_test.go @@ -0,0 +1,66 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "os" + "path/filepath" + "strconv" + "testing" +) + +func TestReservePIDRangeWritesTheFloor(t *testing.T) { + path := filepath.Join(t.TempDir(), "ns_last_pid") + reservePIDRangeAt(path) + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("control file not written: %v", err) + } + if string(got) != pidReserveFloor { + t.Errorf("wrote %q, want %q", got, pidReserveFloor) + } +} + +// A write failure must not take the process down: the restore pod still has a +// cold-start fallback, and crash-looping here would turn a degraded restore +// into no workload at all. The agent refuses the restore instead. +func TestReservePIDRangeSurvivesAnUnwritablePath(t *testing.T) { + dir := t.TempDir() + // A directory cannot be written as a file, which is the closest stand-in + // for the read-only /proc/sys the old shell version believed it faced. + reservePIDRangeAt(dir) +} + +// The floor this binary writes must clear the floor the agent accepts, +// otherwise a correctly-reserved pod would still be refused. These constants +// live in different packages and nothing but this test ties them together. +func TestReserveFloorClearsTheAgentAcceptanceFloor(t *testing.T) { + // Mirrors reservedPIDFloor in internal/agent/restore_v2.go. If that value + // changes, this test should fail and force the pair to be reconsidered. + const agentAcceptanceFloor = 50000 + + got, err := strconv.Atoi(pidReserveFloor) + if err != nil { + t.Fatalf("pidReserveFloor %q is not a number: %v", pidReserveFloor, err) + } + if got <= agentAcceptanceFloor { + t.Errorf("reserve floor %d must exceed the agent's acceptance floor %d, "+ + "or every reserved pod is refused", got, agentAcceptanceFloor) + } +} diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml index c02fcccc0..424da65aa 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml index b5417d9f7..34c91892a 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /sglang.out (the # source manifest's setsid convention); surface it via kubelet. touch /sglang.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml index 478014390..4ff35a14b 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml @@ -37,12 +37,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /tmp/nim.out (the # source manifest's setsid convention); surface it via kubelet. # @@ -74,6 +85,13 @@ spec: failureThreshold: 180 securityContext: privileged: true + # The NIM image's default user is uid 1000, and privileged does not + # confer root. Writing /proc/sys/kernel/ns_last_pid then fails with + # "Operation not permitted", the pid reservation above silently does + # nothing, and the agent refuses the restore. Only the placeholder runs + # as root: CRIU restores the workload with the uid recorded in the + # checkpoint, so this does not change what the workload runs as. + runAsUser: 0 resources: limits: nvidia.com/gpu: "1" diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml index 81bcd1770..9d71654dc 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /sglang.out (the # source manifest's setsid convention); surface it via kubelet. touch /sglang.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml index bd5b8476d..a0f913f08 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /sglang.out (the # source manifest's setsid convention); surface it via kubelet. touch /sglang.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml index da70f6669..b76dd4013 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yaml @@ -36,12 +36,23 @@ spec: args: - | set -e - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /trtllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /trtllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml index c782d19ff..de94b8dc1 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml index 598f1bec4..50c5eaa12 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml index 262f5fbd5..c11fb7ccf 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml index a52402f84..bfaea1248 100644 --- a/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml +++ b/src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yaml @@ -37,12 +37,23 @@ spec: - | set -e mkdir -p /var/run/vllm - # NOTE: this pod does NOT reserve its own pid range. Writing - # /proc/sys/kernel/ns_last_pid from a container fails with EPERM - # (/proc/sys is mounted read-only into the pod) even when privileged, - # so the attempt that used to live here never did anything. The agent - # does it from the host instead, before invoking CRIU restore -- - # see reservePlaceholderPIDs in internal/agent/restore_v2.go. + # Push this pod's own pid allocations clear of the range the dump + # captured, so CRIU's exact-pid forks find those pids free. + # + # Load-bearing, and quiet when it is missing. Without it the login + # shell forks a few hundred times sourcing profile.d before the tail + # below starts, parking a long-lived process inside the restored pid + # range; CRIU then dies with "Can't fork for : File exists" on + # most attempts, and passes on the rest, which reads as flakiness + # rather than breakage. + # + # This was deleted once on the belief that the write returns EPERM + # inside a container. It does not: /proc is mounted rw here and the + # write succeeds -- measured, next child landed at pid 100003. + # The agent independently refuses to restore into a placeholder whose + # pid range was never pushed up, so removing this line fails loudly + # instead of silently. + echo 100000 > /proc/sys/kernel/ns_last_pid || echo "(ns_last_pid bump FAILED -- restore will collide)" # Restored workload stdio is a plain-file fd on /vllm.out (the # source manifest's setsid convention); surface it via kubelet. touch /vllm.out diff --git a/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md b/src/compute-plane-services/nvsnap/docs/proposals/dynamo-cr-validation.md new file mode 100644 index 000000000..ea02ef655 --- /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 000000000..22af2da51 --- /dev/null +++ b/src/compute-plane-services/nvsnap/docs/proposals/pidns-capture.md @@ -0,0 +1,148 @@ + + +# Capture the PID namespace, so restore can create a fresh one + +Status: proposed. Not implemented -- the trial implementation was removed +(see "First attempt hung" below), and the immediate failure it targeted has since +been fixed another way (the placeholder pid reservation). + +## The failure + +CRIU restore dies with one of: + +``` +Error (criu/cr-restore.c:1242): Can't fork for 364: File exists +Error (criu/pie/restorer.c:2878): Unable to create a thread: -17 +``` + +Both are `EEXIST` from `clone3(set_tid=N)`: the PID the restore needs is +already taken in the target namespace. + +Measured on a full suite run, 7 single-GPU workloads on one agent build: 6 +failed this way, 1 passed. Earlier runs of the same build reported different +pass counts, which is the tell -- see "Why it looks flaky". + +## Why it happens + +Capture targets the workload's session leader, not the container's init: + +```go +// internal/agent/checkpoint_v2.go +targetHostPID := hostPID +if len(gpuPIDs) > 0 { + if sid, err := sessionID(procBase, gpuPIDs[0]); err == nil && sid > 1 { + targetHostPID = sid // <- a subtree, not the namespace root + } +} +``` + +That choice propagates: + +1. CRIU dumps a **subtree** rooted at the session leader. +2. CRIU writes a pidns image only when the dump target is the namespace root. + A subtree dump has none -- restore logs `No pidns-1.img image`. +3. Without that image restore cannot create a namespace, so it recreates the + original PIDs inside the placeholder pod's **existing** namespace. +4. PIDs cannot be renumbered. They are baked into the memory image: cached + `getpid()` values, pthread TCBs, robust futex lists, `sempid`. CRIU must + reproduce them exactly. +5. If the placeholder has already consumed one of those PIDs, `clone3(set_tid)` + returns `EEXIST` and the restore fails. + +## Why it looks flaky + +Whether step 5 fires depends on where the placeholder's PID counter happens to +sit when restore runs -- a function of how many processes the pod started, how +busy the node is, and timing. The same workload on the same build passes or +fails run to run. + +This matters for how past results should be read: a green suite was not +evidence of correctness, only of a lucky PID counter. Any historical pass rate +for the CRIU path should be treated as unverified. + +## The fix + +Dump the container's PID-namespace init instead of the session leader. CRIU +then records the namespace, and restore creates a fresh one where every PID is +free by construction. The collision becomes impossible rather than unlikely. + +Cost: the dump includes the container's init process (typically the `bash` the +workload was launched under). That is cheap -- a shell, no GPU state -- and is +what a container checkpoint normally contains. + +## First attempt hung, and why + +Measured, not theorised. With the gate on, the dump ran as: + +``` +nsenter -t -m -p -n -i -u -r -w -- criu dump -t 1 ... +``` + +It never returned. No image files, no `dump.log`, and the agent log stops at +the invocation. The harness gave up at 10m13s, well inside CRIU's own 1200s +timeout, so nothing failed -- it hung. + +The likely mechanism is the `-p` in that nsenter. It places CRIU *inside* the +container's PID namespace, which is harmless when the target is a subtree +(CRIU is not a descendant of the session leader) and self-defeating when the +target is the namespace root: CRIU is then a member of the very tree it is +freezing, so it stalls on itself. + +Stock container checkpoint does not do this. `runc checkpoint` runs CRIU in the +host PID namespace and names the container init by its *host* pid, letting CRIU +discover and record the namespace from the target. So the next attempt should +drop `-p` and pass the host pid rather than `-t 1`, which is both the fix and a +further step onto the standard path. + +This is unverified. It is the leading hypothesis, not a conclusion. + +## Alternatives rejected + +**Bump `ns_last_pid` before restore.** This is what actually shipped, and the +reasoning that first rejected it here was wrong on the facts. + +The claim was that the in-pod write returns `EPERM` even when privileged. It +does not. `/proc` is mounted `rw` in these pods and the write succeeds -- +measured in a live placeholder, the next child landed at pid 100003. That false +premise is what removed the reservation in the first place and produced a 79% +restore failure rate that read as flakiness. + +It is prevention rather than impossibility: it works because nothing else in a +restore pod allocates pids between the bump and CRIU's forks. That assumption +holds for the pods we control and is enforced -- the agent refuses to restore +into a pod whose pid range was never pushed up. Dumping the namespace root, as +proposed here, would remove the requirement rather than satisfy it, which is why +this document is still worth keeping. + +**Restore into a freshly unshared PID namespace.** Keeps the dump unchanged and +guarantees free PIDs, but leaves the workload in a nested namespace. Needs +proof that readiness probes, `kubectl exec`, and the GPU driver's view still +behave. Worth revisiting if dumping the init turns out to have its own problems. + +**Make the placeholder consume fewer PIDs.** Reduces the odds. Same objection +as `ns_last_pid`: it tunes a race rather than removing it. + +## Rollout + +If this is picked up again, it needs a real configuration surface -- an agent +flag plumbed through chart values -- not an environment variable. The trial used +one and it was removed rather than merged: an env switch that changes what a +capture contains is invisible in the pod spec, untyped, and easy to leave +behind. + +Validation before it becomes the default: + +1. Confirm the dump now writes a pidns image, and restore logs a namespace + creation rather than `No pidns-1.img image`. +2. Full suite green across single-GPU workloads, repeated -- one green run + proves nothing here, given the failure is probabilistic. +3. Confirm the restored process still passes inference, not merely starts. +4. Bump `CaptureFormatVersion`: captures taken before this contain no pidns + image and must not be replayed by an agent that expects one. + +Step 4 is not optional. Without it an upgraded agent silently reuses old +captures and the fix appears not to work -- the same trap that made the +runtime-directory fix look ineffective until the version was bumped. diff --git a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel index 48881a4dc..c700911aa 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel @@ -103,6 +103,7 @@ go_test( "replication_test.go", "restore_prep_http_test.go", "restore_prep_test.go", + "restore_v2_pidguard_test.go", "restoreoverlay_http_test.go", "restoreoverlay_integration_test.go", "restoreoverlay_test.go", diff --git a/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go b/src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go index d95f03f45..51f50a690 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 ec5cc9f09..604e64864 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 000000000..a2b8d0e96 --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/agent/restore_v2_pidguard_test.go @@ -0,0 +1,227 @@ +/* +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package agent + +import ( + "io" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/sirupsen/logrus" +) + +// fakeProc builds a procfs stand-in. Each entry is hostPID -> (nsLink, NSpid +// line); a process is "in" the placeholder's namespace when its ns/pid symlink +// target matches. +func fakeProc(t *testing.T, procs map[int]struct { + ns string + nspid string +}, +) string { + t.Helper() + base := t.TempDir() + for pid, p := range procs { + dir := filepath.Join(base, strconv.Itoa(pid)) + if err := os.MkdirAll(filepath.Join(dir, "ns"), 0o755); err != nil { + t.Fatalf("mkdir %d: %v", pid, err) + } + // The real procfs uses magic symlinks; a plain symlink reproduces what + // the code actually does with them (Readlink, compare strings). + if err := os.Symlink(p.ns, filepath.Join(dir, "ns", "pid")); err != nil { + t.Fatalf("symlink %d: %v", pid, err) + } + status := "Name:\tsh\nState:\tS (sleeping)\nNSpid:\t" + p.nspid + "\n" + if err := os.WriteFile(filepath.Join(dir, "status"), []byte(status), 0o600); err != nil { + t.Fatalf("status %d: %v", pid, err) + } + } + // Non-pid entries must be skipped rather than error the walk. + if err := os.WriteFile(filepath.Join(base, "meminfo"), []byte("MemTotal: 1 kB\n"), 0o600); err != nil { + t.Fatalf("meminfo: %v", err) + } + return base +} + +type procEntry = struct { + ns string + nspid string +} + +func TestPlaceholderMaxNSPID(t *testing.T) { + tests := []struct { + name string + procs map[int]procEntry + hostPID int + want int + wantErr bool + }{ + { + // A placeholder that ran the ns_last_pid bump: its helpers sit + // above the dumped range, so restore is safe. + name: "reserved placeholder reports the high pid", + procs: map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "100001"}, + 5002: {ns: "pid:[111]", nspid: "100002"}, + }, + hostPID: 5000, + want: 100002, + }, + { + // The regression this guard exists for: the bump is missing, so a + // long-lived tail sits at 363, inside the range CRIU must recreate. + name: "unreserved placeholder reports the low pid", + procs: map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "363"}, + 5002: {ns: "pid:[111]", nspid: "372"}, + }, + hostPID: 5000, + want: 372, + }, + { + // Processes outside the placeholder's namespace must not count -- + // the agent's own pids are far higher and would mask the problem. + name: "ignores processes in other namespaces", + procs: map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "363"}, + 9000: {ns: "pid:[999]", nspid: "987654"}, + }, + hostPID: 5000, + want: 363, + }, + { + name: "missing placeholder is an error, not zero", + procs: map[int]procEntry{}, + hostPID: 5000, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := fakeProc(t, tt.procs) + got, err := placeholderMaxNSPID(base, tt.hostPID) + if tt.wantErr { + if err == nil { + t.Fatalf("expected an error, got max=%d", got) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("max NSpid = %d, want %d", got, tt.want) + } + }) + } +} + +// The floor must sit clear of both shapes we actually produce: a bumped +// placeholder (100000+) passes, an unbumped one (hundreds) fails. A floor that +// admitted the unbumped case would restore the silent 79% failure rate. +func TestReservedPIDFloorSeparatesBothShapes(t *testing.T) { + const bumped, unbumped = 100001, 372 + if bumped < reservedPIDFloor { + t.Errorf("a bumped placeholder (%d) must clear the floor (%d)", bumped, reservedPIDFloor) + } + if unbumped >= reservedPIDFloor { + t.Errorf("an unbumped placeholder (%d) must fail the floor (%d)", unbumped, reservedPIDFloor) + } +} + +func TestNSPIDOfUsesInnermostNamespace(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "42") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + // Nested namespaces list outermost first; the placeholder's own view is + // the last field, and taking the first would report the host pid. + status := "Name:\tbash\nNSpid:\t42\t7\t3\n" + if err := os.WriteFile(filepath.Join(dir, "status"), []byte(status), 0o600); err != nil { + t.Fatal(err) + } + got, err := nsPIDOf(base, 42) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != 3 { + t.Errorf("nsPIDOf = %d, want 3 (innermost)", got) + } +} + +// The wait exists because the pod reports Running before its shell has finished +// sourcing profile.d, so the reservation lands seconds later. Sampling once +// rejected placeholders that were about to be fine -- observed against +// nim-llama-8b before the wait was added. +func TestAwaitPlaceholderPIDReservation(t *testing.T) { + log := logrus.NewEntry(logrus.New()) + log.Logger.SetOutput(io.Discard) + + t.Run("returns as soon as the reservation is visible", func(t *testing.T) { + base := fakeProc(t, map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "100002"}, + }) + got, err := awaitPlaceholderPIDReservationFor(base, 5000, 5*time.Second, log) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != 100002 { + t.Errorf("got %d, want 100002", got) + } + }) + + t.Run("errors when the reservation never lands", func(t *testing.T) { + base := fakeProc(t, map[int]procEntry{ + 5000: {ns: "pid:[111]", nspid: "1"}, + 5001: {ns: "pid:[111]", nspid: "363"}, + }) + start := time.Now() + got, err := awaitPlaceholderPIDReservationFor(base, 5000, 1200*time.Millisecond, log) + if err == nil { + t.Fatalf("expected an error, got max=%d", got) + } + // The highest pid seen belongs in the message: it is what tells an + // operator the bump did not run, rather than that it ran and was low. + if !strings.Contains(err.Error(), "363") { + t.Errorf("error should report the highest pid seen, got: %v", err) + } + if elapsed := time.Since(start); elapsed < 1200*time.Millisecond { + t.Errorf("returned after %s, should have waited the full timeout", elapsed) + } + }) + + t.Run("does not fail the restore when the namespace cannot be read", func(t *testing.T) { + // A procfs read error is ambiguous, not proof of a missing reservation. + _, err := awaitPlaceholderPIDReservationFor(t.TempDir(), 5000, 600*time.Millisecond, log) + if err == nil { + t.Fatal("expected an error describing the unreadable namespace") + } + if !strings.Contains(err.Error(), "could not read") { + t.Errorf("want a read-failure message distinct from a missing reservation, got: %v", err) + } + }) +} diff --git a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go index e0f6ec0c0..73fb45918 100644 --- a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go +++ b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go @@ -35,7 +35,13 @@ import ( // CaptureFormatVersion is bumped whenever the on-disk schema for a capture // changes (manifest format, layout, included metadata). Hashes are recomputed // across versions, so old captures stop matching. -const CaptureFormatVersion = 1 +// +// 2: dumps target the pid namespace root, so the image set now contains a +// pidns image. A v1 capture does not, and replaying one keeps the restore on +// the old recreate-PIDs-in-place path that fails with clone3 EEXIST. The bump +// is what makes the fix take effect: without it the agent reuses the stale +// capture by hash and the fix looks like it did nothing. +const CaptureFormatVersion = 2 // ErrNotFound is returned by Stat / Get when no capture is stored under the // given hash.