fix(nvsnap): repair install path - chart drift, disabled L2 fan-out, unbuildable server - #737
Open
balajinvda wants to merge 5 commits into
Open
fix(nvsnap): repair install path - chart drift, disabled L2 fan-out, unbuildable server#737balajinvda wants to merge 5 commits into
balajinvda wants to merge 5 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>
This was referenced Aug 10, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNvSnap deployment scripts now synchronize and verify Helm image values, update the agent image tag, report missing L2 storage configuration, and expand post-install smoke checks. 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 labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
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>
Contributor
Author
|
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
Contributor
Author
|
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. |
The smoke test had five steps and all of them checked liveness: DaemonSet
rollout, server /healthz, blobstore rollout, webhook TLS Secret present,
agent ready count. Every defect found while validating this chart passed
straight through it, because a stale agent is a healthy agent, a silently
disabled L2 tier is a healthy tier, and a server that cannot authenticate
to its own agents still answers /healthz with 200.
Add four checks that assert the deployment is wired and configured the
way the operator asked, not merely alive:
[6] agent image matches the chart's expected tag. The chart pinned an
agent 31 versions behind versions.sh (#731) and nothing noticed.
[7] L2 state is explicit: fail when agent.l2.storageClass names a
class that does not exist, warn loudly when unset, so a degraded
restore path is a decision rather than an accident.
[8] every component that calls the agent carries NVSNAP_AGENT_TOKEN.
nvsnap-server did not (#736); under --auth-mode=required its
cascade delete 401'd, returned 204 anyway, dropped the catalog row
and orphaned the dump.
[9] auth is enforced rather than configured: an authenticated call
must be accepted and, in required mode, an anonymous one rejected.
Auth values are read nil-safely via (.Values.agent.auth).enabled so the
chart still renders on branches where agent.auth does not exist yet.
Without that the template errors out and the whole chart fails to render
with default values -- caught only because the defaults case rendered
zero objects while the --set cases looked fine.
The ClusterRole gains storageclasses read for step 7; otherwise that
check fails on RBAC rather than on what it is testing.
Verified on nvcf-dgxc-k8s-aws-usw2-dev2. Renders correctly with defaults,
with auth on in both modes, and with L2 set. On its first real run it
failed the release and reported:
[8/9] FAIL: server has no NVSNAP_AGENT_TOKEN — its agent calls will 401
[9/9] FAIL: anonymous call returned 200, want 401 — required mode is
not enforcing
Both were true: the upgrade had been run from a branch whose chart has no
auth wiring, so --set agent.auth.enabled=true was accepted and silently
did nothing, taking a cluster that had been enforcing auth back to open.
That is precisely the failure the old five steps would have called a
successful install.
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
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/deploy/helm/nvsnap/templates/post-install-smoke.yaml`:
- Around line 259-268: Update the workload validation loop around the kubectl
jsonpath check to inspect the NVSNAP_AGENT_TOKEN env entry’s
valueFrom.secretKeyRef, not merely its name. Require each checked workload to
reference secret nvsnap-agent-token with key token, while preserving the
existing OK/FAIL reporting and conditional server validation.
- Around line 94-96: Update the L2 validation in the post-install smoke test to
require agent.l2.snapshotClass whenever agent.l2.storageClass is set, and verify
the referenced VolumeSnapshotClass exists before reporting L2 enabled. Extend
the RBAC rules to grant get access to volumesnapshotclasses in the
snapshot.storage.k8s.io API group, while preserving the existing StorageClass
validation.
- Around line 272-279: Update the token retrieval around TOK in the auth
smoke-test block to run within an if condition so a missing nvsnap-agent-token
Secret does not terminate the script under set -euo pipefail. Mark the smoke
check as failed and skip the authenticated curl request when retrieval fails or
produces no token; only set AUTHED from the request when a token is available.
🪄 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: aa0cfe41-7734-4d6e-8fc8-233e2d82a6b2
📒 Files selected for processing (1)
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/post-install-smoke.yaml
Smoke test, step 7 (major): a named snapshotClass that does not exist is now a failure, because the snapshot-clone promote silently has nowhere to go. Deliberately not requiring snapshotClass whenever storageClass is set, as review suggested: internal/checkpointstore/storage_profile.go only needs it for StrategySnapshotClone (GKE PD, EBS), while SharedVolumePromoter provisioners (nvmesh, EFS) promote zero-copy and take no snapshot. Requiring it unconditionally would fail every nvmesh cluster, including the one this was validated on, which runs L2 correctly with it empty. When unset the step now prints the resolved provisioner so the choice is visible. Smoke test, step 8 (major): verify where NVSNAP_AGENT_TOKEN comes from, not just that the name is present. A literal value, or a secretKeyRef pointing at the wrong Secret or key, yields a token the agent will not accept -- failing exactly like no token at all while passing a name-only check. Now asserts secretKeyRef nvsnap-agent-token/token. Smoke test, step 9 (minor): a missing Secret no longer kills the script under set -e, so the remaining checks still run and report. install-nvsnap.sh (minor): preserve the StorageClass query's exit status. Masking it made a denied or unreachable API look identical to "no RWX class exists", sending the operator to provision storage they may already have. Failure now says so explicitly. install-nvsnap.sh (minor): stay silent when -f/--values is passed. That is a file this script does not parse, so claiming L2 is disabled would be an assertion on evidence we do not have. RBAC gains volumesnapshotclasses read for the step 7 check. Verified: chart renders with defaults, with auth in both modes, with L2 set, and with snapshotClass set and unset. Installer verified on nvcf-dgxc-k8s-aws-usw2-dev2 across all three branches -- unset lists the nvmesh candidates, --set stays silent, and a failed query reports the distinct message. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
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.
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.