fix(nvsnap): migrate remaining workloads to criu-v2 and repair the helm chart - #472
Merged
Conversation
Each cuda-checkpoint spawn pays a fixed ~2.7s cuInit driver-attach. On the common restore path a process needs both a restore and an unlock, previously two separate spawns. The cuda plugin now issues a single combined "resume" action (restore then unlock in one process), and nvsnap-cuda-checkpoint.c gains that action. Same driver operations, same order, one fewer spawn per GPU pid; the win scales with pid count. Pins CRIU ref to 1e926fa4d, bumps base v0.0.12 / app v0.2.22. Measured on aws-dev1 (H100, criu-v2), agent restore, on top of Lever A: - vllm-small 32.2 -> 25.8s, vllm-8b 38.5 -> 32.4s, e5-mistral 40.8 -> 36.6s, vllm-qwen32b 48.5 -> 44.3s. Cumulative vs pre-optimization baseline: -42% to -56%. 5/5 single-GPU e2e PASS. Design + full table in docs/proposals/single-gpu-restore-speedup.md and docs/BENCHMARK.md. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…uses the container glibc The criu-v2 engine ran criu with LD_LIBRARY_PATH=/criu-bundle/lib to resolve criu's own transitive deps (libnftables -> libmnl), since DT_RUNPATH on the criu binary is not inherited by its libraries. But that env var is also inherited by cuda-checkpoint, which criu spawns per GPU process. On a container whose glibc is newer than the bundle's (e.g. sglang v0.5.x-cu129, glibc 2.36) cuda-checkpoint aborts with "GLIBC_2.36 not found", which silently disables the CUDA plugin -- GPU memory is never released and the dump then fails on an un-released device mapping. Make the bundle self-contained the standard way: set RPATH=$ORIGIN on each bundled library so the loader finds siblings without LD_LIBRARY_PATH, then drop LD_LIBRARY_PATH from the criu-v2 dump and restore env entirely. cuda-checkpoint now inherits a clean env and resolves libc/libm from the target container. No CRIU fork change is needed (the fix is in packaging). Validated on aws-dev1: vllm-small criu-v2 e2e PASS (no regression) and gemma-4-31B sglang cu129 criu-v2 e2e PASS (checkpoint 3m16s/36G, restore 1m12s, post-restore inference OK). Adds a gemma-sglang test-e2e workload as the regression test. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…ore+unlock Harden the Lever B resume coalesce so the CRIU cuda plugin behaves correctly against a cuda-checkpoint build without the combined `resume` action: detect support once via the help output and fall back to separate restore then unlock when unavailable. Also fixes a latent bug where the pre-resume else-if chain would have skipped unlock for a process needing both restore and unlock. Bumps CRIU ref to 31d90a8a8, base v0.0.15 / app v0.2.26 (+ manifest tag sync). Validated on aws-dev1: vllm-small (resume path exercised) and gemma-4-31B sglang cu129 criu-v2 e2e both PASS. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
- versions.sh: baseline comment reflected the historical v0.0.1 reset as if current; reword to say bumped-from-there. - e5-mistral-replay.yaml: init-container comment hardcoded a stale v0.0.20 agent; point at the synced nvsnap-agent tag instead. - single-gpu-restore-speedup.md: status/phasing said Lever B was unimplemented while the body describes the resume coalesce as shipped; sync both. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
libnvsnap_intercept.so is installed in /etc/ld.so.preload, so the loader forces
it into every process in the target mount namespace -- including CRIU, which
runs against the bundled glibc rather than the container's. The library was
linked with g++ (it has C++ sources under src/gpu), so it pulled libstdc++,
libgcc_s and libm from the *container*. On an image whose glibc is newer than
the bundle's, those resolve against symbol versions the bundle's libc does not
export and CRIU dies before it starts:
/criu-bundle/criu: /criu-bundle/lib/libc.so.6: version `GLIBC_2.38' not found
(required by /lib/x86_64-linux-gnu/libstdc++.so.6)
This is why the failure only appeared on newer engine images and why it was
invisible to ldd of the criu binary -- the dependency arrives through the
preload, not through CRIU's own link.
Linking the C++ runtime statically takes DT_NEEDED from five entries to libc
alone. libc is safe on its own because glibc is backward compatible, so a
library built here loads against any newer container glibc.
A verify-deps step runs as part of the link and fails the build if DT_NEEDED
gains anything outside the allowed set, so a future dependency cannot quietly
reintroduce the problem. The agent image builds this library, so the check runs
during image build rather than during a customer's restore.
Ref: NO-REF
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
The sglang manifests were pinned to v0.4.5.post3-cu125. Validating checkpoint and restore against an engine build that old says little about what customers deploy, and it hid a packaging defect that only newer container images expose. Move all four manifests to v0.5.15.post1-cu129. That build faults during CUDA graph capture for these models on driver 580 -- it aborts in the 'breakable' prefill graph backend with an illegal memory access before serving. A control run without the interception library reproduces it identically, so this is an engine issue rather than something nvsnap introduces. Disabling only the prefill graphs avoids it; decode graphs stay enabled, so the checkpoint still runs against a process holding captured CUDA graph state. Ref: NO-REF Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
Mechanical output of scripts/sync-versions.sh after the agent version bump. Ref: NO-REF Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…convention criu-v2 nsenters CRIU into the container's mount namespace, where /etc/ld.so.preload applies to it. Three workloads still carried the legacy interception stack, so the loader force-loaded libnvsnap_intercept.so into CRIU itself and the dump hung before seize completed. The legacy engine ran CRIU from the agent's namespace and never saw that file, which is why the same manifests used to work. Isolated with a plain `sleep` victim and no GPU: preloaded into the victim the dump succeeds (813-line dump.log); preloaded into CRIU it hangs (36 lines, zero images). These now follow the criu-v2 convention -- no interception stack, workload launched under setsid, stdio to a rootfs file so CRIU restores those fds as plain files. nim additionally re-launches its stock entrypoint chain under setsid (its server is the image CMD) and writes stdio to /tmp, since the NIM image runs as non-root and cannot write to /. Restore placeholders are now generated (see following commit) rather than hand-written, so a source and its placeholder cannot disagree. sglang-8b and sglang-small both pass e2e on the deployed agent build with no agent-side guard active, so the manifest change alone is the fix. Refs: #470
…conformance The criu-v2 migration was applied manifest by manifest with nothing verifying the result, so it ended up ragged in two independent dimensions: some sources migrated with legacy placeholders, some the reverse. That drift caused two incidents that were chased as workload-specific bugs. A placeholder carries no workload-specific logic -- it is a pid1 that bumps ns_last_pid, tails the file the source redirected stdio to, and waits. It is a pure function of a few fields on the source pod, so derive it instead of maintaining a second copy by hand. go generate ./internal/manifests/... write go run ./internal/manifests/gen -check report drift Conformance covers what actually went wrong: no manifest writes /etc/ld.so.preload, no criu-v2 placeholder execs restore-entrypoint, source/restore images agree, and every manifest carries apiVersion and kind. The last check exists because a programmatic edit dropped them and it only surfaced as a kubectl validation error mid-run. Gated on reproducing the known-good placeholders before being allowed to write. It found two real drifts immediately: nim-llama-8b had a clean source with a legacy placeholder, and it caught the same half-migration being repeated. Refs: #470
Two independent problems, both from this library being force-loaded into processes it was never meant to touch. The atfork child handler called pthread_create. POSIX allows only async-signal-safe calls in a fork child until exec, and pthread_create allocates TLS and takes the loader lock -- which a thread that did not survive the fork may hold. CRIU forks during dump, so with this library loaded into it the child deadlocked and the parent blocked in wait4 forever, stalling the dump before seize completed. Isolating each behaviour showed installing signal handlers and starting threads are individually harmless; only the atfork handler wedged it. The child now records that a worker is needed and one is created later from a safe context. Separately, the library has no business running inside our own tooling at all. Workloads enable it via /etc/ld.so.preload, which the loader applies to every process in the mount namespace -- including the CRIU that nsenters in, and the cuda-checkpoint and iptables-restore helpers it execs. No environment gate can undo that: the constructors run before any NVSNAP_* variable is read, and NVSNAP_LIGHTWEIGHT=1, NVSNAP_DISABLE_QUIESCE=1 and NVSNAP_LOG_LEVEL=0 were each measured to still hang. Every constructor now returns early when loaded into a binary under the bundle directory. Verified in both directions: inert inside the bundle and for a real criu invocation, still intercepting outside it. A build gate fails when a constructor is added without the guard, checked to fail as well as pass. Refs: #470
The chart could not be installed or upgraded. `helm template` hid it because it
does not validate; only install/upgrade does, failing with "apiVersion not
set".
Two causes. A guard written `{{- if X -}}` chomps the newline after the action,
gluing the following line onto the end of the license comment above it -- and
where that line was `apiVersion:`, the resource silently lost it. Four
resources were affected: both ServiceAccounts, the agent DaemonSet and the
blobstore Deployment. Dropping the trailing dash keeps the newline.
Separately, two templates placed their feature guard below the license header,
so a disabled feature still emitted the header as a comment-only document,
which kubectl also rejects. The guard now comes first.
Verified both ways: renders 104 valid documents with those features off, and
the guarded resources still appear when enabled.
This is why the deployed release was a month stale with hand-patched images:
upgrading was impossible, so RBAC never advanced and the L2 backend could not
start.
…le, declare gdrdrv Three defects found while validating the cachedir capture path against an NVMesh L2 StorageClass. L2 PVC sizing has two branches: measure the capture, or estimate from vRAM. The measure branch was gated on CaptureMethod=="rootfs" only, and cachedir mode was added later with its own method string. Every cachedir capture therefore took the vRAM estimate: a 14.25 GB capture requested 96 GiB (80 GiB H100 default x 1.2) and stranded the difference, permanently, because the L2 StorageClass is typically Retain. Test added, checked to fail without the fix. pvc_promote_state was never written on this path. The agent's write returns hash-not-found because Backend.Put runs before the catalog row carries its hash, and that was swallowed permanently; the server's compensating write is a one-shot check at end-of-capture that races the async promote and loses for any capture large enough to matter. Observed: promote completed roughly two minutes after that check had already run, leaving the state empty forever. Terminal state writes now retry until the row catches up. Restore was unaffected -- the webhook gates on the rox PVC being Bound -- but NVCA gates warm-start on this value and would wait indefinitely. The GPU device externals globbed /dev/nvidia* only, so /dev/gdrdrv was never declared. Matching on device names rather than major numbers is deliberate: the NVIDIA majors are allocated dynamically and differ per node. This does not by itself fix NIM checkpointing, which needs a different CRIU mechanism for an open fd on that device; the comment says so, so it is not later mistaken for handled. Refs: #465, #469
…ding them test-bench.sh emitted "| GCP | Hyperdisk-ML |" on every row regardless of where it ran. An NVMesh-on-AWS run was therefore recorded as Hyperdisk-ML, which makes the storage comparison these rows exist for impossible to read, and puts a wrong attribution into a customer-facing results table. Both are now derived per run: Storage from the L2 StorageClass provisioner (and parameters.type, since the GKE PD driver multiplexes volume types), Cloud from the node provider ID. Either can be overridden with BENCH_STORAGE / BENCH_CLOUD. On the current AWS/NVMesh cluster this emits "| AWS | NVMesh |". Note the Restore "Model DL" column remains misleading on the cachedir path: it measures wall-clock between markers that cover scheduling and volume attach, not downloading. Today it read 61.8s for a restore where vLLM logged weight load at 1.48s off the mounted volume. Not fixed here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change migrates workloads to CRIU-v2 restore conventions, adds generated restore manifests and conformance checks, updates CRIU and interception runtime behavior, improves PVC state handling, adds a gemma-sglang workload, and introduces cluster uninstall tooling. ChangesCRIU-v2 workload migration
Runtime and state handling
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SourcePod
participant RenderRestore
participant RestorePod
participant NvSnapAgent
SourcePod->>RenderRestore: source Pod YAML
RenderRestore->>RestorePod: derived restore placeholder
NvSnapAgent->>RestorePod: POST /v1/restore
NvSnapAgent->>RestorePod: restore checkpoint into namespace
RestorePod->>SourcePod: expose restored workload output
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small.yaml (1)
74-81: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHalving
failureThresholdconflicts with this manifest's own "first deploy is slow" note.With
initialDelaySeconds: 30/periodSeconds: 10, dropping 120 → 60 cuts the readiness budget from ~20 min to ~10.5 min. Line 15 documents that the first run includes TensorRT engine compilation, and there's no persistent engine-cache volume here (onlyshm), so every fresh pod recompiles. The pod won't be killed (readiness, not liveness), but it can stay un-ready and stall the e2e/bench wait — easy to mistake for the separate post-seize failures this PR calls out.🔧 Suggested fix
- failureThreshold: 60 + failureThreshold: 120🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small.yaml` around lines 74 - 81, Restore the readinessProbe failureThreshold in the trtllm workload to 120, preserving the approximately 20-minute readiness budget required for first-deploy TensorRT engine compilation. Leave the existing initialDelaySeconds, periodSeconds, and timeoutSeconds unchanged.src/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.c (1)
1-2: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestore the SPDX license header.
This change removes the file’s copyright and Apache-2.0 SPDX metadata. Restore the original header to preserve licensing provenance and compliance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.c` around lines 1 - 2, Restore the original copyright notice and Apache-2.0 SPDX license metadata at the top of nvsnap-cuda-checkpoint.c, before the existing drop-in replacement description, preserving the original licensing provenance.
🧹 Nitpick comments (1)
src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b.yaml (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack the webhook stop-gap as a ticket rather than an inline note.
The manifest records deferred work (moving the command wrapper into the admission webhook) as a comment. Repo guidance is to file a ticket for follow-up work instead of leaving it in code; a referenced issue here would also let the other four migrated manifests point at the same item. Want me to open a
NVIDIA/nvcfissue for this?As per coding guidelines: "Reference related issues in commits and Pull Request descriptions; file a ticket for follow-up work instead of leaving a TODO in code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b.yaml` around lines 17 - 18, Remove the deferred-work comment from the manifest near the command wrapper. Track the admission-webhook migration as a repository issue instead, and reference the resulting issue consistently from this and the other four migrated manifests only if the project’s established issue-reference convention requires an inline reference.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml`:
- Around line 28-33: Update the restore workload’s imagePullSecrets to include
nim-pull-secret alongside nvsnap-pull-secret, and update the corresponding
restore template generation in internal/manifests/restore.go so both secrets are
emitted for NIM image pulls.
In `@src/compute-plane-services/nvsnap/docs/BENCHMARK.md`:
- Around line 19-32: The benchmark summary is inconsistent: the table contains
four A+B workloads while the text claims 5/5 passing tests. Update the `5/5
single-GPU e2e PASS` statement to either document the missing fifth workload in
the benchmark table or change it to 4/4 and explicitly identify the measured
workload set.
In `@src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go`:
- Around line 253-258: Update the earlier environment comment near the command
setup to remove the claim that LD_LIBRARY_PATH is injected. Describe that the
bundled libraries resolve through their $ORIGIN RPATH and explicitly note that
LD_LIBRARY_PATH is deliberately omitted to avoid overriding the target
container’s glibc; keep the surrounding cmd.Env behavior unchanged.
- Around line 303-306: Update the filepath.Glob error return in the
gpuDevPatterns loop to wrap the failure with fmt.Errorf using the current pat
value and %w, preserving the underlying error while identifying the GPU device
pattern that failed.
In
`@src/compute-plane-services/nvsnap/internal/checkpointstore/percapture_pvc.go`:
- Around line 461-462: Add tests for the asynchronous ready-state retry
initiated by retryTerminalState: use a fake catalog that first returns
ErrCatalogHashNotFound and then succeeds, and assert that pvcStateReady is
eventually written. Also cover retry exhaustion or a non-retryable error, using
the existing checkpoint-store test helpers and symbols around
retryTerminalState.
In `@src/compute-plane-services/nvsnap/internal/manifests/restore.go`:
- Around line 137-141: Update the restore failure threshold parsing in the
manifest metadata handling to reject malformed or non-positive
nvsnap.io/restore-failure-threshold values instead of silently retaining the
default of 60. Propagate a validation error from the surrounding restore
configuration flow, while preserving the existing default only when the
annotation is absent.
In `@src/compute-plane-services/nvsnap/lib/nvsnap_intercept/Makefile`:
- Around line 112-117: The constructor validation awk rule must require
nvsnap_self_disabled as the first executable statement, rather than accepting
any mention within 12 lines. Update the constructor scan in the Makefile to skip
comments and non-executable syntax, detect the first executable statement, and
fail unless it is the early self-disabled guard.
In `@src/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/quiesce.c`:
- Around line 728-733: Update nvsnap_quiesce_worker_restart_if_needed so
g_quiesce_worker_needs_restart is cleared only after nvsnap_start_quiesce_worker
successfully creates the worker. Preserve the flag when creation fails so a
later invocation can retry the restart.
In `@src/compute-plane-services/nvsnap/scripts/test-bench.sh`:
- Line 699: Replace the literal “\n” in the Markdown introduction generated by
the here-document with an actual line break, preserving the existing Cloud and
Storage text and formatting.
---
Outside diff comments:
In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small.yaml`:
- Around line 74-81: Restore the readinessProbe failureThreshold in the trtllm
workload to 120, preserving the approximately 20-minute readiness budget
required for first-deploy TensorRT engine compilation. Leave the existing
initialDelaySeconds, periodSeconds, and timeoutSeconds unchanged.
In `@src/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.c`:
- Around line 1-2: Restore the original copyright notice and Apache-2.0 SPDX
license metadata at the top of nvsnap-cuda-checkpoint.c, before the existing
drop-in replacement description, preserving the original licensing provenance.
---
Nitpick comments:
In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b.yaml`:
- Around line 17-18: Remove the deferred-work comment from the manifest near the
command wrapper. Track the admission-webhook migration as a repository issue
instead, and reference the resulting issue consistently from this and the other
four migrated manifests only if the project’s established issue-reference
convention requires an inline reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0b279ac0-222d-4a17-abf6-69fb47efe8ca
⛔ Files ignored due to path filters (1)
src/compute-plane-services/nvsnap/internal/manifests/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (52)
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-rbac.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/blobstore.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/grafana-dashboard.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/post-install-smoke.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/server.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/webhook.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/agent-daemonset-crio.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/agent-daemonset.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gemma-4-31b-nim-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gemma-4-31b-nim.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/benchmarks/whisper-large-v3-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-replay.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/e5-mistral-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/gemma-sglang.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-8b-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2-restore.yamlsrc/compute-plane-services/nvsnap/docker/agent/Dockerfile.basesrc/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.csrc/compute-plane-services/nvsnap/docs/BENCHMARK.mdsrc/compute-plane-services/nvsnap/docs/proposals/single-gpu-restore-speedup.mdsrc/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.gosrc/compute-plane-services/nvsnap/internal/agent/l2_integration.gosrc/compute-plane-services/nvsnap/internal/agent/l2_integration_test.gosrc/compute-plane-services/nvsnap/internal/agent/restore_v2.gosrc/compute-plane-services/nvsnap/internal/checkpointstore/percapture_pvc.gosrc/compute-plane-services/nvsnap/internal/manifests/conformance_test.gosrc/compute-plane-services/nvsnap/internal/manifests/restore.gosrc/compute-plane-services/nvsnap/internal/manifests/workloads.yamlsrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/Makefilesrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/include/nvsnap_intercept.hsrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/gpu/init.csrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/init.csrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/libuv_intercept.csrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/quiesce.csrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/self_disable.csrc/compute-plane-services/nvsnap/scripts/test-bench.shsrc/compute-plane-services/nvsnap/scripts/test-e2e.shsrc/compute-plane-services/nvsnap/scripts/versions.sh
sbaum1994
approved these changes
Jul 27, 2026
Two are defects introduced by this PR and would have shipped broken. The generated restore placeholder hardcoded a single imagePullSecret. NIM declares nim-pull-secret alongside ours because its images come from nvcr.io/nim/, so the placeholder could not pull the image it was meant to restore into. Pull secrets now come from the source pod like every other derived field. nvsnap_quiesce_worker_restart_if_needed cleared the restart request before creating the worker, so a pthread_create failure discarded the request permanently and left a forked child with no quiesce poller. The flag is now cleared only once a worker exists. The rest: - The constructor guard check accepted nvsnap_self_disabled() anywhere within twelve lines, including after unsafe work or inside a comment. It now requires the guard to be the first statement, verified against both bypasses. - Terminal promote-state retry had no coverage: added tests for the retry succeeding once the catalog row carries the hash, stopping on a non-retryable error, and not retrying intermediate states. - restore-failure-threshold silently fell back to the default on malformed input, which can make a restore give up early and present as a workload failure. It is now rejected with the offending value named. - Wrapped device-glob errors with the pattern that failed. - Corrected a comment claiming LD_LIBRARY_PATH is injected; it is deliberately omitted in favour of RPATH resolution. - test-bench.sh emitted a literal \n in the results header. - BENCHMARK.md claimed 5/5 passing against a four-row table.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/quiesce.c (1)
728-741: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCall the deferred restart from a child path The helper is exported but never invoked anywhere, so a fork-without-exec child can still run without a quiesce poller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/quiesce.c` around lines 728 - 741, Invoke nvsnap_quiesce_worker_restart_if_needed from the fork-without-exec child path so deferred worker restarts are processed in the child. Locate the existing child-after-fork handling and call the helper there, preserving its current retry behavior when nvsnap_start_quiesce_worker fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/compute-plane-services/nvsnap/internal/checkpointstore/percapture_pvc_test.go`:
- Around line 642-720: Update TestSetStateRetryStopsOnNonRetryableError and
flakyCatalog so the second catalog call deterministically returns a configured
hard error before any retry can succeed. Configure the test double with that
error from the start, then assert the call count stops immediately after the
non-retryable failure instead of assigning hardErr after a sleep.
In `@src/compute-plane-services/nvsnap/internal/manifests/conformance_test.go`:
- Around line 171-187: The RenderRestore threshold tests only check whether an
error or success occurs, allowing unrelated failures or ignored values to pass.
Strengthen the invalid-value assertions in the threshold test loop to require
the error message to mention nvsnap.io/restore-failure-threshold, and update the
valid-value assertion to verify the rendered manifest contains failureThreshold:
120.
In `@src/compute-plane-services/nvsnap/scripts/uninstall-nvsnap.sh`:
- Around line 76-78: The PVC cleanup loop at
src/compute-plane-services/nvsnap/scripts/uninstall-nvsnap.sh:76-78 must select
only NvSnap-owned claims using established NvSnap labels and/or the documented
rox-/rwx- naming convention, rather than all claims in the namespace. Update the
PV handling at
src/compute-plane-services/nvsnap/scripts/uninstall-nvsnap.sh:85-101 to derive
eligible PVs exclusively from those selected claims before changing reclaim
policy or deleting them.
- Around line 151-153: Update the cleanup flow around the nvsnap-cleanup
DaemonSet so it polls every cleaner pod and waits for each to report successful
completion with DONE before deletion. Add a bounded timeout, propagate pod or
cleanup failures, and prevent kubectl delete from running until all pods finish
successfully; retain the existing rollout status and diagnostic logging as
appropriate.
- Around line 77-101: Update the PVC deletion flow before the “orphaned PVs
previously claimed by $NAMESPACE” scan to wait for the selected PVCs to finish
deleting, or poll their associated PVs until they leave the Bound state. Ensure
the later Retain-to-Delete patch and PV deletion logic in the orphaned-PV loop
runs only after those state transitions are observable.
- Around line 45-49: Add tests for the uninstall script’s run() flow, mocking
kubectl and helm to cover ownership filtering plus both successful apply and
command-failure paths; if this shell behavior cannot be tested in the project’s
test framework, document the concrete reason in the pull request instead.
---
Outside diff comments:
In `@src/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/quiesce.c`:
- Around line 728-741: Invoke nvsnap_quiesce_worker_restart_if_needed from the
fork-without-exec child path so deferred worker restarts are processed in the
child. Locate the existing child-after-fork handling and call the helper there,
preserving its current retry behavior when nvsnap_start_quiesce_worker fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3044c09-01f0-42ff-9546-b4818f4f6ae8
📒 Files selected for processing (10)
src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yamlsrc/compute-plane-services/nvsnap/docs/BENCHMARK.mdsrc/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.gosrc/compute-plane-services/nvsnap/internal/checkpointstore/percapture_pvc_test.gosrc/compute-plane-services/nvsnap/internal/manifests/conformance_test.gosrc/compute-plane-services/nvsnap/internal/manifests/restore.gosrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/Makefilesrc/compute-plane-services/nvsnap/lib/nvsnap_intercept/src/quiesce.csrc/compute-plane-services/nvsnap/scripts/test-bench.shsrc/compute-plane-services/nvsnap/scripts/uninstall-nvsnap.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- src/compute-plane-services/nvsnap/internal/manifests/restore.go
- src/compute-plane-services/nvsnap/scripts/test-bench.sh
- src/compute-plane-services/nvsnap/internal/agent/checkpoint_v2.go
- src/compute-plane-services/nvsnap/lib/nvsnap_intercept/Makefile
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml
- src/compute-plane-services/nvsnap/docs/BENCHMARK.md
…ngful The uninstall script had three defects in a tool whose whole job is issuing irreversible deletes safely. Ownership was namespace membership alone, but NVSNAP_NAMESPACE is configurable and a shared namespace can hold volumes we do not own. Selection is now our rox-/rwx- naming plus nvsnap labels, and PV eligibility is derived from those claims rather than from the claim's namespace. PVC deletion is asynchronous. The PV scan ran immediately afterwards and skipped volumes still Bound; they went Released moments later, after the script had moved on, stranding exactly the capacity it exists to reclaim. It now waits for the claims to clear. `rollout status` only proves the cleaner pods started, not that rm -rf finished, so the subsequent DaemonSet delete could truncate cleanup. It now waits for every node to report completion and fails loudly if any does not. Added scripts/tests/uninstall-nvsnap_test.sh, which drives the script against fake kubectl/helm and asserts the selection boundary directly: our claims and their released PVs are removed, an unrelated claim in the same namespace and its PV are untouched, and a Bound PV is never touched at all. That test immediately caught a real regression -- an earlier edit of mine had silently deleted the entire PV reclamation step, which bash -n and a dry-run both accepted without complaint. Test quality, per review: - The non-retryable retry test raced a sleep against the first retry; the error is now scripted up front so the branch is reached deterministically. - The threshold test accepted any render error as success and never checked that a valid value was applied. It now requires the error to name the annotation and asserts failureThreshold: 120 reaches the manifest.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.sh`:
- Around line 79-86: Capture each uninstall-nvsnap.sh invocation’s exit status
immediately after assigning out, including the dry-run and --apply calls, and
assert that both statuses indicate success before continuing with output and
call assertions. Use the existing test assertion helpers and apply the same
pattern to both invocation blocks.
- Around line 17-30: Strengthen the fake kubectl/helm command handlers and their
assertions in the uninstall test: explicitly recognize only the expected command
forms, fail unknown commands, and validate complete argument lists including the
nvsnap-system namespace, targeted reclaim-policy patch, and required
patch-before-delete ordering. Extend the cases around check/refute and the
referenced command stubs so unexpected mutations such as helm uninstall cannot
succeed silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70b60f2b-c358-47b7-928e-4f961903ce7a
📒 Files selected for processing (4)
src/compute-plane-services/nvsnap/internal/checkpointstore/percapture_pvc_test.gosrc/compute-plane-services/nvsnap/internal/manifests/conformance_test.gosrc/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.shsrc/compute-plane-services/nvsnap/scripts/uninstall-nvsnap.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- src/compute-plane-services/nvsnap/internal/checkpointstore/percapture_pvc_test.go
- src/compute-plane-services/nvsnap/internal/manifests/conformance_test.go
- src/compute-plane-services/nvsnap/scripts/uninstall-nvsnap.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/compute-plane-services/nvsnap/internal/agent/pathsafe.go (2)
102-123: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
pathVarGuard's "covered by construction" guarantee only holds for a hardcoded var-name list.The doc comment states a new route is automatically covered, but the guard only inspects
id,hash, andpod-uid. A future route using a differently-named path variable that still gets joined onto a hostPath directory would silently bypass this protection while the comment implies it's safe. Consider either validating allmux.Vars(r)generically (route templates here are only ever resource identifiers), or adding arouter.Walk-based test asserting every registered route's path variables are in the guarded set, so the invariant is enforced rather than just documented.♻️ Option: validate all vars generically
func pathVarGuard(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - for _, k := range []string{"id", "hash", "pod-uid"} { - v, ok := mux.Vars(r)[k] - if !ok { - continue - } - if err := validPathSegment(k, v); err != nil { + for k, v := range mux.Vars(r) { + if err := validPathSegment(k, v); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } } next.ServeHTTP(w, r) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe.go` around lines 102 - 123, Update pathVarGuard to validate every variable returned by mux.Vars(r), rather than only the hardcoded id, hash, and pod-uid names, using validPathSegment for each entry before invoking the next handler. Keep the existing bad-request response behavior and update the comment to reflect generic route-variable coverage.
125-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate anchor/clean/resolve-root logic with
resolveWithinRoot.
joinWithinRootrepeats the "anchor at/, clean, join onto root,EvalSymlinks(root)" steps already present inresolveWithinRoot(Line 51-58 above). Since both functions implement the same security-critical boundary check, extracting the shared prefix into one helper reduces the chance the two diverge if one gets a future fix and the other doesn't.♻️ Proposed shared helper
+// anchoredJoin cleans relPath, anchors it under root, and resolves root's +// real path. Shared by resolveWithinRoot (read side) and joinWithinRoot +// (write side). +func anchoredJoin(root, relPath string) (target, realRoot string, err error) { + cleaned := filepath.Clean("/" + relPath) + if cleaned == "/" { + return "", "", fmt.Errorf("empty path") + } + target = filepath.Join(root, cleaned) + realRoot, err = filepath.EvalSymlinks(root) + if err != nil { + return "", "", fmt.Errorf("resolve root: %w", err) + } + return target, realRoot, nil +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe.go` around lines 125 - 166, Extract the shared relative-path anchoring, cleaning, root joining, and real-root resolution from resolveWithinRoot and joinWithinRoot into one helper. Update both functions to reuse that helper while preserving their existing distinct handling for existing files versus missing targets and retaining the same boundary checks and errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 382-387: The downloadToFile path currently validates relPath with
joinWithinRoot but later writes through a path-based os.Rename, leaving a
symlink-swap window. Replace the MkdirAll and rename flow with a
symlink-resistant open/write mechanism that resolves and creates destination
components relative to destDir, ensuring the downloaded file cannot escape the
intended root.
---
Nitpick comments:
In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe.go`:
- Around line 102-123: Update pathVarGuard to validate every variable returned
by mux.Vars(r), rather than only the hardcoded id, hash, and pod-uid names,
using validPathSegment for each entry before invoking the next handler. Keep the
existing bad-request response behavior and update the comment to reflect generic
route-variable coverage.
- Around line 125-166: Extract the shared relative-path anchoring, cleaning,
root joining, and real-root resolution from resolveWithinRoot and joinWithinRoot
into one helper. Update both functions to reuse that helper while preserving
their existing distinct handling for existing files versus missing targets and
retaining the same boundary checks and errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: faa566d4-61f6-46e3-a894-323342a4a706
📒 Files selected for processing (5)
src/compute-plane-services/nvsnap/internal/agent/agent.gosrc/compute-plane-services/nvsnap/internal/agent/cascade_fetch.gosrc/compute-plane-services/nvsnap/internal/agent/pathsafe.gosrc/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.gosrc/compute-plane-services/nvsnap/internal/agent/restore.go
balajinvda
force-pushed
the
nvsnap-restore-leverb
branch
from
July 29, 2026 02:34
aeb6d26 to
f62721d
Compare
Contributor
Author
|
Split the path-hardening commit out to #519 so it can be reviewed on its own and merge without waiting on this PR's GPU e2e gate. This branch was force-pushed to drop it; nothing else changed (previous tip |
Two findings from CodeRabbit on #472. Both matter more than usual here: this script issues irreversible deletes, and a manual teardown using the same selection logic once deleted another team's volume. Assert the exit status. Both invocations captured output and discarded $?, so with set -e off the script could fail after issuing the expected calls and the test would still report zero failures. Reject unmodeled commands. The fakes fell through to success for anything they did not recognise, so a regression could issue an extra mutation -- a stray delete, a namespace-wide wipe -- and pass. The fakes now enumerate every command the script is sanctioned to run and record UNMODELED for anything else, which both invocations assert against. Turning this on immediately surfaced twelve unmodeled calls; all were legitimate and are now modeled explicitly, so the list doubles as the reviewed inventory of what this script may do. Assertions now compare whole argument strings rather than fragments. "delete pvc rox-abc123" matched even if -n nvsnap-system were dropped, which would delete a same-named claim in whatever namespace kubectl defaults to. Added an ordering check that patch-Retain-to-Delete precedes the PV delete. Substring assertions cannot see order, and deleting a Retain PV first strands the backing volume -- the exact leak the step exists to stop. Verified by swapping the two statements and watching the assertion fail. 17 assertions, all passing. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.sh (1)
81-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the actual cleanup branches.
The fake Helm command always reports an absent release at Lines [81-83], so the test never executes
helm uninstall. The only apply-mode invocation at Lines [105-107] uses--keep-node-state, so the default privileged node-state cleanup branch is also skipped.Add a present-release case with exact
helm statusandhelm uninstallarguments. Add a separate--applycase that verifies the cleanup DaemonSet lifecycle.Also applies to: 105-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.sh` around lines 81 - 83, Update the fake Helm command case in uninstall-nvsnap_test.sh so helm status can simulate an installed release and verify exact helm uninstall arguments, then add a separate --apply scenario without --keep-node-state to exercise privileged node-state cleanup. Assert the cleanup DaemonSet lifecycle in that scenario, including its creation and removal, while preserving existing absent-release and keep-node-state coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.sh`:
- Line 70: Update the fake kubectl command handling in the test fixture to
reject or record any apply -f invocation, and extend the dry-run assertions near
the existing delete and patch checks to fail when kubectl apply is called.
Preserve acceptance of the existing read-only commands such as rollout status
and logs.
---
Nitpick comments:
In `@src/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.sh`:
- Around line 81-83: Update the fake Helm command case in
uninstall-nvsnap_test.sh so helm status can simulate an installed release and
verify exact helm uninstall arguments, then add a separate --apply scenario
without --keep-node-state to exercise privileged node-state cleanup. Assert the
cleanup DaemonSet lifecycle in that scenario, including its creation and
removal, while preserving existing absent-release and keep-node-state coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c70640dd-68de-4975-8548-2b157e217734
📒 Files selected for processing (1)
src/compute-plane-services/nvsnap/scripts/tests/uninstall-nvsnap_test.sh
Follow-up review catch, and a gap the previous commit introduced: making `apply -f` a modeled command without asserting against it in the dry-run block. Dry run checked for no delete and no patch, but the node-state step mutates with apply -- it creates a privileged DaemonSet that rm -rf's host paths. A regression running that during dry run would have passed. Also asserts dry run issues no helm uninstall. Verified by forcing the apply to run unconditionally and watching the assertion fail. 19 assertions. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
162 commits of drift since the 2026-07-20 merge-base. 18 conflicts, all in manifests, docs or versions.sh; no Go source, intercept library, helm template or script conflicted. Resolutions, each checked rather than guessed: - Image tags (agent-daemonset x2, e5-mistral-replay, gemma-4-31b-nim and the restore halves, versions.sh): took this branch's v0.2.32 over main's v0.2.26. Newer, and now consistent across every manifest. - initContainers on sglang-small, sglang-8b, trtllm-small and nim-llama-8b: took this branch, which has none. The merge-base already carried the uvloop extraction block, so this is a deliberate deletion by the criu-v2 migration (e3850d5), not main adding something we missed. Verified by diffing the merge-base rather than inferring from the conflict shape. - Restore placeholders: took this branch's generated versions, then confirmed with `gen -check` that the checked-in set matches a fresh render. - BENCHMARK.md: took MAIN's "5/5 single-GPU e2e PASS" over this branch's "all four workloads above". The table here has four rows so both are internally consistent, but main's is the later and broader claim about what was actually validated, and silently narrowing a published test result would be a regression in accuracy. Only that hunk; this branch's Cloud/Storage column work in the same file merged clean. Worth recording for anyone reading this later: main's 583463b (#263) shares titles with the first commits here (Lever B, RPATH bundle, resume feature-detect), which raised the prospect of the same work being applied twice through files that merged without conflict. It is not: restore_v2.go and nvsnap-cuda-checkpoint.c are byte-identical between main and this branch, so #263 was an earlier slice of this same branch that already landed. Only checkpoint_v2.go still differs, carrying the gdrdrv change main lacks. Verified: build clean; manifests and agent tests pass; helm lint passes; uninstall test 19/19; intercept library builds with both verify-deps and verify-self-disable gates passing; every manifest parses and carries apiVersion + kind; no conflict markers anywhere. Still gated on ./scripts/test-e2e.sh vllm-small per CLAUDE.md rule 10, which needs GPU nodes currently held for QA. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
CI's "Check BUILD files match their sources" failed: internal/manifests and internal/manifests/gen had no BUILD.bazel at all. Both packages were added on this branch before that check landed on main (#491), so the merge is what surfaced it rather than anything new here. Generated with `bazel run //:gazelle` as the check instructs, not hand-written, so the result is byte-for-byte what CI regenerates and compares against. Gazelle also rewrote eight unrelated BUILD files under src/libraries/java/nv-boot-parent/. Those are outside this PR and outside the check's scope (check-gazelle passes only the Go roots, and its own test asserts "leaves src/libraries/java out of scope"), so they were reverted rather than swept in. Verified by running the failing job's steps locally: check-gazelle now reports "BUILD files are up to date", and test-check-gazelle, check-nested-modules and test-check-nested-modules all pass. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
bazel build passed but `bazel test //...` failed on the manifests package: the conformance checks read workload YAML off disk, and bazel sandboxes tests to their declared inputs, so the glob found nothing. This is my doing -- adding the BUILD file in the previous commit is what put these tests under bazel for the first time. `go test` supplies the whole source tree, so they passed there and still do. The test's own guard turned a silent problem into a loud one: manifests() fatals on an empty glob rather than returning zero files, so the failure was "no workload manifests found" instead of four checks vacuously passing over nothing. That is the behaviour worth keeping -- a conformance suite that finds no inputs must not look green. Exposes the manifests as a filegroup and declares it as test data. Marked `# keep` because gazelle cannot infer a data dependency from a runtime path and would otherwise drop the attribute on its next run. Verified: the test passes under bazel; check-gazelle still reports BUILD files up to date; the whole nvsnap bazel suite is 17/17. Confirmed it is not passing vacuously by planting an ld.so.preload line in sglang-small.yaml and watching the bazel test fail with the right message, then reverting. (First attempt at that proof appended the line as a YAML comment and the test correctly ignored it -- read() strips comments so explanatory prose cannot false-positive. The real mutation used a live key.) Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
balajinvda
added a commit
that referenced
this pull request
Aug 5, 2026
One conflict, in the restore-pod egress NetworkPolicy, and it needed a real hand-merge rather than picking a side. Both changes are wanted: this branch: (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) ... -}} main (#472): .Values.webhook.agentHostCIDR ... }} This branch carries #561's condition, which lets the policy render under pod networking without an operator-supplied node CIDR. Main carries #472's chart repair, which drops the trailing dash -- `-}}` chomps the following newline and glues the next line onto the preceding comment. Taking --ours would have silently reverted the chomping fix; taking --theirs would have made the init-container strategy unavailable under pod networking again. Kept both: this branch's condition with main's non-chomping close. Verified by rendering rather than by reading: - pod networking, no agentHostCIDR -> policy renders (the #561 behaviour) - hostNetwork, no agentHostCIDR -> policy absent (unchanged, correct) - rendered output starts at the Source comment and apiVersion with nothing glued to it (the #472 fix intact) - helm lint clean Also: build clean, 14 internal packages pass / 0 fail, check-gazelle up to date. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
criu-v2 changed where CRIU runs: it nsenters into the container's mount
namespace instead of running from the agent's.
/etc/ld.so.preloadis aproperty of that namespace, so under criu-v2 the loader force-loads
libnvsnap_intercept.so into CRIU itself.
Three workloads were never migrated off the legacy interception stack, so their
dumps hung before seize completed. Two incidents previously chased as
workload-specific bugs — a GLIBC_2.38 load failure and a dump hang — were this
one unmigrated convention.
Isolated with a plain
sleepvictim and no GPU:The mechanism is ours: a
pthread_atforkchild handler callingpthread_create. That is a POSIX violation and it deadlocks CRIU's forkedchild while the parent blocks in wait4.
Separately, the Helm chart could not be installed or upgraded at all, which is
why the deployed release was a month stale with hand-patched images and the L2
backend could never start.
What changed
convention: no interception stack, workload under setsid, stdio to a rootfs
file. nim additionally re-launches its stock entrypoint chain under setsid
and writes to /tmp, since that image runs as non-root.
hand-maintained, plus a conformance suite over all 28 manifests.
inert when the library is loaded into our own bundle binaries. Build gates
for both, checked to fail as well as pass.
{{- if X -}}guards were swallowing the newline and commentingout
apiVersionon four resources; two templates emitted comment-onlydocuments when their feature was disabled.
measured capture (14 GB capture requested 96 GiB on a Retain StorageClass);
terminal
pvc_promote_statewrites now retry instead of being dropped;/dev/gdrdrvis declared alongside the nvidia devices."GCP | Hyperdisk-ML" on every row.
Customer Release Notes
Fixes checkpoint/restore for SGLang, TensorRT-LLM and NIM workloads, which
could hang during checkpoint. Fixes Helm chart installation and upgrade. Fixes
over-allocation of L2 cache volumes.
Plan Summary
Helm chart templates change; RBAC gains storageclasses/PVC/Job permissions that
the L2 backend has always required but never received, because upgrading was
impossible. Agent DaemonSet rolls.
Usage
Testing
both on the deployed agent build with no agent-side guard active, so the
manifest change alone is demonstrated as the fix.
GB zero-copy promote, weights loaded in 1.48s off the shared read-only mount.
go test ./internal/...green (13 packages). New tests for cachedir L2sizing and manifest conformance, each verified to fail without its fix.
with the affected features both enabled and disabled.
Not covered: trtllm-small and nim-llama-8b get past seize but fail on
pre-existing defects tracked separately. Multi-pod L2 fan-out is unexercised.
Notes
The
["nvidia*", "gdrdrv"]glob does not by itself fix NIM checkpointing — anopen fd on that device needs a different CRIU mechanism. The code comment says
so explicitly so it is not later mistaken for handled.
The bench Restore "Model DL" column is still misleading on the cachedir path:
it measures wall-clock covering scheduling and volume attach, not downloading.
It read 61.8s for a restore where vLLM logged 1.48s. Worth fixing before those
numbers are used for storage comparisons.
References
Closes #470. Refs #465, #469.
Related Merge Requests/Pull Requests
None
Dependencies
None
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests