fix(upgrade): bound the cordoned window so a wedged upgrade cannot strand a node - #167
Merged
Conversation
…rand a node
Every FAILING upgrade path already un-cordons: error returns, panics, and
the restart supervisor's watchdog all run the same idempotent undrain. What
none of them cover is an upgrade that never fails and never finishes.
The download has no timeout by design — the Windows asset is ~984 MB and a
slow link is not a failure — so the only cancellation is the caller's
context. When the caller is a hypervisor guest-agent exec, that context
outlives the operator's patience: qemu-guest-agent has no kill, so mayfly
giving up on polling does not stop the guest process, and the RPC stream
stays open. A blackholed TCP connection therefore parks io.Copy forever
with the scheduler cordoned — a node that reports status ok, restarts
nothing, and silently takes no work.
Two bounds, both of which turn a hang into an ordinary error that the
existing undrain then handles:
- StallTimeout (2m): zero bytes on a connection that is supposed to be
streaming. Reset per read, not per progress tick, so a genuinely slow
but live transfer still completes.
- InstallTimeout (30m): a backstop over the whole cordon-to-swap phase,
for hangs that are not the download. Armed after the drain, since a
long drain is somebody's job finishing, not a hang; disarmed at the
swap, after which the restart supervisor owns the cordon. The fleet's
slowest real upgrade goes cordon-to-swap in ~40s.
A hard kill needs no handling and gets none: `draining` lives only in the
scheduler's memory, so a daemon killed while cordoned comes back serving.
Tests cover the stall, the backstop, a slow-but-live transfer that must NOT
be mistaken for a stall, and the drain timeout — each asserting exactly one
Uncordon and an untouched binary.
…ming it Review of #167 caught that the budget's doc oversold its reach, and chasing that down turned up something worse than an inaccurate comment. The watchdog's only lever is cancelling a context, and only the two downloadFile calls read one. verifyChecksum and extractBinary take no context at all; probeVersion builds its own. So a wedge in any of those ran to completion — and then the upgrade CARRIED ON, swapping the binary and restarting, after having logged "install phase exceeded its budget; aborting". A log that contradicts what the daemon did is worse than either the hang or the abort on its own. Fixed by checking ctx at the two phase boundaries that matter: before staging, and immediately before the swap — the step past which doing nothing stops being a valid way to back out. A budget that expires inside an uninterruptible local step now stops the upgrade at the next boundary instead of being silently advisory. TestRun_ExpiredBudgetNeverSwaps burns the budget inside the Probe seam (uncancellable by construction) and asserts the install binary and .old backup are untouched and the scheduler was uncordoned exactly once. The comments now say what is actually true: the budget interrupts the downloads and halts everything else at a boundary. Also documents that 30m over the 984 MB Windows asset implies a ~550 KB/s sustained floor — safe to hit (node keeps its old binary and resumes serving) and three orders off the fleet's measured 40s, but a real number that InstallTimeout exists to raise; and that the budget arms regardless of whether we cordoned, since bounding a wedged upgrade is worth doing either way. Corrects the previous commit message: this file adds 6 tests, not 4. Note for CI: the atomic.Bool + AfterFunc/Reset concurrency in downloadFile and the install watchdog is exactly what -race should cover, and -race could not run on the dev box (requires cgo; no gcc). Wants a Linux CI run.
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
1 participant
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.
Bounds the window in which an
ephemerd upgradecan hold the scheduler cordoned.What this is not
I opened this investigation believing a v0.2.3 Windows upgrade had failed and left the node cordoned on the old binary. Reading the node's own log showed that never happened, and the correction is worth recording because it shaped the fix:
The upgrade completed in 46 seconds and was serving Windows jobs 79 seconds later. The
{"draining":true,"version":"v0.2.2","uptime":"6h54m26s"}snapshot that prompted this was a healthy in-flight upgrade — that uptime resolves to21:30:22, one second before the swap. It was the legitimate cordoned window, sampled at the worst possible instant.Runalready un-cordons on every error return, on panic, and via the restart supervisor's watchdog, withTestRun_UndrainsOnEveryPreSwapFailurecovering it. So "uncordon on abort" and "move the drain later" were both already correct and are not changed here.The real hole
An upgrade that never fails and never finishes.
downloadFiledeliberately uses a client with no timeout, so the only cancellation is the RPC context — and since QGA has no kill, a caller giving up does not stop the guest process or close the stream. A blackholed TCP connection parksio.Copyindefinitely with the scheduler cordoned. Nothing in the existing error paths fires, because there is no error.Two bounds, both converting a hang into an ordinary error the existing undrain already handles:
StallTimeout(2m) — reset per read, so a slow-but-live transfer still completes; only a genuinely silent connection trips it. This matters given the Windows asset is ~984 MB (Release: the ~1 GB Windows asset upload fails or hangs on every release #166).InstallTimeout(30m) — a backstop over the whole cordon-to-swap phase, armed after the drain and disarmed at the swap.On hard kill
Deliberately unhandled.
s.draininglives only in the scheduler's memory (scheduler.go:489), so a daemon SIGKILLed or force-stopped while cordoned comes back up serving. A startup reconcile would be dead code; that reasoning is documented in the source rather than left for the next reader to re-derive.Tests
4 new tests in
pkg/upgrade/stall_test.go.go test ./...all ok, withpkg/upgrade,pkg/schedulerandcmd/ephemerdre-run under-count=1so none of it was cached.GOOS=linuxandGOOS=windowsbuilds clean,go vetclean. (GOOS=darwin go build ./...fails inCode-Hex/vzfor want of cgo + the macOS SDK — pre-existing;GOOS=darwin go build ./pkg/upgrade/is clean.)Verifying on a node
Point an upgrade at a mirror that accepts the connection then goes silent (
--url). The node must return todraining: falseon the old binary within ~2 minutes and logupgrade did not complete; scheduler UNCORDONED.Pairs with the mayfly-side change, which stops reporting a successful upgrade as a failure.