feat(nvsnap): enable UV_USE_IO_URING after CRIU SQ-array restore fix#225
Open
balajinvda wants to merge 9 commits into
Open
feat(nvsnap): enable UV_USE_IO_URING after CRIU SQ-array restore fix#225balajinvda wants to merge 9 commits into
balajinvda wants to merge 9 commits into
Conversation
libuv/uvloop servers (vLLM, e5-mistral) previously had to pin UV_USE_IO_URING=0 under criu-v2: the restored libuv SQPOLL io_uring ring aborted the event loop on the first epoll_ctl_flush (SIGABRT, nvsnap#107). Root cause was in CRIU's io_uring restore, which recreated a fresh ring but never rebuilt the SQ-array identity map (sq_array[i] = i) that liburing/libuv write once at setup; every restored submission then resolved to SQE 0 and completions never returned. The CRIU fork now rebuilds that identity map on restore (io-uring-cr commit 0a9963ed0). Pin the base build to it and re-enable UV_USE_IO_URING=1 on the single-GPU criu-v2 workloads. - versions.sh: NVSNAP_CRIU_REF -> 0a9963ed0; base v0.0.9; app v0.2.19. - vllm-small, vllm-8b, vllm-qwen32b, e5-mistral: UV_USE_IO_URING 0 -> 1. Validated e2e on aws-dev1 (kernel 6.8): all four checkpoint, restore, and serve inference with io_uring on, zero event-loop aborts. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-17 04:33:48 UTC | Commit: a9701c1 |
…fixes - vllm-mp: TP=1 multi-process (separate EngineCore + ZMQ RPC) — criu-v2 e2e PASSES; the historical multi-process restore blocker does not apply to the in-namespace engine. - vllm-tp2: TP=2 eager with the peer-state sever profile (disable custom all-reduce, symm-mem, NCCL P2P/NVLS/SHM -> PYNCCL sockets) — criu-v2 e2e PASSES: 5.3G checkpoint, 57s restore, both TP workers and both GPUs restored, inference serves. First criu-v2 multi-GPU checkpoint/restore. Default config (peer state on) hangs per-rank cuda-checkpoint even with all ranks locked first; a transparent sever is required (issue #25). - test-e2e.sh: workload cases for both; node selection now picks the node with the MOST free GPUs instead of first-fit (first-fit caused repeated UnexpectedAdmissionError on the delete->recreate placeholder cycle). Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…inary Promote nvsnap-cuda-checkpoint.c (a drop-in cuda-checkpoint CLI on the public CUDA driver checkpoint API, cuCheckpointProcess*, driver 570+) from the research tree into docker/agent/ and compile it in a new cuda-cli-builder stage in Dockerfile.base (nvidia/cuda:12.8.1-devel; links against the toolkit stub, resolves the node's real libcuda at runtime via the existing wrapper). The build fails if the provenance marker string is missing from the produced binary. Replaces the committed NVIDIA prebuilt binary (removed): the public release is x86-64 only, so this unblocks arm64 (no committed binaries per arch), and gives us an extensible CLI (lock --timeout today; r580 gpuPairs migration next). build_app's stale binary-copy requirement is removed — the app image only overlays the wrapper; the binary comes from base. Validated as base v0.0.10 / app v0.2.20 on aws-dev1 (driver 580.126): on-node binary provenance-verified, then the full criu-v2 regression suite passed 6/6 with every checkpoint/restore driven by this CLI — vllm-small, vllm-8b, e5-mistral, vllm-qwen32b (64G), vllm-mp (multi-process), vllm-tp2 (TP=2 multi-GPU, config-severed). Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…ckpoint) Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe change builds CUDA checkpoint tooling from source, updates nvsnap image and CRIU versions, adds vLLM multi-process and tensor-parallel manifests, enables io_uring in selected workloads, extends end-to-end workload selection, and adds Docker build, test-image, and restore-speedup documentation. ChangesCUDA checkpoint tooling
vLLM workload and restore support
nvsnap release rollout
Build and test image updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E2E as test-e2e.sh
participant K8s as Kubernetes
participant Workload as vLLM workload
participant Restore as vLLM restore pod
E2E->>K8s: Apply vllm-mp or vllm-tp2 manifest
K8s->>Workload: Start vLLM server
Workload-->>E2E: Report /v1/models readiness
E2E->>K8s: Apply restore manifest with checkpoint ID
K8s->>Restore: Start restore placeholder
Restore-->>E2E: Report restored /v1/models readiness
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/CONTRIBUTING.md`:
- Around line 17-20: Update the `cuda-checkpoint` documentation sentence in
`CONTRIBUTING.md` to replace the em dash between “arm64” and “no committed
binaries” with standard ASCII punctuation, preserving the existing meaning and
concise wording.
In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp.yaml`:
- Around line 23-30: Disable automatic ServiceAccount token mounting by adding
automountServiceAccountToken: false under spec in both
src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp.yaml (lines
23-30) and src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml
(lines 25-32).
In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml`:
- Around line 1-2: Replace the em dash in the header comment for the vLLM TP2
workload with ASCII punctuation, preserving the comment’s meaning and all other
manifest content.
🪄 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: 44fb3a12-d86c-4a85-b116-88eb732813a7
📒 Files selected for processing (29)
src/compute-plane-services/nvsnap/CONTRIBUTING.mdsrc/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.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.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-qwen32b.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-small.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yamlsrc/compute-plane-services/nvsnap/docker/agent/Dockerfile.basesrc/compute-plane-services/nvsnap/docker/agent/cuda-checkpointsrc/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.csrc/compute-plane-services/nvsnap/scripts/build-agent.shsrc/compute-plane-services/nvsnap/scripts/test-e2e.shsrc/compute-plane-services/nvsnap/scripts/versions.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml (2)
28-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable service-account token automounting.
This privileged workload does not need Kubernetes API credentials. Without
automountServiceAccountToken: false, the default service-account token may be mounted into a compromised vLLM process. The restore manifest already disables this token.Proposed fix
spec: + automountServiceAccountToken: false tolerations:🤖 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/vllm-tp2.yaml` around lines 28 - 35, Update the workload pod specification in vllm-tp2.yaml to set automountServiceAccountToken to false, matching the existing restore manifest behavior and preventing the default service-account token from being mounted.
58-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not mask vLLM startup failures with an infinite sleep.
If
vllm serveexits, the shell, tail process, and infinite sleep keep the container running. WithrestartPolicy: Never, readiness remains false while the pod never reports a failed workload, causing E2E runs to wait until an external timeout. Track the server PID and exit when it terminates.Proposed fix
nohup setsid vllm serve \ --model TinyLlama/TinyLlama-1.1B-Chat-v1.0 \ --host 0.0.0.0 \ --port 8000 \ --max-model-len 2048 \ --tensor-parallel-size 2 \ --enforce-eager \ --disable-custom-all-reduce \ - --gpu-memory-utilization 0.3 > /vllm.out 2>&1 < /dev/null & + --gpu-memory-utilization 0.3 > /vllm.out 2>&1 < /dev/null & + vllm_pid=$! tail -F /vllm.out & - while true; do sleep 30; done + wait "$vllm_pid"🤖 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/vllm-tp2.yaml` around lines 58 - 68, Update the vllm serve startup flow in the workload command to capture its background process ID, monitor that process instead of using an unconditional infinite sleep, and exit with the server’s failure status when it terminates. Keep log streaming through tail, but ensure the container reports the vLLM startup failure so restartPolicy: Never does not leave the pod running indefinitely.src/compute-plane-services/nvsnap/docker/agent/Dockerfile.base (1)
11-18: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake arm64 support end-to-end.
The new builder can compile the CLI for arm64, but this Dockerfile still hard-codes x86_64 loader paths at Lines 73, 82, and 88. An arm64 image build will therefore fail or produce an unusable CRIU bundle. Make the bundle creation architecture-aware, or narrow the comment to claim arm64 support only for the standalone CLI.
🤖 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/Dockerfile.base` around lines 11 - 18, Update the CRIU bundle creation logic associated with the Dockerfile’s hard-coded x86_64 loader paths at lines 73, 82, and 88 to select architecture-appropriate paths for both x86_64 and arm64. Ensure the resulting bundle and runtime loader configuration remain usable on each supported architecture, preserving the end-to-end arm64 support claim.
🤖 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.
Outside diff comments:
In `@src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml`:
- Around line 28-35: Update the workload pod specification in vllm-tp2.yaml to
set automountServiceAccountToken to false, matching the existing restore
manifest behavior and preventing the default service-account token from being
mounted.
- Around line 58-68: Update the vllm serve startup flow in the workload command
to capture its background process ID, monitor that process instead of using an
unconditional infinite sleep, and exit with the server’s failure status when it
terminates. Keep log streaming through tail, but ensure the container reports
the vLLM startup failure so restartPolicy: Never does not leave the pod running
indefinitely.
In `@src/compute-plane-services/nvsnap/docker/agent/Dockerfile.base`:
- Around line 11-18: Update the CRIU bundle creation logic associated with the
Dockerfile’s hard-coded x86_64 loader paths at lines 73, 82, and 88 to select
architecture-appropriate paths for both x86_64 and arm64. Ensure the resulting
bundle and runtime loader configuration remain usable on each supported
architecture, preserving the end-to-end arm64 support claim.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b15e880f-f824-4c70-9094-54c748011b1b
📒 Files selected for processing (6)
src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yamlsrc/compute-plane-services/nvsnap/docker/agent/Dockerfile.basesrc/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.c
🚧 Files skipped from review as they are similar to previous changes (4)
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2-restore.yaml
- src/compute-plane-services/nvsnap/docker/agent/nvsnap-cuda-checkpoint.c
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp-restore.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp.yaml
Backfills the SPDX copyright + license header on the 23 remaining Dockerfiles under src/compute-plane-services/nvsnap. Every source file in the tree (Go, C, Python, YAML, shell, Dockerfile) now carries the header. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
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/tests/uvloop-mp-test/Dockerfile (1)
45-48: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRun the test container as a non-root user. Add a dedicated user before
CMDand runapp_simple.pyunder it; this test image only serves on port 8000 and does not need root privileges.🤖 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/tests/uvloop-mp-test/Dockerfile` around lines 45 - 48, Update the Dockerfile before the CMD instruction to create a dedicated non-root user, switch to that user with USER, and keep the existing app_simple.py command running on port 8000.Source: Linters/SAST tools
🧹 Nitpick comments (1)
src/compute-plane-services/nvsnap/tests/uvloop-mp-test/Dockerfile (1)
10-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin test-image dependencies for reproducible builds.
The PPA and pip installs currently resolve moving versions, including
pip,uvloop, andaiohttp. Use a committed constraints/lock file with tested versions, or document and continuously validate the floating-version contract.Also applies to: 27-31, 36-37
🤖 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/tests/uvloop-mp-test/Dockerfile` around lines 10 - 24, Pin the test image’s moving dependencies by adding and committing a constraints or lock file covering the deadsnakes/PPA packages and pip-installed packages such as pip, uvloop, and aiohttp. Update the Dockerfile installation steps to consume that file and ensure all dependency-install blocks, including the referenced sections, use the tested versions consistently.
🤖 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.
Outside diff comments:
In `@src/compute-plane-services/nvsnap/tests/uvloop-mp-test/Dockerfile`:
- Around line 45-48: Update the Dockerfile before the CMD instruction to create
a dedicated non-root user, switch to that user with USER, and keep the existing
app_simple.py command running on port 8000.
---
Nitpick comments:
In `@src/compute-plane-services/nvsnap/tests/uvloop-mp-test/Dockerfile`:
- Around line 10-24: Pin the test image’s moving dependencies by adding and
committing a constraints or lock file covering the deadsnakes/PPA packages and
pip-installed packages such as pip, uvloop, and aiohttp. Update the Dockerfile
installation steps to consume that file and ensure all dependency-install
blocks, including the referenced sections, use the tested versions consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3d419e49-255f-4dca-a235-7419b9cc1583
⛔ Files ignored due to path filters (1)
src/compute-plane-services/nvsnap/docker/agent/Dockerfile.appis excluded by!**/*.app
📒 Files selected for processing (22)
src/compute-plane-services/nvsnap/docker/Dockerfile.agentsrc/compute-plane-services/nvsnap/docker/Dockerfile.serversrc/compute-plane-services/nvsnap/docker/agent/Dockerfilesrc/compute-plane-services/nvsnap/docker/agent/Dockerfile.app.criuv2src/compute-plane-services/nvsnap/docker/agent/Dockerfile.app.patchsrc/compute-plane-services/nvsnap/docker/agent/Dockerfile.localsrc/compute-plane-services/nvsnap/docker/criu-builder/Dockerfilesrc/compute-plane-services/nvsnap/docker/criu-builder/Dockerfile.extractsrc/compute-plane-services/nvsnap/docker/init/Dockerfilesrc/compute-plane-services/nvsnap/docker/libuv/Dockerfilesrc/compute-plane-services/nvsnap/docker/libzmq/Dockerfilesrc/compute-plane-services/nvsnap/docker/nvsnap-blobstore/Dockerfilesrc/compute-plane-services/nvsnap/docker/nvsnap-l2-wait/Dockerfilesrc/compute-plane-services/nvsnap/docker/phase2/Dockerfilesrc/compute-plane-services/nvsnap/docker/pyzmq/Dockerfilesrc/compute-plane-services/nvsnap/docker/uvloop/Dockerfilesrc/compute-plane-services/nvsnap/placeholder/Dockerfilesrc/compute-plane-services/nvsnap/placeholder/Dockerfile.simplesrc/compute-plane-services/nvsnap/placeholder/Dockerfile.vllmsrc/compute-plane-services/nvsnap/scripts/Dockerfile.intercept-buildsrc/compute-plane-services/nvsnap/tests/uvloop-mp-test/Dockerfilesrc/compute-plane-services/nvsnap/tests/uvloop-mp-test/Dockerfile.pip
CRIU restore speedup (Lever A): the fork now madvise(MADV_HUGEPAGE)s the premapped private-VMA arena before faulting in the process image, so the content populates as 2MB pages instead of ~800k 4KB pages. Pins CRIU ref to 1ddd5c9c3, bumps base v0.0.11 / app v0.2.21. Measured on aws-dev1 (H100, criu-v2), vllm-small 3.2G: - CPU pages restore phase: 13.5s -> 3.8s (-72%) - CRIU restore total: 40.3s -> 28.2s (-30%) - agent restore: 44.3s -> 32.2s (-27%) Single-GPU regression 5/5 PASS (vllm-small, vllm-8b, e5-mistral, vllm-qwen32b 64G). No RSS bloat: AnonHugePages=0 in the final process (huge pages are transient to the staging arena, split on remap). Design + before/after in docs/proposals/single-gpu-restore-speedup.md. Review fixes (CodeRabbit): - automountServiceAccountToken: false on vllm-mp / vllm-tp2 source pods. - Replace em-dashes with ASCII in CONTRIBUTING.md, vllm-tp2*.yaml. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/docs/proposals/single-gpu-restore-speedup.md`:
- Around line 54-57: Reconcile the workload count in the measured single-GPU
regression statement: either add the missing fifth workload to the parenthetical
list or change “5/5 PASS” to “4/4 PASS” so the claim matches the listed
workloads.
- Around line 7-8: Update the document’s status and affected sections to
distinguish completed work from planned work: mark Lever A and its measured
results as implemented, retain locked decisions as decided, and label proposed
phasing and remaining levers as planned or not yet implemented. Remove the
conflicting “design sketch, pre-implementation” wording while preserving the
existing measurements and proposal details.
- Around line 15-20: Update all affected diagrams in the proposal, including the
referenced sections, to use `text` language tags on their fenced blocks and
replace Unicode box-drawing glyphs with ASCII characters. Preserve the diagrams’
layout, timings, and meaning while ensuring committed documentation uses only
standard ASCII.
- Around line 12-13: Align the restore-time baseline and target throughout the
proposal: update the opening timing, the CRIU/agent breakdown at the referenced
timing section, and the A+B projection so they consistently use either CRIU-only
(~40.3s to ~19s) or agent-inclusive (~44.3s to ~23s) measurements. Preserve the
stated overhead when using the agent-inclusive projection.
🪄 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: 10f409d0-2953-4201-b5be-f2c2f6f932fa
📒 Files selected for processing (20)
src/compute-plane-services/nvsnap/CONTRIBUTING.mdsrc/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/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-mp.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2-restore.yamlsrc/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yamlsrc/compute-plane-services/nvsnap/docs/proposals/single-gpu-restore-speedup.mdsrc/compute-plane-services/nvsnap/scripts/versions.sh
🚧 Files skipped from review as they are similar to previous changes (14)
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/nim-llama-8b-restore.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/gemma-4-31b-nim.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/benchmarks/whisper-large-v3-restore.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small-restore.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-8b-restore.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/sglang-small.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/agent-daemonset-crio.yaml
- src/compute-plane-services/nvsnap/scripts/versions.sh
- src/compute-plane-services/nvsnap/deploy/k8s/agent-daemonset.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/trtllm-small.yaml
- src/compute-plane-services/nvsnap/CONTRIBUTING.md
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-mp.yaml
- src/compute-plane-services/nvsnap/deploy/k8s/workloads/vllm-tp2-restore.yaml
- BENCHMARK.md: add the Lever A huge-page restore section (matched before/after for all 4 single-GPU workloads, -27% to -51%). - single-gpu-restore-speedup.md (CodeRabbit review): - status reflects implemented (A) vs proposed (B) vs out-of-scope (C). - one consistent restore-time boundary (agent-restore) throughout. - ASCII-only diagrams with text-tagged fences. - regression count corrected to 4/4. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/docs/BENCHMARK.md`:
- Around line 18-20: Revise the benchmark summary in BENCHMARK.md to scope the
faster-restore claim to the four tested single-GPU workloads rather than all
workloads, and soften or remove the assertion that gains increase with CPU-side
memory footprint. Preserve the reported vllm-small phase timings and
percentages.
🪄 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: c5da3790-9205-4bb9-8b25-8fd5ee64520f
📒 Files selected for processing (2)
src/compute-plane-services/nvsnap/docs/BENCHMARK.mdsrc/compute-plane-services/nvsnap/docs/proposals/single-gpu-restore-speedup.md
Comment on lines
+18
to
+20
| Every single-GPU workload restores faster; the win grows with the process's | ||
| CPU-side memory footprint. Phase split from the vllm-small `-v4` restore.log: | ||
| the CPU pages-restore phase dropped 13.5 s to 3.8 s (-72%); the cuda_plugin GPU |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid overstating the benchmark conclusion.
The data covers four tested single-GPU workloads, not every possible workload, and the improvement is not monotonic with CPU-side footprint: e5-mistral-7b improves by 51% at 15G, while vllm-qwen32b improves by 37% at 64G. Please scope this to the tested workloads and soften the footprint claim.
Suggested wording
-Every single-GPU workload restores faster; the win grows with the process's
-CPU-side memory footprint.
+All four tested single-GPU workloads restore faster; larger CPU-side footprints
+generally benefit, although the improvement is not monotonic in this sample.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Every single-GPU workload restores faster; the win grows with the process's | |
| CPU-side memory footprint. Phase split from the vllm-small `-v4` restore.log: | |
| the CPU pages-restore phase dropped 13.5 s to 3.8 s (-72%); the cuda_plugin GPU | |
| All four tested single-GPU workloads restore faster; larger CPU-side footprints | |
| generally benefit, although the improvement is not monotonic in this sample. | |
| Phase split from the vllm-small `-v4` restore.log: the CPU pages-restore phase dropped 13.5 s to 3.8 s (-72%); the cuda_plugin GPU |
🤖 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/docs/BENCHMARK.md` around lines 18 - 20,
Revise the benchmark summary in BENCHMARK.md to scope the faster-restore claim
to the four tested single-GPU workloads rather than all workloads, and soften or
remove the assertion that gains increase with CPU-side memory footprint.
Preserve the reported vllm-small phase timings and percentages.
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
2 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
Three pieces, all validated end-to-end on aws-dev1 (H100, criu-v2):
io_uring_setup'd ring has a kernel-zeroed SQ array; the identity map is written once at setup and never re-established at restore, so libuv aborted post-restore. The CRIU fork re-writes it, soUV_USE_IO_URING=1is safe again.cuda-checkpoint.What changed
1ddd5c9c3(SQ-array fix + madvise huge-page restore); basev0.0.11/ appv0.2.21.cuda-checkpointbuilt from source (docker/agent/nvsnap-cuda-checkpoint.c) in acuda-cli-builderstage; committed NVIDIA prebuilt removed (unblocks arm64, extensible).vllm-mp(multi-process),vllm-tp2(TP=2 eager, peer-state-severed) +test-e2e.shcases; node selection picks most-free-GPU node.madvise(MADV_HUGEPAGE)s the premapped private-VMA arena before faulting in the process image, so it populates as 2MB pages instead of ~800k 4KB pages. Best-effort, degrades to a no-op on non-THP nodes, never affects correctness. Design indocs/proposals/single-gpu-restore-speedup.md.Restore speedup: before / after (agent restore, single GPU)
Every single-GPU workload restores faster; the win grows with the process's CPU-side memory footprint (the huge-page fault-reduction mechanism).
Phase-level (vllm-small
-v4restore.log): CPU pages restore 13.5s -> 3.8s (-72%); CUDA plugin restore ~27s unchanged; CRIU restore total 40.3s -> 28.2s. No RSS bloat:AnonHugePages=0in the final restored process (huge pages are transient to the staging arena and split on the remap to final addresses).Testing
criu-v2 single-GPU regression 5/5 PASS on the Lever A build, every checkpoint/restore driven by the source-built CLI: vllm-small, vllm-8b (38.5s), e5-mistral (40.8s), vllm-qwen32b (64G, 48.5s). Multi-GPU: vllm-mp + vllm-tp2 (TP=2, config-severed) PASS - first criu-v2 multi-GPU C/R. Default-config multi-GPU still needs the transparent sever (issue #25), tracked separately.
Customer Release Notes
criu-v2 GPU checkpoint/restore is ~30% faster to restore (huge-page memory install), handles io_uring/uvloop and multi-process single-GPU workloads, and builds cuda-checkpoint from source (arm64-capable).
References
Relates to #71
Summary by CodeRabbit