The CSI driver becomes a SimplyblockDriver the operator reconciles - #513
Merged
Merged
Conversation
Every cluster running simplyblock already holds the CSI driver's objects, applied by the chart, so the first SimplyblockDriver on an upgraded cluster takes a deployment over rather than making one. The document said the opposite, that nothing is migrated because the kind does not exist, and §4.3 is the handover it needed. The object set, the derivation, and the refusals are measured against a live 26.2.7 release rather than recalled. Every chart name is literally <object name>-csi-<part>, so an object named simplyblock (beside the ControlPlane singleton) derives exactly the running names and adoption needs no mapping table. The spec is seeded from what is running, because the first reconcile after adoption has to be a no-op, and a field the translation cannot express is a live deployment reconfigured by the reconcile that adopts it. Three gaps the adoption path forced into the open: - §4.1 claimed every applied object becomes a child by controller reference. Twelve of them are cluster-scoped, where that owner reference is unresolvable, so they carry storage.simplyblock.io/managed-by and a finalizer instead. - The chart's nodeSelector and tolerations are the controller plugin's while the CRD's were the node plugin's, so controllerNodeSelector and controllerTolerations are added. - enableVolumeSnapshots was cited in §2 and §8 and absent from the appendix. §3.4 settles Q1 and Q4 together: a Kubernetes cluster holds one SimplyblockDriver. Two objects deriving the twelve cluster-scoped names get one object written twice, and the ClusterRoleBindings are where that surfaces, since each names a ServiceAccount together with its namespace and two controllers alternate the subject. Below the names the node plugin's kubelet socket path is per driver name, which no object name reaches. A validating webhook denies the second at admission, and the controller refuses for the object written while the webhook was not serving. Q1 and Q4 retire their numbers rather than reuse them, Q3 narrows to the field adoption still needs, and Q6 opens: which control planes configure the one driver, given that a StorageClass is cluster-scoped and the format the driver reads already carries a list. The test plan gains adoption and singleton coverage, 117 scenarios against 75. I-12 asserted that two drivers in one namespace were accepted and is struck through, superseded by I-18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… cluster Two open questions answered, and the second reaches a second document. Q5, whether a pinned sidecar image has to survive adoption: it does. spec.sidecarImages carries one optional field per sidecar, and the six are named rather than mapped, since a key nobody validates is a typo nobody notices. Unset takes the version this operator release ships. The distinction the adoption path turns on is between a pin and a default: the running image is compared against the default of the chart version the release was rendered from, so a sidecar somebody moved stays where it was put and one nobody touched is not frozen at a tag that stopped being maintained. Each field carries the registry pattern spec.image carries, because the node plugin is privileged and mounts /dev, /sys, and the kubelet's plugin directory, so a sidecar beside it runs with that access. The count was also wrong. The chart references eight images for this deployment, of which one is the driver and one is the cluster's snapshot-controller, so there are six sidecars and not seven. The snapshot controller stays out of the spec because it is the cluster's rather than the deployment's, and an adopted deployment never installs it. Q6, which control planes configure the one driver: one, because a Kubernetes cluster now holds one ControlPlane. That is the same limit the operator already lives under, since its ClusterRole, its APIService, and its two webhook configurations are cluster-scoped objects at fixed names and a second install contends with the first over all of them. Several backend clusters do not need several control planes: the driver's configuration carries one clusters entry per StorageCluster against the single endpoint, which is what the live secret holds today. design-controlplane.md carried "one per namespace" in four places, and its own §12 Q1 asked whether that was the right limit. Both are answered here, so §3.1 is rewritten, the non-goal and the appendix comment follow it, and Q1 retires its number. design-crd-model.md's spine diagram and inventory row say cluster too. A fuller re-sync of design-controlplane.md against this decision is its own pass. Q1, Q4, Q5, and Q6 have now retired their numbers rather than reusing them, and Q3 narrows to whether driverName stays settable for deployments that have not set it. Q2 and Q3 are what is left. The test plan reaches 124 scenarios. U-74 asserted that adoption re-images the sidecars and is struck through, superseded by U-83 to U-87. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ssion The first slice of design-simplyblockdriver.md: the API type and the webhook that keeps a Kubernetes cluster to one CSI driver deployment. The reconciler is not here. The kind is born at v1alpha2 and registers no older version, because nothing was ever persisted at one, so it needs no conversion function and appears in no storage rewrite. §7.3 of the upgrade design says so and the two kinds already living there agree; the driver design's own §3 named a v1alpha1 path, which was stale in the same way the ClusterDeploymentConfig and OperatorOps designs were, and it now says v1alpha2 with the reason. SimplyblockDriverValidator denies a CREATE where a SimplyblockDriver already exists anywhere in the cluster. The rule is CREATE and not UPDATE, since an edit to the object that exists is not a second one and a rule over every operation would lock the running deployment's own spec. The denial names the namespace and the name that hold the deployment, because an administrator who is told only that one exists has to go looking, possibly in a namespace they were not watching. The object it names is the oldest, with namespace and name breaking a tie, so this webhook and the controller half that is still to come reach the same answer from the same list without a lock between them. failurePolicy=Fail, like every other validator here. What it blocks while unavailable is creating a driver, which is deployment-time rather than data-path, and the object written in that window is the controller's to catch. The test was red first, against a handler that admitted everything: the two denial cases failed and the four admissions passed, which is the evidence that the rows assert the denial rather than the shape of the code. check-crds.py reports no finding against the new type. It does not reach it in a default run, because API_DIR is still operator/api/v1alpha1 and every kind born at v1alpha2 is outside it, so the audit was run with --paths. Generated artifacts follow: deepcopy, the CRD, the kustomization entry, the manager role's new rule, the webhook manifest, the chart, and dist/install.yaml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The spine the apply path is built on, and the part of it the design argues hardest about. Every name is derived as <object name>-csi-<component>, and names_test.go holds that derivation against the nineteen literals a 26.2.7 chart install actually writes. That test is the claim adoption rests on: a SimplyblockDriver named simplyblock lands on the objects already running, so taking them over is a Get on each rather than a mapping table maintained against chart history. It was proven red by shortening the infix to "-", which broke all nineteen. The registration is the one name that is not derived. It is spec.driverName, because that is what every PersistentVolume records in spec.csi.driver, so the cluster knows it and this object does not get to pick it. Ownership splits by scope rather than by kind. A namespaced object becomes a child by controller reference and goes with the garbage collector. A cluster-scoped one cannot, because Kubernetes treats a cluster-scoped object owned by a namespaced one as having an owner it cannot resolve and never collects it, so those carry storage.simplyblock.io/managed-by and the finalizer will delete them. Proven red by removing the scope test, which left a DaemonSet with no controller reference. setOwnership is additive on labels. Adoption meets objects a chart labeled, and rewriting a running object to drop app.kubernetes.io/managed-by while adding ours is a write with no reason behind it. mayDelete is the right the label grants, and it stops at another controller's value, at no value, and at the same value under the pre-rename simplyblock.io prefix. A cluster-scoped object is shared ground, so deleting one this controller did not mark is deleting somebody else's. The finalizer constant is not here. It has no user until the reconciler exists, and an unused constant is a lint failure rather than a placeholder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cars The objects that are not the two workloads. Each is built to match what a 26.2.7 chart install leaves running, because adoption reconciles toward the state that is running and a field this code invents is a live deployment reconfigured on the reconcile that adopts it. The RBAC is five ClusterRole and ClusterRoleBinding pairs and two ServiceAccounts, with the chart's rules copied verbatim. The rules are the part a reader tidies and each is load bearing: the node plugin deletes pods to recover a stuck mount, the provisioner patches volumesnapshotcontents/status, the attacher patches volumeattachments/status. Each binding names its ServiceAccount together with this driver's namespace, which is the field two drivers would alternate and the reason §3.4 keeps a cluster to one. A test asserts no rule is a wildcard, since a wildcard in a privileged sidecar's role is the escalation primitive that turns a sidecar compromise into cluster-admin. The registration sets every field the API server would otherwise default, so the built object equals the live one and adoption produces no diff to explain. The snapshot class is unstructured rather than typed. The external-snapshotter client is in no go.mod in this monorepo, the object is three fields, and the operator has to work against whichever version of the CRD the cluster serves rather than the one a pinned client module was generated from. Pulling a module for that trade is the worse side of it. The six sidecar images are pinned to this release and overridden one at a time through spec.sidecarImages, which is what Q5 settled. A test holds that a pin on one does not move another, because a pin that dragged the other five with it would freeze a deployment at the moment somebody fixed one thing. Every table was proven to bite by perturbing what it asserts: shortening the name infix broke all nineteen names, dropping "delete" from the node role's pods rule and adding a wildcard to the provisioner's broke the rule tables, flipping podInfoOnMount and hard-coding the snapshot class's driver broke the registration tests, and ignoring the attacher override broke the sidecar test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two plugins, built to match the pod specs the chart renders. This is the part of the apply path where invented fields cost the most: a difference in the node DaemonSet is a rolling restart of every node plugin in the cluster, taken on the reconcile that adopts a running deployment. Placement is per plugin and the two do not leak into each other, which is what the controllerNodeSelector and controllerTolerations fields were added for. The chart puts nodeSelector and tolerations on the controller StatefulSet while this kind's unprefixed pair is the node plugin's, so a translation that fed one into the other would move the node plugin off the workers whose volumes it attaches. A test asserts each pair reaches its own plugin and nothing else. The csi-link arguments and volumes are deliberately absent. They are gated on a chart value that is off in every deployment measured, and a deployment that has them is one the translation has to grow a field for rather than one this file guesses at. Same for the TLS helpers. The node plugin's privilege is asserted rather than assumed: privileged, with SYS_ADMIN and SYS_MODULE, host networking, and the matching DNS policy. It loads the NVMe-oF transports and mounts in the host namespace, so losing any of that is losing every attach in the cluster, and a test is cheaper than finding out. The registrar takes no resource block, because the chart gives it none and inventing one is a restart nobody asked for. The same reasoning fixes the registration's defaulted fields in the previous commit. Proven by perturbation: pointing the registrar at the controller socket, feeding the node selector into the controller's pod spec, and swapping the two resource blocks each broke the test that covers them. podspec.go carries the volume and env shorthands, so workloads.go reads as the list the chart renders rather than as nested struct literals. That is the only form in which the two can be compared by eye when one of them moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The apply path, wired into the manager. A SimplyblockDriver now produces the
deployment instead of the chart, which is the half that had to exist before the
chart can stop rendering it.
The object set is eighteen: two ServiceAccounts, two ConfigMaps, five
ClusterRole and ClusterRoleBinding pairs, the two workloads, the CSIDriver
registration, and the snapshot class when the toggle is on. Applied with
server-side apply under a stable field manager, which is the same call that
creates an absent object and takes over one a Helm release left behind.
Two objects the design listed are deliberately not in it, both found by reading
what the cluster actually runs rather than what the chart renders:
- simplyblock-csi-secret-v2 is the StorageCluster reconciler's. It upserts one
{cluster_id, cluster_endpoint, cluster_secret} entry per cluster it creates or
adopts, so the credentials are already operator-managed. Claiming it here
would put two controllers on one object, alternating its contents, which is
the failure §3.4 keeps a cluster to one driver to avoid and is no better
between two kinds. The deployment mounts it and does not own it.
- simplyblock-csi-secret, the v1 Secret, is mounted by nothing. Checked against
every pod in the namespace, not inferred from the templates.
That also answers where the control plane's address comes from: not from here.
§4.1 has this controller resolve the ControlPlane and write the endpoint, and
the endpoint is already in the Secret another controller writes. The two
ConfigMaps this deployment does own carry no cluster state at all.
The finalizer deletes the cluster-scoped half, because the garbage collector
will not, and only where the managed-by label says it may. A ClusterRole another
controller marked is left standing, which a test covers by marking one
storagepool and watching it survive.
The singleton's controller half refuses an object that is not the oldest,
records DuplicateDriver, and applies nothing. DeploymentHolder is exported and
shared with the validating webhook rather than written twice, since the design's
whole argument is that the two must pick the same object out of the same list.
A test asserts list order does not change the answer.
status.phase is Installing and says the objects are applied. Deriving it from
what the plugins report is §4.2 and is the next slice, so the status says what
was done rather than claiming a readiness nothing measured.
Proven by perturbation: dropping the mayDelete guard deleted another
controller's ClusterRole, and dropping the holder comparison let a second driver
apply a DaemonSet.
check-reconcilers.py reports no error. The apply uses the current
ApplyConfiguration API rather than the deprecated client.Apply patch type.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s it Seven templates go and one arrives. node.yaml, controller.yaml, driver.yaml, node-rbac.yaml, controller-rbac.yaml, snapshotclass.yaml, and nodeserver-config-map.yaml rendered eighteen objects between them; the chart now writes one SimplyblockDriver and the operator applies those eighteen. The CR has its own values block rather than being driven from the keys the deleted templates used. simplyblockDriver: carries the image in the chart's repository/tag/pullPolicy shape, the driver name, the two placement groups, the replica count, the resource blocks, and the two toggles. What used to be image.csi, top-level driverName, controller.replicas, node.tolerations, snapshotclass.create, and the six sidecar image entries is now one block a reader can take in at once. sidecarImages defaults to empty, and that is the design's point rather than an oversight: an unset override takes the version the operator ships, so a deployment that never pinned a sidecar is not frozen at whatever tag its chart version happened to carry. Two objects the chart still renders, deliberately: - simplyblock-csi-secret-v2, because the StorageCluster reconciler upserts into it and a fresh install needs it seeded before the plugins can mount it. - the cluster-wide snapshot controller and its CRDs, which are not this deployment's. §4.1 has the operator supply them where a cluster has none, and that is a later slice. simplyblock-csi-secret, the pre-v2 credential, goes with the templates. No pod in a measured deployment mounts it. serviceAccountAuth was the one setting that would have regressed silently. The chart offered it per plugin and controlplane_deploy.yaml reads it to decide which accounts the control plane trusts, so moving the driver out without a field for it would have left the switch half-connected. spec.enableServiceAccountAuth is that field, one for the deployment rather than one per plugin, since a control plane has to be configured for whichever credential the plugins present and two plugins disagreeing means configuring it for both. csiLink stays unexpressed and off. It needs three arguments, a projected token, and a CA ConfigMap, which is a spec surface rather than an oversight to fix in passing, and no measured deployment enables it. helm template renders the CR and no CSI DaemonSet, StatefulSet, or CSIDriver: the three workloads left in the output are the numa plugin, Prometheus, and the control plane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit moved the driver's eighteen objects out of the chart and left the chart writing the CR that produces them, which kept the driver a thing Helm installs and only changed how. The CR is a resource somebody declares beside their StorageCluster, so it goes with them and the chart renders nothing about the driver at all. simplyblockdriver_cr.yaml is gone and so is the simplyblockDriver values block, which nothing reads once the template is gone. The chart no longer carries the driver's image, its name, its placement, its replica count, or its sidecar overrides, because none of those are a Helm release's business now. One value could not go with them. SB_K8S_ADMIN_SERVICE_ACCOUNTS on the management API lists which accounts it will accept a pod token from, and that is the control plane's half of service-account auth rather than the driver's. It becomes controlplane.trustCSIServiceAccounts, next to the workload that reads it, and the two former per-plugin switches collapse into it since the control plane has to trust both accounts or neither. That leaves the switch in two places by nature: this value is what the control plane accepts and spec.enableServiceAccountAuth is what the plugins present, and a deployment that sets one without the other gets a driver whose tokens are refused or a control plane trusting accounts nothing presents. Both default off and the values comment says so. helm template now renders no CSIDriver, no CSI DaemonSet, and no CSI StatefulSet. The three workloads left are the numa plugin, Prometheus, and the control plane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and API-validation/doc consistency issues that should be fixed before merging (notably defensive handling of a zero deployment holder and tightening the required image field validation).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR migrates the CSI driver deployment from Helm-managed templates into an operator-reconciled SimplyblockDriver CRD, introducing a singleton admission guard plus a controller that applies (and later will adopt) the driver’s full object set, while removing the chart templates/values that previously rendered CSI objects.
Changes:
- Add
SimplyblockDriverAPI type + generated CRDs, plus controller logic that applies the CSI workloads/RBAC/registration and enforces cluster-wide singleton behavior. - Add a validating webhook to deny creation of a second
SimplyblockDriveranywhere in the cluster, and wire it into the operator manifests/chart. - Remove Helm chart templates/values for CSI DaemonSet/StatefulSet/RBAC/CSIDriver/VolumeSnapshotClass and update design/test-plan docs accordingly.
File summaries
| File | Description |
|---|---|
| operator/internal/webhook/simplyblockdriver_validator.go | New validating webhook enforcing the SimplyblockDriver singleton on CREATE. |
| operator/internal/webhook/simplyblockdriver_validator_test.go | Unit tests for singleton admission behavior and denial messaging. |
| operator/internal/controllers/driver/workloads.go | Builders for the CSI node DaemonSet and controller StatefulSet (chart-parity). |
| operator/internal/controllers/driver/workloads_test.go | Unit tests asserting workload wiring, args, mounts, and field isolation. |
| operator/internal/controllers/driver/simplyblockdriver_controller.go | New reconciler applying the driver’s object set, finalizer cleanup, singleton enforcement. |
| operator/internal/controllers/driver/simplyblockdriver_controller_test.go | Unit tests for desired object set, singleton controller-half behavior, and finalizer deletion rules. |
| operator/internal/controllers/driver/sidecars.go | Sidecar image defaults + per-sidecar override resolution. |
| operator/internal/controllers/driver/registration.go | Builders for CSIDriver and VolumeSnapshotClass (unstructured) plus snapshot toggle helper. |
| operator/internal/controllers/driver/registration_test.go | Unit tests for registration naming/defaulting and snapshot class contents. |
| operator/internal/controllers/driver/rbac.go | Builders for driver ServiceAccounts, ClusterRoles, and ClusterRoleBindings (chart-parity). |
| operator/internal/controllers/driver/rbac_test.go | Unit tests asserting RBAC completeness, binding subjects, and rule shape (no wildcards). |
| operator/internal/controllers/driver/podspec.go | Helper constructors for pod-spec fragments (hostPath, configMap, secret, etc.). |
| operator/internal/controllers/driver/ownership.go | Ownership split: controller refs for namespaced objects; label-based ownership for cluster-scoped. |
| operator/internal/controllers/driver/ownership_test.go | Unit tests for ownership semantics and deletion guard behavior. |
| operator/internal/controllers/driver/names.go | Centralized name derivation matching prior chart literals; driverName defaulting helper. |
| operator/internal/controllers/driver/names_test.go | Tests ensuring derived names reproduce chart names and that other driver names are disjoint. |
| operator/internal/controllers/driver/holder.go | Shared “deployment holder” selection logic used by webhook + controller to avoid flapping. |
| operator/internal/controllers/driver/config.go | Builders for the two ConfigMaps mounted by the driver (endpoint/credentials intentionally excluded). |
| operator/docs/tests/test-plan-simplyblockdriver.md | Updated test plan with new singleton/adoption scenario taxonomy and coverage counts. |
| operator/docs/designs/crd-redesign/design-crd-model.md | Update ControlPlane singleton scope to “per Kubernetes cluster”. |
| operator/docs/designs/crd-redesign/design-controlplane.md | Update design narrative to match per-cluster ControlPlane singleton model. |
| operator/docs/designs/crd-redesign/design-api-upgrade.md | Update upgrade/adoption object inventory for CSI driver to include RBAC/ServiceAccounts. |
| operator/dist/install.yaml | Regenerated install manifest including the new CRD, webhook, and RBAC updates. |
| operator/config/webhook/manifests.yaml | Add validating webhook registration for SimplyblockDriver. |
| operator/config/rbac/role.yaml | Expand manager RBAC for applying driver objects (apps/statefulsets, rbac bind/escalate, csidrivers, snapshots). |
| operator/config/crd/kustomization.yaml | Include the new SimplyblockDriver CRD base in kustomize. |
| operator/config/crd/bases/storage.simplyblock.io_simplyblockdrivers.yaml | New generated CRD base for SimplyblockDriver. |
| operator/cmd/main.go | Wire in the new reconciler and register the new validating webhook endpoint. |
| operator/api/v1alpha2/zz_generated.deepcopy.go | Generated deepcopy updates for new API types. |
| operator/api/v1alpha2/simplyblockdriver_types.go | New SimplyblockDriver API types (spec/status enums, validation markers). |
| helm-charts/charts/simplyblock-operator/values.yaml | Remove CSI-driver Helm values; add control-plane toggle for trusting CSI service accounts. |
| helm-charts/charts/simplyblock-operator/templates/snapshotclass.yaml | Remove Helm template for VolumeSnapshotClass (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/simplyblock-operator-webhook.yaml | Add webhook rule for validating SimplyblockDriver CREATE. |
| helm-charts/charts/simplyblock-operator/templates/secret.yaml | Remove legacy CSI secret template (simplyblock-csi-secret) from chart. |
| helm-charts/charts/simplyblock-operator/templates/roles/manager_role.yaml | Mirror operator RBAC expansion in the chart-rendered manager role. |
| helm-charts/charts/simplyblock-operator/templates/nodeserver-config-map.yaml | Remove CSI nodeserver ConfigMap template (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/node.yaml | Remove CSI node DaemonSet Helm template (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/node-rbac.yaml | Remove CSI node RBAC Helm template (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/driver.yaml | Remove Helm template for core CSIDriver registration (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/controlplane_deploy.yaml | Switch service-account auth wiring to the new controlplane.trustCSIServiceAccounts value. |
| helm-charts/charts/simplyblock-operator/templates/controller.yaml | Remove CSI controller StatefulSet Helm template (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/controller-rbac.yaml | Remove CSI controller RBAC Helm template (now operator-owned). |
| helm-charts/charts/simplyblock-operator/templates/config-map.yaml | Remove Helm template for CSI config ConfigMap (now operator-owned). |
| helm-charts/charts/simplyblock-operator/crds/storage.simplyblock.io_simplyblockdrivers.yaml | Add new CRD to chart crds/ so Helm installs it on first install. |
Review details
Files not reviewed (1)
- operator/api/v1alpha2/zz_generated.deepcopy.go: Generated file
- Files reviewed: 43/45 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…eport design-simplyblockdriver.md §4.2. status.phase stops being a placeholder that says Installing forever and becomes the four-way answer the design specifies, with the counts that explain it beside it. The pair the section rests on is U-14 against U-15, and the code is ordered so they cannot collapse. The controller plugin decides first and on its own: it is what creates and deletes volumes, so provisioning has stopped whatever the node plugins are doing, and the word is Unavailable rather than a claim about data, because existing attachments survive it. Only then do the node counts decide, and every node plugin being down is still Degraded, because provisioning works and a worker with no plugin is a worker that cannot attach rather than a cluster that cannot provision. nodesTotal of zero is Ready with a message saying no worker matches the selector, and a Normal NoMatchingWorkers event. A selector that matches nothing is a configuration somebody wrote, and §4.2 is explicit that the phase must not pretend the deployment is broken. A missing CSIDriver registration holds at Installing even with both plugins up, since a kubelet that never saw the driver will not ask it for anything. Events mark the arrival rather than the state, so a deployment that sits Degraded says so once instead of on every two-minute resync. The messages carry the numbers. A phase a reader cannot check against the counts behind it sends them to kubectl describe to learn which worker is short, which is the thing status exists to save them. Written test-first against a derive() that returned Installing for everything: the eight phase rows and both count rows failed, then passed. Test plan: U-11 to U-19 move from a gap to a test name, and the coverage summary goes to 15 of 130. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
design-simplyblockdriver.md §4.3. The first reconcile on a cluster that ran simplyblock before this kind existed meets a deployment with volumes attached and workloads running on them, and takes it over rather than rebuilding it. Nothing in the sequence deletes an object. Recreating the node DaemonSet restarts every node plugin in the cluster at once, and recreating the registration takes the cluster's ability to attach a volume away for as long as it is absent, so the apply is a server-side apply that takes the fields and leaves the object. The Helm metadata is handled in the order that leaves no gap. The resource policy goes on in the same apply that takes the fields, because Helm reads that annotation off the live object rather than out of the stored manifest, so a release still tracking the object cannot prune it out from under the controller. Only once that apply has landed are the release's six labels and two annotations removed, so the object is never left carrying neither the release's claim nor this controller's. helm.sh/resource-policy stays: what it says, that this object outlives the release, is the part that became permanently true. Removing them is a merge patch rather than part of the apply. A server-side apply governs only the fields it sets, so a label this controller never writes stays owned by whoever did, and the keys have to be nulled explicitly. The refusal reads the driver name off the running node plugin's kubelet registration path rather than off the registration object, because that path is where the name takes effect. A mismatch is not repairable: spec.driverName is immutable, so the edit that would fix the object is the one admission rejects, and every PersistentVolume already provisioned records the running name. The phase holds at Installing and the counts are still published, since the plugins are running and what is blocked is the handover. §4.3 also specified a second refusal, over the control-plane endpoint and credentials. It is retired here and in the design: those live in the credentials Secret the StorageCluster reconciler owns, so this deployment never writes them and has nothing to compare. The three test-plan rows that asserted it are struck through rather than deleted. status.origin is written once and never revised, because what it records is the state the first reconcile found rather than what the controller did most recently. Proven by perturbation: not recognizing the Helm label, dropping the resource policy, and failing to parse the registration path each broke the test that covers them. Test plan: the coverage pass this deserved. Every unit row a fake client can reach now names a real test, 52 of 89, and the gap rows say which of four things each remaining row is waiting on rather than repeating that the kind does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each TODO names the design section it owes, the test-plan rows it would close, and what it is actually waiting on, so that coming back to one does not start with rediscovering why it stopped. §5 version skew is the largest and the reason the kind exists. It waits on two things outside this repository: GET /_meta/version on the management API, which design §5.3 records as absent, and the releases.yaml a driver release declares its compatible control planes in. The fetch and the pattern matching are implementable today and worth nothing alone, since with no control-plane version to test against every deployment reports no skew. §4.1 snapshot installation is still the chart's. It puts the controller in kube-system and annotates it to survive. What is missing here is the discovery check, the apply where it comes back empty, and status.snapshotSupport reading Installed in that case. csiLink lost its wiring when the driver left the chart. It needs a spec surface before it can be turned on again, and that decision belongs with the csi-link design rather than being guessed at from the template it used to be rendered from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An audit of the implementation against design-simplyblockdriver.md found eleven discrepancies. Four were bugs, three were the design being stale, and the rest were behavior specified and silently missing, which is now marked. The severe one: a duplicate SimplyblockDriver took the finalizer before the holder check ran, and cluster-scoped names carry no namespace, so a second driver of the same name in another namespace derived the holder's twelve cluster-scoped objects exactly. Deleting the duplicate, the object that applied nothing, would have removed the running deployment's five ClusterRole and ClusterRoleBinding pairs, its CSIDriver registration, and its snapshot class, taking the cluster's ability to attach a volume with them. The holder check now runs first, and finalize re-checks rather than trusting it, because an object may carry a finalizer from before this ordering existed and the cost of being wrong is the whole data path. TLS was being turned off by adoption. Both plugin templates carried simplyblock.tlsEnv, tlsVolumeMount, and clientTlsVolume unconditionally, gated on tls.enabled, and this kind has no field for any of it. The apply lists env, volumes, and volumeMounts explicitly, so adopting such a deployment would have reconciled its TLS away and rolled both plugins doing it. Adoption now refuses a deployment carrying configuration the spec cannot express, TLS or csi-link, rather than silently dropping it. It is the same treatment driverName gets and for the same reason: a difference adoption cannot represent is a refusal, not a change to accept. Every event carried an empty action, because it was passed status.phase, which is empty until a status write has landed. action is required on events.k8s.io/v1, so the first event an object ever emitted was likely rejected. It is the reason now, as every other recorder call in this operator passes it. A VolumeSnapshotClass applied while enableVolumeSnapshots was true outlived both the toggle going false and the object's deletion, because the finalizer walked the same toggle-dependent set. It walks a set that always includes the class. The design was wrong three times. Appendix A, which claims to be the only place the type appears whole, was missing spec.enableServiceAccountAuth; the appendix and the shipped type are now identical, checked by diff rather than by reading. §4.1 still had the operator resolving the ControlPlane and owning the Secrets, which §4.3 and the code both contradict. And §4.1 described a livenessprobe sidecar on the node DaemonSet that the chart never rendered and the code correctly does not build, which also made its own count of six sidecars inconsistent. Four TODOs added for what is specified and missing: step 6's verification of the handover before the release's claim is stripped, the seeding of a spec from a running deployment which is unowned in both documents, status.snapshotSupport's Detected half together with the SnapshotsEnabled event, and §6.2's four gauges. The snapshot TODO also records that the apply is not tolerant of a cluster serving no snapshot API, where it fails the whole reconcile rather than skipping one object. Each fix was proven by perturbation: removing the holder re-check let a duplicate delete the holder's ClusterRole, removing the configuration check adopted a TLS deployment, and removing the class from the finalizer's set left it orphaned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A design that specifies a kind also specifies what moving an existing deployment
onto that kind costs, and that cost is the upgrade tool's to pay. Until now it
was discoverable only by reading every design and noticing, which is how the
four requirements this branch created went unrecorded.
The marker is the words "Upgrade tool", in bold, followed by a colon, at the
head of a paragraph, and the paragraph says what the tool has to do and what
breaks if it does not:
grep -rn '\*\*Upgrade tool:\*\*' operator/docs/designs/
The bold is part of the token rather than decoration, because a design that
merely mentions the upgrade tool in prose is not stating a requirement and a
sweep matching those returns a list nobody trusts. design-api-upgrade.md carries
the convention and deliberately no copy of the list: a second place to update is
the one that goes stale, and the requirement belongs beside the decision that
created it.
Four are marked in design-simplyblockdriver.md, all of them found by this
branch rather than invented for the occasion:
- Apply the CRD before the chart upgrade that references the kind. Helm applies
crds/ on install and skips it on upgrade, so the failure is a template error
naming the wrong thing.
- Write the SimplyblockDriver, with its spec seeded from what is running. The
chart no longer renders it. driverName is the row that costs the most, since
defaulting it on a deployment registered under another name produces an object
no edit can repair.
- Refuse the upgrade for a deployment configured with TLS or csi-link, before
anything is annotated or applied. The kind has no field for either and the
controller refuses the handover, which is a bad moment to find out.
- Follow the values that moved, and note that controlplane.trustCSIServiceAccounts
stayed behind in the chart while spec.enableServiceAccountAuth went to the CR.
One without the other is a driver whose tokens are refused.
Also corrects §12.1's CSI row, which still listed both Secrets as adopted by
SimplyblockDriver. §4.3 denies it: secret-v2 is the StorageCluster reconciler's
and the driver only mounts it, and the pre-v2 Secret is mounted by nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mpty holder spec.image is Required and its pattern admitted the empty string. Required on a string is satisfied by "", and the pattern the optional image fields carry opens with an empty alternative, so `image: ""` passed schema validation and produced a container that cannot start. The alternative is removed from this one field and kept on the six sidecar overrides, where an unset value is the point. The CRD, the installer, and the chart's copy are regenerated, and the design's appendix carries the same pattern, checked against the shipped type by diff. deploymentHolder returns a zero key for an empty list, and Reconcile compared against it without checking. A list that came back without the object the same reconcile had just read is a stale cache rather than a second driver, and refusing on it would have held an install at Installing behind a message naming nobody. The object in hand is taken as the holder, and the next pass corrects it if that was wrong. finalize already had this guard, added with the duplicate finalizer fix; Reconcile did not, which is the inconsistency the review found. The third finding, that the test plan still said the reconciler does not exist, was already fixed: the review ran against the first push, before the phase and adoption commits, and the coverage summary was rewritten in each of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
noctarius
pushed a commit
that referenced
this pull request
Sep 12, 2026
Every deployment the chart rendered got TLS unconditionally under tls.enabled: simplyblock.tlsEnv, simplyblock.tlsVolumeMount, and simplyblock.clientTlsVolume on both plugins. The SimplyblockDriver kind that replaced the chart's rendering (#513) had no field for any of it, so adoption refused a deployment carrying it outright (workloads.go's TODO(simplyblockdriver)), and any cluster with tls.enabled=true had no way to move its CSI driver onto the new kind at all — confirmed live in simplyBlockDeployGCP: the e2e workflow had to drop TLS from its control plane just to exercise the new driver-adoption path. spec.tls (enableTLS, enableMutualTLS, provider) reproduces the three Helm values as one nested struct, following the same pattern spec.sidecarImages already set for a chart value that had to survive the move. The env, the volume, and the mount are byte-for-byte what the chart rendered — FDB_TLS_* included, even though neither plugin reads it — because workloads.go's own rule is that a Created deployment's objects have to match an already-running one for adoption to be a no-op rather than a rolling restart. The client-certificate Secret each plugin mounts is derived, not a spec field: <object name>-csi-{controller,node}-client-tls, the same names.go pattern every other object already follows, and the literal names this chart's controlplane_certificates.yaml already writes for cert-manager. A field naming them again would be a second place for the two to disagree. Adoption's TLS refusal is now a comparison rather than a blanket no, the same shape as the existing driverName check: the running node plugin's SB_TLS_CONNECT is compared against what spec.tls would produce, and only a disagreement refuses. Unlike driverName, this one is repairable — an administrator edits spec.tls to describe what is actually running, and the next reconcile adopts. Generated: the CRD, deepcopy, the chart's copy, dist/install.yaml. The design document and its test plan are updated in the same change (12 new unit rows, all covered). The api-design checker's foreign-value allow-list gained "cert-manager", alongside its existing ext4/tcp entries: a product's own name is the one exception to this API group's PascalCase enum rule, and cert-manager's is lowercase-hyphenated the same way tls.provider already spells it everywhere else in this operator. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar
added a commit
that referenced
this pull request
Sep 14, 2026
Found while auditing values.yaml for dead configuration: leaf values with no live template reference anywhere in the chart. - spdkdev.create, benchmarks — zero references anywhere in the chart. - storagenode.numDataChunks, numParityChunks, journalManager — superseded by the cluster-scoped StripeSpec/enableJournalDevice sizing fields (PR #446/#448). - controlplane.observability.graylog.pullPolicy, .retentionPeriod, grafana.pullPolicy, thanos.pullPolicy — repository/tag are wired for all three containers, but none of them sets imagePullPolicy at all (falls back to the Kubernetes default), so these four were pure dead config. The controller.yaml tolerations/imagePullPolicy copy-paste fixes from the original version of this change are dropped: templates/controller.yaml was deleted from main when the CSI controller moved to the SimplyblockDriver CRD (#513), so that file and its bug no longer exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar
added a commit
that referenced
this pull request
Sep 14, 2026
Found while auditing values.yaml for dead configuration: leaf values with no live template reference anywhere in the chart. - spdkdev.create, benchmarks — zero references anywhere in the chart. - storagenode.numDataChunks, numParityChunks, journalManager — superseded by the cluster-scoped StripeSpec/enableJournalDevice sizing fields (PR #446/#448). - controlplane.observability.graylog.pullPolicy, .retentionPeriod, grafana.pullPolicy, thanos.pullPolicy — repository/tag are wired for all three containers, but none of them sets imagePullPolicy at all (falls back to the Kubernetes default), so these four were pure dead config. - logicalVolume.* (the whole block) — templates/job.yaml, its last live reader, has since been deleted; nothing in the chart reads any of these keys anymore. The controller.yaml tolerations/imagePullPolicy copy-paste fixes from the original version of this change are dropped: templates/controller.yaml was deleted from main when the CSI controller moved to the SimplyblockDriver CRD (#513), so that file and its bug no longer exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
noctarius
added a commit
that referenced
this pull request
Sep 17, 2026
Nothing created a SimplyblockDriver on any path. The chart stopped rendering the CSI plugins when the operator took them over and rendered nothing in their place, the operator bootstraps no object, and the upgrade tool's seeding step is not built, so a fresh install came up with an operator, a control plane, and no way to provision a volume. The chart renders it, named simplyblock beside the ControlPlane of that name, because that name is what every child's name derives from and an upgraded cluster's running objects answer to exactly those names. It carries helm.sh/resource-policy: keep for the two reasons the ControlPlane does: an uninstall must not take the driver out from under attached volumes, and with the operator deleted first nothing would be left to release the finalizer. Which profiles render one is a list rather than a negation. Both of today's profiles run workloads that mount simplyblock volumes, so both are on it, and a profile added later renders no driver until somebody decides it should. The negation would give every future profile a CSI deployment for saying nothing, and a driver registers a provisioner and claims the node plugin's socket on every worker. A driver block in values.yaml carries the settings design-simplyblockdriver.md §8 moved onto the spec, which the chart had no home for since #513 took the templates out: driverName, the plugin image, the controller replicas, both placement pairs, snapshots, service-account auth, and the six sidecars. TLS is derived from the chart's existing tls values rather than duplicated, with the lower-case provider spelling mapped onto the API's. values.schema.json refuses an unknown key and a sidecar image outside the allowed registries at render time, which is where a value silently doing nothing is cheapest to find. §4.3 said the chart does not render the object, which is what left the kind with no producer, and it is corrected. The seeding it describes is still the upgrade tool's: a fresh install is right with the defaults, and an upgraded cluster is not, because driverName is immutable and a default reconfigures a live deployment on the pass that adopts it. check-rendered-objects.sh requires the object for both profiles, and was red for both before this. Co-Authored-By: Claude Fable 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
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.
The CSI driver stops being something a Helm release installs and becomes a
resource the operator reconciles. Two design commits settle the model, five
build it, and two take the chart apart.
What changes for a user
A
SimplyblockDriveris declared beside theStorageCluster, and the operatorapplies the eighteen objects the deployment is made of: two ServiceAccounts, two
ConfigMaps, five ClusterRole and ClusterRoleBinding pairs, the node DaemonSet,
the controller StatefulSet, the
CSIDriverregistration, and the snapshot class.helm templaterenders noCSIDriver, no CSI DaemonSet, and no CSIStatefulSet. The seven templates that used to produce them are gone, along with
the values that fed them.
helm uninstallstops being a teardown. It removes the operator and leavesthe driver running, because the driver is a custom resource's now and a custom
resource is what removes it.
The design decisions
A Kubernetes cluster holds one driver, enforced by a validating webhook and
again by the reconciler for the object written while the webhook was not
serving. Twelve of the objects a driver owns are cluster-scoped, and two drivers
do not get a copy each — they get one object written twice, with the
ClusterRoleBindings alternating their subject between the two namespaces. Below
the names, the node plugin registers at
/var/lib/kubelet/plugins/<driverName>/csi.sockand the kubelet registers oneplugin per driver name, which no object name reaches.
Ownership splits by scope. Namespaced objects are children by controller
reference. Cluster-scoped ones cannot be: Kubernetes treats a cluster-scoped
object owned by a namespaced one as having an owner it cannot resolve and never
collects it. Those carry
storage.simplyblock.io/managed-byand a finalizerdeletes them, and only where the label says this controller may.
Names are derived and reproduce the chart's. Every object is
<name>-csi-<component>, which is exactly what the chart wrote literally, so adriver named
simplyblocklands on an already-running deployment and can takeit over in place rather than deleting and rebuilding it. A test holds the
derivation against the nineteen literals a 26.2.7 install produces.
A pinned sidecar survives adoption.
spec.sidecarImagescarries oneoptional field per sidecar; unset takes the version the operator ships. The
distinction is between a pin and a default, so a sidecar somebody deliberately
moved stays put and one nobody touched is not frozen at whatever tag its chart
version carried.
One
ControlPlaneper Kubernetes cluster. The same limit the operatoralready lives under, since its ClusterRole, its APIService, and its two webhook
configurations are cluster-scoped objects at fixed names.
design-controlplane.mdand
design-crd-model.mdare updated to match.Two things found by reading the cluster rather than the chart
simplyblock-csi-secret-v2is the StorageCluster reconciler's. It alreadyupserts one
{cluster_id, cluster_endpoint, cluster_secret}entry per cluster,so the credentials are operator-managed today. The driver mounts it and does not
own it: two controllers writing one object alternate its contents.
simplyblock-csi-secret, the pre-v2 credential, is mounted by nothing.Checked against every pod in a live namespace, not inferred from the templates.
It goes with the chart templates that rendered it.
Not in this PR
status.phasereadsInstallingand says the objects areapplied. Deriving it from what the plugins report is design §4.2 and is next.
possible are here; the field-ownership handover and its two refusals are not.
(design §5.3).
csiLink. Three arguments, a projected token, and a CA ConfigMap, whichis a spec surface rather than something to bolt on. Off in every measured
deployment.
tool's, per
design-api-upgrade.md§12.Two things to know before deploying
The CRD ships in the chart's
crds/directory, which Helm applies oninstall and skips on upgrade. An existing release needs it applied directly:
Service-account auth is now a switch in two places by nature.
controlplane.trustCSIServiceAccountsis what the control plane accepts, andspec.enableServiceAccountAuthis what the plugins present. Both default off.Verification
Full operator suite green,
golangci-lintreports 0 issues, all nine housestyle gates pass,
codespellclean,check-crds.pyandcheck-reconcilers.pyreport no findings.Every test was proven to bite by perturbing the behavior it covers and capturing
the failure, since these are new builders rather than bug fixes: shortening the
name infix broke all nineteen names, removing the scope test left a DaemonSet
unowned, dropping
deletefrom the node role's pods rule broke the RBAC table,pointing the registrar at the controller socket broke the wiring test, dropping
the
mayDeleteguard deleted another controller's ClusterRole, and dropping theholder comparison let a second driver apply a DaemonSet.
🤖 Generated with Claude Code