fix(operator): drop allowedTopologies from the generated DHCHAP StorageClass - #484
Merged
Merged
Conversation
boddumanohar
changed the title
…geClass
The StorageClass the operator generates for a `dhchap: true` pool with a
non-empty `allowedNodes` cannot provision a single volume. It carries an
allowedTopologies term on the pool's node label, and external-provisioner
matches that term against the topology keys cached in the selected node's
CSINode object. Those keys are frozen when the CSI node plugin registers
(NodeGetInfo reads Node labels once), so a pool label written afterwards by
syncNodeLabels is not among them and every PVC fails, even on an allowed node:
ProvisioningFailed: error generating accessibility requirements: topology
map[...] from selected node "worker-1" is not in requisite:
[map[simplyblock.io/pool.simplyblock.simplyblock-cluster.pool-a:allowed]]
Reproduced on Talos 1.12 and OpenShift RHCOS 9.6. It clears only when the
csi-node DaemonSet restarts, and forcing that re-registration was already
rejected in #417 as node-wide disruptive.
Stop setting allowedTopologies. The restriction is already carried by the
dhchap_node_label parameter, which CreateVolume turns into the provisioned PV's
nodeAffinity — what the scheduler filters Pods on and what kubelet re-checks at
every mount. Nothing else about the class changes, binding mode included.
No migration and no new RBAC: a generated DHCHAP class has never provisioned
anything, so there is no install base to repair, and this ships in the same
release as the feature.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
boddumanohar
force-pushed
the
fix/dhchap-sc-binding-mode
branch
from
September 4, 2026 13:26
3bcaf86 to
319bcc2
Compare
The spec built its own StorageClass with createStorageClassWithParams, which clones the plain pool StorageClass (AllowedTopologies empty) and injects dhchap_node_label. So it reconstructed half of what the operator generates and never touched the other half — which is how a class that could not provision a single volume passed CI for a release. Create a StoragePool CR with dhchap + allowedNodes instead and use the StorageClass the operator generates from it, asserting that class carries dhchap_node_label and no allowedTopologies before provisioning through it. The operator now supplies the node label and the backend allowed-host entry too, so the sbctl pool/add-host and setNodeLabel scaffolding goes away with it. Skipped outside OPERATOR_MODE, where nothing reconciles the CR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
boddumanohar
marked this pull request as ready for review
September 4, 2026 14:35
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ector The operator now writes dhchap_node_selector on the classes it generates, and CreateVolume prefers it. dhchap_node_label stays readable as a deprecated alias so a StorageClass written before the rename keeps gating its volumes: StorageClass parameters are immutable, so such a class cannot be migrated in place, only replaced. dhchap_node_selector wins when a class carries both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
noctarius
previously approved these changes
Sep 8, 2026
dhchap_node_label is in no released tag (v26.2.7, v26.2.8, v26.2.8-3 all lack it), so nothing installable reads it. A class the operator generated before the rename carries the unsatisfiable allowedTopologies term too and never provisioned a volume, and being create-only it is not rewritten on upgrade, so the alias rescued nothing there either. That leaves only hand-written classes on a main build, which cost one re-apply. Renaming before the first release avoids owing a deprecation cycle for a name that never shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
noctarius
approved these changes
Sep 8, 2026
geoffrey1330
approved these changes
Sep 8, 2026
This was referenced Sep 8, 2026
noctarius
added a commit
that referenced
this pull request
Sep 12, 2026
…gain #525 moved StoragePool to v1alpha2 and with it the StorageClass contract: a class is joined to its pool by three labels rather than by its name, and the operator generates exactly one, for the pool a StorageCluster creates itself. The suite had not moved with it. STORAGE_CLASS_NAME named simplyblock-<ns>-<cluster>-<pool>, which is the name the deleted simplyblockStorageClassName built and nothing writes any more, so every claim in the GCP pipeline stayed Pending and all twelve volume-mounting specs failed on a pod that never became ready. Every spec now writes the class it provisions through. That is the shape the contract asks for, and it is the right one regardless: a class's parameters are immutable, so a shared one is a shared decision about the filesystem, the QoS ceilings and the subsystem packing that no spec needing a different answer can edit. createStorageClass builds one from the cluster under test and the pool the suite was pointed at instead of copying a base class, which is what carried a stale cluster_id or an earlier spec's filesystem into everything derived from it — and what made the suite skip itself, silently and green, whenever the class it copied was missing. The DHCHAP topology spec authored its own class before #484 and was moved onto the generated one precisely so the class users get would be exercised. Users now author it, so it authors it: from the pool's status.uuid, with the three assignment labels, so the pool genuinely publishes it in status.storageClassNames and holds its own deletion behind it. What #484 guards is still guarded, by what the class does not carry: no allowedTopologies term, because external-provisioner matches those against the CSINode topology keys frozen at csi-node registration. The default pool is the other half. ensureDefaultPool wrote spec.volumeDefaults absent, and the apiserver only applies the defaults declared inside an object that is present, so filesystem was never defaulted, the generated class carried no csi.storage.k8s.io/fstype, and the node plugin fell back to ext4 — neither what the CRD declares nor what every release before this one formatted with. Writing the block empty asks for exactly the declared defaults: XFS, and no ceiling of any kind. The guard is an envtest, because a fake client does not default and this is invisible everywhere else until a volume is mounted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013S4FDT9P2xzeZjWefcXjER
boddumanohar
added a commit
that referenced
this pull request
Sep 16, 2026
…#277) Implements design-issue-277-client-side-compression.md against current main rather than rebasing the abandoned PR #402 branch, which predates the csi-driver restructuring (#497), the v1alpha2 CRD redesign, and the SimplyblockDriver-managed DaemonSet (#513). Built on atlas-lib/volstack's three LVM layers per the user's request, in place of PR #402's flat atlas-lib/lvm/vdo package, which retires here with zero importers. - atlas-lib/lvm: a built-in "vdo" VolumeProvisioning handler, registered by the package's own init (the registry existed with nothing real registered against it — CreateLogicalVolume silently dropped compression/deduplication before this). Manager.ForgetDevice/HasOrphanedDMNodes, new lvmdevices/ dmsetup primitives. - atlas-lib/volstack/layers: lvmPhysicalVolume, lvmVolumeGroup, and lvmLogicalVolume now tolerate total path loss (the member device gone entirely, not merely unreadable) without erroring the whole Down walk; lvmVolumeGroup.Release forgets stale system.devices entries (folds in the still-open PR #545). - atlas-lib/kube: client_compression/client_deduplication StorageClass parameters, storage.simplyblock.io/vdo-capable label and its managed-by annotation, the shared vdo-capable marker path. - operator: VolumeDefaults.EnableClientCompression/EnableClientDeduplication (v1alpha2), threaded through ClassParameters and the v1alpha1 conversion's hub-only stash; the csi-node DaemonSet's postStart hook probes dm-vdo alongside its existing nvme-tcp/nvme-rdma modprobes (no hostPID needed — the existing probes already prove that), with RBAC to self-label. - csi-driver: vdoCapableSegment (twin of dhchapAllowedNodeSegment) pins PV nodeAffinity from CreateVolume, deliberately not via StorageClass AllowedTopologies — that mechanism was found broken for DHCHAP and removed in PR #484, for a reason that applies identically to a self-probed capability label. A rawDeviceLayer adapter lets the three LVM layers run through the real volstack.Runner without adopting fabric/filesystem (Phase 1, unrelated, unwired work), wired into NodeStageVolume/ NodeUnstageVolume/restageVolume/NodeExpandVolume only for volumes that request either parameter. - Dockerfile_base: the vdo package (vdoformat), x86_64 only. Deviations from the merged design are called out inline in the doc's 2026-09-16 revision notes (§5, §7.1, §4.1, Q9). Not implemented, matching the design's own already-open items: the zero-capable-node pool event (§5.1, Q13), CSI metrics (§13, Q6), the SetFeatures live-toggle path (Q1), the PVC-size floor admission webhook (Q2, despite the doc's stale "Resolved" note — no such webhook exists on main), and periodic capability re-checking (Q12). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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
3 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.
Problem
The StorageClass the operator generates for a
dhchap: truepool cannot provision a single volume. It carries anallowedTopologiesterm on the pool's node label, and external-provisioner matches that term against the topology keys cached in the selected node'sCSINodeobject. Those keys are frozen when the CSI node plugin registers, so a pool label written afterwards is not among them and every PVC fails, even on an allowed node:Reproduced on Talos 1.12 and OpenShift RHCOS 9.6. It clears only when the csi-node DaemonSet restarts, and forcing that re-registration was already rejected in #417 as node-wide disruptive.
Fix
Stop setting
allowedTopologies. The restriction is already carried by thedhchap_node_labelparameter, whichCreateVolumeturns into the provisioned PV'snodeAffinity— that is what the scheduler filters Pods on and what kubelet re-checks at every mount. Nothing else about the class changes, binding mode included.No migration and no new RBAC: a generated DHCHAP class has never provisioned anything, so there is no install base to repair, and this ships in the same release as the feature.
testing
successful e2e run
🤖 Generated with Claude Code