fix(nvsnap): disable L2 on a storage driver that qualified nothing - #2100
Open
balajinvda wants to merge 2 commits into
Open
balajinvda wants to merge 2 commits into
balajinvda wants to merge 2 commits into
Conversation
An L2 StorageClass whose provisioner matched no storage profile was promoted anyway, on the default snapshot-clone ReadOnlyMany shape. That assumes the driver supports volume snapshots and binds ReadOnlyMany across nodes, neither of which it has proven. On Weka, VAST or FSS the result is either a promote-time failure or, worse, a claim that binds and reads wrong. As NVCA's storage capability catalog puts it, an access mode a driver merely accepts is not a qualification. resolveL2Promoter now returns a distinct errUnqualifiedStorage and the caller disables L2. An unreadable StorageClass counts as unqualified too, since an unknown class cannot be a qualified one. The three log lines that announced a fallback now say L2 is disabled and name the remedy: add the provisioner to the nvsnap-storage-profiles ConfigMap, which is already wired and already overrides the built-in table. There is deliberately no override flag. One would exist solely to preserve behaviour for operators depending on the old fallback, and nvsnap is not deployed anywhere yet, so there are none. A flag added for a migration window nobody needs does not get removed later, and #1023 is trying to shrink this surface rather than grow it. Also corrects the doc comment on ResolveStorageProfile, which claimed the caller disabled L2 on a miss while the caller in fact fell back to ROX. That gap between the contract and the behaviour is how the assumption survived; the comment now records it rather than quietly reading true. Three tests, mutation-checked against a revert to the old fallback: an unknown provisioner (csi.weka.io) fails closed, an unreadable class fails closed, and NVMesh still resolves, so backends that already worked are unaffected. Relates to #2099 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/nvcf/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughPromoter resolution now returns errors for unqualified storage and promoter-construction failures. The L2 backend disables L2 when resolution returns an error. New tests cover built-in and ConfigMap profiles, missing StorageClasses, and volume-type-specific capabilities. ChangesL2 promoter qualification
Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Unqualified storage disables L2 without stopping the agent; requests can continue through the L3 fallback. The change is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…mmends The fail-closed error tells operators to qualify their driver by adding an entry to the nvsnap-storage-profiles ConfigMap. Nothing tested that path, which is poor for advice printed in an error. The first three tests only covered the built-in table. Five more, all through resolveL2Promoter with a fake client so the ConfigMap read, the parse and the overlay merge are exercised together rather than just ResolveStorageProfile's pure merge: - a ConfigMap entry qualifies a provisioner the built-ins never heard of, which is the documented remedy working end to end - a ConfigMap entry outranks the built-in for the same key, so an operator can correct a built-in that is wrong for their cluster - a malformed ConfigMap leaves the driver unqualified and L2 off, rather than becoming a silent qualification - an absent ConfigMap still resolves the built-ins, the common case - parameters.type keeps hyperdisk-ml and pd-ssd apart, since conflating them would assume cross-node ReadOnlyMany on a driver that has not qualified it, which is the assumption this branch exists to remove Mutation-checked by ignoring the overlay: the two ConfigMap cases turn red. The first attempt at that mutation reported zero failures because dropping the variable stopped the package compiling and the harness counted no FAIL lines, so the run was rewritten to keep it building. A mutation that does not compile proves nothing. startL2Backend itself stays uncovered: it builds its clients from rest.InClusterConfig, so testing it needs a seam in agent startup. After this change its whole contribution is propagating the resolver's error, and the decision logic is fully covered here. Worth revisiting if that caller grows logic. Relates to #2099 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
This was referenced Sep 24, 2026
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
1 participant
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
An L2 StorageClass whose provisioner matched no storage profile was promoted
anyway, on the default snapshot-clone ReadOnlyMany shape:
That assumes the driver supports volume snapshots and binds ReadOnlyMany across
nodes. It has proven neither. Customers run Weka, VAST and FSS, none of which are
in the built-in table, so the outcome there is either a promote-time failure or,
worse, a claim that binds and reads wrong. As the NVCA storage capability catalog
puts it: an access mode a driver merely accepts is not a qualification.
The reason this survived is a doc comment that read true.
ResolveStorageProfilesaid a miss meant "caller disables L2". The caller did not; it fell back to ROX.
What changed
resolveL2Promoterreturns a distincterrUnqualifiedStorageand the callerdisables L2. An unreadable StorageClass counts as unqualified too, because an
unknown class cannot be a qualified one. The three log lines that announced a
fallback now say L2 is disabled and name the remedy.
The remedy already exists and needs no code: the
nvsnap-storage-profilesConfigMap overlay is wired in
l2_integration.goand overrides the built-intable, so an operator qualifies Weka, VAST or FSS by adding an entry.
The
ResolveStorageProfilecomment now records what it used to get wrong,rather than quietly being corrected.
Customer Release Notes
Not customer visible. nvsnap is not deployed anywhere yet.
Plan Summary
Not applicable.
Usage
To qualify a backend, add an entry to the
nvsnap-storage-profilesConfigMap:Testing
go build,go test ./...,check-gazelle.Eight tests in
l2_integration_unqualified_test.go, all driven throughresolveL2Promoterwith a fake client so the StorageClass read, the ConfigMapread, the parse and the overlay merge are exercised together rather than only
ResolveStorageProfile's pure merge.Failing closed:
csi.weka.io) is reported unqualifiedsilent qualification
Not over-failing:
The documented remedy, which had no coverage at all before:
parameters.typekeeps hyperdisk-ml and pd-ssd apart, since conflating themwould assume cross-node ReadOnlyMany on a driver that has not qualified it
Mutation-checked twice. Reverting the fail-closed return to the old
nilturnsthe suite red, and ignoring the ConfigMap overlay turns the two overlay cases
red. The first attempt at the second mutation reported zero failures because
dropping the variable stopped the package compiling and the harness counted no
FAIL lines; it was rewritten to keep the build green. A mutation that does not
compile proves nothing.
startL2Backendis not covered. It builds its clients fromrest.InClusterConfig, so testing it needs a seam in agent startup, and afterthis change its entire contribution is propagating the resolver's error. The
decision logic is covered above. Worth revisiting if that caller grows logic.
Notes
No override flag, deliberately. One would exist only to preserve behaviour for
operators depending on the old fallback, and with nvsnap undeployed there are
none. A flag added for a migration window nobody needs does not get removed
later, and #1023 is trying to shrink this configuration surface rather than grow
it. If a deployment later turns out to need a window, adding the flag then is
cheap and will be backed by a real case.
This is step 1 of #2099. Step 2 is to stop holding a second opinion about the
same StorageClasses: NVCA publishes qualified access modes in the
nvcf-storage-capabilitiesConfigMap, which nvsnap can read at runtime. That isa read of an existing ConfigMap, not a change to NVCA, and the two Go modules
stay independent.
References
Relates to #2099
Relates to #1023
Related Pull Requests
None
Dependencies
None
Summary by CodeRabbit