Skip to content

fix(talosctl): reboot after a failed drain during upgrade#13802

Open
lexfrei wants to merge 2 commits into
siderolabs:mainfrom
lexfrei:fix/upgrade-drain-half-applied
Open

fix(talosctl): reboot after a failed drain during upgrade#13802
lexfrei wants to merge 2 commits into
siderolabs:mainfrom
lexfrei:fix/upgrade-drain-half-applied

Conversation

@lexfrei

@lexfrei lexfrei commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What? (description)

Two related fixes to how talosctl handles a failed Kubernetes drain, plus the drainNodes return contract that ties them together:

  • talosctl upgrade no longer leaves a half-applied upgrade when the drain fails. In upgradeViaLifecycleService the sequence is: pull installer image → write the new image to disk (upgradeInternal) → drain the node → reboot into the new image. When the drain failed, the command returned early — after the disk was already mutated but before the reboot. It now warns and continues to the reboot so the staged image is always activated.
  • A failed drain no longer strands nodes as SchedulingDisabled. drainNodes records each node name before cordoning it, but on error it previously returned a nil map, so the deferred uncordonNodes (gated on len(nodeNames) > 0) was skipped and the cordoned nodes were never restored. drainNodes now returns the partially-populated map alongside the error, so callers that proceed past a failed drain can uncordon the nodes they cordoned.
  • talosctl reboot --drain gets the same treatment. reboot.go is the other caller of drainNodes; it returned on a drain error before registering its deferred uncordon, so it too abandoned cordoned nodes. The uncordon defer is now registered before the error check. reboot still aborts on a drain failure (there is no staged image forcing it onward), but it restores the nodes it cordoned first.

Why? (reasoning)

upgradeInternal writes the new installer image to disk, but the node only switches to it on the subsequent reboot. If the drain fails — for example because the apiserver is unreachable, or the cluster is not yet bootstrapped — the old code did return err before rebootInternal, leaving the upgrade half-applied: the new image is staged on disk, the node keeps running the old version, there is no reboot to activate it, and the operator gets no signal because the earlier progress output already printed upgrade completed. The next unrelated reboot then silently boots into the staged image.

Once the image is committed to disk, aborting before the reboot is the worst outcome. The drain is a best-effort courtesy, so a failed drain must not strand a half-applied upgrade. Continuing the upgrade past a failed drain then exposed a second problem: drainNodes threw away the list of nodes it had already cordoned, so those nodes rebooted into the new version and stayed SchedulingDisabled forever. Returning the partial map fixes that for both callers, and auditing both callers of the changed function surfaced the same pre-existing gap in reboot --drain.

Behavior change to note for automation: during upgrade, a failed drain is now non-fatal — the command prints a WARNING to stderr, reboots, and exits 0 (the upgrade did apply). Tooling that keys off the exit code will now see success-with-warning rather than a failure on a drain that could not run. This is consistent with the existing best-effort handling in the same file, where a non-fatal upgrade error is surfaced via cli.Warning rather than aborting.

Alternatives considered: draining before writing the image, which reorders the deliberate install→drain→reboot flow and widens the workload-disruption window during multi-node streaming installs; and classifying "apiserver genuinely absent" vs. a transient drain error, which is hard to do reliably and still leaves the disk half-applied on a live cluster. Both are riskier and less complete than warning and continuing.

Acceptance

  • you linked an issue (if applicable) — N/A
  • you included tests (if applicable) — the drain/upgrade/reboot paths take a live *global.ClientFactory + gRPC + k8s clientset with no mock seam, and the package has no unit tests for this flow; adding one would need a refactor far larger than the fix
  • you ran conformance (make conformance) — not run (containerized)
  • you formatted your code (make fmt) — gofmt clean
  • you linted your code (make lint) — not run (needs repo container plugin)
  • you generated documentation (make docs) — N/A, no docs affected
  • you ran unit-tests (make unit-tests) — not run (containerized)

Verified locally after each change: go build ./cmd/talosctl/..., go vet ./cmd/talosctl/cmd/talos/..., gofmt.

talosctl upgrade writes the new installer image to disk before it
drains the Kubernetes node and reboots into it. When the drain step
failed - for example because the apiserver is unreachable or the
cluster is not yet bootstrapped - the command returned early, before
the reboot. That left the upgrade half-applied: the new image was
staged on disk while the node kept running the old version, with no
reboot to activate it and no signal to the operator, since the earlier
progress output already read "upgrade completed". The next unrelated
reboot would then silently boot into the staged image.

Once the image is committed to disk, aborting before the reboot is the
worst outcome. The drain is a best-effort courtesy and must not strand
the upgrade, so warn about the failed drain and continue to the reboot
so the staged image is actually applied.

Draining cordons each node before evicting its pods, so a drain that
fails after the cordon leaves the node unschedulable. drainNodes threw
away the collected node map on error, so the deferred uncordon was
skipped: the node rebooted into the new version but stayed
SchedulingDisabled, and with the reboot succeeding the command still
exited 0 - an orphaned cordon with no error surfaced. Return the
partial map on error so the existing uncordon step restores the node
to schedulable once it is Ready again.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
@lexfrei
lexfrei force-pushed the fix/upgrade-drain-half-applied branch from e68a396 to 03fb11c Compare July 20, 2026 21:18
talosctl reboot --drain cordons each node before evicting its pods.
When the drain failed, rebootRun returned the error before it
registered the deferred uncordon, so nodes that were already cordoned
were left SchedulingDisabled and the operator had to uncordon them by
hand.

Register the uncordon defer before the drain error check. Unlike the
upgrade path there is no staged image forcing a reboot, so aborting on
a failed drain is still correct - but the nodes that were cordoned must
be restored to schedulable first. On the abort path the nodes never
rebooted, so the deferred wait-for-Ready returns immediately and the
uncordon proceeds. This relies on drainNodes now returning the nodes it
cordoned even on error.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
@lexfrei
lexfrei marked this pull request as ready for review July 20, 2026 21:51
@talos-bot talos-bot moved this from To Do to In Review in Planning Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants