[PRODENG-3633] Add overall apply/reset deadline and surface retry errors - #668
Conversation
Option A from the ticket: a Manager-level timeout wrapper rather than threading context.Context through all 30+ phase implementations. phase.Manager gains a Deadline time.Duration field. Run races each phase's Run() against the deadline in a goroutine; on timeout it returns an error naming the in-progress phase instead of blocking forever, and does not start the next phase. The phase's goroutine is left running on timeout rather than force-stopped -- launchpad is a short-lived CLI process, so it exits shortly after and reclaims it. This does not make individual waits (WaitGroup.Wait, channel ops) cancellable, only bounds the total time Run can spend. Wired through a new Product.SetTimeout(time.Duration) method rather than adding a parameter to Apply/Reset, so none of the existing callers (cmd/apply.go, cmd/reset.go, test/integration, test/smoke/*) need to change signatures; only cmd/apply.go and cmd/reset.go call it, from a new --timeout flag defaulting to 90m (matching the longest existing smoke test timeout in the Makefile). --timeout 0 disables the deadline. Separately, pkg/product/mke/config/cluster_spec.go's pingHost (the MKE health-check retry loop) only logged "waiting for MKE ... to become healthy" on every attempt and never the actual failure reason mid-retry, which is why diagnosing the original PRODENG-3594 deadlock cost a full 50-minute CI run. Added retry.OnRetry to log the real error on each failed attempt. The final returned error already carried every attempt's error via retry-go's own Error type; nothing was actually being discarded, it just wasn't visible while waiting. Audited every other sync.WaitGroup use in the codebase (connect.go, disconnect.go, run_hooks.go) for the double-send-on-channel pattern that caused the original deadlock: none have it, all use defer wg.Done() with mutex-protected shared state instead of a fixed-capacity channel. Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
|
Added two more live AWS verifications beyond the original truncated-phase run, since that one only proved the deadline could cut off a phase that would have finished on its own — not that it catches a real unbounded hang, and not that it stays out of the way on a legitimate successful apply. Genuine hang, not just a short timeout on normal work: pointed the host at No false positive on the happy path: full, unmodified All AWS infrastructure from both runs torn down and confirmed clean afterward. Full details and log excerpts in the PR description's Testing section. Written by AI: claude-sonnet-5 |
What
Add an overall deadline for
apply/reset, and surface real errors during the MKE health-check retry loop instead of a repeated generic message.Why
PRODENG-3633: today there is no overall deadline on apply/reset, so any wait that sits outside a bounded retry can block forever with no output and no failure. The specific bug that originally triggered this (
pingHost'sWaitGroup.Wait()deadlock) is already fixed under PRODENG-3594; this ticket is about the fact that nothing would catch the next one.How
phase.Managergains aDeadline time.Durationfield.Runraces each phase against it in a goroutine; on timeout it returns an error naming the in-progress phase and does not start the next one. This is a Manager-level wrapper (Option A), not acontext.Contextthreaded through all 30+ phase implementations (Option B) — smaller diff, doesn't make individual waits cancellable, but launchpad is a short-lived CLI so the abandoned goroutine is reclaimed when the process exits after returning the error.Product.SetTimeout(time.Duration)method rather than a parameter onApply/Reset, so none of the existing callers (cmd/apply.go,cmd/reset.go,test/integration,test/smoke/*) need signature changes.--timeoutflag onapply/reset, default 90m (matches the longest existing smoke test timeout in the Makefile),--timeout 0disables it.pingHost's retry loop now logs the real failure viaretry.OnRetryon every attempt instead of only "waiting for MKE ... to become healthy" repeated with no reason. The final returned error already carried every attempt's error viaretry-go's ownErrortype; nothing was being discarded, it just wasn't visible while waiting.sync.WaitGroupuse in the codebase (connect.go,disconnect.go,run_hooks.go) for the double-send-on-channel pattern that caused the original deadlock: none have it.Testing
pkg/phase/manager_test.go: no-deadline/within-deadline completes normally, exceeded-deadline names the hung phase and prevents the next phase from starting, phase errors still propagate with a deadline set.make unit-test,make lint,go vetall clean.examples/terraform/aws-simple, ranlaunchpad apply --timeout 20s. Ran through 6 real phases, failed cleanly 20.2s in whilePrepare hosts(which would have finished on its own) was running:192.0.2.1(RFC 5737 TEST-NET-1, guaranteed unroutable) and ranlaunchpad apply --timeout 30s. The underlying SSH dial's own i/o timeout took real, non-trivial time per attempt (2 failed attempts logged within the 30s window); the deadline correctly cut offOpen Remote Connectionat 30.2s — well before the phase's own 60-attempt retry budget would have exhausted naturally, which at this per-attempt duration could run considerably longer than expected:launchpad applyat the default 90m timeout against a real single-manager stack. Completed successfully — all 42 MKE install steps, "Cluster is now configured" — in 3m59.682s, far under the deadline. Confirms the mechanism does not interfere with normal operation.terraform destroy, 28/28 resources each time, confirmed no orphans viaterraform state listand a tag-filtereddescribe-vpcs).Links
Checklist