Repository navigation
feat(cli): add stratum node release and hold for the startup taint - #116
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
1886459 to
bfd989c
Compare
849f697 to
57b2908
Compare
bfd989c to
a74d97f
Compare
57b2908 to
1859056
Compare
a74d97f to
eee5a9d
Compare
1859056 to
ad6184d
Compare
eee5a9d to
5892876
Compare
ad6184d to
26f8f3a
Compare
PR brief
Adds Update, 2026-11-04: I reviewed it again at Worth a look
Went well
DetailsWho it affects: Cluster admins on a control plane, and operators running Risk: Low. The two new commands write taints and annotations on the named Node only. Hard to undo: Nothing. The annotations Upgrade notes: How you'd spot a problem: Command output and the Node's annotations. No new log line, event or metric. Proof
Docs and changelog: A "Releasing and holding a node" section in Depends on: Nothing open. #108 and #115 are merged. Findings
Review coverage: All 12 files read (+1130/-15), each finding checked by two reviewers and against the code. All comments checked: jrbing, jsco2t, Copilot rounds and author replies. Copilot comments on #115's code are not counted here. |
jrbing
left a comment
There was a problem hiding this comment.
Approve with nits. Correctness looks sound: get/mutate/Update with RetryOnConflict, idempotent release/hold, no other writer of spec.taints.
- Hold → drain → release leaves the node cordoned (
pkg/cli/nodegate.go:71,:106;configuration.md). The drain hint has no matchinguncordon, andspec.unschedulableis never cleared, yet release prints "workloads can now schedule onto it". (Also raised on #117.) - Nit:
configuration.md:159says--bydefaults to thesudouser; the code falls back to the current user (usuallyroot) whenSUDO_USERis unset.
|
Removed |
jrbing
left a comment
There was a problem hiding this comment.
Approving. Please still address the findings in my earlier review comment.
26f8f3a to
e251372
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues can leave runtime labels inconsistent with containerd’s loaded handlers and prevent burn-in pods from running.
10 open issues
Serialize handler reconciliations with runtime changes · New Recover state and labels when saving the new config fails · New Prevent stale labels when reconciliation fails · New Refuse to apply when a previously recorded config is missing · New Normalize the matching taint value to true · New Persist pending state before changing the containerd drop-in · New Track loaded handler configuration, not just handler names · New Report missing or malformed containerd configs as errors · New Parse imported registry TOML while skipping contention warnings · New Document that drained nodes must be uncordoned after release · New
What changed in this PR
Adds manual control of Stratum’s startup taint through node hold and node release. The stacked changes also connect node configuration, runtime-handler management, and RuntimeClass scheduling across the CLI and cluster lifecycle.
Changes:
- Add hold/release commands with actor validation, audit annotations, and conflict retries.
- Carry node labels, taints, and runtime handlers through joins and restarts; expose runtime management through CLI and MCP.
- Add lifecycle tests and operator documentation for startup gates and runtime scheduling.
| File | Description |
|---|---|
pkg/cli/up.go |
Adds node flags and worker-join preflight validation. |
pkg/cli/up_worker_runup_test.go |
Captures node settings passed to worker preparation. |
pkg/cli/up_worker_nodefacts_test.go |
Tests node flag forwarding and early refusals. |
pkg/cli/runtime.go |
Adds runtime apply, check, and restart commands. |
pkg/cli/runtime_test.go |
Tests runtime commands and validation. |
pkg/cli/root.go |
Registers runtime and node command groups. |
pkg/cli/nodegate.go |
Adds node hold and release commands. |
pkg/cli/nodegate_test.go |
Tests gate commands and actor defaults. |
pkg/cli/mcp_test.go |
Adds runtime tools to expected MCP lists. |
pkg/cli/mcp_surface_test.go |
Updates exposed MCP tools and counts. |
internal/supervisor/legacy.go |
Excludes own children from legacy-supervisor detection. |
internal/supervisor/legacy_test.go |
Tests child-process exclusion. |
internal/runtime/runtime.go |
Shares containerd restart and readiness handling. |
internal/runtime/runtime_test.go |
Updates fake supervisor process identities. |
internal/runtime/registertaints_test.go |
Tests taint parsing and kubelet rendering. |
internal/runtime/profile_render_test.go |
Tests protection of engine-managed taints. |
internal/runtime/kubelet.go |
Renders node registration taints. |
internal/runtime/handlersapply.go |
Applies handlers and tracks pending runtime changes. |
internal/runtime/handlersapply_test.go |
Tests runtime checks, restarts, and pending records. |
internal/profile/profile.go |
Reserves the kubelet registration-taint setting. |
internal/operator/manifests_ownership.go |
Tracks bundled RuntimeClass ownership. |
internal/operator/manifests_ownership_test.go |
Tests RuntimeClass ownership and cleanup. |
internal/nodestate/nodestate.go |
Persists node labels and taints. |
internal/nodestate/nodestate_test.go |
Tests node-setting persistence. |
internal/mcp/stratumwrite.go |
Adds the runtime apply handler. |
internal/mcp/stratumwrite_test.go |
Tests runtime apply host restrictions. |
internal/mcp/stratumread.go |
Adds runtime diagnostics. |
internal/mcp/catalog.go |
Registers runtime MCP tools and access tiers. |
internal/mcp/catalog_test.go |
Updates expected runtime tool registration. |
internal/k8sutil/nodelabels.go |
Reconciles declared and runtime-handler labels. |
internal/k8sutil/nodelabels_test.go |
Tests label updates and preservation. |
internal/k8sutil/nodegate.go |
Implements conflict-retried hold/release mutations. |
internal/k8sutil/nodegate_test.go |
Tests gate mutations, refusals, and retries. |
internal/k8sutil/k8sutil.go |
Defines the runtime-handler label prefix. |
internal/ha/join.go |
Carries node settings through control-plane joins. |
internal/ha/join_test.go |
Tests joined node settings. |
internal/ha/join_recovery_test.go |
Tests settings refreshed during join recovery. |
internal/cluster/worker.go |
Saves and updates worker node settings. |
internal/cluster/runtimehandlers.go |
Coordinates runtime changes, persistence, and labels. |
internal/cluster/runtimehandlers_test.go |
Tests runtime orchestration and locking. |
internal/cluster/runtimeclass_test.go |
Tests RuntimeClass staging and removal. |
internal/cluster/run.go |
Adds runtime-aware labeling and RuntimeClass staging. |
internal/cluster/run_test.go |
Adapts node-registration fixtures. |
internal/cluster/restamp_test.go |
Tests containerd-start labeling callbacks. |
internal/cluster/prepare.go |
Loads controller node settings and warns about taint changes. |
internal/cluster/nodelabels_test.go |
Tests registration-time and saved-state labeling. |
internal/cluster/nodeipmove_test.go |
Tests controller settings refreshed on restart. |
internal/cluster/nodefacts.go |
Defines node-setting validation and merge rules. |
internal/cluster/nodefacts_test.go |
Tests node configuration and lifecycle behavior. |
internal/cluster/importerstacks.go |
Reserves the bundled RuntimeClass stack. |
internal/cluster/importerstacks_test.go |
Includes RuntimeClasses in bundled-stack checks. |
internal/cluster/dashhint_test.go |
Adapts registration tests to node-state arguments. |
internal/cluster/config.go |
Parses and validates node settings and RuntimeClasses. |
internal/cluster/config_worker_test.go |
Tests supported worker configuration fields. |
internal/addons/runtimeclass.go |
Renders RuntimeClasses with handler selectors. |
internal/addons/runtimeclass_test.go |
Tests selectors and string-safe rendering. |
docs/docs/runtime/index.md |
Documents runtime management and pending changes. |
docs/docs/networking-storage/index.md |
Updates supported worker configuration fields. |
docs/docs/installation/index.md |
Updates worker configuration guidance. |
docs/docs/installation/configuration.md |
Documents node settings, hold/release, and RuntimeClasses. |
docs/docs/cluster-admin/mcp.md |
Documents runtime tools, counts, and credentials. |
docs/docs/appendices/paths-and-names.md |
Lists runtime paths, labels, and gate annotations. |
Review effort: Balanced
e251372 to
9348bdc
Compare
loag-ciq
left a comment
There was a problem hiding this comment.
Approving. Low findings only; see the PR brief. The uncordon note after hold and the release message are worth fixing in a follow-up.
…t is there `stratum node hold` skipped adding the startup taint when the node carried stratum.ciq.com/unverified with any effect. A node that registered with it as PreferNoSchedule, which spec.node.taints accepts, was reported held while the scheduler could still place new pods on it. A hold now adds the NoSchedule taint unless that exact taint is there, and a release still removes every effect of the key. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…ode stays cordoned HoldNode skipped a NoSchedule stratum.ciq.com/unverified taint the node already carried whatever its value, so a node tainted unverified=false stayed that way and a burn-in job tolerating unverified=true could not run on it. A hold now sets that taint's value to true. hold tells the operator to drain the node, which also cordons it, but release removes only the taint, so the documented hold, drain, release sequence left the node unschedulable while release reported that workloads could schedule. release now checks for a cordon and, when it finds one, says to uncordon the node instead; it still leaves the cordon, which is the operator's. hold's drain hint and configuration.md name the uncordon step, and hold's messages say that pods tolerating the taint still schedule rather than promising that none do. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…e release report only the taint it removed The refusal to replace a runtime-handler drop-in stratum did not write sat in writeHandlers, which every node start runs too, so a worker with such a file and saved handlers stopped starting. The refusal moves to runtime.RequireOwnDropIn, which runtime apply runs with its other preflight checks; a node start replaces the file as before, now warning that it did. release said workloads can now schedule onto the node whenever its cordon check found no cordon, including when the check itself failed and when another NoSchedule taint still kept pods off. It now reports what it did, removing the startup taint, and warns with the uncordon step when it cannot tell whether the node is cordoned. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…sudo configuration.md said node release and hold record the user who ran sudo, but defaultActor falls back to the current user, usually root, when SUDO_USER is unset. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
… identity stratum node writes as release --force on a node without the startup taint still printed that it removed the taint, so a reader or a script took an untainted node for one gated a moment ago. ReleaseNode now returns a ReleaseResult, whether it removed the taint and whether the released node is still cordoned, read from the Node its update wrote, and release prints a re-verify as one. The second Get, and its could-not-check warning, go. nodeGateClientset said it writes through admin.conf, but AdminClientset picks super-admin.conf, the system:masters identity, whenever it exists. It now builds on NodeAdminClientset, which documents that, and says that the audit log records that shared identity, so --by is the record of who acted. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
The section ran release's four changes, its --by default and the uncordon step into long sentences, said release "says so" without the step to run, and ended on "Neither command" right after a sentence naming drain and uncordon. It is now Releasing and holding a node, with one change per sentence, the --by rules (the sudo user else the current user, no whitespace) and the RFC 3339 UTC time, the uncordon and drain commands in console blocks, and release and hold named. The three paths-and-names links follow the new heading. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…runtime apply RequireSharedConfDParses repeated the glob and decode loop at the top of checkSharedConfD, so the apply preflight and runtime check could drift on which conf.d files they read and which they call unparsable. Both now read readSharedConfD, which decodes every file containerd imports and keeps each file's parse error. A test checks that valid files, stratum's registry drop-in and an nvidia-ctk drop-in among them, pass both, and that a broken file fails both. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…n this cluster `stratum node release` and `hold` took an empty node name, as an unset shell variable gives, and built the admin client before the API server rejected it. They now refuse it as a usage error before connecting. A conflict retry read the Node again by name, so a Node deleted and registered again in between was released or held although the operator checked the first one. The retry now refuses a Node with a different UID with ErrNodeReplaced and changes nothing. The drain and uncordon hints named `stratum kubectl`, which honors a KUBECONFIG set for another cluster, so following them could act on a same-named node there. They now pass --kubeconfig with this control plane's admin.conf. The hold hint also says that a drain's cordon keeps the burn-in job off, so uncordon after the maintenance while the node is still held, run the burn-in, then release. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…uster A drained node is cordoned, so a burn-in job that tolerates only the startup taint cannot schedule until it is uncordoned. The hold section now says to uncordon after the maintenance while the node is still held, run the burn-in, then release, and to leave a cordon set for another reason. The drain and uncordon commands pass --kubeconfig with admin.conf, so a KUBECONFIG set for another cluster does not take them there. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…n about emptyDir in the drain hint A release that conflicted read the Node again and released it whatever had changed. If another operator held the node in between, for a fault found after the first operator checked it, the retry removed that hold. ReleaseNode now records the hold annotation from its first read, and a retry that finds a different one refuses with ErrHeldMeanwhile and changes nothing. Other conflicts still retry. The drain hint did not say that a drain refuses pods with emptyDir volumes, CoreDNS among them, so following it could leave the node cordoned with those pods still running. `hold` now says so, and to add --delete-emptydir-data only if losing that data is acceptable. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
… delete their data The drain step now says that a drain refuses pods with emptyDir volumes, CoreDNS among them, and leaves them running, and to retry with --delete-emptydir-data only once losing that data is acceptable. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…t a cleared hold fall to the taint check The retry compared only stratum.ciq.com/held, whose stamp has second precision, so a re-hold by the same actor within the same second with a new reason was undone. A release that landed in between cleared the record and was refused as "held meanwhile"; it now falls to the taint check, which reports the node as not held. Signed-off-by: JR Bing <jrbing@users.noreply.github.com> Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…S replica before it hold's hint and long help, and the docs, said a drain moves the running pods; DaemonSet and static pods stay. CoreDNS has no PodDisruptionBudget, so draining the node that runs its last Ready replica stops cluster DNS: the hint and the docs now say to check for another replica first, as the upgrade reference does. Signed-off-by: JR Bing <jrbing@users.noreply.github.com> Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…eased CoreDNS does not tolerate stratum.ciq.com/unverified, so on a single-node cluster its replicas cannot schedule back after an uncordon while the node is still held. The hold hint and the docs said DNS returns on uncordon; they now say it returns once the node is uncordoned and released. Signed-off-by: JR Bing <jrbing@users.noreply.github.com> Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
`stratum runtime apply` always restarts containerd, and ApplyHandlers reports Unloaded on that path only with an error, which the command returns before printing. The message that told the operator to run `stratum runtime restart` was never printed. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…etry sees a repeat hold The stamp had second precision, so two holds by the same actor with the same reason within one second wrote identical annotations. A release whose retry came after the second hold took it for the one it had read and removed it. gateStamp now writes RFC 3339 with nanoseconds, so the retry refuses with ErrHeldMeanwhile. The reason for comparing the record and its reason moves from an inline comment into ReleaseNode's doc. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…seconds Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…es not parse, in runtime restart and registry apply `runtime apply` already refused a shared conf.d file that does not parse, since containerd would not start again with it. `runtime restart` and `registry apply` restarted it anyway. `runtime restart` now refuses and restarts nothing. `registry apply` writes the config, which pulls read live, but does not restart containerd into it, and returns an error that names the file and says to run `stratum runtime restart` once it is fixed. Both read the same check as `runtime apply` and `runtime check`. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
…ode hold A worker that registered onto an existing Node without its declared taints warned to add them with `stratum kubectl taint`. When the startup taint is among them, the warning now names `stratum node hold`, which adds it with the hold record, and keeps kubectl for any other taint. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
The join warning named `stratum node hold` for any missing taint keyed stratum.ciq.com/unverified, but hold adds only stratum.ciq.com/unverified=true:NoSchedule. For a declared unverified=true:NoExecute, following it would leave running pods in place rather than restore the eviction. The warning now names hold only for that exact taint, and keeps `stratum kubectl taint` for any other value or effect. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com>
c21b36c to
edf3929
Compare
jrbing
left a comment
There was a problem hiding this comment.
Approving at edf3929. My earlier findings are addressed: release reports a remaining cordon with an uncordon hint, hold explains drain/uncordon/burn-in, and the docs now cover the --by fallback to the current user. The release retry logic (UID check, newer-hold refusal, nanosecond stamps), --clear leaving a foreign drop-in alone, and the removed Unloaded branch (unreachable without an error) all check out. 44/44 checks green.
Follow-up, not blocking: Copilot's request for running-cluster inttest coverage of hold/release (scheduling behaviour and cordon preservation).
|
Successfully created backport PR for |
|
Successfully created backport PR for |


What
stratum node releaseandstratum node hold, the manual control for thestratum.ciq.com/unverifiedstartup taint that a GPU node joins behind.release <node> [--by <who>] [--force]:Nodes().Updateagainst the resourceVersion it read, retried on a conflict, it removes every taint keyedstratum.ciq.com/unverifiedand writesstratum.ciq.com/verified=<who>@<RFC3339 UTC>. This is thefinishNodeshape.--forceis given.hold <node> --reason <text> [--by <who>]: in the same kind of single update, it addsstratum.ciq.com/unverified=true:NoSchedule, removesverified, and writesstratum.ciq.com/held=<who>@<RFC3339>with the reason in its ownstratum.ciq.com/held-reasonannotation, so readers that splitheldat the last@never see the free text. Release removes both. Holding a node that's already held keeps one taint and replaces the record. The command prints how to drain the node, because NoSchedule leaves running pods in place.AdminClientset--byto$SUDO_USER, then the current user; whitespace is refused because readers split the value at the last@Also in this PR:
stratum runtime applyfixesThese follow up on #115 and change
runtime applybehaviour:runtime applychecks every file in the shared conf.d before it changes anything. containerd imports them all, so a restart would replace a working containerd with one that can't start.runtime applywon't replace a file at the runtime-handler drop-in path that stratum didn't write.runtime checkalready tells the operator to move such a file aside.runtime checkandruntime applynow share it, so they agree on which files don't parse.Tests:
TestAForeignDropInIsRefusedByApplyAndReplacedByAStart,TestRequireSharedConfDParses,TestApplyAndCheckAgreeOnTheSharedConfD,TestApplyRuntimeHandlersRefusesASharedConfDThatDoesNotParseandTestApplyRuntimeHandlersRefusesADropInItDidNotWrite.Differences from the ticket
stratum.ciq.com/heldis new. A taint value must be a label value, so--reasoncan't live on the taint.pkg/cli/nodegate.go, becausepkg/cli/node.goalready holds the exportedNodeLayoutAPI. The logic lives ininternal/k8sutil/nodegate.go, next toUnverifiedTaintandVerifiedAnnotation.internal/nodejobstays unexported, as the ticket says.Tests
--forceoverrides thatverified--bydefaults toSUDO_USER--forceDocs
A "Releasing a node" subsection in
docs/docs/installation/configuration.md, next to the example that joins with the taint.docs/docs/appendices/paths-and-names.mdlists thestratum.ciq.com/unverifiedtaint and theverified,heldandheld-reasonannotations.