Skip to content

fix: set PersistentVolume.spec.nodeAffinity for topology-gated volumes - #410

Closed
boddumanohar wants to merge 8 commits into
mainfrom
fix/pv-node-affinity-403
Closed

fix: set PersistentVolume.spec.nodeAffinity for topology-gated volumes#410
boddumanohar wants to merge 8 commits into
mainfrom
fix/pv-node-affinity-403

Conversation

@boddumanohar

@boddumanohar boddumanohar commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

The bug

Closes #403.

StorageClass.AllowedTopologies is only consulted once, at the initial WaitForFirstConsumer scheduling decision. What actually persists a node constraint for the life of a volume is PersistentVolume.spec.nodeAffinity, and that's only set if CreateVolume tells external-provisioner to via AccessibleTopology.

CreateVolume never did that for StorageClasses that select their cluster directly via cluster_id — the path DHCHAP's allowed-node gate uses. So a DHCHAP-gated PVC's pod could be deleted and recreated onto a node outside the pool's AllowedNodes (no drain or failure needed — a plain restart is enough). The backend still rejects the connection at the auth layer, but nothing stops the pod from landing there in the first place.

Zone/region multi-cluster routing was never affected — that path already echoed its topology segment correctly.

The fix

CreateVolume now merges DHCHAP's allowed-node segment into AccessibleTopology.

Matching has to be by the exact per-pool label key, not a shared prefix: buildAccessibleTopology reports every DHCHAP allowed-node label a node carries, and a node can belong to more than one pool's AllowedNodes. A prefix match would AND unrelated pools' labels into one nodeAffinity. So createStorageClassIfNotExists writes the exact key into a new StorageClass parameter (dhchap_node_label) next to the AllowedTopologies term it already builds, and CreateVolume pins only that literal key — req.Parameters can't reconstruct it alone (only pool_name and the cluster UUID are available).

Known limitation

