Skip to content

fix(nvsnap): warn when L2 fan-out is disabled at install time #733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
balajinvda wants to merge 1 commit into from
Closed

fix(nvsnap): warn when L2 fan-out is disabled at install time #733

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/compute-plane-services/nvsnap/scripts/install-nvsnap.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,32 @@ else
info "skipping cert-manager + webhook (--without-webhook)"
fi

# 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

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

rwx_re='smb\.csi|nfs\.csi|efs\.csi|filestore\.csi|azurefile|excelero|nvmesh|cephfs'
Comment on lines +230 to +241

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

candidates=$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{" ("}{.provisioner}{")"}{"\n"}{end}' 2>/dev/null \
| grep -iE "$rwx_re" || true)
Comment on lines +242 to +243

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

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
Comment on lines +242 to +252

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

fi
fi

# ─── 6. Helm install ───────────────────────────────────────────────────

step "[6/7] helm install / upgrade nvsnap"
Expand Down
Loading