From 55b5e9e2e86a10ad0d1fba5d7d116320245fecb5 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 08:51:58 -0700 Subject: [PATCH 1/5] fix(nvsnap): recreate the source's runtime directories on warm restore A workload started with a shell preamble that creates a runtime directory fails on warm restore. vLLM TP=4 dies ~55s in with zmq.error.ZMQError: No such file or directory for ipc path "/var/run/vllm/" because /var/run/vllm does not exist in the restored container. The setup step is missing because restore execs the capture-recorded entry argv, read from /proc//cmdline -- the process image AFTER any exec. Bash runs the mkdir as a child and then, via its last-command exec optimization, replaces itself with the engine, so at capture time PID 1 is the engine and the mkdir is nowhere in the process image. The recorded manifest confirms it: entry_argv starts at /usr/bin/python3 with no bash and no mkdir, and the restored pod logs "APIServer pid=1". Preferring the Pod's command/args instead is not viable -- ENTRYPOINT-only images (NIM, whisper) carry only args, or nothing, in the Pod spec, so exec'ing those drops the image entrypoint binary. That is already documented in rootfs_l2_overlay.go from a previously observed failure. Since the commands are unrecoverable, record their result: capture walks the source container's /run and /var/run and stamps the directories, with mode and ownership, into the manifest; the restore shim recreates them before exec on both warm paths. Bounded to 64 entries and depth 4, and scoped to those two roots -- they hold runtime scaffolding rather than data, so recreating them empty is cheap and cannot mask a missing volume. Captures taken before this keep working: the env var is absent and the shim skips the step. Closes #942 Co-Authored-By: Balaji Ganesan --- .../nvsnap/cmd/nvsnap-rootfs-restore/main.go | 65 ++++++++++++++++ .../cmd/nvsnap-rootfs-restore/main_test.go | 51 +++++++++++++ .../nvsnap/internal/checkpointstore/store.go | 28 +++++++ .../internal/rootfsonly/orchestrator.go | 74 +++++++++++++++++++ .../nvsnap/internal/webhook/cachedir.go | 1 + .../internal/webhook/restore_entrypoint.go | 26 +++++++ .../internal/webhook/rootfs_l2_overlay.go | 1 + 7 files changed, 246 insertions(+) diff --git a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go index ca951b7b1..98bdd3068 100644 --- a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go +++ b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go @@ -74,6 +74,10 @@ import ( const mergedRoot = "/nvsnap-merged" +// envRuntimeDirs carries the capture's EntryRuntimeDirs as a JSON array. +// Must match the name the webhook injects. +const envRuntimeDirs = "NVSNAP_RUNTIME_DIRS" + type volMount struct { Name string `json:"name"` MountPath string `json:"mountPath"` @@ -129,6 +133,8 @@ func runNoOverlay() error { fmt.Fprintln(os.Stderr, "nvsnap-rootfs-restore: page-cache prewarm disabled (NVSNAP_PREWARM=0)") } + recreateRuntimeDirs(os.Getenv) + if err := unix.Chdir(cwd); err != nil { if err2 := unix.Chdir("/"); err2 != nil { return fmt.Errorf("chdir %q (and / fallback): %w", cwd, err2) @@ -145,6 +151,62 @@ func runNoOverlay() error { return nil } +// runtimeDir mirrors checkpointstore.EntryRuntimeDir. Declared here rather +// than imported so the shim stays a standalone static binary the webhook can +// drop into any workload image. +type runtimeDir struct { + Path string `json:"path"` + Mode uint32 `json:"mode"` + UID uint32 `json:"uid"` + GID uint32 `json:"gid"` +} + +// recreateRuntimeDirs recreates the ephemeral directories the source +// container had before its entrypoint ran. +// +// The commands that created them are not recoverable: a container started as +// `bash -c 'mkdir -p /var/run/vllm; vllm serve ...'` runs the mkdir as a child +// and then replaces itself with vllm, so /proc/1/cmdline at capture time holds +// only the engine. Restoring into a pristine container therefore leaves the +// directory missing, and anything binding a unix socket under it fails with +// ENOENT (vLLM's ZMQ IPC socket). +// +// Best-effort by design: most workloads need none of these, so a directory we +// cannot create is reported and skipped rather than failing the restore. +func recreateRuntimeDirs(getenv func(string) string) { + raw := getenv(envRuntimeDirs) + if raw == "" { + return + } + var dirs []runtimeDir + if err := json.Unmarshal([]byte(raw), &dirs); err != nil { + fmt.Fprintf(os.Stderr, "nvsnap-rootfs-restore: ignoring malformed %s: %v\n", envRuntimeDirs, err) + return + } + for _, d := range dirs { + if d.Path == "" || !filepath.IsAbs(d.Path) || strings.Contains(d.Path, "..") { + continue + } + mode := os.FileMode(d.Mode).Perm() + if mode == 0 { + mode = 0o755 + } + if err := os.MkdirAll(d.Path, mode); err != nil { + fmt.Fprintf(os.Stderr, "nvsnap-rootfs-restore: runtime dir %s: %v\n", d.Path, err) + continue + } + // MkdirAll applies the umask, so set the recorded mode explicitly -- + // a group-writable runtime dir must stay writable for a workload that + // drops privileges after start. + if err := os.Chmod(d.Path, mode); err != nil { + fmt.Fprintf(os.Stderr, "nvsnap-rootfs-restore: chmod %s: %v\n", d.Path, err) + } + if err := os.Chown(d.Path, int(d.UID), int(d.GID)); err != nil { + fmt.Fprintf(os.Stderr, "nvsnap-rootfs-restore: chown %s: %v\n", d.Path, err) + } + } +} + func parseConfig(getenv func(string) string) (config, error) { var c config c.capturedDir = getenv("NVSNAP_CAPTURED_DIR") @@ -374,6 +436,9 @@ func run() error { } _ = os.Remove("/.nvsnap-oldroot") + // After pivot_root, so the paths resolve inside the restored tree. + recreateRuntimeDirs(os.Getenv) + // chdir into the captured working directory so the entrypoint's // relative paths resolve as they did pre-capture. Fall back to "/" // if the recorded cwd no longer exists in the merged tree. diff --git a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go index eb9e6b76f..8a714492b 100644 --- a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go +++ b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go @@ -18,6 +18,8 @@ limitations under the License. package main import ( + "os" + "path/filepath" "strings" "testing" ) @@ -130,3 +132,52 @@ func TestParseMountpoints(t *testing.T) { } } } + +// TestRecreateRuntimeDirs covers the vLLM ENOENT case: the source container's +// entrypoint created /var/run/vllm before starting, bash exec'd itself away so +// the mkdir is absent from the recorded argv, and restore must recreate the +// directory or the engine's unix socket bind fails. +func TestRecreateRuntimeDirs(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "var", "run", "vllm") + + env := func(k string) string { + if k == envRuntimeDirs { + return `[{"path":"` + target + `","mode":493,"uid":0,"gid":0}]` + } + return "" + } + recreateRuntimeDirs(env) + + fi, err := os.Stat(target) + if err != nil { + t.Fatalf("runtime dir not recreated: %v", err) + } + if !fi.IsDir() { + t.Fatalf("%s is not a directory", target) + } + if got := fi.Mode().Perm(); got != 0o755 { + t.Errorf("mode = %o, want 755 (umask must not narrow it)", got) + } +} + +// Malformed or hostile input must not abort a restore: the workload may not +// need these directories at all, so every one of these is a skip, not a fail. +func TestRecreateRuntimeDirsIgnoresBadInput(t *testing.T) { + for name, val := range map[string]string{ + "empty": "", + "not json": "{{{", + "wrong type": `{"path":"/x"}`, + "relative": `[{"path":"var/run/x","mode":493}]`, + "parent escape": `[{"path":"/tmp/../etc/x","mode":493}]`, + } { + t.Run(name, func(t *testing.T) { + recreateRuntimeDirs(func(k string) string { + if k == envRuntimeDirs { + return val + } + return "" + }) + }) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go index e0f6ec0c0..63e2ffc13 100644 --- a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go +++ b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go @@ -258,6 +258,34 @@ type Manifest struct { // time. The shim chdir()s here before exec so relative paths in the // entrypoint resolve as they did pre-capture. Empty → "/". EntryCwd string `json:"entry_cwd,omitempty"` + + // EntryRuntimeDirs are directories that existed under the source + // container's ephemeral runtime roots (/run, /var/run) at capture time. + // + // EntryArgv is /proc//cmdline, which is the process image AFTER any + // exec. A container started as `bash -c 'mkdir -p /var/run/vllm; vllm + // serve ...'` runs the mkdir as a child and then, via bash's last-command + // exec optimization, REPLACES itself with vllm -- so by capture time the + // mkdir is no longer anywhere in the process image and cannot be + // recovered from argv. Restore then execs into a pristine container where + // the directory does not exist, and anything binding a unix socket there + // fails with ENOENT (vLLM's ZMQ IPC socket, observed). + // + // We cannot replay the setup commands (they are gone), and we cannot + // prefer the Pod's command/args instead -- ENTRYPOINT-only images carry + // only args, or nothing, in the Pod spec. So we record the directories + // themselves and recreate them before exec. + EntryRuntimeDirs []EntryRuntimeDir `json:"entry_runtime_dirs,omitempty"` +} + +// EntryRuntimeDir is one directory to recreate in the restored container +// before the entrypoint is exec'd. Mode and ownership are carried so a +// workload running as a non-root UID can still write inside it. +type EntryRuntimeDir struct { + Path string `json:"path"` + Mode uint32 `json:"mode"` + UID uint32 `json:"uid"` + GID uint32 `json:"gid"` } // VolumeMeta is a single captured volume's metadata. Volume.Name == "rootfs" diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go index b065d20ad..99612051d 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go @@ -21,10 +21,12 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "strconv" "strings" + "syscall" "github.com/sirupsen/logrus" "go.opentelemetry.io/otel/attribute" @@ -306,6 +308,11 @@ func (c *Capturer) Capture(ctx context.Context, req CaptureRequest) (checkpoints } else { log.Warn("could not read source /proc//cmdline; restore will rely on the pod's command/args") } + entryRuntimeDirs := readEntryRuntimeDirs(procRoot, entryPID) + if len(entryRuntimeDirs) > 0 { + log.WithField("runtime_dirs", len(entryRuntimeDirs)). + Debug("recorded source runtime directories for restore") + } // Authoritative capture-method stamp so the restore side dispatches // deterministically (no inferring from manifest shape — the @@ -344,6 +351,7 @@ func (c *Capturer) Capture(ctx context.Context, req CaptureRequest) (checkpoints TotalSizeBytes: totalSize, FileCount: totalFiles, EntryArgv: entryArgv, + EntryRuntimeDirs: entryRuntimeDirs, } if c.NodeName != "" { manifest.CapturedOnNodes = []string{c.NodeName} @@ -634,6 +642,72 @@ func (c *Capturer) logger() logrus.FieldLogger { // NUL-terminated args. Best-effort: returns nil on any read error or // empty cmdline (kernel threads, races), in which case the restore // webhook falls back to the restored pod's explicit command/args. +// runtimeDirRoots are the ephemeral trees a container conventionally creates +// socket/PID directories in before the workload starts. Deliberately narrow: +// these hold runtime scaffolding rather than data, so recreating them empty is +// cheap and cannot mask a missing volume. /tmp is excluded -- it is large, +// noisy, and its contents are not addressable the way a socket path is. +var runtimeDirRoots = []string{"/run", "/var/run"} + +const ( + // Bounds on the recorded set. A runtime tree is normally a handful of + // shallow directories; anything beyond this is a workload using /run as + // scratch, which we do not try to reproduce. + maxRuntimeDirs = 64 + maxRuntimeDepth = 4 +) + +// readEntryRuntimeDirs records directories under the source container's +// ephemeral runtime roots so restore can recreate them. See the +// EntryRuntimeDirs doc comment for why argv cannot supply this. +// +// Best-effort throughout: a container with no /run, an unreadable tree, or a +// racing teardown yields fewer entries rather than a failed capture. +func readEntryRuntimeDirs(procRoot string, pid int) []checkpointstore.EntryRuntimeDir { + containerRoot := filepath.Join(procRoot, strconv.Itoa(pid), "root") + var out []checkpointstore.EntryRuntimeDir + seen := make(map[string]bool) // /var/run is usually a symlink to /run + + for _, root := range runtimeDirRoots { + hostRoot := filepath.Join(containerRoot, root) + resolved, err := filepath.EvalSymlinks(hostRoot) + if err != nil { + continue + } + _ = filepath.WalkDir(resolved, func(p string, d fs.DirEntry, err error) error { + if err != nil || !d.IsDir() { + return nil //nolint:nilerr // unreadable subtree is not fatal + } + rel, rerr := filepath.Rel(resolved, p) + if rerr != nil || rel == "." { + return nil + } + if strings.Count(rel, string(filepath.Separator)) >= maxRuntimeDepth { + return fs.SkipDir + } + inContainer := filepath.Join(root, rel) + if seen[inContainer] || len(out) >= maxRuntimeDirs { + return nil + } + info, ierr := d.Info() + if ierr != nil { + return nil + } + rd := checkpointstore.EntryRuntimeDir{ + Path: inContainer, + Mode: uint32(info.Mode().Perm()), + } + if st, ok := info.Sys().(*syscall.Stat_t); ok { + rd.UID, rd.GID = st.Uid, st.Gid + } + seen[inContainer] = true + out = append(out, rd) + return nil + }) + } + return out +} + func readEntryArgv(procRoot string, pid int) []string { data, err := os.ReadFile(filepath.Join(procRoot, strconv.Itoa(pid), "cmdline")) if err != nil || len(data) == 0 { diff --git a/src/compute-plane-services/nvsnap/internal/webhook/cachedir.go b/src/compute-plane-services/nvsnap/internal/webhook/cachedir.go index 13509d5fc..699ad879f 100644 --- a/src/compute-plane-services/nvsnap/internal/webhook/cachedir.go +++ b/src/compute-plane-services/nvsnap/internal/webhook/cachedir.go @@ -426,6 +426,7 @@ func (m *Mutator) tryL2CacheDir(ctx context.Context, pod *corev1.Pod, hash strin corev1.EnvVar{Name: "NVSNAP_NO_OVERLAY", Value: "1"}, corev1.EnvVar{Name: "NVSNAP_PREWARM_DIR", Value: m.CacheDir}, corev1.EnvVar{Name: "NVSNAP_ORIG_COMMAND", Value: string(argvJSON)}, + corev1.EnvVar{Name: envRuntimeDirs, Value: runtimeDirsJSON(manifest.EntryRuntimeDirs)}, corev1.EnvVar{Name: "NVSNAP_ORIG_CWD", Value: manifest.EntryCwd}, // NOTE: do NOT set HF_HUB_OFFLINE here. It only suppresses benign HF // negative-cache (.no_exist) warnings, but vLLM's arg_utils keys off diff --git a/src/compute-plane-services/nvsnap/internal/webhook/restore_entrypoint.go b/src/compute-plane-services/nvsnap/internal/webhook/restore_entrypoint.go index 8bf13bd43..8a6fad83c 100644 --- a/src/compute-plane-services/nvsnap/internal/webhook/restore_entrypoint.go +++ b/src/compute-plane-services/nvsnap/internal/webhook/restore_entrypoint.go @@ -90,6 +90,8 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + + "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/checkpointstore" ) const ( @@ -114,6 +116,11 @@ const ( // safely express. DefaultHostBundleRoot = "/var/lib/nvsnap/bundle" + // envRuntimeDirs carries the capture's recorded runtime directories to the + // restore shim, which recreates them before exec. Must match the constant + // in cmd/nvsnap-rootfs-restore. + envRuntimeDirs = "NVSNAP_RUNTIME_DIRS" + // envOrigCommand and envOrigArgs are read by restore-entrypoint's // cold-start fallback path (cmd/restore-entrypoint/main.go). // Values are JSON-encoded string arrays — empty arrays / unset @@ -417,3 +424,22 @@ func hasCapability(list []corev1.Capability, c corev1.Capability) bool { } return false } + +// runtimeDirsJSON encodes the capture's recorded runtime directories for the +// restore shim. Returns "" when there are none, so the env var is present but +// empty and the shim skips the step -- and so captures taken before this was +// recorded keep working unchanged. +// +// Marshal cannot fail for this type; on the impossible error we return "" and +// let restore proceed, since a missing runtime dir degrades one workload +// rather than failing every restore. +func runtimeDirsJSON(dirs []checkpointstore.EntryRuntimeDir) string { + if len(dirs) == 0 { + return "" + } + b, err := json.Marshal(dirs) + if err != nil { + return "" + } + return string(b) +} diff --git a/src/compute-plane-services/nvsnap/internal/webhook/rootfs_l2_overlay.go b/src/compute-plane-services/nvsnap/internal/webhook/rootfs_l2_overlay.go index f0075a7a4..ca1e98d07 100644 --- a/src/compute-plane-services/nvsnap/internal/webhook/rootfs_l2_overlay.go +++ b/src/compute-plane-services/nvsnap/internal/webhook/rootfs_l2_overlay.go @@ -300,6 +300,7 @@ func (m *Mutator) tryL2RootfsOverlay(ctx context.Context, pod *corev1.Pod, hash {Name: "NVSNAP_CAPTURED_DIR", Value: capturedMountPath}, {Name: "NVSNAP_SCRATCH_DIR", Value: scratchMountPath}, {Name: "NVSNAP_ORIG_COMMAND", Value: string(argvJSON)}, + {Name: envRuntimeDirs, Value: runtimeDirsJSON(manifest.EntryRuntimeDirs)}, {Name: "NVSNAP_ORIG_CWD", Value: manifest.EntryCwd}, {Name: "NVSNAP_ROOTFS_VOLUMES", Value: string(volsJSON)}, } { From 600a81e209a505ea37e2cda07263e3b2203a0e8a Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 09:28:51 -0700 Subject: [PATCH 2/5] fix(nvsnap): keep the runtime-dir walk inside the container and bound it Review found three problems with the runtime-directory capture. The walk canonicalized its root with EvalSymlinks. /proc//root is a magic link, not an ordinary symlink, so resolving it rewrites the path to the equivalent HOST path: the walk left the container's view and would have recorded the node's own /run tree as if it belonged to the pod, then recreated those directories inside restored containers. Walk the /proc//root path as given instead. WalkDir does not follow symlinks, which also gives the /var/run -> /run dedupe for free. The entry cap only stopped recording, not walking, so a workload with a large /run still paid the full traversal. Return SkipAll at the cap. Restore widened an explicitly recorded 0000 directory to 0755, losing the source's access policy. Create with 0755 so intermediate parents are traversable, then chmod to the recorded mode. Restore also guarded paths with a "contains .." check, which is both too weak and beside the point: this runs as root in the workload's mount namespace, so it must not be a general create-any-directory primitive. Confine it to the same roots capture collects from and require an already clean path. The test that was supposed to cover traversal built its input with filepath.Join, which normalizes ".." away -- so it asserted nothing and passed against a guard that did not block traversal. Build the string by concatenation, assert the escape target is absent afterwards, and cover outside-root and prefix-lookalike inputs. Adds a high-fanout test for the cap and depth/missing-root tests for the walk. Co-Authored-By: Balaji Ganesan --- .../nvsnap/cmd/nvsnap-rootfs-restore/main.go | 44 +++++++-- .../cmd/nvsnap-rootfs-restore/main_test.go | 75 +++++++++----- .../rootfsonly/entry_runtime_dirs_test.go | 98 +++++++++++++++++++ .../internal/rootfsonly/orchestrator.go | 25 +++-- 4 files changed, 205 insertions(+), 37 deletions(-) create mode 100644 src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go diff --git a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go index 98bdd3068..53157413b 100644 --- a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go +++ b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main.go @@ -133,7 +133,7 @@ func runNoOverlay() error { fmt.Fprintln(os.Stderr, "nvsnap-rootfs-restore: page-cache prewarm disabled (NVSNAP_PREWARM=0)") } - recreateRuntimeDirs(os.Getenv) + recreateRuntimeDirs(os.Getenv, runtimeDirRoots) if err := unix.Chdir(cwd); err != nil { if err2 := unix.Chdir("/"); err2 != nil { @@ -151,6 +151,27 @@ func runNoOverlay() error { return nil } +// runtimeDirRoots are the only trees the shim will create directories in. +// Must match the roots the capture side collects from +// (internal/rootfsonly.runtimeDirRoots). +var runtimeDirRoots = []string{"/run", "/var/run"} + +// underAllowedRoot reports whether p is an absolute, already-clean path at or +// below one of roots. Requiring the path to be clean is what rejects traversal: +// "/run/../etc" is not equal to its own Clean(), so it never reaches the +// prefix check. +func underAllowedRoot(p string, roots []string) bool { + if p == "" || !filepath.IsAbs(p) || filepath.Clean(p) != p { + return false + } + for _, r := range roots { + if p == r || strings.HasPrefix(p, r+string(filepath.Separator)) { + return true + } + } + return false +} + // runtimeDir mirrors checkpointstore.EntryRuntimeDir. Declared here rather // than imported so the shim stays a standalone static binary the webhook can // drop into any workload image. @@ -173,7 +194,7 @@ type runtimeDir struct { // // Best-effort by design: most workloads need none of these, so a directory we // cannot create is reported and skipped rather than failing the restore. -func recreateRuntimeDirs(getenv func(string) string) { +func recreateRuntimeDirs(getenv func(string) string, allowedRoots []string) { raw := getenv(envRuntimeDirs) if raw == "" { return @@ -184,14 +205,21 @@ func recreateRuntimeDirs(getenv func(string) string) { return } for _, d := range dirs { - if d.Path == "" || !filepath.IsAbs(d.Path) || strings.Contains(d.Path, "..") { + // Confine to the roots capture collects from, and require an already + // clean path. This runs as root in the workload's mount namespace, so + // it must not be a general "create any directory" primitive just + // because the manifest asked for one. + if !underAllowedRoot(d.Path, allowedRoots) { + fmt.Fprintf(os.Stderr, "nvsnap-rootfs-restore: refusing runtime dir outside the allowed roots: %q\n", d.Path) continue } + // Capture always serializes Mode, so 0 means the source directory + // really was 0000 -- reproduce it rather than widening to 0755 and + // quietly loosening the source's access policy. MkdirAll still needs a + // traversable mode to create intermediate parents, so create with 0755 + // and narrow to the recorded mode immediately afterwards. mode := os.FileMode(d.Mode).Perm() - if mode == 0 { - mode = 0o755 - } - if err := os.MkdirAll(d.Path, mode); err != nil { + if err := os.MkdirAll(d.Path, 0o755); err != nil { fmt.Fprintf(os.Stderr, "nvsnap-rootfs-restore: runtime dir %s: %v\n", d.Path, err) continue } @@ -437,7 +465,7 @@ func run() error { _ = os.Remove("/.nvsnap-oldroot") // After pivot_root, so the paths resolve inside the restored tree. - recreateRuntimeDirs(os.Getenv) + recreateRuntimeDirs(os.Getenv, runtimeDirRoots) // chdir into the captured working directory so the entrypoint's // relative paths resolve as they did pre-capture. Fall back to "/" diff --git a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go index 8a714492b..5684890ba 100644 --- a/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go +++ b/src/compute-plane-services/nvsnap/cmd/nvsnap-rootfs-restore/main_test.go @@ -138,16 +138,10 @@ func TestParseMountpoints(t *testing.T) { // the mkdir is absent from the recorded argv, and restore must recreate the // directory or the engine's unix socket bind fails. func TestRecreateRuntimeDirs(t *testing.T) { - root := t.TempDir() - target := filepath.Join(root, "var", "run", "vllm") + sandbox := t.TempDir() + target := filepath.Join(sandbox, "vllm") - env := func(k string) string { - if k == envRuntimeDirs { - return `[{"path":"` + target + `","mode":493,"uid":0,"gid":0}]` - } - return "" - } - recreateRuntimeDirs(env) + recreateRuntimeDirs(envFunc(`[{"path":"`+target+`","mode":493,"uid":0,"gid":0}]`), []string{sandbox}) fi, err := os.Stat(target) if err != nil { @@ -161,23 +155,60 @@ func TestRecreateRuntimeDirs(t *testing.T) { } } -// Malformed or hostile input must not abort a restore: the workload may not -// need these directories at all, so every one of these is a skip, not a fail. +// A recorded 0000 directory must come back as 0000, not widened to 0755. +func TestRecreateRuntimeDirsPreservesZeroMode(t *testing.T) { + sandbox := t.TempDir() + target := filepath.Join(sandbox, "locked") + + recreateRuntimeDirs(envFunc(`[{"path":"`+target+`","mode":0,"uid":0,"gid":0}]`), []string{sandbox}) + + fi, err := os.Stat(target) + if err != nil { + t.Fatalf("dir not created: %v", err) + } + if got := fi.Mode().Perm(); got != 0 { + t.Errorf("mode = %o, want 0 (an explicit 0000 must not be widened)", got) + } +} + +// Malformed or hostile input must not abort a restore, and must not create +// anything. The traversal string is built by concatenation, NOT filepath.Join, +// because Join normalizes ".." away and would silently make this a clean path +// that legitimately gets created -- which is exactly how an earlier version of +// this test passed against a guard that did not actually block traversal. func TestRecreateRuntimeDirsIgnoresBadInput(t *testing.T) { + sandbox := t.TempDir() + escapeTarget := filepath.Join(sandbox, "etc", "pwn") + traversal := sandbox + "/run/../etc/pwn" + outside := filepath.Join(t.TempDir(), "elsewhere") + for name, val := range map[string]string{ - "empty": "", - "not json": "{{{", - "wrong type": `{"path":"/x"}`, - "relative": `[{"path":"var/run/x","mode":493}]`, - "parent escape": `[{"path":"/tmp/../etc/x","mode":493}]`, + "empty": "", + "not json": "{{{", + "wrong type": `{"path":"/x"}`, + "relative": `[{"path":"var/run/x","mode":493}]`, + "parent escape": `[{"path":"` + traversal + `","mode":493}]`, + "outside root": `[{"path":"` + outside + `","mode":493}]`, + "root itself": `[{"path":"/","mode":493}]`, + "prefix look-al": `[{"path":"` + sandbox + `-evil/x","mode":493}]`, } { t.Run(name, func(t *testing.T) { - recreateRuntimeDirs(func(k string) string { - if k == envRuntimeDirs { - return val - } - return "" - }) + recreateRuntimeDirs(envFunc(val), []string{sandbox}) }) } + + for _, p := range []string{escapeTarget, outside, sandbox + "-evil/x"} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("created %s (err=%v); the path guard regressed", p, err) + } + } +} + +func envFunc(val string) func(string) string { + return func(k string) string { + if k == envRuntimeDirs { + return val + } + return "" + } } diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go new file mode 100644 index 000000000..a5d5af74e --- /dev/null +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go @@ -0,0 +1,98 @@ +/* +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 rootfsonly + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/checkpointstore" +) + +// fakeContainerRoot builds a //root tree and returns procRoot. +func fakeContainerRoot(t *testing.T, pid string, dirs ...string) string { + t.Helper() + procRoot := t.TempDir() + for _, d := range dirs { + if err := os.MkdirAll(filepath.Join(procRoot, pid, "root", d), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", d, err) + } + } + return procRoot +} + +func recordedPaths(dirs []checkpointstore.EntryRuntimeDir) map[string]bool { + out := make(map[string]bool, len(dirs)) + for _, d := range dirs { + out[d.Path] = true + } + return out +} + +func TestReadEntryRuntimeDirs(t *testing.T) { + procRoot := fakeContainerRoot(t, "7", + "run/vllm", "run/lock/sub", "var/run/other") + + got := recordedPaths(readEntryRuntimeDirs(procRoot, 7)) + + for _, want := range []string{"/run/vllm", "/run/lock", "/run/lock/sub", "/var/run/other"} { + if !got[want] { + t.Errorf("missing %s; got %v", want, got) + } + } +} + +// The depth bound must prune rather than record arbitrarily deep trees. +func TestReadEntryRuntimeDirsRespectsDepth(t *testing.T) { + procRoot := fakeContainerRoot(t, "7", "run/a/b/c/d/e/f") + + for p := range recordedPaths(readEntryRuntimeDirs(procRoot, 7)) { + if p == "/run/a/b/c/d/e" || p == "/run/a/b/c/d/e/f" { + t.Errorf("recorded %s beyond the depth bound", p) + } + } +} + +// A workload can put a large tree under /run. The cap must bound what we +// record; the walk terminating (rather than continuing and discarding) is what +// keeps capture latency bounded, and the observable contract is that we stop at +// exactly maxRuntimeDirs. +func TestReadEntryRuntimeDirsStopsAtCap(t *testing.T) { + dirs := make([]string, 0, maxRuntimeDirs*4) + for i := 0; i < maxRuntimeDirs*4; i++ { + dirs = append(dirs, fmt.Sprintf("run/d%03d", i)) + } + procRoot := fakeContainerRoot(t, "7", dirs...) + + got := readEntryRuntimeDirs(procRoot, 7) + if len(got) > maxRuntimeDirs { + t.Fatalf("recorded %d dirs, want at most %d", len(got), maxRuntimeDirs) + } +} + +// A missing /run (or an unreadable one) yields no entries rather than failing +// the capture: most workloads need none of this. +func TestReadEntryRuntimeDirsMissingRoots(t *testing.T) { + procRoot := fakeContainerRoot(t, "7", "opt/only") + + if got := readEntryRuntimeDirs(procRoot, 7); len(got) != 0 { + t.Errorf("got %v, want none", got) + } +} diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go index 99612051d..c1f0a9b7c 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go @@ -669,24 +669,35 @@ func readEntryRuntimeDirs(procRoot string, pid int) []checkpointstore.EntryRunti seen := make(map[string]bool) // /var/run is usually a symlink to /run for _, root := range runtimeDirRoots { + // Walk the /proc//root path as-is. It must NOT be canonicalized: + // /proc//root is a magic link, so EvalSymlinks (and realpath, and + // anything else that resolves it) rewrites it to the equivalent HOST + // path, which silently takes the walk outside the container's view and + // would record the node's own runtime tree as if it were the pod's. + // + // WalkDir does not follow symlinks, which also gives the /var/run -> + // /run dedupe for free: on a distro where /var/run is a symlink it is + // reported once as a non-directory and never descended into. hostRoot := filepath.Join(containerRoot, root) - resolved, err := filepath.EvalSymlinks(hostRoot) - if err != nil { - continue - } - _ = filepath.WalkDir(resolved, func(p string, d fs.DirEntry, err error) error { + _ = filepath.WalkDir(hostRoot, func(p string, d fs.DirEntry, err error) error { if err != nil || !d.IsDir() { return nil //nolint:nilerr // unreadable subtree is not fatal } - rel, rerr := filepath.Rel(resolved, p) + rel, rerr := filepath.Rel(hostRoot, p) if rerr != nil || rel == "." { return nil } if strings.Count(rel, string(filepath.Separator)) >= maxRuntimeDepth { return fs.SkipDir } + // Stop the whole walk at the cap rather than skipping entries: a + // workload can put a large tree under /run, and continuing to + // traverse it would add capture latency for entries we discard. + if len(out) >= maxRuntimeDirs { + return fs.SkipAll + } inContainer := filepath.Join(root, rel) - if seen[inContainer] || len(out) >= maxRuntimeDirs { + if seen[inContainer] { return nil } info, ierr := d.Info() From dfe680b17a39e8cd9e5ef47dc9de36dd2a24da47 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 09:36:57 -0700 Subject: [PATCH 3/5] build(nvsnap): register the runtime-dirs test in BUILD.bazel The BUILD-files-match-their-sources check regenerates with gazelle and diffs; the new test file was missing from go_test srcs. checkpointstore is already in deps, so srcs is the only change. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/rootfsonly/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/BUILD.bazel b/src/compute-plane-services/nvsnap/internal/rootfsonly/BUILD.bazel index 9369941ab..bb4f5f55f 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/BUILD.bazel +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/BUILD.bazel @@ -35,6 +35,7 @@ go_test( "classify_test.go", "composer_test.go", "entry_argv_test.go", + "entry_runtime_dirs_test.go", "enumerate_test.go", "orchestrator_test.go", "pidresolver_test.go", From 10195fe40cde818d9814c862193a05d9d321a841 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 11:36:07 -0700 Subject: [PATCH 4/5] fix(nvsnap): bump CaptureFormatVersion for the EntryRuntimeDirs schema change CaptureFormatVersion feeds the capture hash precisely so a manifest schema change invalidates older captures. Adding EntryRuntimeDirs changed the schema without bumping it, so a capture taken before the fix hashes identically to one taken after. The consequence is worse than a stale artifact. On upgrade the agent finds the old hash, short-circuits with "capture skipped: hash already exists", and reports a successful commit of zero files -- so every existing deployment would keep replaying pre-fix captures, and the workloads this change exists to fix would keep failing with no signal as to why. Bumping to 2 makes captures taken by the fixed agent hash differently, so they are retaken once and the recorded runtime directories are present. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/checkpointstore/store.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go index 63e2ffc13..3b9f9a8b5 100644 --- a/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go +++ b/src/compute-plane-services/nvsnap/internal/checkpointstore/store.go @@ -35,7 +35,12 @@ 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: 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 // ErrNotFound is returned by Stat / Get when no capture is stored under the // given hash. From 5a89784ea0e9e3101f17a53d9d1af79794a5ae06 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 18 Aug 2026 13:53:53 -0700 Subject: [PATCH 5/5] test(nvsnap): assert the runtime-dir walk's metadata and exact bounds Review found the tests could pass against a walk that did less than it should. The metadata was discarded before asserting, so recording zero mode or ownership went unnoticed -- and restore recreates directories from those fields, so a zero mode produces a directory the workload cannot write to. Set a non-default mode on a fixture and compare mode, uid and gid against the source. The depth test only asserted absence beyond the bound, and the cap test accepted any count up to the maximum. A walk that stopped early, or recorded nothing at all, satisfied both. Assert the boundary directory is present, and that the cap yields exactly maxRuntimeDirs. Also drop the pid parameter that every caller passed the same value for, which the unparam linter flags. Co-Authored-By: Balaji Ganesan --- .../rootfsonly/entry_runtime_dirs_test.go | 92 +++++++++++++------ 1 file changed, 66 insertions(+), 26 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go index a5d5af74e..907ac369b 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/entry_runtime_dirs_test.go @@ -21,78 +21,118 @@ import ( "fmt" "os" "path/filepath" + "sort" + "strconv" + "syscall" "testing" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvsnap/internal/checkpointstore" ) -// fakeContainerRoot builds a //root tree and returns procRoot. -func fakeContainerRoot(t *testing.T, pid string, dirs ...string) string { +// fakeContainerRoot builds a //root tree and returns +// procRoot. The PID is fixed: nothing under test varies with it. +const fixturePID = 7 + +func fakeContainerRoot(t *testing.T, dirs ...string) string { t.Helper() procRoot := t.TempDir() for _, d := range dirs { - if err := os.MkdirAll(filepath.Join(procRoot, pid, "root", d), 0o755); err != nil { + if err := os.MkdirAll(filepath.Join(procRoot, strconv.Itoa(fixturePID), "root", d), 0o755); err != nil { t.Fatalf("mkdir %s: %v", d, err) } } return procRoot } -func recordedPaths(dirs []checkpointstore.EntryRuntimeDir) map[string]bool { - out := make(map[string]bool, len(dirs)) +func byPath(dirs []checkpointstore.EntryRuntimeDir) map[string]checkpointstore.EntryRuntimeDir { + out := make(map[string]checkpointstore.EntryRuntimeDir, len(dirs)) for _, d := range dirs { - out[d.Path] = true + out[d.Path] = d } return out } +// Recording the paths is not enough: restore recreates these directories with +// the recorded mode and ownership, so metadata that is silently zero would +// produce directories the workload cannot write to. func TestReadEntryRuntimeDirs(t *testing.T) { - procRoot := fakeContainerRoot(t, "7", - "run/vllm", "run/lock/sub", "var/run/other") + procRoot := fakeContainerRoot(t, "run/vllm", "run/lock/sub", "var/run/other") + + // A non-default mode the umask would not produce by accident. + src := filepath.Join(procRoot, strconv.Itoa(fixturePID), "root", "run", "vllm") + if err := os.Chmod(src, 0o731); err != nil { + t.Fatalf("chmod: %v", err) + } + var st syscall.Stat_t + if err := syscall.Stat(src, &st); err != nil { + t.Fatalf("stat: %v", err) + } - got := recordedPaths(readEntryRuntimeDirs(procRoot, 7)) + got := byPath(readEntryRuntimeDirs(procRoot, fixturePID)) for _, want := range []string{"/run/vllm", "/run/lock", "/run/lock/sub", "/var/run/other"} { - if !got[want] { - t.Errorf("missing %s; got %v", want, got) + if _, ok := got[want]; !ok { + t.Errorf("missing %s; got %v", want, keys(got)) } } + d, ok := got["/run/vllm"] + if !ok { + t.Fatal("/run/vllm not recorded") + } + if d.Mode != 0o731 { + t.Errorf("Mode = %o, want 731 (restore recreates with this)", d.Mode) + } + if d.UID != st.Uid || d.GID != st.Gid { + t.Errorf("UID/GID = %d/%d, want %d/%d (source ownership)", d.UID, d.GID, st.Uid, st.Gid) + } } -// The depth bound must prune rather than record arbitrarily deep trees. +// The depth bound must prune at the boundary, not before it: a walker that +// stopped one level early would satisfy an absence-only assertion. func TestReadEntryRuntimeDirsRespectsDepth(t *testing.T) { - procRoot := fakeContainerRoot(t, "7", "run/a/b/c/d/e/f") + procRoot := fakeContainerRoot(t, "run/a/b/c/d/e/f") - for p := range recordedPaths(readEntryRuntimeDirs(procRoot, 7)) { - if p == "/run/a/b/c/d/e" || p == "/run/a/b/c/d/e/f" { - t.Errorf("recorded %s beyond the depth bound", p) + got := byPath(readEntryRuntimeDirs(procRoot, fixturePID)) + + if _, ok := got["/run/a/b/c/d"]; !ok { + t.Errorf("/run/a/b/c/d is within the depth bound and must be recorded; got %v", keys(got)) + } + for _, tooDeep := range []string{"/run/a/b/c/d/e", "/run/a/b/c/d/e/f"} { + if _, ok := got[tooDeep]; ok { + t.Errorf("recorded %s beyond the depth bound", tooDeep) } } } -// A workload can put a large tree under /run. The cap must bound what we -// record; the walk terminating (rather than continuing and discarding) is what -// keeps capture latency bounded, and the observable contract is that we stop at -// exactly maxRuntimeDirs. +// A workload can put a large tree under /run. Assert the exact cap: accepting +// "at most maxRuntimeDirs" would also pass for a walk that recorded nothing. func TestReadEntryRuntimeDirsStopsAtCap(t *testing.T) { dirs := make([]string, 0, maxRuntimeDirs*4) for i := 0; i < maxRuntimeDirs*4; i++ { dirs = append(dirs, fmt.Sprintf("run/d%03d", i)) } - procRoot := fakeContainerRoot(t, "7", dirs...) + procRoot := fakeContainerRoot(t, dirs...) - got := readEntryRuntimeDirs(procRoot, 7) - if len(got) > maxRuntimeDirs { - t.Fatalf("recorded %d dirs, want at most %d", len(got), maxRuntimeDirs) + if got := readEntryRuntimeDirs(procRoot, fixturePID); len(got) != maxRuntimeDirs { + t.Fatalf("recorded %d dirs, want exactly %d", len(got), maxRuntimeDirs) } } // A missing /run (or an unreadable one) yields no entries rather than failing // the capture: most workloads need none of this. func TestReadEntryRuntimeDirsMissingRoots(t *testing.T) { - procRoot := fakeContainerRoot(t, "7", "opt/only") + procRoot := fakeContainerRoot(t, "opt/only") - if got := readEntryRuntimeDirs(procRoot, 7); len(got) != 0 { + if got := readEntryRuntimeDirs(procRoot, fixturePID); len(got) != 0 { t.Errorf("got %v, want none", got) } } + +func keys(m map[string]checkpointstore.EntryRuntimeDir) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +}