CSI-addons volume replication: design, driver services, and operator wiring - #548
Open
geoffrey1330 wants to merge 196 commits into
Open
geoffrey1330 wants to merge 196 commits into
geoffrey1330 wants to merge 196 commits into
Conversation
… rebuilt (#542) * feat(operator): StorageNode moves to v1alpha2, and its operations are rebuilt Both kinds shipped in a shape that predates the group's conventions, so this adds a v1alpha2 hub, a v1alpha1 spoke, and the conversion between them, and rewrites the operations controller against design-storagenode.md in internal/controllers/node/. What moves on the entity is §15.1. spec.storageNodeSetRef becomes spec.clusterRef plus spec.nodeSet, spec.overrides becomes spec.config and stops being an override of anything, spec.socketIndex becomes spec.slot, the failure domain becomes a label rather than an index on both the spec and the status, and the four per-node fields that reached nothing move to StorageCluster.spec.storageNodes. Most of spec.config is immutable by marker, and the three fields with exactly one legitimate writer are guarded by a webhook instead. Status gains a typed phase, a provisioning step, observedGeneration, and a device summary that is two counts rather than a string — which also corrects a rendering that reported total/online against a documented online/total. Appendix C lands with it. StorageCluster.spec.storageNodes is the workload every storage node runs as, which storagecluster_types.go recorded as deliberately absent while this kind was still v1alpha1. The operation is the larger half. It gains a status.step holding a declared statemachine graph per action, an Aborted phase and the spec.abort that reaches it, and the seventh HostMaintenance action that retires the node-drain coordinator. status.triggered goes with nothing replacing it: every step completes on a predicate over current state, and every call is skipped when its target is already at or past what the call would produce, so a step recorded without its side effect having fired is safe to re-enter. The two drain counters regroup under status.drain, where volumesTotal is written once and replaces a pending count that had to be kept in step with it. Two of the conversion's rows needed more than an assignment. spec.clusterRef is not on the stored object and a conversion has no client, so it is read from the controller owner reference the upgrade's reparent step puts there before the storage version moves. And status.subPhase is not a function of status.step: Migrating means the volume drain under Remove and the relocation restart under Migrate, and Restarting means the wait for the node under Migrate, so the action is an input to both directions — which is the defect §6.3 splits the value in two to remove. StorageNodeMetrics joins the four readings in metrics.simplyblock.io/v1alpha2, served rather than stored for the reason they are. It sits between two of them: a device's reading is one drive and a cluster's is the whole fleet, and a node's is the machine, which is the unit placement is decided against and the unit a drain moves volumes off. StorageNode.status.resources.capacity carries the same pair with hysteresis; this is the same measurement without the damping. One thing is not what the design specifies, and §15.4 records it. §8.4 fans the drain out as one PersistentVolumeOps per volume, and that kind has not been written — the StoragePool rework hit the same wall. The fan-out is the VolumeMigration that exists and works, tracked by the same label and cleaned up by the same cascade, and it becomes a PersistentVolumeOps when that kind lands. Blocking a drain that works today would have been the worse trade. One key deliberately does not move to the storage.simplyblock.io prefix. The per-slot storage-node-uuid label on a worker Node is what external-provisioner caches in CSINode, and it hard-errors CreateVolume when a live Node's topology keys do not match the cached set, so §5.2's own rule that the key must never change for a worker's lifetime outranks the prefix migration. The upgrade tool's key rewrite moves it in step with the CSI driver that reads it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(operator): the node domain moves to controllers/node, and StorageNodeSet retires design-crd-model.md §7.10 assigns StorageNode, StorageNodeOps, StorageDevice, StorageDeviceOps, and the storage-node workload to one package, and only the operation had moved. The rest follows here, and with it the retirement §15.3 was blocked on. The entity controller arrives against the provisioning machine of §4.2. Adding a backend node is not idempotent and the call adds every socket of a worker at once, so two objects for one worker that both observe an empty status.uuid would add it twice; the claim is made in Kubernetes first, as an optimistic-lock patch on the transition into Posting, and status.postedAt and the List over siblings that stood in for it are gone. Adoption is a branch of the same machine rather than a second path, reached from the host check by an upgrade Secret and from either gate by a backend node already at the worker's address. HostMaintenance stops being unreachable. §10 has the entity controller raise it when it sees a worker cordoned, which is what the Node watch is for, and the eight-phase drain coordinator in a fleet object's status goes with it. The workload becomes the cluster's. The DaemonSet, the two Services, the EndpointSlice, the serving certificates, the ServiceAccount and its role, and the per-node ConfigMap are children of the StorageCluster now, driven by a second controller on that kind which writes none of its status. Several sets per cluster collapse to one workload, because growth is nodes rather than sets and what differs between hardware generations is per node already. The per-node ConfigMap loses its merge with it: a node carries its whole configuration, so an entry is a rendering of one object rather than a resolution of two. skipKubeletConfiguration is written out rather than substituted. It is the one rename in the migration that also inverts, so a mechanical one would have turned kubelet configuration on for every cluster that never mentioned it. StorageDevice moves in the same change, because §7.10 puts it here and a device cannot discover itself. Its cluster label stops going through the set: a node names its own cluster, so a label on a device no longer depends on a third object being readable. The latency controller's reading moves onto the nodes. A fleet-wide list made every node's measurement a write to one object shared by all of them, and a stale snapshot of that list silently dropped entries a concurrent reconcile had written. Its own package move belongs to controllers/volume, which design-persistentvolumeops.md creates. internal/controllers/testsupport is the shared test package §7.10 names. cluster and pool each rolled a local copy of the same four helpers; the moved device suites use this one rather than adding a third. The storage-version guard is what found the rest. Marking v1alpha2 as stored put StorageNode into TestTheOperatorReadsEveryKindAtItsStoredVersion's guarded set, and every remaining v1alpha1 reader — the device mirror, the node subscription, the validating webhook, and the deployment band's worker check — now reads at the version a fresh install answers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(operator): regenerate the installer and clear what the linter found dist/install.yaml is generated from the same kustomize build the CRDs and the chart are, and the node domain's move changed all three. Only the first two were regenerated, so the installer still granted the retired StorageNodeSet and still withheld storagenodemetrics from the aggregated view role. The linter's five findings are the move's own loose ends. Two functions stopped being able to fail once the StorageNodeSet lookup behind them went, so both lose an error nothing could return. The metric labels a terminal phase reports under get names rather than sharing a spelling with the control plane's device status, which is a different vocabulary that happens to use one of the same words. The conversion fixtures' cluster name becomes a constant now that a third file names it. And the status-subresource constant goes with the device suites that moved out of the package. `make build`, `make lint`, and `make test` are the repository's own entry points and are what should have been run: `go build` alone does not regenerate, and it is the regeneration that was missing. * feat(operator): the StorageNode admission guard, and the sizing stamp its conversion needs §3.2 and §3.4 are the guard. It answers two different kinds of question, and the split is which of them a request can decide on its own. On update it holds the three fields with exactly one legitimate writer. spec.workerNode, spec.config.pcieAllowList, and spec.config.sizing are the operator's, so a +k8s:immutable marker would lock the operator out along with everyone else and no marker at all would let a user invalidate a layout claim by editing a string. The guard already existed for the worker; the other two are new, and the allow list is guarded rather than marked because a migration merges the drives bound on the target host into it. On create it resolves spec.clusterRef and checks the node against the cluster it names. The reference is immutable from creation, so a node naming a cluster that is not there can never be corrected and the rejection asks for the delete-and- rewrite that is the only remedy. Then the device class: a cluster is built out of one class of backend storage, because an erasure-coding stripe placed across both is written and rebuilt at the slower one's rate — so a list mixing PCI addresses with paths is refused, a list of the class the cluster is not is refused, and the PCI filters are refused on a LogicalBlock cluster rather than silently selecting nothing. And the sizing: a user's node is held to the fleet's, while the operator may write a node that differs, because a rolling hardware upgrade is exactly the case where it should. The step is what the conversion's own doc comment promised. spec.config.sizing is required on the hub and has no v1alpha1 spelling at all, so a node stored as v1alpha1 converts up without one and the next write of it is refused. A conversion cannot fill it in — it has no client and runs inside the API server's request path — so the value travels in the stash annotation ConvertTo already reads, and stamp-storage-node-sizing is what writes it. It runs after reparent-storage-nodes, because the cluster it reads is the owner that step establishes, and it refuses rather than writes when the cluster states no core count: a stamp built from nothing would be refused by the field's own minimum one step later and be harder to attribute there. One bug the tests found before the code shipped. The guard compared sizing blocks with ==, and the core count is a pointer, so two separately decoded objects held two pointers to the same number and every update read as a re-size. It refused every edit a user is entitled to make. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… creates a cluster and its nodes (#543) * feat(operator): the ClusterDeploymentConfig controller, which is what creates a cluster and its nodes Retiring StorageNodeSet left nothing producing StorageNode objects: the set's reconciler was the only one, so a deployment following the shipped chart got a DaemonSet and no backend nodes. This is the producer the redesign puts in its place, and it makes a whole deployment one reviewable document rather than a set of objects somebody assembles by hand. The document is ephemeral and everything follows from that. It owns nothing, nothing references it, and nothing reads it after the expansion, so there is no finalizer and deleting it deletes a document. It specifically does not own the StorageCluster it created: an owner reference would make deleting the document delete the cluster and every volume in it. A draft is validated on every reconcile and expanded on none. Validation writes what it found and nothing else, so a reviewer sees the problems before approving rather than after, which is the whole value of the gate and why a draft naming a worker that does not exist is storable at all. It is also the only check a device gets, since admission does not look at devices: a config discovery wrote cannot be wrong about them, a hand-written one can, and approving without reading is how a device mistake becomes an immutable document. The expansion is create-only, which is §6's decision and the one with a real cost. A document that names an existing cluster without asking to is refused with ClusterExists rather than merged, because merging would have the operator decide what a difference means, and the differences that matter are of the form: this node's device list changed, whose only correct handling is not to apply it to a node that already has data on those devices. Growth is a second document naming the cluster in spec.clusterRef, which keeps every config a record of one deployment action. Two shorthands are spent here and nowhere else. spec.environment resolves into the distribution flags on the cluster's workload, and the cluster's sizing is copied into every node's spec.config.sizing, so the nodes carry what they were built with and deleting the document loses nothing. The device class is read off the groups and stamped onto the cluster, because the device lists already say which class the deployment uses and the document carries no field for it. CreatingNodes is the step that must be idempotent, and it is by construction: a node is identified by its cluster, its worker, and its slot, so the step lists what exists and creates only the slots that do not. Three passes over one document produce the same two nodes, which is the test that matters. What is not here: discovery, which is OperatorOps's (§8), and the approval webhook of §5.2. Both are next steps rather than stubs. * feat(operator): a fresh install discovers the fleet by itself An operator installed into a cluster with disks in it can tell the administrator what it found, and that is a better first experience than an empty namespace and a document to write by hand. So a fresh install raises one OperatorOps with action Discover, whose output is a ClusterDeploymentConfig in Draft that nobody has approved and nothing acts on. The guard is the whole of it. A discovery run is read-only and the draft it writes is inert, but Probing creates one Job per worker, so a run that fired on every restart would put a Job on every node of the fleet each time the operator was upgraded. It therefore runs only where no previous result exists, which is three questions rather than one: whether any OperatorOps has ever run, whether any ClusterDeploymentConfig exists, and whether any StorageCluster is deployed. A terminal run counts, and so does a failed one — the administrator has seen the answer either way, and this operator is not the thing to decide they want another. It is a leader-elected Runnable rather than a reconciler, because there is no object whose desired state it converges toward and the question has one answer per installation. The create is idempotent by name on top of the guard, since two replicas answering the same three questions at once would both conclude yes. A failure at any point is logged and swallowed: the operator works without the run, and what is lost is a draft rather than a capability. An administrator who does not want it says so by writing an object named initial-discovery, which the guard then finds and declines behind. The chart's operator_customresources.yaml is rewritten to the path this creates. It was still shipping a StorageNodeSet that nothing has reconciled since #542, and a StorageCluster beside it that would have made the expansion refuse with ClusterExists. Nothing in the chart applies the file — it is a hand-apply reference — so what it documents is now a ClusterDeploymentConfig an administrator reviews, which is what discovery writes, and the pool sample is corrected to the v1alpha2 shape it has had since the pool's own move. * feat(operator): discovery reads what a machine is for, and proposes the storage tier first A discovery run knew a worker's disks, its memory, its taints, and whether it was cordoned, and nothing about what the machine was for. So an OpenShift infrastructure node was indistinguishable from an ordinary worker, and a draft proposed the two as though the choice between them did not matter. It matters in both directions, and neither is the obvious one. An infra node is the tier a cluster's own infrastructure runs on, and simplyblock storage is infrastructure. A fleet with disks in its infra nodes meant those disks to be the storage, and on OpenShift those are also the nodes that do not count against a subscription's core limit. So an infra node with disks is preferred over a worker with disks rather than avoided, and a draft now proposes it first. A control-plane node is the opposite: a data path on an etcd host is a placement almost nobody intends. It is left out unless spec.discover.enableControlPlaneNodes says otherwise, which is what a combined three-node or single-node deployment sets. There is no field beside it for infra nodes, because those need no asking for. The role is a label and not a taint, and that is the whole reason this was invisible. A taint is the cluster refusing to schedule there, which the run already honored; a label is the cluster saying what the machine is for. The two do not coincide: Kubernetes taints its control-plane nodes, so those were excluded by accident, and OpenShift usually does not taint its infrastructure ones, so those passed every check the run had. The draft carries the preference as structure rather than as a note. Each role gets a node set of its own, the infrastructure one first, so a reviewer who wants only that tier deletes a block instead of moving hostnames between them. The hardware grouping inside each set is untouched: two infra nodes with identical disks are still one group. A fleet of plain workers gets exactly the document it got before, one set named for what it is. Two exclusions that were silent now say why. A worker left out for a taint says which taint, a cordoned one says it is cordoned, and a control-plane node says what it is and which field would include it. KubeNode.Taints was recorded for exactly this and nothing had ever read it. * fix(operator): what the review found in the expansion Nine findings, and they share a shape: every one of them was a way for a document to be expanded into something it did not describe, without failing and without saying anything. The worst of them broke two-socket deployments outright. CreatingNodes reads the slot count off the cluster's spec.storageNodes, and nothing put socketsToUse or nodesPerSocket there, because ClusterTemplate had no field for either. So every cluster a document created ran one storage node per worker on socket 0, whatever the document said, and the two-socket path only worked when growing a cluster somebody had configured by hand. Both fields move onto the template, which is where its own doc comment says the layout a cluster cannot change later belongs. Three more changed behavior silently. A growth document's nodes left config.expand unset, so the control plane read them as part of an initial layout rather than as an addition to rebalance onto. An OpenShift deployment left enableKubeletConfiguration nil, which the renderer reads as skipping the kubelet configuration, inverting what every OpenShift setting this product ships does. And a create that lost a race to another actor swallowed AlreadyExists and reported ClusterCreated, which is the ClusterExists case §6 exists to refuse, arriving by a different route. Two were about what a document can express and the expansion cannot honor. Groups may each name their own interfaces, and one DaemonSet serves every node of a cluster, so the first non-empty value won and the rest were discarded; a document whose groups disagree is now a finding rather than a guess. The same shape again for a worker listed in two groups: a StorageNode is identified by its worker and its slot, so the first group creates the nodes and the second group's devices, fault group, and memory settings never reach them. The rest are smaller. The ready-to-deploy marker was an annotation, and a label selector cannot see one, so the marker was invisible to the only thing it exists for. An approved document held because the control plane is unavailable reported Draft, which the API defines as a document nobody has approved. And a pass that created a node and then failed to persist status.nodeRefs left that node out of the record permanently, because the next pass saw it as one that already existed; the record is rebuilt from the slots the document describes rather than accumulated. Eleven tests, one per finding and two for the pair that have an inverse worth pinning: a document that creates its own cluster must not ask for a rebalance onto its own initial layout. * fix(deployment): decline the initial run when nothing can be inspected A fresh install raised one discovery run unconditionally, and on a cluster with no usable worker that run could only fail. A failed run is still an object holding operatorops-finalizer, and an uninstall deletes the operator's namespace and its Deployment together, so the controller that would release the finalizer can be gone before it sees the delete. The namespace then stays Terminating. That is what hung `make undeploy` in e2e: a single-node kind cluster whose only machine is the control-plane node. The guard gains a fourth question, asked with the same predicate the run itself applies. UsableWorker is extracted for that reason rather than restated, so the bootstrap cannot drift into raising runs the run declines, and the worker loop now reads its decision from it and keeps the events for the explanation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rules worked out a reason for every machine they dropped and the run reported a count. A reviewer reading "78 refusal(s)" cannot tell a fleet with no disks from one whose disks are bound to a userspace driver they could reclaim, or from one whose disks carry a partition table somebody left behind. All three were on the lab this was found on, and the run said none of them. The refusals now go out as events, and a per-worker explanation goes into the status message, which is what kubectl shows. Separating the reason from the noise needed a distinction the data did not carry: a machine presents sixteen network block devices and four disks, so device rules mark themselves as pre-filters and Plan.Explain folds in only the refusals about disks that were genuine candidates, deduplicated and counted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g it A controller reported only as takenByUserspace collapsed four states into two. The bool was derived from a driver name the report already carried, so it added nothing, and it lost the distinction that decides what to do next: uio_pci_generic and vfio-pci differ in what they suggest about who bound the device, and a controller bound to nothing at all read as healthy. It also said nothing about whether the binding is live. Bound and in use are different states, and only the second forbids reclaiming the disks. The holder need not be simplyblock either: a userspace binding is equally how a hypervisor passes a disk through to a guest, so a machine whose controllers are in use may be serving something this product knows nothing about, and a refusal reading as "leftovers, take them" would be an instruction to break it. So the wire carries the driver and an InUse the probe measures, pci.CheckHolders fills it in over the process table, and the classification stays a method rather than a field. A controller whose holders could not be read leaves InUse false and the failure is returned, not swallowed, because dropping it turns unknown into free, and that is the error that reclaims a running guest's disk. Collect routes it to the report's Unreadable list. ReportVersion goes to 2. Decode refuses an older report by design, so probes from before this are re-run rather than misread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion table Three changes, each of which was a way for a run to report that a fleet full of disks has none. A controller bound to a userspace driver with nothing using it is a disk this fleet owns and nothing is driving, so the draft now proposes it. The kernel presents no block device for such a controller, so it could not reach a draft through the device reading at all -- but everything a NodeGroup needs to name an NVMe device is its PCI address, and that is the one thing the controller has. A held controller is still left alone, whoever holds it. What is lost with the block device is the disk's size and its content, so the group it lands in is named for its count rather than a capacity. The initial run waives a partition table. A machine that has held data before carries one on every disk, and the run meant to show a fleet what it has was reporting that it has nothing. The waiver stays narrow on its own terms: it admits a disk whose only refusal is the table, so a boot disk stays out on its mount and on the kernel refusing an exclusive open, which are refusals of their own. The probe is pulled on every run. It ships in the operator's image, so an operator deployed from a moving tag was replaced by a pull while its probes were not, and a node holding the previous layer kept running the previous probe. The report version then refused those reports and the run waited on machines that would never answer, which is how an upgrade produced a stalled discovery rather than a wrong one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The approval rules guard an approved document by reading oldSelf.approved, and
the field was a bool tagged omitempty, so a document nobody had approved was
stored without the key at all. The rule then found nothing to read and failed,
which denied the first approval and every one after it:
spec: Invalid value: "object": no such key: approved
evaluating rule: an approved deployment config is immutable
Nothing could be deployed. The gate the whole path runs through was shut against
everyone, including the only transition it was ever meant to allow.
The field is defaulted to false and always serialized, which is the same
requirement read twice: a reviewer sees the gate they are asked to open, and the
rules find the field they read. The has() guards carry the documents already
stored without the key, which cannot be fixed by defaulting alone -- one of them
is the draft this was found on. Every other CEL rule in the group already guards
its reads this way; these two were the exception.
The rules keep their meaning: approval is one-way, and an approved spec is
frozen. They are declared on the spec rather than the object, so the controller
can still mark an approved document with the label the expansion selects on and
still report on what it did with it.
The tests are new because nothing in the tree could have caught this. A fake
client cannot tell a stored false from an absent key, so the package gains an
envtest suite and the rules are exercised against a real apiserver.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The DaemonSet that runs SPDK selects workers by io.simplyblock.storagenodeset, and nothing wrote that label on the provisioning path. Writing it was the retired StorageNodeSet's job; when provisioning was rebuilt around StorageNode the migration path kept doing it and the new path did not, so migrate.go held the only two calls to LabelWorker in the tree. What that produced was a deadlock rather than a failure. A node holds at CheckingHost waiting for its worker's storage-node API to answer, and the process that would answer cannot be scheduled until the label it is waiting on exists. Neither side reports anything wrong, because neither side is wrong. A deployment config expanded into a cluster and three nodes, and the fleet then sat there: the DaemonSet had DESIRED 0 and every node was in Provisioning. Enrollment goes in the workload reconcile, before the DaemonSet, because that is the order a reader wants it in: the selector is written, then the thing that selects on it. It is per worker rather than per node, since two nodes on one worker are two slots of one machine and LabelWorker rewrites that machine's whole slot label set from the nodes that want it. Two returns of a result beside a non-nil error go with it. controller-runtime discards the RequeueAfter in that case and logs a warning about it, so the retry they asked for was never the retry that happened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ve up A storage node added without a management interface is refused by the control plane with "No management interface with IP found in provided interfaces", and the draft named none: discovery read the interfaces only to rank NUMA nodes by NIC speed, so every document it wrote left the field empty and every node_add it led to failed. Nothing validated it either, so the refusal arrived as a task that gave up on a document that looked complete. The probe now reads what choosing one needs. sysfs carries no addresses, so they come through a seam on the inventory Config whose default reads this process's network namespace -- the host's, for a caller with host networking. Bridges are read from the bridge directory the kernel exports rather than guessed from br0 and cni0 and docker0. The choice is a ranking rather than a match, because a fleet's machines do not agree on what their NICs are called. The interface holding the node's InternalIP wins outright, since the operator addresses the worker by that everywhere else; failing that, the fastest hardware interface that is up and holds a reachable address. The cluster's own plumbing never wins, and a machine presenting nothing suitable is named nothing rather than named wrongly. The interface joins the group signature, because a NodeGroup names one for every worker it lists and two machines that call their NICs different things cannot be described by one group. Resolving now asks again when the add is over. It matched the backend node the add was asked to produce and waited out its deadline when there was none, holding the cluster's one node-add slot while it did -- so every other node waited behind a node that was never coming. The task window is the evidence: no node_add still in it means the add this step waits on has finished, and an add that finished without a node is one to ask for again. Report version goes to 3 for the two new interface fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The window publishes what is running, so a task's whole history is the one event raised when it leaves it. That event said the task was no longer running, which is as true of one that succeeded as of one that exhausted its retries, and it was raised as Normal either way. The control plane says which. A node_add that gave up came back with retry 11 and a function_result of max retry reached, and the operator decoded both and read neither: the event was built from the previous snapshot, and the finished task was skipped by the loop that had it in hand. So the finished task is kept by id rather than skipped, and the event carries the outcome. The retry count decides the verdict, because the status says done for both and the schema already names the count as the one number separating a task that is slow from one that is failing. The control plane's own result goes into the note whatever the verdict, since that is where the answer actually is. The recorder in the tests kept the reason and dropped the message, so a test could not have caught any of this. It keeps both now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ating a step A step does its work and then records it, so a recording that fails leaves the step not having happened as far as the next pass is concerned, and the next pass does the work again. Writing's work is creating a document and telling a reviewer about it. The status write was a bare Update of the object the reconcile had started from, so anything that touched the run in between lost it the race. What that produced on a live cluster was a run that wrote one document, reported writing it twice, and reported finding it already there -- describing to a reviewer a race that nobody had, in the only record a finished run leaves. It is now a patch against a fresh read with an optimistic lock, retried here rather than paid for by the step. That is the shape the deployment config's own status write in this package already uses, and the one reconciler in the tree that was not using it. The two other bare status updates are left alone and are not the same hazard: one is a best-effort clear that logs and continues, and the other is a one-shot upgrade step rather than a reconcile loop. The tests reach the failure through a client interceptor, because a fake client never produces a conflict and so could not have caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A StorageNode's name is a cluster name, a worker hostname, and a slot, so it
outgrows the 63 bytes a label value allows on any fleet whose machines carry
fully qualified hostnames. The mirror wrote that name into a label unchanged:
metadata.labels: Invalid value:
"discovered-initial-discovery-cluster-vm03.simplyblock4.localdomain-0":
must be no more than 63 bytes
What that costs is not a truncated label. The API server refuses the whole
object, so the mirror for every device on that node fails to reconcile and none
of them is ever published -- the devices of the machines with the longest names,
which is every machine on a fleet that uses domain names.
The three values now go through the kube.Formula the package already has for
this, which leaves a value that fits exactly as it is and cuts one that does not
with a digest over the whole input. Both halves matter. The labels exist for a
person to select on, so a value nobody can type is a selector nobody can write;
and cutting without the digest would map every node of a long-named cluster to
one label, answering "which devices are in this node" with the whole cluster's.
Only these three are changed. The same raw names reach labels that a DaemonSet
and a disruption budget select on, where the writer and the selector have to move
together; these are written and read back by nothing, which is what makes them
safe to correct alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-entering a step is ordinary. A status write loses a race, approval bumps the
generation, the operator restarts mid-expansion -- and the step runs again. So a
step has to be idempotent against its own prior success, and this one read "a
StorageCluster by that name already exists" and refused, on a document whose own
status.clusterRef said it had put it there.
What that cost was the deployment. The config went Failed at AwaitingCluster
naming the cluster it had created itself, and the nodes it had not created yet
were never created:
spec.cluster.name is discovered-initial-discovery-cluster and a
StorageCluster by that name already exists; set spec.clusterRef to add
nodes to it instead
status.clusterRef is the record that separates the two cases, and it was already
written by the pass that created the cluster. A document that finds its own
cluster resumes; one that finds a cluster it never recorded still refuses, which
is what the check was for.
The AlreadyExists branch gets the same treatment, because a cached read that has
not caught up with this document's own create reaches the identical state by the
other route.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The read that seeds the update is served from the informer cache, and the
DaemonSet controller rewrites status on every pod transition. So while pods are
rolling -- which is exactly when this reconcile runs -- the cached
resourceVersion is stale and the update loses:
reconcile the daemon set: Operation cannot be fulfilled on daemonsets.apps
"simplyblock-storage-node-ds-...": the object has been modified; please
apply your changes to the latest version and try again
The conflict carries no meaning worth failing over: somebody counted a ready pod.
It failed the whole workload pass regardless, so the service, the endpoint slice,
the certificates, and the worker enrollment were all re-run on a back-off for it.
The write is retried on a fresh read, which is the shape the rest of the tree
already uses. The desired object is rebuilt per attempt so a retry does not carry
the version the previous one was refused for.
The sibling writes are the same shape and are left alone, because they are not
the same exposure: nothing but this operator writes the service, the endpoint
slice, or the roles, and two replicas racing over them is what leader election
already prevents. The DaemonSet is the one with another controller writing it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The v1alpha2 port rewrote partitionsPerDevice against the new spec type instead
of carrying it across, and came out one too high in both branches: 1 where a
dedicated journal device means 0, and 2 where a journal partition per device
means 1. The comment recording what the numbers were for was dropped in the same
change, which is how they went unnoticed -- with the contract gone, 1 and 2 read
as plausibly as 0 and 1, and nothing tested it.
The numbers are not a preference. The backend compares what a device already
carries against 1 + this count, repartitions when they differ, and cannot
repartition a device whose table SPDK's gpt module has claimed:
bdev_open: bdev nvme_5n1 already claimed: type exclusive_write by module gpt
spdk_nbd_start: could not open bdev nvme_5n1, error=-1
So asking for one partition too many does not lay a disk out differently. On a
machine that has never run this product it silently builds the wrong layout, and
on one that has -- where the disks already carry the count the fleet was built
with -- it makes the node impossible to add at all.
The contract is restated on the function this time, and the mapping is tested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every node operation held while its cluster was anything but active. That is
right for the operations the gate was written for: one that moves data while the
cluster is rebalancing or unready is either rejected by the control plane or
succeeds into a layout nobody described.
Removal is the exception, because it is the repair rather than the thing being
protected. A node is removed from an unready cluster precisely to make the
cluster ready, so holding the removal closes a loop with no way out:
Warning ClusterNotReady cluster ... is unready rather than active
The node could not be removed until the cluster was active, and the cluster could
not become active while the node it was stuck on was still in it. A node whose
add never finished puts a deployment there and leaves it there, with the only
operation that would fix it refusing to run.
Nothing else is exempt. The gate keeps an operation that moves data off a cluster
that cannot take it, and an exemption wider than the one case that needs it is a
gate that stops meaning anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A device scope is opened when a node resolves its backend id and closed when the
node is torn down, and the two run at different moments with different things
readable. The open has the node freshly placed in its cluster, so the cluster's
id is there to build the key from. The teardown may run with the cluster already
gone -- a cluster deleted with its nodes is the ordinary case -- and the close
needed that same id to rebuild the key it had added under.
Without it the close was skipped and the scope stayed in the set, so the manager
kept a stream open for a node the control plane no longer has:
cpinformer stream disconnected, reconnecting
{"subscription": "device", "scope": ".../b39b01f9-...",
"err": "watch device ...: unexpected status 404 Not Found"}
It reconnects on a 3s-to-30s backoff for the life of the process, one leaked
stream per removed node.
The close is now by the node's own id, which is the thing the scope is about and
the one identifier a teardown always has. RemoveLeaf drops every scope ending in
it and nothing else; an empty id matches nothing rather than everything, because
a caller with no id knows nothing rather than asking for a purge.
The cluster argument that only existed to rebuild the key goes with it, and the
teardown no longer takes a cluster it does not read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing a node is a sequence of calls against a backend node: suspend it, move
its volumes, verify, delete it. The last step already reads a 404 as success,
because a node the control plane no longer knows about is the outcome the delete
asked for. The earlier steps did not, so a removal that found its node missing at
Suspending reported the 404 as a step that could not be advanced, and retried it
for as long as the operator ran:
the step could not be advanced ... step: Suspending
error: suspend node ...: the control plane answered 404:
'StorageNode 7838e194-... not found'
A removal refusing to finish because the thing it is removing is gone. It also
holds the node's finalizer, so the object never goes either, and the deployment
is stuck on a node that does not exist.
The node can be missing before the step that would have removed it in more than
one way: an earlier attempt got that far and lost its response, the add being
undone never registered it, or somebody else removed it. None of them is a
failure of this operation, so the whole sequence is short-circuited rather than
each step patched.
The check asks the control plane rather than the stream's cache. A cache that has
not synced reports every node missing, and reading that as "already removed"
would finish a drain that never moved a volume.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A storage node takes a drive by formatting it, and a drive carrying anything already is not usable until it is. Nothing said so: a discovered document named the disks and left the question to a field further down that it never set, so every deployment onto machines that had held data before failed inside the control plane rather than at the document. The flag goes on the document because the document is what somebody approves. It is destructive, and the cluster's own field is immutable once the cluster exists, so a default applied further down would format disks without the draft ever saying so and could not be undone afterwards. A reviewer reads it in the draft and strikes the line if any of those drives should be left alone, which the note beside it says in as many words. It is named for what is wanted rather than how it is done, because the how differs by class: an NVMe device is formatted to a 4K block size and a logical block device has its signatures wiped. One field covers both, so a document does not have to know which class the expansion resolves it to. Only the NVMe half is carried today -- the cluster has a field for it -- and the logical-block half has nowhere to go yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cap exists because a node add reboots its host, and two hosts rebooting at once costs the control plane its own fault tolerance. Each waiting node enforced it by counting how many siblings had already claimed a slot, which works for exactly one of them: reconciles are serialized per object and not across objects, so every waiting node reads the count before any of them has written its claim. The first add is therefore correctly alone, and the instant it finishes every remaining node sees the same free slot and takes it. A cap that counts other people's writes holds once and then means nothing -- on a three-node deployment the second and third nodes were added together, which is the thing the cap was written to prevent. The decision is now made from the set that is waiting rather than from who has written a claim: the contending workers are ordered, and a node goes on only if its own place in that order is within the number of free slots. Two nodes reading one set reach one answer without either having written anything, and the answer does not change between passes, so a node told to wait is not overtaken by one told to wait beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The document stopped once the objects existed. What that left was a cluster serving nothing behind a document reporting Expanded, with nothing anywhere saying that one more thing was required of anybody: the activation was a StorageClusterOps that existed, was implemented, and was raised by nobody. The document is the one thing that knows when the deployment it describes is whole. status.nodeRefs names every StorageNode the expansion created, so the wait is over what this deployment built rather than over whatever happens to name the cluster -- a node somebody added later is not one it is waiting on, and a node it made that never came up is one it must not pass over. So the expansion gains a step after CreatingNodes: wait for its own nodes to report Online, then ask. The request is a StorageClusterOps like any other, raised by name so re-entering the step finds the one it raised rather than asking twice, and owned by the document that raised it. What happens to the operation afterward is that operation's business. The step carries the longest deadline of the expansion, which is not because activating is slow: every node this document created has to come online first, and the node-add cap serializes them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Releasing an operation's lock is a compare-and-swap on purpose: it re-reads the
cluster, checks the lock is still this operation's, and writes with an optimistic
lock, so a release that lost a race never clears a lock somebody else now holds.
Two operations running against one cluster with neither knowing is worse than a
lock held a moment too long.
The conflict was reported as a reconciler error to preserve that, and it is not
the only thing that does. The cluster's own reconciler writes its status on every
pass, so a conflict here is ordinary rather than exceptional, and what it
produced was a failed reconcile with a stack trace for an operation that had
just succeeded:
release the lock on cluster ...: Operation cannot be fulfilled on
storageclusters ...: the object has been modified
Retrying on a fresh read keeps the property intact, because the check is inside
the retry: every attempt re-reads and re-tests whose lock it is, so a lock taken
by somebody else between two attempts is found on the next read and cleared by
neither. What would be wrong is swallowing the conflict without the re-read --
that lets the caller reach a terminal phase and drop its finalizer while the
cluster stays locked by an object that no longer exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fault The phases after the creation path are a reading of the status the control plane publishes, and everything the operator had no reading for fell through to Unavailable -- which the field documents as not serving, and not because anybody asked. Three statuses landed there that are the opposite of that. A cluster being expanded and a cluster being activated are not serving precisely because somebody asked, and one that has never been activated is not serving because it is not finished. So a deployment reported a fault for its whole length, and an activation reported one for as long as it ran. unready, in_creation, and in_expansion now read as Provisioning, which is the word StorageNode already uses for a thing being built rather than a second word for one idea. in_activation is its own phase, because activation is not only the last step of a deployment: an expansion ends in one and so does recovering from a suspension, long after anything was being built. Unavailable keeps its meaning by being what is left -- a status this operator has no reading for, rather than the name for every cluster that is not serving. The mapping table that lived in storagecluster_controller_test.go is replaced by one in phase_test.go covering every status rather than six of them, because two tables for one function drift and these two would have had to disagree first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…alls it (#544) * feat(operator): ControlPlane moves to v1alpha2, and the operator installs it The registered ControlPlane was an observation with a spec field attached: the chart brought the control plane up, and the object polled a readiness endpoint it learned about from an environment variable. design-controlplane.md replaces both halves, and this is that rework. spec.source says which control plane the object means. A managed one is what the operator installs, and an external one is an endpoint and a credentials Secret the operator probes and reports on. status.endpoint publishes the resolved base URL either way, so one object answers where the control plane is rather than every controller resolving SIMPLYBLOCK_WEBAPI_BASE_URL for itself. The install is a five-step machine over atlas-lib/statemachine: the FoundationDB operator, its RBAC, and the FoundationDBCluster; a wait on that cluster reporting itself available; the object store; the management API with the services beside it; and a wait on the readiness probe. Every step is a server-side apply under a stable field manager, which is what makes re-entering one a no-op and why the machine carries no triggered flag. Steady state re-applies the same objects on every pass, so an object somebody deleted comes back and one somebody edited is corrected — which is why no operation exists for checking the install. The phase is the worst verdict across the probe and eight watched components, and only the management API and the database may produce Unavailable. That asymmetry is the safety property: Unavailable holds every controller in the operator, so the set of things able to cause it is a closed list, and a component added without a decision lands in the non-essential default. ControlPlaneOps is new, with the three operations that are not expressible as desired state. Restart recycles a scope of workloads, draining first only when it names something depended on. Upgrade runs Preflight, drains, writes the image onto the entity, and verifies the reported version afterward, so a rollout that failed back is a Failed operation rather than an Available control plane running the old version. Backup asks FoundationDB rather than implementing one, and does not own the FoundationDBBackup it creates. A validating webhook refuses an operation naming an external control plane at creation, since all three act on something the operator installed. What the install covers is a base control plane, which is what the chart renders with observability disabled, measured against a live deployment. The observability half stays with the chart: none of it appears in a step of the machine, and every component of it is non-essential. controlplane.managedByOperator is the chart flag for §12 Q2 and defaults to the operator. With it set the chart renders only the ControlPlane object beside the FoundationDB CRDs, the Prometheus configuration, and the log-collector RBAC; setting it to false renders the previous templates byte-for-byte. TLS is the one configuration the spec cannot express, so the chart refuses the two together rather than bringing a control plane up in plaintext. v1alpha1 stays as the spoke. Its conversion now resolves an absent image toward a managed source, because the hub requires exactly one member of spec.source and every object stored at v1alpha1 is one the chart installed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(operator): make the ControlPlane CRD generate the same way twice CI failed the helm-sync and build-installer drift gates on output that varied between generator runs rather than on anything stale. Two properties of the ControlPlane CRD came out differently each time controller-gen ran, and both had the same cause: two sources disagreeing about one value, resolved by map iteration. spec.source carried a +kubebuilder:validation:XValidation marker and +k8s:immutable. controller-gen v0.21.0 emits a field's marker-derived rules and its injected immutability rule into one x-kubernetes-validations list, in an order that varied run to run — four of six one way locally, two of six the other. It is the only field in the repository carrying both, which is why this has never flaked before. shortNames is resource-level and both versions of a kind feed one CRD. v1alpha2 declared shortName=cp and v1alpha1 declared a bare scope, so the merge picked one per run and the short name appeared in five of twelve regenerations. StorageNode already repeats its short name on both versions, and this follows it. Fixing the first uncovered a real bug, which a CEL test written against a real apiserver caught: +k8s:immutable on spec.source emits self == oldSelf over the whole struct, so it froze the image inside the block along with the choice of block. design-controlplane.md §3.2 says the block is immutable and its members are not, and the Upgrade action's Applying step writes spec.source.managed.image — which the apiserver rejected. The rule is now explicit about what it freezes: which member is set cannot change, and what is inside it can. Both rules move onto ControlPlaneSource, where two markers of the same kind are emitted in source order, and the field keeps only Required. cel_validation_test.go covers all three outcomes against envtest, including the image edit that used to be refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(operator): regenerate the CRDs after the v1alpha1 prose edit A doc comment becomes a CRD description, so the house-style fixes to ControlPlaneSpec.Image and ControlPlaneStatus.Message left the four derived copies of the ControlPlane CRD carrying the old text: config/crd/bases, the embedded copy the upgrade tool applies, dist/install.yaml, and the chart's. No behavior changes. The regeneration is the whole of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(operator): the eight findings of the review on #544 Every one was real, and three were asserted as correct by tests written with the code they covered. The chart flag did not hand the install back. With managedByOperator false the templates were gated out and the ControlPlane still named a managed source, so the operator installed the same objects the chart had just rendered and both owned them, which is the outcome the flag exists to avoid. It now names an external source pointing at the in-cluster Service, which is what the operator's side of a chart-installed control plane actually is: something that already exists, to be resolved and probed rather than applied. That needs a source with no static credential, so credentialsSecretRef becomes optional and an absent one means an unauthenticated probe. The readiness endpoint takes none. Initializing converted to a phase v1alpha2 rejects. The two enums share no value, and only Ready was mapped, so a control plane observed while FoundationDB was starting became an object the API server refuses on write. Both directions are now written out, and the downward table is no longer derived by inverting the upward one: the hub holds four phases and this version two, so Degraded and Unavailable land on the value that tells a v1alpha1 reader the same thing. The operation's parameters were mutable after admission. Preflight reads spec.upgrade.image and Applying writes it several steps later, so clearing the block between them dereferenced nil. The spec is now frozen except for abort, which is the one field meant to be set after the operation starts, and applying and verifying guard the block besides. The singleton was per name and not per Kubernetes cluster. A "simplyblock" object in a second namespace reconciled, applied the same fixed-name cluster-scoped roles under the same managed-by label, and either one's finalizer deleted what the other needed. The older object holds the install and the younger reports that it does not, which is the rule SimplyblockDriver already used for the same reason. A Restart scoped to the FoundationDB cluster passed validation, recycled nothing, found a healthy database on the wait that followed, and reported success. The scope is checked against what can be rolled rather than against the component table. Deleting a running operation released the control plane's lock mid-rollout. The webhook took create only, so the controller's deletion path dropped the lock and the finalizer from any step. It now takes delete as well and refuses the four steps that have started something nothing else would finish, which is the guard StorageBackupOps already carried. caBundleSecretRef was declared and wired to nothing, so an endpoint signed by a private CA could not be reached. Resolution now builds the transport the probes use, and a bundle that is named and unusable is an error rather than a silent fall back to the system trust store. status.endpoint was published and read by nothing. Both control-plane clients now resolve it per call and keep their startup client where the object has published none, so an external control plane is reachable and adopting this changes nothing for a deployment that has not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(operator): rebuild the installer against the current develop dist/install.yaml is generated from every CRD rather than from the ones a branch touches, so it carries develop's kinds as well as this branch's and has to be rebuilt whenever either moves. This picks up the ClusterDeploymentConfig fields and the StorageCluster phase develop added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(operator): deployment profiles, and the source names they settle A deployment is one of two things, and the chart now says which: standalone, where this cluster hosts its own control plane, or managed, where a control plane elsewhere manages this cluster's storage. The API's two source members are renamed to match, because the word "managed" was about to mean opposite things in the chart and in the CRD. spec.source.local is a control plane this cluster hosts and the operator installs. spec.source.managed is one somewhere else that this cluster registers with. The word is about what manages the storage clusters rather than about who runs the operator, which is the sense the hosted offering uses. Neither name has shipped in a release, so the rename costs nothing but the edit. controlplane.profile replaces controlplane.managedByOperator. The bool had two states and the profiles have two, but not the same two: the bool chose who installed locally, and the profile chooses whether anything is installed locally at all. The chart-installs-locally path is dropped, so nothing renders the control-plane workloads any more and the templates that only held them are deleted. Their observability halves stay. Three guards replace the one the bool carried. An unknown profile is refused by name. The managed profile is refused without an endpoint, since nothing local can stand in. TLS with standalone is still refused, and the message no longer points at a value that no longer exists. A fourth guard is new and is the one that matters. Helm deletes what the old release manifest held and the new one does not, and before this the chart's manifest held the FoundationDBCluster: upgrading a 26.2.x release in place would have pruned the database holding every cluster definition, node registration, and lvol record, and the operator would then have built an empty one. The chart now looks for a control plane an earlier release installed and refuses, naming the annotations that make the upgrade safe. The ControlPlane object itself carries helm.sh/resource-policy: keep for the same reason, and for a second one: Helm deletes the operator and the CR in one uninstall, and with the operator gone first nothing remains to release the finalizer, so the object cannot go and the release sits in `uninstalling`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(helm): deployment.profile, and drop operator.enabled The profile moves out of the controlplane block. It selects the shape of the whole deployment rather than only where the control plane is, and a managed one will carry more than a source member: an edge cluster somebody else administers differs in what it runs and in how its StorageClusters are configured, and those settings belong beside the profile rather than repeated across the blocks they touch. operator.enabled goes. It was true in any deployment anybody wants: with it false the chart renders the CRDs, a snapshot controller, RBAC, and a secret, and neither the operator nor the CSI driver, which is not a deployment so much as half of one. Sixteen templates lose the gate. Helm evaluates a subchart condition as a boolean value path and cannot read a profile string, so Prometheus and the reloader take their own. That is the ordinary shape and truer besides: neither is implied by the operator existing, and a managed deployment reports to the control plane that administers it rather than to a Prometheus of its own. NOTES.txt is rewritten. Its wait command asked for a phase called Ready, which this branch renamed to Available, so it would have waited out its timeout and failed on every install. It also carried a branch for the operator being disabled, which nothing can now reach, and lost the first letter of "Wait" somewhere before this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(helm): cut the values comments to what a reader has to set The deployment block ran to twenty-two lines of comment for one string, most of it explaining why the value sits where it does rather than what setting it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: state the constraint rather than the system that was not built The house style gives three shapes to avoid: counterfactuals describing a system nobody built, rebuttals leading with what a thing is not, and notes reporting the writer's own history. The comments on this branch carried forty-four of them. Twenty-three are rewritten to state the constraint the code works under. Twenty-one are kept: each names the defect a regression test guards, which is the test's reason for existing and what the regression-test skill asks for. Twenty-one em dashes are replaced by the mark that was meant, which is a colon, a comma, parentheses, or a full stop in every case. The rename in an earlier commit moved symbols and struct tags, so eight comments were left describing the members by their old names. The conversion's said it writes the managed member where it writes the local one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): restore the admission configurations this branch dropped templates/simplyblock-operator-webhook.yaml opened with a guard on .Values.operator.enabled. Removing that value in f0b72bc left the guard reading nothing, so the file rendered empty and the upgrade pruned the webhook Service together with both admission configurations. The operator went on registering thirteen handlers and serving them on :9443 with nothing in the cluster routing to them, which is every validating and mutating webhook it has, not only the ones this branch adds. cert-rotation reported it twice, once per configuration, as a certificate it could not update. The guard is dropped at its source in sync-from-operator.sh and the template regenerated. check-rendered-objects.sh is the test. helm template exits zero for a template that renders to nothing, which is why neither the chart lint in CI nor a render check caught this, so it asserts the objects each profile must produce rather than that the render succeeded. Run against the unchanged chart it names the three missing objects in both profiles, and against the pre-fix template it names .Values.operator.enabled too. Verified on a cluster: both configurations present, the CA injected into all ten validator entries, and a ControlPlaneOps naming an absent ControlPlane refused by vcontrolplaneops.simplyblock.io. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): drop the HAJMCOUNT env var, which reads a value nothing defines daad350 added storagenode.haJMCount to values.yaml with no value and the HAJMCOUNT env var that reads it. a50e7d1 renamed the key to storagenode.journalManager inside a 255-line rewrite of values.yaml and left the template on the old name, so the reference has been dangling since April. Because the key was empty from the day it was added, the rename changed nothing observable: the variable rendered as the empty string before it and after it. The setting itself survived the rename on a different path. The operator reads journalManager from the cluster deployment config and sends it as ha_jm_count, so nothing is lost by removing the env var; no chart template reads journalManager at all. Both charts carry the same template, and charts/README.md documented the key as a supported value. check-values-references.sh is the test: it reports every .Values path a template reads that values.yaml does not define. haJMCount in both charts was its only finding across 510 defined paths, and it would equally have caught the .Values.operator.enabled guard fixed in the previous commit. Removing the variable means the container sees it unset rather than set to empty. That is unobservable here, since the whole template is gated on storagenode.create, which defaults to false, but the consumer is an external image and this notes the difference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(operator): name the reconcile paths after the members they serve The rename to local and managed moved the API and the predicates and left the names around them on the old vocabulary, so the dispatch read as its own inverse: isManaged called reconcileExternal, and isLocal called reconcileManaged. The behavior was right and only the names were not, which is the shape that survives review and produces a wrong edit later. The two entry points swap, so the local one moves out of the way first. Eight locals and parameters that bind a LocalControlPlane were named managed, and one that binds a ManagedControlPlane was named external. All of it through gopls rename rather than a search and replace. Two of these are shipped API documentation. A field's doc comment becomes its CRD description, so kubectl explain answered the local member with "Managed is a control plane the operator installs" and the managed member with "External is a control plane that already exists". status.endpoint carried the same inversion. Regenerated into config/crd/bases, the chart's crds, the copy the upgrade tool embeds, and dist/install.yaml. The hold message on an object that names neither member said "neither a managed nor a remote control plane" and now names the two that exist. A rename has no failing test to write first: the compiler and the suite are the guard, and both are green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cluster A ClusterDeploymentConfig is a draft until spec.approved is set, and setting it is what makes the document immutable. A draft naming a worker that does not exist, or a cluster that already exists, was therefore admitted and frozen, and the expansion refused it afterward with nothing left to edit. The two kinds of edit are now answered differently. A draft is admitted on its structure alone, however wrong it is about the world, because that is what a draft is for. The edit that sets spec.approved is answered against the live cluster: every worker named by every group exists as a Node, spec.clusterRef resolves when it is set and the cluster does not already exist when it is not, a growth document's device class matches the cluster's, and no other approved document already creates the same cluster. Every problem is reported at once rather than one per apply, because approval is what freezes the document and an approver fixing one mistake at a time is the same failure in slow motion. A fifth check the design's list does not name is here for the same reason: a document naming neither a cluster nor a clusterRef is structurally valid and describes no deployment, and the expansion refuses it once it can no longer be edited. The device class and the target cluster name are read through the deployment package rather than derived a second time, so admission and the expansion cannot disagree about what a document says. design-clusterdeploymentconfig.md 5.1 and 5.2 are the specification. 5.2's premise is not quite right and the file says so: the API server validates CEL before it calls a validating webhook, so on a current CRD the schema's immutability rejection arrives first and the webhook's restatement answers only for a cluster whose CRD predates those rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ithdrawn Deleting an Ops object withdraws the record of an operation, which is a different thing from asking the operation to stop. design-crd-model.md 3.1 has both channels ask one question of one graph: a step declaring an abort edge may be withdrawn, and a step declaring none may not. ControlPlaneOps and StorageBackupOps carried that guard. StorageClusterOps and StorageNodeOps carried only their finalizers, which a delete with --force --grace-period=0 skips, so a Promoting migrate could be withdrawn and take the topology re-point it still owed with it. Both guards read which steps refuse from the controller's own graph, through the new UnabortableSteps in each package, and state only the prose: what the step is in the middle of, which a bool cannot say. A test holds each table equal to the graph's answer in both directions, so an entry for a step the graph can abort from, or a missing entry for one it cannot, is a failure rather than a drift. OperatorOps and StoragePoolOps deliberately get no guard. 3.1 derives the webhook from the graph, and both of those abort from every step: discovery changes nothing, and the pool's only action is declared and unimplemented. A fail-closed webhook that can never deny would buy nothing and make those two kinds undeletable whenever the operator is down. It is worth revisiting when StoragePoolOps.Migrate is built on PersistentVolumeOps, or when OperatorOps gains a second action. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stop from Four packages kept a map of abortable steps beside their graphs, plus a predicate over it and a test asserting the map only named steps some graph declared. StateDef.Abortable puts the property where the state is, so the map key is the state and that class of drift stops existing. The sharper gain is precision. Abortability is a property of (action, step), but a table keyed on the step alone flattens every action of a MultiConfig together. Nothing collides today, and nothing could express it if it did. Inside StateDef the question cannot be asked wrongly, and the reconcilers now ask Machine.CanAbort, which answers for the graph the machine was built from. The callers that hold a step and no machine — the DELETE guards reading a step out of a status — ask UnabortableStates or UnabortableMultiStates instead. The multi form unions across actions, because that is what a per-step table can hold, and it takes the union rather than the intersection so a guard reading it refuses where two actions disagree. StorageClusterOps, StorageNodeOps, and now ControlPlaneOps hold their refusal tables equal to that answer in both directions. StorageBackupOps deliberately does not: its guard refuses Restoring, which the graph can abort from, because a delete leaves nothing behind where an abort leaves an object the controller cleans up from. Binding a test there would force the weaker answer onto the stronger channel, and the file now says so. Also fixes the missing trailing newlines and the two linter findings the previous two commits left in the webhook package. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dist/install.yaml is a generated artifact that is committed, and the three commits that added the ClusterDeploymentConfig, StorageClusterOps, and StorageNodeOps guards regenerated config/webhook/manifests.yaml and the chart without it. An install from dist/install.yaml would have created none of the three webhooks, and the drift check in operator_manifests.yaml fails on it. Produced by the root `make build`, which is the target that covers every generated artifact at once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fleet kinds embed the operator's ClusterDeploymentConfig spec, so an edit to that API invalidates them too. Several landed without a regeneration here — spec.approved gaining a default and its explanation, and enableDriveFormat among them (2895275) — so the schema the fleet CRDs describe has been behind for a while. No fleet workflow checks for the drift, which is why nothing said so. Pure generator output, from the root `make build`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#583) * docs(operator): design non-disruptive test failover (TestFailover CRD) * docs(operator): updated design non-disruptive test failover (TestFailover CRD) * feat(operator): TestFailover CRD and controller for non-disruptive test failover * ran make operator-build-installer * fixed linter issue * fix(testfailover): stage the bubble clone with a real VolumeContext * ran make operator-build-installer * fix(testfailover): stage the bubble clone with a complete PV * feat(testfailover): recover a consistency group (scope=Group) * fix linter issue * fix(testfailover): resolve group source UUID and member PVCs correctly * fix(testfailover): read group policy from the group, not a placement heuristic * fix(testfailover): recover in-place CG drills from a fresh source generation, not the DR target * reverted test failover within the same cluster * fixed failing operator manifest CI
geoffrey1330
changed the title
…PI is absent
The operator crashed at startup on every cluster outside the hub. The
TestFailover controller unconditionally watched OCM ManifestWork
(.Owns(&workv1.ManifestWork{})), but ManifestWork is served only on the
hub -- managed clusters read it from the hub and never serve it locally.
Without the CRD the controller's cache never syncs and the manager exits
("failed to wait for testfailover caches to sync ... *v1.ManifestWork"),
taking every other controller (replication, storagecluster, ...) down
with it. That blocked DR failback, since the managed cluster's operator
reconciles the reverse ReplicationPair.
Gate the registration on the work.open-cluster-management.io API group
actually being served (serverHasAPIGroup, via the existing discovery
client pattern). On the hub it registers as before; off the hub it logs
and skips. Installing the ManifestWork CRD on managed clusters is the
wrong workaround -- it belongs only on the hub.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s API group The previous gate checked the work.open-cluster-management.io API *group*, but a managed cluster serves that same group/version for AppliedManifestWork (the work-agent's local record) while never serving ManifestWork. So the group-level check saw the group as served, registered the hub-only TestFailover controller anyway, and the manager still crashed at startup when the ManifestWork watch could not sync (confirmed live 2026-10-01: operator on the managed cluster Error at 2m32s). Check the ManifestWork resource specifically via ServerResourcesForGroupVersion. The regression test now covers the AppliedManifestWork-only managed cluster, which is the case the group-level check got wrong. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The tasks-runner-backup-merge container ran
simplyblock_core/services/backup_merge_service.py, which does not exist in
the control-plane image -- the service is tasks_runner_backup_merge.py,
following the same tasks_runner_* convention as every sibling runner. The
container crash-looped ("python3: can't open file ... backup_merge_service.py")
and pinned the whole tasks pod in CrashLoopBackOff (120+ restarts observed
live 2026-10-01).
The existing TestTheServicePoolsRunWhatTheyDeclare only checks the command
is a .py module, so the wrong name passed it. Add a regression test pinning
the real module name.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t its end A volume's PV keeps the handle it was created with across fail-overs, and the chain of replication relationships behind that handle alternates between the sites: every fail-over adds a hop to the other side. resolveToLocalReplica walked the chain to its active end for every csi-addons Replication RPC. After an unplanned fail-over A->B, Ramen makes the old primary on A secondary: DemoteVolume (and DisableVolumeReplication on VR deletion) on site A resolved to the chain's end -- the NEW primary on site B -- and demoted it (live 2026-10-02, realbed WordPress: the demote fenced the live primary's paths and took demote snapshots of it; the VRG on A never became secondary, so the fail-back never got PeerReady). The driver needs to know which clusters are its own. The operator marks the clusters it manages `local` in the CSI secret's entries (an entry another site registered for cross-cluster handle resolution is not), and the resolver returns the chain's last member on a local cluster. A secret that marks no cluster local -- an operator predating the flag -- keeps the previous behaviour. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The old primary of an unplanned fail-over is reaped by the control plane once its fail-over completed; when the site returns Ramen still demotes it and deletes its VolumeReplication. Resolved to that member, the demote and the detach got a 404 and the VR stayed Degraded (live 2026-10-02, site A, 80e3e748). A 404 on a member of a chain the backend records is nothing left to demote or detach: success. A 404 on a handle without any relationship stays NotFound. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the chain's active end After an unplanned fail-over the recovered site's old primary is reaped once its fail-over completed. Ramen still resyncs that site's secondary VR (and reads its status). Addressed to the reaped member both got a 404 and the VR stayed Degraded (live 2026-10-02, site A). sbcli's replication_failback is addressed to the failed-over clone and, given the original source cluster, re-aims the clone's replication at that site's node so only the delta ships: Resync falls back to the chain's active end with the local cluster as the fail-back source, and the status read follows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…iver A volume replicated to another site is promoted there under a PV that keeps the handle it was created with, which names the cluster the volume came from: that site's driver must be able to reach the control plane for that cluster too. Until now each operator wrote only its own cluster into simplyblock-csi-secret-v2, and the cross-registration was a manual merge of the sites' secrets (realbed deploy.sh csi). Each operator now writes, next to its own cluster (marked local, the flag the driver's replication-chain resolver keys on), one entry per other cluster the control plane lists, with the secret the list carries -- empty when the control plane withholds it, and the driver then authenticates with its API token. Entries another operator on the same Kubernetes cluster marked local are left alone; a foreign entry whose cluster the control plane no longer lists is dropped. The own entry never waits on the list: when the list cannot be read, peers are registered on the next sync. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ocal member The control plane's failover endpoint takes the volume that holds the data (the pairing's source) and creates the clone on the replication target. The local member on the promoting site is the volume being replaced -- demoted, or already reaped: on 2026-10-02 the fail-back promote on site A hit the reaped 80e3e748 and 404ed while the live primary on B held the data. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
sbcli's replication_failback takes the volume that holds the data (the failed-over clone) and, given the recovered site as source cluster, re-aims its replication at that site's node. Addressed to the local member -- the demoted or reaped old primary -- it configured nothing for the live clone: after the unplanned fail-over of Gitea the clones on A had no replication and lastGroupSyncTime stayed empty (2026-10-02). The pairing's status is the active end's for the same reason. Demote and Disable keep the local member; Promote already uses the active end. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…volume carries
A PersistentVolume without fsType -- a static PV adopting an existing
volume (the storage operator's test fail-over clone), or one Ramen restored
without the field -- gave the node plugin an empty fsType, which it turned
into "ext4" before the device was looked at; the filesystem layer then
refused every such XFS volume ("the volume carries xfs and this plan asks
for ext4"), and the bubble VM never started (realbed 2026-10-03).
An empty FsType now expresses no opinion: the layer mounts the filesystem
the device carries, formats a blank device as DefaultFsType (ext4), records
what it found as its params, and the node plugin remembers that type for
the volume. A named filesystem keeps refusing a device carrying another.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…umeMode A VM's disk is a Block claim. The drill's bubble PV and PVC omitted the mode, defaulted to Filesystem, and the kubelet asked the node plugin to mount the raw guest disk (mount: exit status 32); the bubble VM never started (realbed 2026-10-03). The source PV's volumeMode is recorded on the clone and set on both objects. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The console templates and their values block of feature/control-center-ui (PR #512), copied as they are: self-contained (sbcc.* helpers only), so the chart deploys the UI with the control plane on this line too. One addition: controlCenter.service.nodePort pins the node port of a NodePort Service. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… by 8 hops The PV keeps the original handle while every relocate and fail-over appends a clone, so the chain behind a volume grows by one per move and is never compacted. Both walks (the node plugin's redirect to the active volume and the controller's resolveChain) stopped after 8 hops; the ninth move of a volume -- an unplanned fail-over on 2026-10-03 -- left its clone one hop out of reach, the node fell back to the stashed context and attached the original on the partitioned site, and the VM never started. The walks now track the members visited and stop on a loop, with a bound of 256 as a guard far above any real chain. Tests: a 12-move chain alternating clusters, and a looping pair. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The hand-written sourceVolumeMode entry differed from the generated one in wrapping; the Manifests check keeps the three copies identical to the generator's output. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…erated TestFailover CRD Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…om the hub A site's storage cluster is built from objects on the site's API server (the OperatorOps discovery, the ClusterDeploymentConfig draft it writes, the StorageCluster the approved draft expands into), which a hub managing the site through OCM does not reach. StorageSiteDeployment is the hub-side request: the site, the discovery filters, the sizing, and the one-way approval. Its controller (hub-only, registered with TestFailover when the ManifestWork API is served) carries the request through one ManifestWork in the site's hub namespace -- the discovery first, then a server-side apply of the sizing and the approval onto the draft once it names nodes -- and projects the draft, the StorageCluster and its nodes back through ManagedClusterViews. Phases Pending, Discovering, Drafted, Deploying, Online, Failed; conditions Delivered, Discovered, Approved, Ready; the cluster's uuid and pool are in the status, which is what a StorageClass names. Deleting the request orphans the work's resources: the storage cluster is never torn down by withdrawing the request. The console's ClusterRole may manage the kind and read ManagedClusters. The TestFailover CRD copies are the generator's rendering (as on #618). Design: simplyblock-dr docs/design/control-center-managed-discovery.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…/install.yaml Lint (lll) on the chain-walk change and its tests; the installer bundle carries the TestFailover CRD's sourceVolumeMode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… CRD Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tion csi-driver: replication RPCs resolve the right chain member; a plan without fsType mounts what the volume carries
operator: StorageSiteDeployment — deploy a managed site's storage from the hub
…/simplyblock-operator into feat/csi-auto-cross-registration # Conflicts: # operator/internal/controllers/cluster/storagecluster_controller.go
…tion operator: register every cluster of the control plane with the CSI driver
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
4 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.
CSI-addons volume replication and non-disruptive test-failover
Two DR capabilities for simplyblock, both driven from the operator/hub:
standard
VolumeReplicationAPI so Ramen can promote, demote, and resync them.volume or consistency group onto the DR target, proves the data is recoverable,
and tears down, without touching the live source or its replication.
Part 1 — CSI-addons volume replication
The storage-side half of the DR story: the design, the vendored csi-addons
controller-manager, the driver's Replication and Identity gRPC services, the
lifecycle verbs, the control-plane client, and the operator's sidecar and RBAC.
Design
design-csi-addons-replication.mdand its test plan.preflight with a one-owner role, the P0-6 test-failover read path, and metrics
export.
Vendoring the csi-addons controller-manager (P0-5)
VolumeReplication/VolumeReplicationClassare served and reconciledalongside the driver.
Phase 1 — driver Replication + Identity services and sidecar wiring
Identity service, wired into the driver and its gRPC server.
controlplane/replication.go)and regenerated control-plane API bindings, plus RPC error classification.
SimplyblockDriverCRD and driver controller grow thecsi-addons sidecar — its
node-idenv, the endpoint, and the RBAC it needs.Phase 2 — lifecycle verbs
with the planned-gate force-escalation fix.
volumeIDFrom(req)helper resolves the volume id fromReplicationSource.Volume.VolumeId, so every verb addresses the same volume.Fixes
drtest-clones are created as internal volumes,the same as landing volumes.
TokenReviewverb, the csi-addons controller-manager roles, andthe
simplyblockdrivercontroller roles.quay.io/csiaddonsimage pattern and correctthe csi-addons endpoint.
Part 2 — TestFailover (non-disruptive test-failover)
A new
TestFailoverCRD and controller on the hub that run a DR rehearsalwithout disturbing production: recover a replicated volume — or a whole
consistency group — into an isolated
bubblenamespace on the DR target,leaving the source and the running replication untouched.
Core mechanism
sourceClusterfrom the hub through an OCMManagedClusterView, learning the backend handle without a hand-extracted id.already on the target's backend — nothing is shipped, nothing is promoted.
ManifestWork, driven by a non-blockingstate machine (ResolvingSource → ResolvingPoint → Cloning → Placing → Ready),
with a finalizer-driven teardown that reclaims the clone.
Consistency-group scope (
scope: Group)generation.
record, and reads the group's replication policy off the group (
policy_id)rather than a placement heuristic.
Bubble clone staging
volumeAttributesandfsTypeonto the synthesizedbubble PV, so the node plugin stages and mounts the clone correctly.
VolumeContexton the static bubble PV.Cross-cluster only
(in-place) recovery was removed — the
bubbleClustermust differ fromsourceCluster, and a same-cluster drill is rejected up front.Design
design-test-failover.mdand its test plan.Areas touched
csi-driver/internal/csi/controller/replication.gocsi-driver/internal/csi/csiaddons/identity/csi-driver/internal/csi/node/VolumeContextguard for the static bubble PVatlas-lib/controlplane/replication.gooperator/internal/controller/testfailover_controller.gooperator/internal/webapi/{consistency_group,group_failover}.gooperator/internal/controllers/driver/operator/api/v1alpha2/{simplyblockdriver,testfailover}_types.gohelm-charts/.../simplyblock-operatoroperator/docs/designs/design-csi-addons-replication.md,design-test-failover.md+ test plansshared/openapi.json