createStorageClassIfNotExists is create-once (StorageClass parameters are immutable in the Kubernetes API — see #401/#409), so this doesn't backfill existing pools. A DHCHAP-gated pool whose StorageClass was already created before this fix ships won't get dhchap_node_label on the next reconcile; its PVCs stay exposed to the original #403 bug until that StorageClass is deleted (e.g. by deleting and recreating the Pool CR, or manually deleting the StorageClass so the next reconcile regenerates it with the new parameter).

Test plan

🤖 Generated with Claude Code

@boddumanohar
boddumanohar marked this pull request as draft August 11, 2026 14:46
boddumanohar and others added 8 commits August 12, 2026 10:27
@boddumanohar @claude
fix: set PersistentVolume.spec.nodeAffinity for topology-gated volumes (
49d3024 
#403)

CreateVolume never populated AccessibleTopology for StorageClasses that
select their cluster directly via cluster_id (the common case), so
external-provisioner never set PV.spec.nodeAffinity. A bound PVC could then
be rescheduled onto any node, even when the StorageClass gates provisioning
to specific nodes via AllowedTopologies (DHCHAP's allowed-node label today,
and VDO's node-local state in the upcoming #402). Extract only the
segments that represent a genuine per-node constraint from the CSI
AccessibilityRequirements and echo them back, leaving plain
NVMe-oF-backed volumes (which behave identically from any node) unpinned.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
fix: scope hard-pin topology matching to the exact per-pool label key
7470f3d 
The prior version matched any "simplyblock.io/pool." prefixed segment in
AccessibilityRequirements, but external-provisioner copies a node's *entire*
registered topology (buildAccessibleTopology dumps every pool-allowed label
the node carries) regardless of which pool the current StorageClass gates
on. A node can belong to more than one DHCHAP pool (AllowedNodes is set
independently per Pool CR), so prefix-matching could AND unrelated pools'
labels into one PV's nodeAffinity, or pin an otherwise-ungated volume that
happened to land on a node hosting some unrelated DHCHAP pool.

Thread the exact expected label key through instead: upsertStorageClass now
also writes it into StorageClass Parameters (dhchap_node_label) alongside
the AllowedTopologies term it already builds, and CreateVolume matches only
that literal key rather than a shared prefix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
docs: clarify hardPinTopologyKeys' extension point isn't a per-featur…
1c69e10 
…e ritual

The prior comment read as if every future topology-gated feature would need
its own new StorageClass parameter like paramDHCHAPNodeLabel. That's only
true when the gate key is itself scoped per resource (DHCHAP's, keyed by
pool). A feature gated on a single fixed, global key — e.g. VDO's upcoming
simplyblock.io/vdo-capable — needs no new parameter at all: the literal key
can be hardcoded and its relevance decided from params already present.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
revert: scope this fix to the CSI driver only, accept multi-pool over…
c659fd3 
…lap as a known gap

#403 doesn't actually block #402 (it says so explicitly, and #402 already
works around it with a nodeSelector pin for its own verification), so
there's no need to carry the operator-side plumbing this fix grew to close
a real but narrow edge case: a single Kubernetes node listed in more than
one DHCHAP-gated pool's AllowedNodes.

Revert the dhchap_node_label StorageClass parameter and the operator change
that set it. hardPinTopologySegments goes back to matching the
"simplyblock.io/pool." prefix, with the multi-pool-overlap behavior now
documented as a deliberately accepted limitation (in both a code comment and
a test that documents current behavior rather than asserting correctness)
instead of solved. operator/ is now byte-for-byte identical to origin/main;
this PR is CSI-driver-only again.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
docs: restructure comments into shorter, scannable paragraphs
bed0532 
Same content, easier to read: split the two dense wall-of-text comments on
hardPinTopologyKeyPrefixes and the CreateVolume merge into short paragraphs
with topic sentences and bullets, and trimmed the now-redundant duplicate
rationale in the test comment down to a pointer at the source of truth.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
fix: pin PV nodeAffinity to the exact DHCHAP allowed-node label, not …
c2d6b5d 
…a prefix

CreateVolume never populated AccessibleTopology for StorageClasses that
select their cluster directly via cluster_id, so external-provisioner never
set PersistentVolume.spec.nodeAffinity for DHCHAP-gated pools. A bound PVC's
pod could then be rescheduled onto any node, even one outside the pool's
AllowedNodes.

CreateVolume now merges DHCHAP's allowed-node segment into AccessibleTopology
alongside the zone/region segments already used for multi-cluster routing.
Matching is by the exact per-pool label key, threaded through as a new
StorageClass parameter (dhchap_node_label) written right next to the
AllowedTopologies term the operator already builds — not a shared
"simplyblock.io/pool." prefix, which would AND together any other DHCHAP
pool's label a node happens to also carry (Pool.Spec.AllowedNodes can overlap
across pools) and wrongly restrict, or over-restrict, the resulting
nodeAffinity.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
refactor: shrink fix to a minimal diff, drop tests and verbose comments
ef6b914 
Same fix, smallest possible footprint: trimmed multi-paragraph doc comments
down to one-line pointers and dropped the new test functions on both the
csi-driver and operator sides.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar @claude
refactor: rename hardPinTopologyKeys/hardPinTopologySegments to dhcha…
fab4d9e 
…pAllowedNodeSegment

hardPinTopologyKeys returned a []string that only ever held zero or one
element, and hardPinTopologySegments's generality (matching a list of keys)
was never exercised beyond that. Collapse both into one function that
returns the single key/value pair it actually deals with — clearer name,
less indirection.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar
boddumanohar force-pushed the fix/pv-node-affinity-403 branch from 79dfafd to fab4d9e Compare August 12, 2026 08:33
@boddumanohar boddumanohar mentioned this pull request Aug 12, 2026
Merged
5 tasks
@boddumanohar

boddumanohar commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

Merged into #417, which now includes this PR's fix (PV nodeAffinity for AllowedNodes/pod-restart) alongside the DHCHAP hostNQN/connect fix, since both address the same DHCHAP feature. Closing this one as superseded.

boddumanohar added a commit that referenced this pull request Aug 13, 2026
@boddumanohar @claude
test(csi): add e2e coverage for the DHCHAP allowed-node scheduling fix (
5c95155 
#403)

Adds a second SPDKCSI-DHCHAP spec covering the bug #410 fixed: CreateVolume
never populated AccessibleTopology for StorageClasses that select their
cluster directly via cluster_id (a DHCHAP pool's StorageClass does exactly
that), so external-provisioner never set PersistentVolume.spec.nodeAffinity.
A pod consuming an already-bound PVC could then be deleted and recreated
onto any node, even one outside the pool's allowed nodes — no drain or
failure needed, a plain restart was enough.

The spec labels a node the way the operator labels a DHCHAP pool's allowed
nodes, sets the matching dhchap_node_label StorageClass parameter, pins the
first pod there so CreateVolume sees that topology, asserts the resulting PV
actually got the expected nodeAffinity, then strips the test's own placement
pin and recreates the pod to confirm the PV's nodeAffinity alone — not
anything this test set up — is what keeps it on the same node.

Also fixes two more unparam findings the new call sites tripped:
waitForControllerReady's timeout is nolint'd like its sibling
waitForNodeServerReady already was, and sbctlE drops its unused stdout
return.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: CreateVolume doesn't set PersistentVolume.spec.nodeAffinity, so a bound PVC's pod can land on any node

1 participant

@boddumanohar