Repository navigation
[Backport release-1.37] feat(cli): add stratum node release and hold for the startup taint - #203
Merged
Merged
Conversation
) * fix(runtime): refuse a runtime apply that would replace a foreign drop-in or restart into a broken conf.d writeHandlers replaced whatever file sat at the runtime-handler drop-in's path when handlers were declared, even one without stratum's generated marker, so runtime apply destroyed an operator's file there that runtime check tells them to move aside. It now refuses, naming the file, and leaves it as it was. runtime apply checked containerd's main config imported the drop-in but never parsed the shared conf.d files containerd also imports, so a malformed file added while containerd ran had the apply stop a working containerd for one that could not start. RequireSharedConfDParses now runs with the other preflight checks, before anything is written. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com> * feat(cli): add stratum node release and hold for the startup taint A node can join behind the stratum.ciq.com/unverified=true:NoSchedule taint, which keeps workloads off it until it is verified, but nothing in stratum lifted it or put it back. `stratum node release <node>` removes every taint keyed stratum.ciq.com/unverified and records stratum.ciq.com/verified=<who>@<RFC3339> in one Node update against the resourceVersion it read, retried on a conflict. It refuses a node without the taint unless --force, which records it as verified anyway. `stratum node hold <node> --reason` adds the taint back, removes the verified record, and records stratum.ciq.com/held=<who>@<RFC3339>: <reason>, in the same single update. The reason goes in an annotation because a taint's value must be a label value. NoSchedule leaves running pods in place, so hold says how to drain the node. Both run on a control plane through its admin.conf, leave every other taint and annotation as they found it, and record --by, which defaults to the user who ran sudo. Usage errors refuse before the API server is contacted. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com> * fix(cli): give a hold's reason its own annotation stratum.ciq.com/held carried <who>@<time>: <reason>, and readers split the gate annotations at their last '@' so an actor can be an email address. A free-text reason with an '@' in it, such as an RMA contact, broke that split. The reason now goes in stratum.ciq.com/held-reason, and held carries the same <who>@<time> stamp as verified, so one rule parses both. A release removes both hold annotations. Also from review: - The k8sutil errors named the CLI's flags (--by, --reason, --force), advice a caller without those flags cannot follow. They now say what is wrong; stratum node names the flag and adds the --force hint for ErrNotHeld. - The hold command repeated HoldNode's reason check and its wording. Both now call ValidateHoldReason. - The docs said neither command changes any other annotation, but a release removes the hold record; they now say so, as does release's help. - The conflict test only showed that a conflict leads to a second update. Its first update now also changes the stored Node as another writer would, and the test checks that change survives, which fails if the read moves out of the retry. - Paths and Names lists the startup taint and the verified, held and held-reason annotations. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com> * docs(configuration): write node release and hold as root console commands The docs guide writes a root command as `# /usr/local/bin/stratum` in a console block. `sudo stratum` fails on a Rocky host, whose sudo secure_path does not include /usr/local/bin. Signed-off-by: cwolford <WolfyChris@users.noreply.github.com> * fix(cli): hold a node with NoSchedule even when a softer startup taint 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> * fix(cli): set a held node's startup taint to true and say a drained node 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> * fix(runtime): refuse a foreign drop-in only in runtime apply, and have 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> * docs(configuration): say --by falls back to the current user without 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> * fix(cli): report a forced release that removed no taint, and name the 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> * docs(configuration): split release and hold into one change per sentence 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> * refactor(runtime): read the shared conf.d once for runtime check and 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> * fix(node): refuse an empty or replaced node, and keep kubectl hints on 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> * docs(configuration): uncordon a held node for its burn-in, on this cluster 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> * fix(node): refuse a release retry that would undo a new hold, and warn 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> * docs(configuration): say a drain refuses emptyDir pods unless told to 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> * fix(node): compare a hold's reason too before a release retry, and let 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> * docs(node): say a drain moves only evictable pods, and check a CoreDNS 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> * docs(node): say a held single node's DNS returns only after it is released 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> * fix(cli): drop runtime apply's unreachable containerd-not-loaded message `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> * fix(node): stamp releases and holds to the nanosecond, so a release retry 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> * docs(configuration): show the release and hold stamp with fractional seconds Signed-off-by: cwolford <WolfyChris@users.noreply.github.com> * fix(runtime): refuse to restart containerd into a conf.d file that does 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> * fix(cluster): point a join that missed the startup taint at stratum node 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> * fix(cluster): name stratum node hold only for the taint it adds 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> --------- Signed-off-by: cwolford <WolfyChris@users.noreply.github.com> Signed-off-by: JR Bing <jrbing@users.noreply.github.com> Co-authored-by: cwolford <WolfyChris@users.noreply.github.com> Co-authored-by: JR Bing <jrbing@users.noreply.github.com> (cherry picked from commit 448bcbc)
✅ 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. |
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.
Backport of #116 to
release-1.37.Original: feat(cli): add stratum node release and hold for the startup taint
Backport PRs run the full CI gauntlet — same required checks as
any other PR to this branch.
If this PR contains conflict markers, the cherry-pick did not apply
cleanly and mergiraf could not resolve it. Resolve on the branch,
push, and un-draft.