fix(nvsnap): repair install path - chart drift, disabled L2 fan-out, unbuildable server - #737
fix(nvsnap): repair install path - chart drift, disabled L2 fan-out, unbuildable server#737balajinvda wants to merge 3 commits into
Conversation
The nvsnap Helm chart pinned agent v0.1.3 while versions.sh was at v0.2.32, so install-nvsnap.sh -- the documented install path -- deployed a 31-version-old agent. test-e2e.sh refused to run against a fresh install on account of the mismatch. sync-versions.sh already targeted the chart via DIRS=(deploy/k8s deploy). The substitution matched a single registry/name:tag token, which is how deploy/k8s spells an image. Chart values split the same reference across repository: and tag: lines, so the pattern matched nothing, the sed no-opped, and the script still printed "Synced nvsnap-agent -> ...". The staleness check below it used the same /name: anchor, so it also matched nothing in the chart and passed vacuously. Nothing could catch the drift, which is why it went unnoticed. Add chart_tag()/set_chart_tag() to read and rewrite the split form, run them alongside the existing sed, and extend verification to compare every chart tag against its expected version. Also check imageRegistry, since the chart composes refs as <imageRegistry>/<repository>:<tag> and a drifting registry breaks every image at once. Verified on nvcf-dgxc-k8s-aws-usw2-dev2: the sync updates only the stale agent tag; disabling the chart-sync step makes verification fail with "pins nvsnap-agent tag v0.1.3, expected v0.2.32"; corrupting imageRegistry fails with the registry message; a clean tree exits 0. e2e vllm-small passes on the resulting install (33G checkpoint, 8m51s). Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
agent.l2.storageClass defaults to empty, which disables the L2 per-capture PVC tier. The install still succeeds and prints a clean success banner, while restore fan-out silently degrades to the L3 peer cascade. The only signal today is an info-level agent log line that scrolls past: L2 disabled: agent.L2.StorageClass not set (cluster prerequisite missing — restore fan-out will fall back to L3 peer cascade) That is a throughput cliff which only shows up under multi-node fan-out, long after install, and it is easy to conclude the cluster is fully configured when it is not. Observed installing on nvcf-dgxc-k8s-aws-usw2-dev2, which has six RWX-capable StorageClasses available and still came up with L2 off. Report the condition during the existing step-5 cluster auto-detect, and list the RWX-capable StorageClasses actually present so the operator can act on it immediately. Deliberately reported rather than auto-selected: the wrong class yields PVCs that never bind, and the right choice depends on cluster topology. RWX capability is not exposed on the StorageClass API, so candidates are matched against known RWX provisioners. Suppressed when the operator has already passed agent.l2.storageClass. Verified on dev2: unset lists the six nvmesh classes with the enabling flag; passing --set agent.l2.storageClass=nvcf-sc emits nothing. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
📝 WalkthroughWalkthroughNvSnap deployment scripts now synchronize and verify Helm image values, update the agent image tag, and report missing L2 storage configuration. The UI adds a tracked HTML entry point and targeted coverage ignore rules. ChangesNvSnap deployment
NvSnap UI entry point
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 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/scripts/install-nvsnap.sh`:
- Around line 242-243: Update the StorageClass query in the candidate-detection
flow to preserve the kubectl pipeline’s exit status instead of masking stderr
and failures with “|| true”. In the surrounding logic that reports no
candidates, distinguish a failed query by printing “Unable to inspect
StorageClasses”, and reserve “No RWX-capable StorageClass detected” for
successful queries that return no matches.
- Around line 242-243: Update the StorageClass candidate collection around the
kubectl JSONPath pipeline so rwx_re is applied only to the provisioner field,
not the combined name-and-provisioner text. Emit or parse separate name and
provisioner fields, filter candidates by the provisioner value, and retain the
class name for the warning recommendation so non-RWX provisioners cannot be
selected by name alone.
- Line 240: Update the EXTRA_HELM_ARGS check around the agent.l2.storageClass
validation to recognize only a --set argument with a non-empty value for the
exact escaped key agent.l2.storageClass, excluding -f and --values arguments and
unrelated keys.
🪄 Autofix
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: f76f88c5-4224-4f72-b1aa-fffe3a160159
📒 Files selected for processing (3)
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yamlsrc/compute-plane-services/nvsnap/scripts/install-nvsnap.shsrc/compute-plane-services/nvsnap/scripts/sync-versions.sh
| # never bind, and the right choice depends on cluster topology. RWX | ||
| # capability isn't exposed on the StorageClass API, so candidates are | ||
| # matched on known RWX provisioners. | ||
| if ! printf '%s\n' "${EXTRA_HELM_ARGS[@]}" | grep -q "agent.l2.storageClass="; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
script='src/compute-plane-services/nvsnap/scripts/install-nvsnap.sh'
rg -n -C 12 -- \
'EXTRA_HELM_ARGS|agent\.l2\.storageClass|--values|(^|[[:space:]])-f([[:space:]]|$)|helm (install|upgrade)' \
"$script"Repository: NVIDIA/nvcf
Length of output: 7453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- guidance files ---'
find .. -name AGENTS.md -o -name .oss-allowlist
echo '--- chart and script references ---'
rg -n -C 4 --glob '!node_modules' \
'agent\.l2\.storageClass|storageClass|values\.yaml|--values|(^|[[:space:]])-f([[:space:]]|$)|EXTRA_HELM_ARGS' \
src/compute-plane-services/nvsnap \
deploy 2>/dev/null | head -400
echo '--- script argument handling and Helm invocation ---'
sed -n '35,65p;230,280p' src/compute-plane-services/nvsnap/scripts/install-nvsnap.shRepository: NVIDIA/nvcf
Length of output: 41664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant guidance ---'
cat -n AGENTS.md 2>/dev/null || true
find src/compute-plane-services/nvsnap -name AGENTS.md -o -name .oss-allowlist
echo '--- nvsnap files ---'
git ls-files | rg '(^|/)(nvsnap|nvsnap-system)(/|$)|install-nvsnap\.sh|Chart\.yaml|values\.yaml' | head -300
echo '--- chart path and storageClass definitions ---'
rg -n -C 6 \
'CHART_DIR|agent:|l2:|storageClass|values\.yaml|--values|(^|[[:space:]])-f([[:space:]]|$)' \
src/compute-plane-services/nvsnap \
deploy/k8s 2>/dev/null | head -500Repository: NVIDIA/nvcf
Length of output: 45250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
script='src/compute-plane-services/nvsnap/scripts/install-nvsnap.sh'
chart='src/compute-plane-services/nvsnap/deploy/helm/nvsnap'
echo '--- script path and Helm inputs ---'
rg -n -C 8 'CHART_DIR|EXTRA_HELM_ARGS|--set|--values|(^|[[:space:]])-f([[:space:]]|$)|helm_cmd' "$script"
echo '--- chart value and template references ---'
rg -n -C 8 'agent|l2|storageClass' "$chart/values.yaml" "$chart/templates" "$chart/README.md"
echo '--- installer documentation ---'
rg -n -C 5 'install-nvsnap|storageClass|--set|values file|--values|-f' \
src/compute-plane-services/nvsnap/docs \
src/compute-plane-services/nvsnap/README.md \
src/compute-plane-services/nvsnap/deploy/README.md 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
script='src/compute-plane-services/nvsnap/scripts/install-nvsnap.sh'
chart='src/compute-plane-services/nvsnap/deploy/helm/nvsnap'
echo '--- script ---'
sed -n '1,70p;225,280p' "$script"
echo '--- chart values ---'
rg -n -C 12 'agent|l2|storageClass' "$chart/values.yaml"
echo '--- chart templates ---'
rg -n -C 10 'agent\.l2|l2\.storageClass|storageClass' "$chart/templates"
echo '--- nvsnap installer docs only ---'
rg -n -C 4 --glob '*.md' \
'install-nvsnap|agent\.l2\.storageClass|--set|--values|helm install|helm upgrade' \
src/compute-plane-services/nvsnap/docs \
src/compute-plane-services/nvsnap/README.md \
src/compute-plane-services/nvsnap/deploy/README.md \
--glob '!archive/**' | head -250Repository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- deterministic check of the current predicate ---'
for name in \
'agent.l2.storageClass=' \
'agent.l2.storageClass=hyperdisk-ml' \
'agentXl2YstorageClass=unrelated' \
'other=agentXl2.storageClass=value'
do
if printf '%s\n' --set "$name" | grep -q 'agent.l2.storageClass='; then
printf 'MATCH %s\n' "$name"
else
printf 'NO MATCH %s\n' "$name"
fi
done
echo '--- accepted option forms from the script ---'
sed -n '40,60p' src/compute-plane-services/nvsnap/scripts/install-nvsnap.shRepository: NVIDIA/nvcf
Length of output: 1037
Match the exact non-empty agent.l2.storageClass value.
The script accepts only --set; -f and --values are rejected. Match the exact key and treat an empty value as unset. Escape the . characters to avoid matching unrelated keys.
🤖 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/install-nvsnap.sh` at line 240,
Update the EXTRA_HELM_ARGS check around the agent.l2.storageClass validation to
recognize only a --set argument with a non-empty value for the exact escaped key
agent.l2.storageClass, excluding -f and --values arguments and unrelated keys.
| candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{" ("}{.provisioner}{")"}{"\n"}{end}' 2>/dev/null \ | ||
| | grep -iE "$rwx_re" || true) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Distinguish query failures from empty results.
Line 242 suppresses kubectl errors. Line 243 uses || true, so permission failures and API failures produce an empty candidates value. Lines 250-252 then report “No RWX-capable StorageClass detected” even when the query failed. Preserve the query status and print an “Unable to inspect StorageClasses” message on failure. Use the “No ... detected” message only after a successful query.
Also applies to: 250-252
🤖 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/install-nvsnap.sh` around lines 242
- 243, Update the StorageClass query in the candidate-detection flow to preserve
the kubectl pipeline’s exit status instead of masking stderr and failures with
“|| true”. In the surrounding logic that reports no candidates, distinguish a
failed query by printing “Unable to inspect StorageClasses”, and reserve “No
RWX-capable StorageClass detected” for successful queries that return no
matches.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter only the provisioner field.
The JSONPath output contains both the StorageClass name and provisioner. grep -iE searches both fields. A class named cephfs or nfs.csi can be reported even when its provisioner is not an RWX candidate. The warning can then recommend a wrong class, leaving L2 PVCs unbound as described in Lines 236-237. Emit separate fields and apply rwx_re only to the provisioner.
Suggested filtering shape
- candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{" ("}{.provisioner}{")"}{"\n"}{end}' 2>/dev/null \
- | grep -iE "$rwx_re" || true)
+ candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.provisioner}{"\n"}{end}' 2>/dev/null \
+ | awk -F '\t' -v re="$rwx_re" 'tolower($2) ~ re { printf "%s (%s)\n", $1, $2 }' || true)📝 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.
| candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{" ("}{.provisioner}{")"}{"\n"}{end}' 2>/dev/null \ | |
| | grep -iE "$rwx_re" || true) | |
| candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.provisioner}{"\n"}{end}' 2>/dev/null \ | |
| | awk -F '\t' -v re="$rwx_re" 'tolower($2) ~ re { printf "%s (%s)\n", $1, $2 }' || true) |
🤖 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/install-nvsnap.sh` around lines 242
- 243, Update the StorageClass candidate collection around the kubectl JSONPath
pipeline so rwx_re is applied only to the provisioner field, not the combined
name-and-provisioner text. Emit or parse separate name and provisioner fields,
filter candidates by the provisioner value, and retain the class name for the
warning recommendation so non-RWX provisioners cannot be selected by name alone.
nvsnap-server could not be built from a clean checkout on any branch:
ui-builder 6/6: RUN npm run build
error during build: Could not resolve entry module "index.html".
.gitignore carried a bare *.html under "Output of the go coverage tool".
That pattern is meant for `go tool cover -html` output, but it also
matched ui/index.html, Vite's entry point, so the file was never
committed. Someone hit the same rule earlier and worked around the
symptom with a !ui/dist/**.html negation rather than narrowing the
pattern.
Scope the ignore to the coverage artifact and drop the negation, which
is no longer needed. Blast radius is exactly one file: ui/index.html was
the only ignored .html in the subtree.
The file itself had to be reconstructed, since it exists in no branch and
no commit. Recovered from ui/dist/index.html, which is the built output
of the real source and differs only in that Vite rewrites the entry
script into hashed asset tags. Verified by rebuilding the ui-builder
stage and diffing the generated index.html against the committed one:
identical once asset hashes are normalised.
Worth noting a first attempt compiled cleanly and produced a working
image while silently dropping class="dark", the favicon link, and the
body background/text classes -- a build-passes-but-UI-is-wrong outcome
that only the output diff caught.
Two adjacent observations, neither addressed here:
- the committed ui/dist is stale; a fresh build of the same sources
produces different asset hashes
- ui/public/favicon.svg does not exist, so the favicon link 404s in
the shipped build too
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
Added a third install-path fix to this PR rather than opening another one. 3. nvsnap-server could not be built from a clean checkoutOn any branch, including main:
Fix scopes the ignore to the coverage artifact and drops the now-unnecessary negation. Blast radius is exactly one file -- On the recovered file
Verified by rebuilding the Worth flagging how easily this could have gone wrong. A first attempt compiled cleanly and produced a working server image while silently dropping Two adjacent observations, not addressed here
|
balajinvda
changed the title
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/ui/index.html`:
- Line 7: Add the missing favicon.svg asset to the deployed web root so the
existing link rel="icon" reference resolves successfully, or remove the link if
no favicon asset is available. Ensure the browser no longer requests a missing
/favicon.svg path.
🪄 Autofix
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: debf99d6-9f78-4493-9cec-9b8c94fc78b8
📒 Files selected for processing (2)
src/compute-plane-services/nvsnap/.gitignoresrc/compute-plane-services/nvsnap/ui/index.html
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>NvSnap · GPU Capture & Restore</title> | ||
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Line 7: Fix the broken favicon reference.
The supplied PR objectives identify /favicon.svg as missing. The browser will request this path and receive a 404. Add the favicon to the deployed web root, or remove this link until an existing asset is available.
🤖 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/ui/index.html` at line 7, Add the missing
favicon.svg asset to the deployed web root so the existing link rel="icon"
reference resolves successfully, or remove the link if no favicon asset is
available. Ensure the browser no longer requests a missing /favicon.svg path.
|
Followed up on the "does anyone still have the original" question. Found two copies on disk and diffed both. The reconstruction holds. 1. Pre-rename upstream -- Differs in exactly one line, and it is the rename that already happened: Every structural attribute matches: 2. A second worktree -- Also one line, but a different wording: That copy is untracked there too and was never committed on any branch, so it is a local working edit rather than an authoritative source. Why this PR keeps "Capture"The committed If "Snapshot" is the intended wording, it is a one-word change here, but it should land as a deliberate edit with Net: the recovered file is byte-exact against the pre-rename original modulo the rename, and consistent with the shipped artifact. The earlier caveat about my first attempt silently dropping the theme classes is resolved. |
2 participants
Two independent install-path fixes found while validating the nvsnap chart on nvcf-dgxc-k8s-aws-usw2-dev2. Combined into one PR to keep review overhead down. Supersedes #732 and #733, which are closed in favour of this.
Neither change depends on the agent auth work in #555.
1. Chart shipped a 31-version-stale agent (#731)
The chart pinned
agent.image.tag: v0.1.3whilescripts/versions.shwas atNVSNAP_APP_VERSION=v0.2.32.install-nvsnap.shis the documented install path, so a fresh install deployed a very old agent.test-e2e.shrefused to run:sync-versions.shalready targeted the chart viaDIRS=(deploy/k8s deploy). The substitution matched a singleregistry/name:tagtoken, which is howdeploy/k8sspells an image. Chart values split the same reference acrossrepository:andtag:lines, so the pattern matched nothing and the sed no-opped, while still printingSynced nvsnap-agent -> ...:v0.2.32.The staleness check below it used the same
/name:anchor, so it also matched nothing in the chart and passed vacuously. Nothing in the pipeline could catch the drift. A verification step that passes without finding anything is the part that let this sit.Fix:
chart_tag()/set_chart_tag()read and rewrite the split form and run alongside the existing sed; verification now compares every chart tag against its expected version, and also checksimageRegistry, since the chart composes refs as<imageRegistry>/<repository>:<tag>and a drifting registry breaks every image at once.Deliberately not asserting that every image in
IMAGESappears somewhere:nvsnap-initandpyzmq-buildercurrently appear in zero manifests, so that assertion would fail falsely.2. Installer silently leaves L2 fan-out disabled
agent.l2.storageClassdefaults to empty, which disables the L2 per-capture PVC tier. The install still succeeds and prints a clean success banner, while restore fan-out degrades to the L3 peer cascade. The only signal is an info-level agent log line that scrolls past during rollout.dev2 has six RWX-capable StorageClasses available and still came up with L2 off.
The condition is now reported during the existing step-5 cluster auto-detect, listing the RWX-capable StorageClasses actually present:
Reported rather than auto-selected: the wrong class yields PVCs that never bind, and the right choice depends on cluster topology. RWX capability is not exposed on the StorageClass API, so candidates are matched against known RWX provisioners. Suppressed when the operator already passed
agent.l2.storageClass.Customer Release Notes
Fixed the nvsnap Helm chart installing an outdated agent image. The installer now reports when L2 restore fan-out is disabled and lists eligible StorageClasses.
Plan Summary
Not applicable.
Usage
./scripts/sync-versions.shnow rewrites chart values and exits non-zero on drift../scripts/install-nvsnap.shprints the L2 warning in step 5 when unconfigured.Testing
On nvcf-dgxc-k8s-aws-usw2-dev2:
pins nvsnap-agent tag v0.1.3, expected v0.2.32.imageRegistrycorrupted: fails with the registry message.--set agent.l2.storageClass=nvcf-scemits nothing.bash -nclean../scripts/test-e2e.sh vllm-smallPASSES on the resulting install: 33G checkpoint, 8m51s. This is also the rule-10 gate that fix(nvsnap): migrate remaining workloads to criu-v2 and repair the helm chart #472 merged without.Notes
The L2 change is advisory; it does not alter install behaviour or exit status.
nvsnap-l2-waitappears in the chart but has no version variable inversions.sh, so it is left unmanaged. Worth deciding separately whether it should be.References
#731
Related Merge Requests/Pull Requests
Supersedes #732, #733.
Dependencies
None
Summary by CodeRabbit
New Features
Updates
v0.2.32.