feat(operator): the API upgrade framework, its preflight, and the CRD installation - #514
Merged
Merged
Conversation
design-api-upgrade.md describes three commands over one cluster, and what they have in common is that the set of things they do grows: every later API version brings new rules to check, new names to bound, and new objects to transform. So none of this is a switch over a fixed list. Six extension points, each an interface with one registry behind it, and adding a rule to the product is writing one value and putting it in one catalog: Discoverer builds the graph (§17), Check validates it (§18, §19.10), Derivation is a naming formula plus the inputs a cluster hands it (§19.2, §19.3), Step is one unit of work (§9.1, §20, §21), Verification is a post-condition a step is not finished without, and Transformation is an object mapping conversion cannot carry (§16). The runner owns every decision the design makes about failure, so that adding a rule cannot change the shape of a stage: discovery completes before anything reads the graph, a blocking finding stops the stage before a step runs, Done is asked before Apply, and the post-conditions are checked even on a step that reported itself done. That last one is the case a rerun gets wrong otherwise, since a run that trusts Done cannot notice that the state it is resuming from is not the state it thinks. migrate walks an atlas-lib/statemachine graph (§23) whose phases are buckets steps register into, so work added later does not change the graph. A failing phase leaves the record on the last one that completed, and the rerun re-enters it. The record is the ConfigMap of §22.1, holding only the three things the cluster cannot answer for, with resourceVersion as the lock that stops two runs from both believing they hold the migration. atlas-lib gains kube.Formula, which is §19.5 and §19.6 as a value: a declarative truncate-and-hash whose limit is a property of the formula rather than of the object the value is written on, because a name copied into a label is held to 63 bytes and not to 253. The digest covers the parts individually, which settles §19.8's ambiguous concatenation by construction: cluster a-b with pool c and cluster a with pool b-c no longer derive one name. The tool is cobra, ships in the operator image, and renders through the Reporter interface, so the same run is a bubbletea progress view at a terminal and plain lines in a Job's log. Completed lines are printed into the terminal's own scrollback rather than an alternate screen, because a migration is something a user reads back afterward. The registries are empty. The catalog is one file listing what would fill them, and the rules command prints it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§17 wants the migration to list every resource relevant to it and build an explicit graph, rather than processing objects as it encounters them, because the ownership edges it is about to move are only safe to move once every dependent of an owner is known. This is that walk, and it is what every check after it reads. One discoverer, declared as data, and a list of the kinds it is instantiated for. What differs between listing StorageNodes and listing StoragePools is a list type and a name, and eighteen copies of one loop is eighteen places for a kind to be listed in the wrong namespace. Three decisions are in the discoverer rather than in its callers. The list is a prototype and each run gets a deep copy, because the declaration lives in the catalog for the life of the process and a run that read into it would hand its objects to the next one. A kind the API server does not serve is reported and skipped, because the alternative is that a cluster missing one CRD gets a preflight that says nothing about the other seventeen, and whether the CRDs are installed and established is §9.2's own check. Any other read failure stops the run, because a graph missing a kind is one every later check draws a wrong conclusion from. The kinds are the ones §7.2 converts, the ones §16 renames, absorbs, or retires, and the objects those two sets own or derive a name from. Task and the four replication kinds are absent: §7.2 leaves them at v1alpha1 untouched, so nothing here has a question to ask about them. The three core kinds are each present because a named check needs one, since the namespaces are what §19.2's derived names are built from and what §19.10's fifth check compares across, the StorageClasses are what a pool derives, and the PersistentVolumes carry the handles of §16.4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ck it §19 is an audit of the formulas that build a Kubernetes identifier out of a name somebody chose, and its point is that none of them is checked anywhere. Nothing validates a label or a name before writing it, and no field in the seventeen registered kinds carries a MaxLength marker, so an overflow becomes a reconcile that retries forever while the resource it was reconciling reports nothing about the name that caused it. Twenty rows, each a value declaring its formula, where the value lands, and what this cluster would hand it. Declaring them rather than leaving each buried in the code that writes the object is what lets one check cover all of them, and what lets a formula a later release adds be checked without a new check being written. A row says which model it belongs to, because a violation breaking a cluster today and one the migration is about to introduce are not the same news. The three target-model rows are §19.8's third uniqueness route: the DaemonSet, the per-node ConfigMap, and the EndpointSlice are named per StorageNodeSet today precisely so several sets can coexist in one cluster, and the retirement of §16.1 makes the cluster their parent, so two sets collapse onto one name. Two storage-node workloads becoming one is what that row exists to refuse, and no naming scheme fixes it. The boundary tests are §30.8's: every row gets an input at the longest length that works, one a byte longer, one that truncates onto a separator a label may not end on, and one long enough to prove the truncated form is still something the API server accepts. The expected numbers are written as the arithmetic that produces them, so a prefix somebody widens shows up as a failure rather than as a name that quietly got shorter. Perturbing the node-type prefix by three characters fails two of them, which is the check that the table is load bearing. Two things are deliberately not here. §19.3's ReplicationSlot row joins a policy name and a claim name, and it needs PersistentVolumeClaim discovery that §19.10's sixth check will bring. And simplyblock.io/drain-node has two call sites that disagree today, since the writer patches the node name verbatim while the reader compares against a sanitized form, so a worker whose name needs sanitizing is written in a form the API server refuses and would never be matched if it were not. The row models what the reader expects, which is what the fix has to produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two checks rather than twenty, because the rows are declared as data. Whether a value fits its limit is a property of one formula and one input, and whether two values collide is a property of one formula and all of them, so each question is asked once and every row a later release adds is asked it too. derived-names-fit is §19.10's first and third checks together, since a name that is too long and a name carrying a character its syntax forbids are refused by the same write and leave the same reconciler requeueing against an object that reports nothing about the name that caused it. It validates the unbounded form, which is what the operator writes today, because reporting on the bounded one would report that every violation is already resolved. It does not ask both questions of one value: apimachinery reports an overlong value as a syntax error of its own, so a length violation would otherwise be printed twice, and the length is the more actionable half. derived-names-unique is §19.10's second, and it covers two of §19.8's four routes. The ambiguous concatenations are one, and the StorageNodeSet retirement is the other, which is the route this migration introduces rather than finds. Distinctness is by source object rather than by input, because a row enumerating a cross product hands one object several values and those are not two objects colliding. The remaining two routes cross a namespace boundary and this check cannot see them, since discovery is scoped to the installation; they belong to §19.10's fifth check, which reads the whole cluster. A row now declares which of §19.5's three fixes resolves it, because §19.11 requires that every violation names the change that resolves it and a finding with no remediation is a gap in the check rather than in the cluster. A test refuses a row that declares none. The reporting grew one field. A collision is about two objects that each contributed something different, and saying so by repeating their names in the detail printed every object twice, so a finding can now carry a note rendered beside the object at the same index. Written() lost its leading articles for the same reason: an article that reads correctly in "derives a StorageClass name" reads wrong in "two objects derive one a StorageClass name". Both checks were proven to bite. Disabling the overflow branch fails the cluster-name test, and disabling the collision branch fails both collision tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A preflight against a cluster with real data in it is a long silence. Fifteen list calls and two checks over twenty naming rules is minutes of nothing, and a run that is working and one that has hung look identical from outside. The reporter learns three things. Section announces a counted group about to run, so a bar can be sized from a catalog rather than guessed at. Work announces how many units the rule now running will walk, and Item names each one as it is reached. Stage loses its step count, which only ever described one of the three groups a stage holds. What the spinner narrates is the section rather than the rule, because a section is a sentence a user recognizes and a rule identity is a slug this tool made up. So Stage and Phase gained a Describe, and the groups are named as activities: collecting simplyblock Kubernetes resources, verifying resources, reparenting what the retired owners hold. The rule and the item it is on follow, dimmed, and they are what changes often enough to tell a slow run from a stuck one. The text reporter is not the terminal one and does not pretend to be. It cannot redraw a bar, so each line carries its position in the section instead, which is what a reader scrolling a Job's log wants. Item prints only under --verbose, since it fires once per object and a log with a line per object is one nobody reads. Discovery names the kind before the read rather than after, because the read is the wait. Watched through a pty to confirm it renders: the completed lines land in the terminal's own scrollback and only the activity line and the bar redraw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§19.10's fifth check, and with it the two of §19.8's four uniqueness routes the name checks cannot reach. Both are the same shape: an identifier that is unique inside a namespace becomes ambiguous in a space that has none, and nothing notices, since the API server accepts both objects and one of them silently takes what the other needs. A StorageCluster's name reaches the worker Nodes as io.simplyblock.node-type carrying the name and nothing else, so two clusters called prod in two namespaces claim the same machines. A VolumeMigration becomes a cluster-scoped PersistentVolumeOps, so two of one name become one object. Neither is visible from inside one installation, so the scope gained a second graph. ClusterWide holds the same kinds read across every namespace, and it holds only the two kinds that need it. It is a second graph rather than a wider first one because almost every check wants the installation and would draw a wrong conclusion from another tenant's objects; a rule reads it only when the question it asks genuinely has no namespace in it. A discoverer says which graph it fills through View, and asking for a cluster-wide view of a kind that is not namespaced is a declaration error rather than a wider read. A group is reported only when it touches the installation's own namespace. A collision between two namespaces that are both somebody else's is real and is their upgrade's problem, and blocking here would fail a preflight on the state of a cluster this installation neither owns nor can fix. That branch has its own test, and removing it fails that test alone. The catalog gained an end-to-end test over a cluster built to break, which is what covers the wiring the per-package tests cannot: that a discoverer's objects reach the check that needs them, that the cluster-wide view reaches only the check that reads it, and that --skip on a naming rule reaches the check walking it rather than only the line the rules command prints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§19.10's sixth check. §16.3 calls the key reprefixing the quietest break in the whole migration, because no API server rejects the old key: a claim annotated simplyblock.io/backup-policy simply stops having a backup policy, and nothing anywhere reports that it used to. Everywhere else the rewrite is mechanical, since it writes the new key, preserves the value verbatim, and leaves the old key for the deprecation window. Two spellings holding two different values are two answers to one question, and picking either would silently discard a value somebody set. So that is what this reports, and only that: spellings that agree are an object the rewrite has already reached, and reporting those would make the rewrite fail its own second run. The inventory is a package rather than a table inside the check, because the migration's rewrite reads it too. It holds thirty rows where §9.4 counts twenty-eight, and the count is not reconciled: the list is what the code actually writes, so a key found later is added rather than the count trusted over the grep that produced it. Some keys have already half moved, since the chart writes simplyblock.io/replication-policy while the operator reads storage.simplyblock.io/replication-policy, which is why this runs before the rewrite rather than inside it. The pre-simplyblock.io spellings are matched with the rest, and a legacy name that differs from the current one, as simplybk/qos-rw-mbytes does from qos-rw-mbps, pairs through the row rather than by swapping a prefix. PersistentVolumeClaim discovery is what the check needed, and it reads every namespace because a claim lives where its workload does. That widens what the cluster-wide graph is for, so its comment now says what actually puts a kind there: a question that is not about one installation, whether because an identifier escapes a namespace or because the objects do. With the claims in hand, §19.3's ReplicationSlot row is no longer deferred. It joins a policy name and a claim name, and it records a discrepancy rather than resolving it: the arithmetic leaves the two of them 252 characters and §19.3's table says 246. The seven characters are unaccounted for, and the arithmetic is what the formula does. Three of the check's branches were proven load bearing by breaking them. The determinism test needed two goes to be able to fail at all, since ordering only matters when objects of several kinds carry the same key, and the fixture now says so in a comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§16.1 is the change that defines the migration: a StorageNodeSet owns its StorageNode objects by controller reference, which is the one real ownership edge in the spine, and it also owns the workload that runs the storage nodes. All of it becomes children of the StorageCluster, and until that reparenting is done the retirement cannot proceed, because deleting a StorageNodeSet today is what tears those objects down. So the preflight now builds that structure and asks §18's questions of it. The spine package is separate from the checks because two things read it: the preflight asks whether it is well formed, and the migration's ownership phase walks the same structure to perform the move. A second traversal written for the second caller is a second answer to what depends on what. The dispositions are declarative for the same reason §12.2's classification is. Neither default is safe: reparenting an object the target model has no owner for leaves an orphan nothing reconciles, and leaving it behind hands it to garbage collection the moment the set goes. So a kind somebody adds to the StorageNodeSet controller without deciding its disposition fails the preflight rather than silently taking a branch. The rules are read from the controller rather than from the design, and the ClusterRole and ClusterRoleBinding it also creates are deliberately absent, since Kubernetes does not let a namespaced object own a cluster-scoped one and they already survive the set on their own. Four checks. ownership-spine covers §18's structural lines, and every fault it reports is one the migration would act on wrongly rather than fail on: a set with no cluster has nowhere to reparent to, a node whose declared set and owning set disagree gets reparented onto whichever of the two the code read, and a node two sets own gets reparented twice. reparenting-is-safe is §20's precondition. operations-in-flight and storage-nodes-online are §18's last three lines, and the offline one is a warning rather than a refusal once the run acknowledges it, because an operator who knows a node is down and intends to proceed is making a call worth recording rather than overriding. Two things are worth knowing about the reparenting check. An empty phase is not terminal, since an object the operator has not reached is one it is about to start, and treating it as finished is how a migration begins underneath one. And the check sees the kinds discovery reads and no others: the graph learns an ownership edge from the dependent rather than from the owner, so an object of an undiscovered kind owned by a set is invisible. That gap is documented where the check is, and the drift that would widen it, a rule without a discoverer, now fails a test in the catalog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #508 removes the label as redundant with io.simplyblock.storagenodeset, which its own body notes falsifies §19.2 in four places and leaves to this document. It does, and in a fifth: §19.9's example marker and §31's definition of done both cite the number the label produced. The 37-character cap on a StorageCluster name goes with it. Nothing bounds one below the 63 bytes a label value allows any more, and both rows that bind it there are §19.5's use-a-UUID cases, so §19.4's claim to close more of the list than any other one-line change no longer holds. The section keeps its number and states what the marker does instead: it turns an overlong cluster reference into a rejected create, it closes only the rows where the cluster name stands alone, and the two rows sharing their 63 bytes with a namespace and a pool name are not a budget any single-field marker can guarantee. The tightest row in §19.2 is now the pool key, which binds three names at once. §19.8's second uniqueness route survives with a different instance rather than being deleted. io.simplyblock.storagenodeset carries the StorageNodeSet name and nothing else, and it lands on a Node, which is cluster-scoped, so two sets of one name in two namespaces still claim the same machines. The namespace-collapse check moves from StorageCluster to StorageNodeSet for the same reason, and the cluster-wide discoverer reads the sets in place of the clusters, since no check asks a cluster-scoped question about a cluster any more. The label-node-type derivation row and its boundary entry are gone. Four tests were anchored to that row and are re-anchored to the node-set label, which is the other bound-the-input case, and two in atlas-lib that used the retired prefix as a fixture now compute their budget from the formula rather than quoting a number the design no longer carries. The §19.11 example finding is rebuilt on a real 64-character set name, and its byte counts were measured rather than written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§17 shows the shape and §27 shows the plan it grows into, and preflight now prints it after discovery and before the findings, so the objects a finding names have already been seen in the structure the finding is about. The second column is what makes it worth printing. A list of objects says what an installation holds. A tree with a disposition against every row says what the migration is about to do to it: the nodes and the workload point at the StorageCluster that takes them over, the set says it is retired once it is empty, and a dependent nothing classifies says so in the one place a reader is already looking. Both loose ends get a root of their own rather than being dropped. A StorageNodeSet whose cluster does not resolve appears under no cluster, and a StorageNode no set owns appears under no set, so a tree that only walked down from the clusters would leave out exactly the two objects §18 refuses on. The namespace is elided where the root already establishes it. Repeated on every row it pushes the column carrying the news off to the right, which is the column the tree exists for. Rendering is a Reporter method rather than a print, because a terminal that redraws a progress bar into the middle of a tree produces something nobody can read. Block takes the lines already formatted, since a tree's alignment is a property of the whole tree and not of one row, and the terminal implementation sends it as one message. Three of the renderer's decisions were proven load bearing by breaking them: dropping the orphan sections, never eliding the namespace, and padding in bytes rather than runes. The last needed the fixture rewritten before it could fail at all, because two rows at one depth carry the same box-drawing prefix and are shifted equally either way. The nodes now sit under different sets, where one carries a trunk of four runes in six bytes and the other four spaces in four, and the comment says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Discovery narrowed the group's kinds to the operator's own namespace, so an installation whose StorageCluster lives in default produced an empty graph, a spine with nothing in it, and eight checks that passed on a cluster they had not seen. §17's premise was wrong. The manager is built with its cache restricted to no namespace and its RBAC is a ClusterRole, so one operator serves the whole cluster and a StorageCluster anywhere is one it reconciles. WATCH_NAMESPACE is set by the chart and read nowhere in the Go, so it bounds nothing, and two independent installations in one cluster is not a state this product reaches: the second operator would watch the first's objects and fight it. So there is one graph rather than two, and the second one goes. The scope's namespace stops meaning the installation and means the operator's own furniture: the Helm release, the conversion webhook, the ControlPlane the chart installs, and the migration record. Reading everything everywhere would be the wrong correction. The workload a StorageNodeSet owns is created in the set's namespace, so it follows the custom resources, and those are kinds a cluster holds thousands of. Discovery runs in two passes: the group's kinds are read cluster-wide, and the workload is read only in the namespaces the first pass found the group in. The installation's footprint is derived rather than configured, because where the custom resources live is a fact about the cluster and not a decision anybody made. Two checks change behavior, not just wiring. namespace-collapse no longer skips a pair outside the operator's namespace. Both halves are reconciled by the operator being upgraded, so skipping them let the migration walk into the collision the check exists to refuse. It also gives up its StorageNodeSet row, which was a derived-name collision wearing the wrong hat, and keeps only the case no naming rule models: a kind whose objects merge because the kind that absorbs them is cluster-scoped. derived-names-unique gained the distinction that makes cluster-wide reading safe. A row now says where its value has to be unique. Two sets of one name in two namespaces derive one ConfigMap name and collide with nothing, because a ConfigMap name is unique per namespace; the same two sets do collide on the node label they claim workers with, which lands on a Node and has no namespace to be kept apart by. Without it, going cluster-wide reported a false collision against every namespaced name in the cluster, which is what the catalog test caught. Reintroducing the narrowing fails five tests, including the two that name the bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three storage nodes on three separate workers, each at socket 0, were reported as three objects fighting over one label key. They were not. The per-slot topology key is written per worker Node, through slotsByWorker in the set's controller, so each of the three writes it onto its own Node, and that is the normal shape of a three-node cluster. The remediation the finding carried said as much: fix, none needed. The mistake was a category one rather than a wrong row. Space was introduced to say where a derived value has to be unique and defaulted to saying it does, which reads every derived label as a name. Most of these labels are selectors, and repetition is the point of them: every worker of a set carries the same set label, and every storage node on a multi-socket worker names the same worker. So a value may share its space, the uniqueness check skips those rows while the length checks keep them, and no row gets a default at all, because the wrong default is what produced this finding. Five of the eight label rows were reclassified against what the label is for rather than what it is built from. The two that stay unique are the ones where sharing is a real conflict: the pool key, which two distinct pools must not reach, and the set label, which is the DaemonSet's node selector and so an exclusive claim on a worker. The two whose classification is not obvious now say why in place. A collision's remediation is also not always its overflow's. Truncate-and-hash resolves both, since the digest covers the parts individually. Bounding an input resolves only the length: two objects can both be inside the limit and still derive one value, and renaming one of them is the only thing that separates them, so that is what the finding now says. Breaking either behavior fails a test: three tests for the shared rows, and one for the remediation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§22 states idempotency per object and §20 orders the reparenting the same way: a StorageNode owned by its set is transferred, one already owned by the cluster is already migrated and continues, and each move is verified before the next. A Step that answered those questions once for a whole kind collapsed three nodes into one all-or-nothing decision, which is the wrong answer for a run killed after the second of them. So an ObjectStep answers five questions about one object, and PerObject turns it into an ordinary Step by walking the graph. The phase graph, the registry, and the runner are unchanged; the fan-out, the per-object skipping, and the per-object verification are implemented once in the adapter rather than in every step that would otherwise restate them. Describe and Done are kept apart, because they look like one question and are not. Describe asks whether the step is about this object at all, and returns nil when it is not, which makes a declination explicit rather than silent. Done asks whether the change Describe named is already present. Splitting them is what makes a rerun's plan shrink as work completes rather than listing the same actions until the migration finishes, and it is what lets a later coverage check tell an object nothing is responsible for from one every step declined. Subjects reports that set. Not every step has a subject, and those keep the plain Step shape. Deploying the conversion webhook, applying the CRDs, handing the Helm release over, and switching the storage version act on the installation rather than on a graph node, and inventing a subject for them would buy nothing. The graph gained the deterministic walk the adapter needs, which the annotation check was already doing for itself and now shares: kinds sorted, objects in discovery order within a kind, so the plan a user reads and the sequence the migration performs are one sequence twice. Three of the contract's decisions were proven load bearing by breaking them: a Plan that ignores Done never shrinks, an object reported done that is not verified passes a change that does not hold, and a nil Describe treated as work applies a step to an object it declined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two corrections to the step contract, and together they leave one interface where there were two. The steps of §9.1 have no object. Deploying the conversion webhook, applying the CRDs, handing the Helm release over, and upgrading the operator change the installation rather than anything in it, and a second interface for them would mean two contracts, two runner paths, and a plan assembled from two walks that happen to agree. So the upgrade is a subject like any other, and one walk covers both. A run's subjects are the upgrade first, then the graph's objects, kinds sorted and objects in discovery order within a kind. Describe now returns nil when there is no operation, which includes a subject already in the state the step exists to produce. A StorageNode already owned by its StorageCluster is not a change to skip, it is a change that does not exist. That makes the plan the outstanding work by construction, with no second question filtering it afterward. It also makes a rerun self-healing, which the arrangement it replaces was not. Describe inspects the subject to decide, so a subject a previous run left in the wrong state describes an action again and is put right. Asking a separate question first and trusting the answer reported a change that does not hold and stopped, which is a worse outcome for the same fact. The test asserting the old behavior is gone, and the one asserting the new is TestStep_ASubjectWhoseStateRegressedIsDescribedAgain. Done stays, and earns its place on the question a nil Describe cannot answer. A subject no step describes is either finished or one nothing has taken responsibility for, and those are very different answers to whether the migration is complete. Covered reports the three sets, which is what the coverage check will read. Verification is no longer an extension point of its own. A step verifies its own subject after applying it, and a graph-wide post-condition is a Check registered for the migrate stage, so the framework declares five kinds of rule rather than six. Three of the contract's decisions were proven load bearing by breaking them: ignoring Describe's nil applies a step to a subject it declined, applying without validating drops the precondition, and dropping the upgrade from the subject list leaves §9.1's steps with nothing to act on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§20's ownership migration, as the first work the plan has ever held. Three steps: the StorageNodes move, the workload §16.1 lists moves, and the StorageNodeSet is retired once both are done. The requirement edges are the safety property rather than a reading order. Kubernetes garbage collection removes a dependent when its owners are gone, so a set deleted with its DaemonSet still under it takes the storage plane with it. The retirement requires both moves, and refuses on anything that still names the set, which is the check §20's ordering exists for. Its Verify is the other half: the set is gone, and everything that used to depend on it is not. Within one object the move is a single update rather than the design's two-stage add-then-remove. An update is atomic, so the object is never persisted without an owner and the collector is never given a window to act in. What the two-stage ordering protects is the sequence between objects, and the requirements express that. Two things fell out of writing the first real step against the contract. The spine is built once per phase and deliberately not rebuilt when a step writes. It describes the state the phase started from, which is what every step's target was resolved against, and rebuilding it mid-phase left Verify unable to name the move it had just made. What has to stay current is the graph, which a step refreshes with what it wrote, because the retirement asks whether a set still has dependents and a stale graph answers that it does. An object already on its cluster resolves to a move already made rather than to nothing. Describe declines it either way, and the difference is what Done reports: on a migrated cluster the objects are finished rather than objects nothing took responsibility for, and the coverage of the migration is measured on exactly that difference. Four of the safety properties were proven load bearing by breaking them: dropping the remaining-dependents check retires a set that still holds one, dropping the requirements retires it before the moves, leaving the old owner reference in place leaves the object collectable, and skipping the graph refresh leaves the retirement reading a set that still looks occupied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
preflight planned for the upgrade stage unconditionally while every step registered belongs to the migration, so StepsFor returned nothing and the command printed no plan at all. The comment above it said the stage was derived from the cluster. It was not. §27 wants that derivation, and §7.4 supplies the signal: the seven converting kinds serve v1alpha1 alone until the upgrade applies the new CRDs, and both versions afterward, so what the API server serves is what the upgrade has already done. Positioned reads them and says which command the cluster is ready for and why. A set applied to some kinds and not others is reported as partial, since §11 refuses to proceed past the state that leaves the operator reconciling one kind at each version, and a version present but not served does not count, because §28 stops serving a version before the CRD drops it. The seven kinds are data now rather than a table in §7.2 alone. Three things read them: the positioning, the storage rewrite, and the coverage of the migration. Their plurals were checked against config/crd/bases rather than guessed. An empty plan and a cluster with nothing to do also looked identical, so preflight now says when a stage has no steps in this build. That is the difference between a report and a silence, and it is the honest answer while §9.1 is unimplemented. The terminal reporter had two faults, both visible in every run. Scrollback came out of order, because bubbletea runs the command a message returns in a goroutine and two Println commands from two updates race. A discoverer's line could land after the summary that followed it. Lines are now queued and flushed one print at a time, each flush reporting back before the next starts, so the order a run is reported in is the order it happened in. The last line of a run was dropped and the final frame was left on screen, because Close quit while prints were still in flight and the model's last View stayed behind. Close now asks the model to quit, and it does so once its queue drains, with an empty View while closing. Watched through a pty: all eight checks print, in order, and the shell prompt follows a clean exit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"no converting kind serves v1alpha2 yet, so the cluster cannot run an operator that reads it" reported the upgrade's own premise as a finding. Nothing serves v1alpha2 before the upgrade: that is where every installation starts, and applying those CRDs is what the upgrade is for. Phrasing it as an inability reads as a diagnosis of a fault, and a user running preflight on a healthy cluster is told their cluster is broken. The three sentences now say where the cluster is and, where it matters, what the stage does about it. Serving one version is a position. A set applied to some kinds and not others is a fault and still says so, since no installation passes through a half-applied CRD set on purpose. The test asserts both halves: that the starting position names itself as one, and that it does not read as an inability. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three sentences are user-facing prose, and the tests pinned them verbatim, so rewording them failed the tests without anything being wrong. That is a test holding the wording still rather than holding a property, and it blocks exactly the improvement it should survive. They now check the facts a reason has to carry, which are the counts and the versions, and the framing it must not carry, which is any reading of a starting position as an inability. Both survive a rewrite and both still catch the regression they were written for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§9.1's ten steps, described in full and performed by none of them. A step that was simply absent left the plan short of what the upgrade owes, and silence reads as nothing to do, so the plan a user reads to decide whether to run the thing was not the whole thing. A step now says what it would do and what is missing. Most of them wait on one thing: §29.1's v1alpha2 package covers two of the eleven new kinds and none of the seven converting ones, and §29.2's conversion functions do not exist, so there is nothing to convert between and nothing to deploy a webhook for. The release handover waits on Helm's Go SDK instead. The refusal is the point. A stage that performed its implemented steps and stopped at the first blocked one would leave a cluster halfway through an upgrade nothing can finish, having reported the earlier steps as successes. So the runner collects what it cannot do before it does anything, and names it. An operator who knows what is missing can exclude it by name, which is the escape hatch --skip already was. Two verbs joined the closed set. §9.1 numbers three verifications separately from the changes they confirm, because each is a distinct thing that can fail, and waiting is where an upgrade spends most of its time, so a plan without either would not describe what a user is about to sit through. The plan names the step rather than the subject where the two carry different information. Every one of §9.1's acts on the upgrade itself, so ten lines reading Upgrade simplyblock/upgrade identified nothing, and the step's name is also what --skip takes. The summary counts them as steps for the same reason, and closes on how many of them this build cannot perform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A flat list of changes was the wrong structure for a thing that runs as a hierarchy. It lost which step owned each change and how many objects one step touches, so the release handover appeared as one ANNOTATE where it is ninety- five of them, and a reader could not tell a step that acts on one object from a step that acts on the whole cluster. A plan is now tasks, one per step, each holding the subtasks it would perform, grouped by the phase the migration walks. A task whose only subject is the upgrade itself prints as one line, since that subject carries no information and repeating it under the step says the step twice. The step's own explanation moved out of the plan and into its source. A sentence describing what a step is belongs where it is read once, not printed on every run of a command whose job is to say what will happen to which object. What a subtask carries is the change itself, an old value and a new one, and a step with nothing concrete to say carries nothing. The unimplemented mark moved from the change to the task, for the same reason. Whether this build can perform a step is a fact about the step, and carrying it on every object the step would touch would say it ninety-five times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Describing a change and performing it need different things, and the earlier reading conflated them. Saying that BackupPolicy nightly becomes StorageBackupPolicy nightly needs the BackupPolicy, which discovery has. Constructing the StorageBackupPolicy needs the v1alpha2 type, which §29.1 has not written. Only the second is blocked, so the plan holds the policies, the claims, and the volumes a run would touch rather than a sentence about them. Two of §16's steps are done rather than described, because they need nothing that does not exist. The annotation rewrite of §16.3 is the quietest break in the migration and the inventory for it is already data, so it writes the new spelling beside the old one, preserving the value verbatim and leaving the old key for the deprecation window; an object carrying both describes nothing, so a rerun plans none of it. The BackupImport deletion needs no target at all. Three describe and refuse. The two absorbed kinds and the renamed one each name the object they would create and, where the target is an Ops, the action it carries. Their Apply, Validate, and Verify refuse rather than succeeding quietly, which a run would otherwise report as a success that changed nothing. The volume handles of §16.4 describe further than expected. Detecting one needs no control plane: the shape is clusterID:poolID:volumeID and lvol.ParseHandle accepts a pool segment that is not a UUID precisely because both spellings occur, so the plan names the volume and the pool by name. What the resolver supplies is the other half, which is the UUID that name resolves to. atlas-lib gained the annotation §16.4 writes and the reason the existing label cannot be it, which is that a handle is 110 bytes and a label value stops at 63. The keys inventory gained Carrying, which answers what the rewrite would do where Conflicts answers what it cannot. A test caught the contract being broken in the writing: normalize-volume- handles described work for a volume that already carried the resolved annotation, where Describe owes nil for a subject already in the target state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
preflight planned for the stage the cluster is positioned for and no other, so a cluster with a StorageCluster, three StorageNodes, and twelve claims in it showed ten unimplemented upgrade tasks and nothing about its own data. The migration is the half that touches that data, and reporting only the stage that comes next hid all of it behind an upgrade that has not run. It reports both now. Which stage the cluster is ready for is still read and still said, because it decides what to run next, but it no longer decides what a reader is allowed to see. The checks moved out of the per-stage plan and run once. They belong to the run rather than to either stage, every one of them is registered for all three, and running them per stage printed each finding twice. Tasks is the new half of Plan that builds a stage's work without them. A stage with nothing to do says so, since an empty section and a section that did not run read alike. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§12 is the riskiest step of the upgrade. The chart that carries the new operator carries the operator and nothing else, so the upgrade that installs it is also the upgrade that deletes the running data plane, and four of the prunes would end the upgrade rather than degrade the cluster. It showed one line where it acts on ninety objects, which is the worst place in the plan to be vague. Reading the deployed release needs no Helm SDK. A release is a Secret of type helm.sh/release.v1 whose data is base64 over gzip over JSON, and the JSON carries the rendered manifest of everything the release installed. Decoding that takes the standard library, and a release old enough to have been stored uncompressed still reads. What the SDK is actually for is the other half of §12.4's difference, which is rendering the chart that would replace it. So the release's objects are discovered and the handover describes one subtask each. They are fetched rather than taken from the manifest, because what the annotation goes on is the live object, and §12.2 turns on Helm reading the annotation from the live object rather than from the stored manifest. An object the manifest names and the cluster no longer holds is skipped. The step stays unimplemented, and its reason is narrower than it was: which objects survive needs the new chart rendered, and guessing that from a name would be the table §12.1 says the set is never read from. An object Helm already refuses to prune is declined rather than rewritten, which is the case the snapshot CRDs are already in. A cluster with no Helm release is not an error. §13.2 has the OLM-installed shape, where the release does not exist, and the absence is how the two channels are told apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading the deployed release failed on a real cluster at simplyblock-caching-node-restart-script-cm, a ConfigMap the chart renders without a namespace. Helm lets a namespaced object inherit the release's, which is what installing with -n means, so a reference built straight from the manifest names a namespaced object with no namespace and the API server refuses the lookup. Which of the two a kind is cannot be read from the manifest either, since a cluster-scoped object legitimately carries none, so the API server is asked. A namespaced kind with no namespace takes the release's, a cluster-scoped one is given none whatever the manifest said, and a kind the cluster no longer serves is skipped along with an object deleted out of band. The cluster-scoped half is asserted against the resolution rather than through a read. The fake client tolerates a namespace on a cluster-scoped lookup where a real API server refuses it, so a test going through Get passed whether or not that branch existed, which is the branch it was written for. Both halves fail when removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The SDK is a library, so neither the Helm binary nor kubectl has to be on the machine running an upgrade. What it brings with it is its own idea of which cluster to talk to, and that is the thing worth guarding: a configuration built the usual way re-resolves KUBECONFIG and the current context, which is exactly the resolution --kubeconfig and --context exist to override, so a run could inspect one cluster and release to another. It would work on every machine where the two agreed. So the RESTClientGetter here answers with the connection it was handed and is incapable of consulting the environment, including the namespace loader, which is built over an empty configuration in memory rather than over loading rules that can reach a file. Building a client without a connection is refused rather than letting Helm find one. Replacing the loader with the ambient rules fails the test that says so. The SDK turns out to be needed for less than it looked. Reading the deployed release is a Secret this repository already decodes, and the values §13.1 translates sit in the same JSON beside the manifest, so both are read without Helm and the release type now carries the values too. What is left is rendering the chart that would replace what is deployed, which is one half of §12.4's difference and the only half that needs a template engine. It renders through the upgrade action rather than the install one, because an upgrade knows the release it replaces, and as a client-side dry run, because §12.4 is about what the templates produce rather than what admission would accept. The chart is loaded from a path for now. §11's argument for embedding the CRDs applies to it as well, since a chart fetched at run time can be a different chart from the one a binary was tested against, and the test loads the repository's own so a template that stops parsing is caught here. It costs 100 modules and 27 megabytes: the binary goes from 38M to 65M once anything in cmd imports the package, which nothing does yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§11 requires the upgrade to apply the CRDs itself rather than fetch them from a release tag, so that the schemas a run installs are the ones the conversion code in the same binary was built against. A run that installed a schema its own converter had never seen would be converting blind. The set is embedded. go:embed cannot reach outside the directory of the file that declares it, so the generated CRDs are copied next to the package that embeds them and `make -C operator manifests` refreshes the copy, which is the arrangement the chart's copy of the same files already uses. A CRD that already holds what it should is not written. §11 says so directly: a CRD is what every custom resource of its kind is served through, and a write that changes nothing can still fail. So the step compares the parsed spec, normalizing the one field the API server defaults, and describes nothing where the two agree. Against a live cluster that skips nineteen of twenty and finds the one that is really behind. Which of §11's groups a CRD is in is read from the CRD rather than listed, since a table of kinds is a second place to edit when a kind is added and the two disagree the first time somebody edits one of them. Applying and verifying are separate steps because §11's requirement is about the set rather than about a CRD: the state that must not be proceeded from is a partially applied one, where the operator reconciles one kind at v1alpha2 and another at v1alpha1. Steps can now enumerate their own subjects. The graph holds what the cluster has, and the CRD for a kind that is new in v1alpha2 is precisely what no discovery can find; without this the step would have had to act on the upgrade and hide twenty objects behind one line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fbpxwB6MrqCGNr62Bw4dy
The design said what the upgrade must do and not what a step is. §29 listed the packages and stopped there, so the contract every unit is written against lived only in the interfaces, and the next person to add a step would have had to reconstruct it from three files. §29.5 through §29.9 are that contract. The five extension points and their one registry each, the Step interface and the five rules the runner relies on, why Describe returns nil for two different reasons and what tells them apart, the Subject model including the steps that act on the upgrade itself and the ones that enumerate subjects no discovery can find, the plan hierarchy and its closed verb set, and the order to write a step in. §11 gains the mechanism it was specified without. The CRDs are copied next to the package that embeds them because go:embed cannot reach outside its own directory. Which of the three groups a CRD is in is read from the CRD rather than from a table, since a list of kinds is a second place to edit when a kind is added. A CRD is written only where it differs, compared on the parsed spec rather than the bytes because the API server defaults spec.conversion and nothing else. An injected conversion CA bundle is kept. A CRD dropping a version that status.storedVersions still names is refused, which is where §24 comes in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fbpxwB6MrqCGNr62Bw4dy
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a few functional inconsistencies/bugs (notably in-cluster client config resolution and error/readonly behavior) that should be corrected before this can be safely relied on in production upgrades.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new simplyblock-upgrade CLI and the underlying operator/internal/upgrade framework to plan and execute a multi-stage API upgrade (preflight → upgrade → migrate), including discovery, validations, plan rendering, Helm integration via the Go SDK, and embedded CRD installation to keep installed schemas aligned with the binary’s conversion logic.
Changes:
- Add the
operator/internal/upgradeframework (registries + discovery/check/step abstractions, planning, findings/reporting, migration phase state machine). - Add the
operator/cmd/simplyblock-upgradeCLI withpreflight,upgrade,migrate, andrulessubcommands. - Embed and ship CRDs and the upgrade binary in the operator build artifacts (Makefile + Dockerfile + committed CRD manifest copies).
File summaries
| File | Description |
|---|---|
| operator/Makefile | Regenerates and copies generated CRDs into the embed-able manifests directory. |
| operator/Dockerfile | Builds and ships simplyblock-upgrade in the operator image. |
| operator/cmd/simplyblock-upgrade/main.go | CLI entrypoint wiring. |
| operator/cmd/simplyblock-upgrade/root.go | Root command and global flags. |
| operator/cmd/simplyblock-upgrade/run.go | Shared session setup (client, reporter, scope, runner). |
| operator/cmd/simplyblock-upgrade/client.go | Cluster connection and scheme setup for multi-version reads/writes. |
| operator/cmd/simplyblock-upgrade/preflight.go | Read-only preflight: discovery, spine render, checks, and dual-stage plans. |
| operator/cmd/simplyblock-upgrade/upgrade.go | Stage 1 runner: checks + apply upgrade stage steps. |
| operator/cmd/simplyblock-upgrade/migrate.go | Stage 2 runner: confirmation + state-machine driven migration. |
| operator/cmd/simplyblock-upgrade/rules.go | Prints the compiled-in catalog (discoverers/checks/derivations/steps/transformations). |
| operator/cmd/simplyblock-upgrade/logger.go | stderr diagnostic logger used by the CLI. |
| operator/internal/upgrade/doc.go | Package-level design overview and extension-point taxonomy. |
| operator/internal/upgrade/rule.go | Rule identity + generic registry implementation. |
| operator/internal/upgrade/catalog.go | Catalog structure and stage/phase filtering helpers. |
| operator/internal/upgrade/catalog/catalog.go | Concrete default catalog composition (discoverers, checks, derivations, steps). |
| operator/internal/upgrade/check.go | Check interface and adapter implementation. |
| operator/internal/upgrade/check/doc.go | Documentation for the check package. |
| operator/internal/upgrade/check/namespace.go | Namespace-collapse validation for kinds that become cluster-scoped. |
| operator/internal/upgrade/check/namespace_test.go | Tests for namespace-collapse behavior and determinism. |
| operator/internal/upgrade/check/annotations.go | Conflicting old/new key spelling validation. |
| operator/internal/upgrade/derive/doc.go | Documentation for derivation formulas inventory. |
| operator/internal/upgrade/derive/rule.go | Derivation rule implementation + typed graph reads. |
| operator/internal/upgrade/derivation.go | Derivation interface + enums for model/fix/uniqueness space. |
| operator/internal/upgrade/discovery.go | Discoverer interface and discovery contract. |
| operator/internal/upgrade/discover/doc.go | Documentation for discovery package. |
| operator/internal/upgrade/discover/kind.go | Generic per-kind discoverer implementation (Everywhere vs Occupied reach). |
| operator/internal/upgrade/discover/release.go | Discoverer for objects installed by the deployed Helm release. |
| operator/internal/upgrade/object.go | ObjectRef/ObjectIdentity helpers for reporting and graph indexing. |
| operator/internal/upgrade/subject.go | Subject model (upgrade-as-subject + per-step enumeration support). |
| operator/internal/upgrade/step.go | Step interface and per-subject coverage evaluation. |
| operator/internal/upgrade/order.go | Stable topo-sort ordering for discoverers/steps by Requires edges. |
| operator/internal/upgrade/phase.go | Migration phase model and state machine config generation. |
| operator/internal/upgrade/migration.go | Migration runner that persists/restores phase snapshots and step position. |
| operator/internal/upgrade/position.go | Cluster positioning logic based on served CRD versions. |
| operator/internal/upgrade/finding.go | Finding model and deterministic rendering helpers. |
| operator/internal/upgrade/graph_test.go | Graph and plan-summary unit tests. |
| operator/internal/upgrade/registry_test.go | Registry and topo-sort behavior tests. |
| operator/internal/upgrade/blocked_test.go | Tests for “described but blocked” steps and stage refusal semantics. |
| operator/internal/upgrade/readonly.go | Read-only client wrapper to prevent mutation during preflight/dry-run. |
| operator/internal/upgrade/keys/doc.go | Documentation for key rewrite inventory package. |
| operator/internal/upgrade/helm/doc.go | Documentation for Helm SDK usage and environment isolation goals. |
| operator/internal/upgrade/helm/config.go | RESTClientGetter that binds Helm to the already-established connection. |
| operator/internal/upgrade/helm/render.go | Dry-run manifest rendering for Helm upgrades. |
| operator/internal/upgrade/helm/config_test.go | Tests ensuring Helm uses the provided connection and ignores env kubeconfig. |
| operator/internal/upgrade/spine/doc.go | Documentation for ownership-spine modeling. |
| operator/internal/upgrade/spine/rules.go | Declarative disposition rules for dependents of retiring owners. |
| operator/internal/upgrade/steps/doc.go | Documentation for migration steps package. |
| operator/internal/upgrade/steps/described.go | Helper for steps that can describe but must refuse to act (blocked deps). |
| operator/internal/upgrade/steps/spine_memo.go | Memoization for expensive spine build across step/subject Describe calls. |
| operator/internal/upgrade/steps/handover_test.go | Tests ensuring handover step enumerates per-object subjects in plans. |
| operator/internal/upgrade/tui/doc.go | Documentation for terminal UI reporter behavior. |
| operator/internal/upgrade/transformation.go | Transformation extension-point contract (pure transform + disposition). |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_tasks.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_storagedevices.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_storageclusterops.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_storagebackups.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_replicationslots.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_replicationpolicies.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_replicationpairs.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_replicationops.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_controlplanes.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_backuppolicies.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| operator/internal/upgrade/crds/manifests/storage.simplyblock.io_backupimports.yaml | Embedded CRD manifest copy used by the upgrade tool. |
| atlas-lib/README.md | Updates atlas-lib package index documentation. |
| atlas-lib/kube/names.go | Documents handle-vs-label length constraints and adds handle annotation key. |
| atlas-lib/kube/derived_test.go | Adds tests for bounded/deterministic derived identifier formulas. |
Review details
- Files reviewed: 106/122 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.
noctarius
force-pushed
the
feat/api-upgrade-framework
branch
from
September 10, 2026 07:11
f2c28b0 to
142272f
Compare
ReadOnlyClient promises to pass reads through and refuse writes, and SubResource(...).Get was refused with the writes. Nothing calls it today, which is why nothing was failing: the cost was a check written later failing with a message about the stage not being allowed to write, on a call that reads. So SubResource wraps the real subresource client instead of replacing it, and delegates the read. Status keeps returning the refusing one, having no read to delegate. The tests for this move out of runner_test.go, where they were a client test in a runner file, and cover both halves: reads and a subresource read pass through, every write and every dry-run write is refused. The subresource read is asserted through an interceptor, since the fake client answers a subresource Get for scale and nothing else, and what is under test is that the call arrives at the client underneath. Two comments said things the code does not do. restConfig resolves flags, then the kubeconfig loading rules, and falls back to the in-cluster configuration through clientcmd rather than in that order itself, which is what makes the binary work as a Job with no kubeconfig mounted. SilenceErrors is false because cobra prints the error and main does not, where the comment claimed the reporter had already rendered it. And two blocked steps named a blocker that was resolved: Helm's Go SDK is a dependency now. What upgrade-operator is actually waiting for is the values translation of §13.1, and what the release handover waits for is the decision about where the new chart comes from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fbpxwB6MrqCGNr62Bw4dy
The tool is what makes a cluster capable of running the new operator. A copy that arrives inside that operator's image has to be pulled before the thing it is a prerequisite for, and reaching it from a workstation means extracting a binary out of a container. So it leaves the image. `make -C operator build-upgrade` builds it, for the UPGRADE_GOOS and UPGRADE_GOARCH it is given and defaulting to the host's. The platform is a variable because the tool runs from wherever somebody administers the cluster from, which is a laptop as often as a jump host. .github/workflows/operator_upgrade_tool.yaml runs that target for the four combinations of linux and darwin with amd64 and arm64, through the Makefile rather than a `go build` spelled out in YAML, so the build somebody runs by hand and the build CI runs are the same one. It publishes nothing: where the tool is released from is undecided, and what is useful before that is knowing it still compiles. The workflow also fails on stale embedded CRDs. They are the tool's own copy of what controller-gen writes (§11), and a copy is a thing that goes stale, so this is the drift check operator_manifests.yaml already runs over config/crd/bases, applied to the copy that ships inside the binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fbpxwB6MrqCGNr62Bw4dy
The comments added with the upgrade tool's build narrated status and reasoning: what is undecided, what would be useful until it is decided, where somebody might be sitting when they run the tool. None of that is about the build. Where the tool is released from becomes Q6 rather than a paragraph repeated in three files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fbpxwB6MrqCGNr62Bw4dy
noctarius
force-pushed
the
feat/api-upgrade-framework
branch
from
September 10, 2026 07:34
447b961 to
3d42b86
Compare
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.
Implements
operator/docs/designs/crd-redesign/design-api-upgrade.md: theframework the API upgrade and the resource-model migration are assembled out of,
the read-only
preflightthat reports the checks and the plan, and the firststeps that actually change a cluster.
It is a framework and not a script because the set of units grows with every API
version. Each kind of unit is an interface with one registry behind it, so
adding a rule to the product is writing one value and putting it in one catalog.
What is here
operator/cmd/simplyblock-upgrade(cobra), withpreflight,upgrade,migrate, andrules. It talks to the cluster through the Kubernetes clientlibraries and to Helm through Helm's Go SDK, so neither
kubectlnorhelmhas to be installed.
operator/internal/upgradeis the framework: five extension points —Discoverer,Check,Derivation,Step, andTransformation— composedexplicitly in
internal/upgrade/catalog/catalog.go. There is no global stateand no
initregisters anything, which is what lets a test run one checkagainst a fake client without the other forty deciding the outcome.
A
Stepis asked about oneSubjectat a time, throughDescribe,Done,Validate,Apply, andVerify. §22 states idempotency per object rather thanper step, so a step that answered once for a whole kind would collapse three
nodes into one all-or-nothing decision.
Describereturns nil where there is nooperation, including a subject already in the target state, which makes the plan
the outstanding work by construction and a rerun self-healing.
Working today against a real cluster:
cache is restricted to no namespace and its RBAC is a
ClusterRole.validation, over every name this product derives (§19.2, §19.3).
would perform it with the writes left out.
installs are the ones the conversion code in the same binary was built
against. A CRD is written only where it differs from what is installed.
per object.
What is deliberately not here
Steps that cannot yet act implement
Blocked, and the runner refuses a stageholding one before applying anything — a stage that stops at its fifth step
leaves the cluster halfway through an upgrade nothing can finish. Eight steps
are in that state, all waiting on the
v1alpha2types (§29.1), the conversionfunctions (§29.2), or the new chart being rendered.
preflightprints them, sothe blocked set is the remaining work.
operator/internal/controller/nodedrain_controller.gocarries TODO markers onlyand is not fixed here.
Notes for review
That is the thing to read before adding one.
RESTClientGetteranswers with the connection the run already holdsand is structurally incapable of reading the environment. The usual
implementation re-resolves
KUBECONFIGand the current context, which wouldlet a run inspect one cluster and release to another, and work fine on every
machine where the two agree.
rather than merely stale:
label-pool-node-keymodelssimplyblock.io/pool.<namespace>.<cluster>.<pool>, and the operator nowwrites
storage.simplyblock.io/storage-pool.<poolUUID>. The check thereforebounds a label key nothing produces. The replacement is uninteresting to §19
by construction — a UUID neither overflows 63 characters nor collides — so
the row is a candidate for deletion rather than repair, and §19.2 of the
design has to be re-synced with whichever it becomes.
Every commit is green on
make -C operator lint, the operator test suite, thenine house-style gates, and codespell with CI's flags.
🤖 Generated with Claude Code
https://claude.ai/code/session_011fbpxwB6MrqCGNr62Bw4dy