fix(nvsnap): warn when L2 fan-out is disabled at install time - #733
Closed
balajinvda wants to merge 1 commit into
Closed
fix(nvsnap): warn when L2 fan-out is disabled at install time#733balajinvda wants to merge 1 commit into
balajinvda wants to merge 1 commit into
Conversation
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>
📝 WalkthroughWalkthroughThe installer now warns when ChangesNVSnap installation
Estimated code review effort: 2 (Simple) | ~10 minutes 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: 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/scripts/install-nvsnap.sh`:
- Line 240: Update the guard around EXTRA_HELM_ARGS to match only an exact
agent.l2.storageClass argument, escaping the dots and anchoring the key; require
at least one non-whitespace value after the equals sign so empty assignments
still trigger the warning.
- Around line 242-252: Update the StorageClass discovery logic before the
warning so kubectl query failures are captured separately from a successful
empty result. Keep the check non-fatal, but track the command status and emit a
distinct diagnostic when the query fails; only report that no RWX-capable
StorageClass exists when the query succeeds with no candidates.
- Around line 230-241: Update the provisioner detection in the installer’s
EXTRA_HELM_ARGS validation to use the L2 backend access-mode contract rather
than an RWX-only allowlist: include the GKE Hyperdisk ML provisioner
(pd.csi.storage.gke.io) and other provisioners supported by the referenced
storage-class definitions. Revise the warning text to describe L2-compatible
RWO/ROX support instead of requiring RWX, while preserving the existing guidance
that users must explicitly select an appropriate storage class.
- Around line 242-243: Update the candidate-building command in the
storage-class discovery logic to filter exclusively on `.provisioner`, not the
combined `metadata.name (provisioner)` output. Emit the storage-class name and
provisioner as separate fields, apply `$rwx_re` only to the provisioner field,
and preserve the resulting class name for recommendations.
🪄 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: a279c243-e151-42e1-9478-5d36dbda5804
📒 Files selected for processing (1)
src/compute-plane-services/nvsnap/scripts/install-nvsnap.sh
Comment on lines
+230
to
+241
| # L2 per-capture PVC fan-out is off unless agent.l2.storageClass names an | ||
| # RWX-capable class. Left empty the install still succeeds, but restore | ||
| # fan-out silently degrades to the L3 peer cascade — a large throughput | ||
| # difference that only shows up under multi-node fan-out, long after the | ||
| # installer has printed a clean banner. Say so at install time. | ||
| # | ||
| # Reported, not auto-selected: picking the wrong class yields PVCs that | ||
| # 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 | ||
| rwx_re='smb\.csi|nfs\.csi|efs\.csi|filestore\.csi|azurefile|excelero|nvmesh|cephfs' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the L2 access-mode contract instead of an RWX-only test.
The supplied manifest defines hyperdisk-ml with provisioner pd.csi.storage.gke.io. It documents RWO and ROX support for this L2 backend, so Hyperdisk ML is valid without RWX. The current regex excludes it. A cluster with the recommended class therefore receives incorrect provisioning guidance.
Detect L2-compatible provisioners and update the warning text to match the actual access modes.
This follows the storage-class contract in src/compute-plane-services/nvsnap/deploy/k8s/nvcf-cluster-prep/storage-classes.yaml.
Also applies to: 244-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 230
- 241, Update the provisioner detection in the installer’s EXTRA_HELM_ARGS
validation to use the L2 backend access-mode contract rather than an RWX-only
allowlist: include the GKE Hyperdisk ML provisioner (pd.csi.storage.gke.io) and
other provisioners supported by the referenced storage-class definitions. Revise
the warning text to describe L2-compatible RWO/ROX support instead of requiring
RWX, while preserving the existing guidance that users must explicitly select an
appropriate storage class.
| # 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
Require an exact, non-empty agent.l2.storageClass value.
The guard checks only for the substring agent.l2.storageClass=. An empty value suppresses the warning, and unescaped . characters can match unrelated keys. Parse the Helm arguments or use an escaped, anchored match that requires a non-empty value.
🤖 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 guard around EXTRA_HELM_ARGS to match only an exact
agent.l2.storageClass argument, escaping the dots and anchoring the key; require
at least one non-whitespace value after the equals sign so empty assignments
still trigger the warning.
Comment on lines
+242
to
+243
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Filter only the provisioner field.
The JSONPath emits metadata.name (provisioner), and grep scans both fields. A class named efs-backup can match even when its provisioner is not RWX-capable. The script can then recommend a class that leaves PVCs unbound. Emit separate fields and apply the regex only to .provisioner.
🤖 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 candidate-building command in the storage-class discovery
logic to filter exclusively on `.provisioner`, not the combined `metadata.name
(provisioner)` output. Emit the storage-class name and provisioner as separate
fields, apply `$rwx_re` only to the provisioner field, and preserve the
resulting class name for recommendations.
Comment on lines
+242
to
+252
| candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{" ("}{.provisioner}{")"}{"\n"}{end}' 2>/dev/null \ | ||
| | grep -iE "$rwx_re" || true) | ||
| echo " WARNING: agent.l2.storageClass is unset — L2 per-capture PVC fan-out is DISABLED." >&2 | ||
| echo " Restore falls back to the L3 peer cascade (slower multi-node fan-out)." >&2 | ||
| if [ -n "$candidates" ]; then | ||
| echo " RWX-capable StorageClasses on this cluster:" >&2 | ||
| echo "$candidates" | sed 's/^/ /' >&2 | ||
| echo " Enable with: --set agent.l2.storageClass=<name>" >&2 | ||
| else | ||
| echo " No RWX-capable StorageClass detected; L2 needs one provisioned first." >&2 | ||
| echo " Reference: deploy/k8s/nvcf-cluster-prep/storage-classes.yaml" >&2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report query failures separately from an empty result.
2>/dev/null and || true convert API errors, permission errors, missing kubectl, and an empty StorageClass list into the same result. The script then tells the operator to provision a class without proving that no class exists. Keep the check non-fatal, but report query failure separately.
🤖 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
- 252, Update the StorageClass discovery logic before the warning so kubectl
query failures are captured separately from a successful empty result. Keep the
check non-fatal, but track the command status and emit a distinct diagnostic
when the query fails; only report that no RWX-capable StorageClass exists when
the query succeeds with no candidates.
Contributor
Author
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.
Found while validating the chart on nvcf-dgxc-k8s-aws-usw2-dev2.
Why
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 silently degrades to the L3 peer cascade. The only signal today is an info-level agent log line that scrolls past during rollout:That is a throughput cliff that only becomes visible under multi-node fan-out, long after install. It is easy to conclude the cluster is fully configured when it is not. dev2 has six RWX-capable StorageClasses available and still came up with L2 off.
What changed
Report the condition during the existing step-5 cluster auto-detect, and list the RWX-capable StorageClasses actually present so the operator can act immediately:
Reported rather than auto-selected on purpose: 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 (
smb.csi,nfs.csi,efs.csi,filestore.csi,azurefile,excelero/nvmesh,cephfs). When none match, the message points at the reference StorageClass manifest instead.Suppressed when the operator already passed
agent.l2.storageClass.Customer Release Notes
The installer now reports when L2 restore fan-out is disabled and lists eligible StorageClasses.
Plan Summary
Not applicable.
Usage
./scripts/install-nvsnap.sh-- warning appears in step 5 when L2 is unconfigured.Testing
On nvcf-dgxc-k8s-aws-usw2-dev2:
--dry-runwithstorageClassunset: warning plus the six nvmesh candidates above.--dry-run --set agent.l2.storageClass=nvcf-sc: no warning emitted.bash -nclean.Notes
Advisory only; it does not change install behavior or exit status. Auto-selection was considered and rejected for the reason above.
References
None
Related Merge Requests/Pull Requests
#732
Dependencies
None
Summary by CodeRabbit