Repository navigation
fix(e2e): budget each linstor wait separately in post-install-prep - #3715
Conversation
The post-install prep script waits out a serial reconcile chain -- the linstor HelmRelease, then the linstor-controller Deployment -- against a single 15-minute deadline anchored at the script's start. The head of the chain therefore eats the window and the tail inherits whatever remains, so an install fails whenever the chain merely runs long rather than never converging. Measured on one such failure: the HelmRelease went Ready 452s in, and the Deployment needed a further ~520s because the controller's DB migration lost its apiserver connection and retried four times before succeeding. Neither figure exceeds 900s on its own; only their sum did, and the install reported the Deployment as though it were broken. Give each wait its own budget, started when the link before it completed -- the shape the LINSTOR node and MetalLB CRD waits further down already have. What that buys is not speed but a guarantee: every link gets its whole allowance regardless of what the links ahead of it spent, so the tail can no longer be failed by the head being slow. It is a trade, and the other half is worth stating. The script's worst case becomes 900 + 900 + 300 + 300 = 40m against the 180m the job carries, where before the two waits shared one 15m window. And a link that is genuinely stuck is reported later rather than sooner: a Deployment that never converges surfaces at 1355s where the shared window surfaced it at 900s, on that run's timings. A HelmRelease that never becomes Ready surfaces at the same moment as before, since its budget still starts with the script. A timeout now also lists the linstor pods and the namespace events. The description names the object waited on, not the link that held it up, and the same "Deployment not Available" has been reached both with the controller pod crash-looping its migration init container and with that pod blocked on a client TLS Secret cert-manager had not issued yet. Telling those apart previously required the run's bundle. The events come from `kubectl events` rather than `kubectl get events`. It orders by when an event was last seen, so a condition repeating for minutes -- what a stuck mount looks like -- stays inside the window instead of being pushed out by newer one-off events, and it carries no --sort-by, so it cannot fail the whole read the way a sort key does when one item lacks it. Both reads keep their stderr and carry a bound. A read that fails silently is indistinguishable from a namespace with nothing to report, which is the distinction being drawn; and the caller is blocked in `wait`, so an unbounded read against a wedged apiserver would hold the install open until the job's own timeout. The client budget is strictly the smaller of the two so kubectl gets to name the reason before the outer kill takes it away. The wait helper is named for linstor because those diagnostics read one namespace unconditionally: the wait is generic over its kubectl arguments, what it dumps on the way out is not. The new unit test runs the script end to end against a stub PATH carrying a virtual clock, so a 15-minute budget elapses in a few hundred forks and the timings are exact rather than wall-clock approximate. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesLINSTOR readiness handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
area/testing
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Overview
This PR replaces one 900s deadline shared across two sequential kubectl wait calls in hack/e2e-post-install-prep.sh (linstor HelmRelease Ready, then linstor-controller Deployment Available) with two independent 900s budgets, each starting when the previous link completed. It also adds bounded pod/event diagnostics on timeout, and a new bats unit-test file (hack/post-install-prep.bats) that drives the script end to end against a stubbed PATH with a virtual clock.
Root-cause assessment (the point this review weighs most heavily)
This is not a bare "increase the timeout" workaround. The bug in the old code was a modeling error: it anchored both waits to one shared clock (DEADLINE) even though the two waits are sequential, independent reconcile phases (HelmRelease -> piraeus-operator -> cert-manager TLS issuance -> Deployment). A slow-but-healthy first phase unavoidably starves the second phase's share of the same window. The PR's own numbers make the point concretely: HR Ready at +452s, Deployment Available a further ~520s later (972s total) — neither figure abnormal on its own, but their sum broke a shared 900s budget and the failure was misreported as "Deployment broken."
Two things support treating this as a genuine fix rather than a tuning knob:
- It brings the two linstor waits in line with a pattern already present in the same file — the node-Online wait (
node_deadline, 300s) and the MetalLB CRD wait (timeout 300) further down already use their own independently-anchored budgets. This PR just makes the file internally consistent instead of introducing a new shape. - The new
hack/post-install-prep.batsfile matcheshack/*.batsand does not carry thee2e-prefix, so it is picked up byBATS_UNIT_FILES(Makefile:161) and runs undermake unit-tests(Makefile:109) in the PR's fast-lane CI job, not just as a manual script.
The cost is disclosed and bounded: worst case for the linstor phase goes from 900s to up to 1800s, which combined with the unchanged 300s node wait + 300s MetalLB wait is well inside the e2e job's timeout-minutes: 180 (confirmed in .github/workflows/pull-requests.yaml:721). A stuck first link (HelmRelease never Ready) still fails at the same ~900s as before, since its budget still starts at script entry — this is proven by the "fails before the Deployment is waited on" bats test. Only the second link's failure-detection latency changes, and only when the first link was itself slow (not stuck).
Regression check
wait_forwas renamed towait_for_linstor; this function is never sourced elsewhere (the script is only ever invoked as an executable, backgrounded, inhack/e2e-install-cozystack.bats:231), so the rename carries no external breakage risk.deadline=$(( $(date +%s) + LINK_BUDGET ))is recomputed insidewait_for_linstoron each call, so it genuinely starts at call-entry time rather than being anchored once — matches the PR's stated design.set -eusemantics preserved:wait_for_linstorstill returns 1 on timeout and the call sites are bare statements, soset -estill aborts the script on a genuine timeout, same as before.- The new diagnostics use
kubectl events(sorts by last-seen, no--sort-byflag) rather thankubectl get events --sort-by=.lastTimestamp, deliberately avoiding the known failure mode where aevents.k8s.io/v1Event with unset.lastTimestampmakes--sort-byfail the whole read silently — this is exercised by a dedicated bats test. - Diagnostic reads are bounded (
timeout -k 5 30 ... --request-timeout=10s) and both keep stderr, so a stuck apiserver during the diagnostic dump cannot itself hang the script.
Test coverage
The bats suite (via a virtual-clock stub PATH) exercises: independent budgets holding under a slow-but-healthy chain, a stuck Deployment failing inside one budget (not two, not unbounded), diagnostics containing both pod and event content, one diagnostic read's failure not suppressing the other, and a stuck HelmRelease short-circuiting before the Deployment wait ever runs. This is solid coverage for pure budget/branching logic.
Non-blocking notes
- Disclosed test gaps, not hidden ones: the PR description explicitly calls out that (a) removing
2>&1from either diagnostic read would leave all six tests green (the harness happens to capture the script's own stderr the same way), and (b) the "one failing read doesn't suppress the other" test is order-dependent on which read runs first. Both are already flagged in the description. Worth hardening in a follow-up (e.g., assert ontimeout's exit code separately, or run both orderings), but this is test-infra self-critique on a non-production path, not something that should block this PR. - Detection latency for a Deployment that never converges roughly doubles in the worst case (from ~900s to up to ~1800s within this script). This is called out plainly in the PR body with the against-180-minutes-job-budget math, so it's an informed trade-off rather than a silent regression.
wait_for_linstor's doc comment correctly notes the function's diagnostics are hardcoded tocozy-linstoreven though the wait itself is generic over itskubectl waitargs — fine given it currently has exactly two linstor call sites, but worth remembering if this helper is ever reused for a non-linstor wait.
Closing
Static review only; no cluster-side verification performed. The change is well-reasoned, consistent with existing conventions in the same file, backed by measured run data in the PR description, and covered by a genuinely exercised (not just present) test suite that is wired into the fast CI unit-test lane. No blocking issues found.
…in post-install-prep (#4434) Backport of #3715 to `release-1.6`. Both linstor waits in `hack/e2e-post-install-prep.sh` still run off one 15m deadline on this branch, so a slow linstor HelmRelease eats the Deployment's budget and install reports the wrong link as broken. Each wait now gets its own budget. Cherry-picked clean with `-x`. ### Testing - `make unit-tests` green, POSIX sh sweep clean. ```release-note NONE ```
2 participants
What this PR does
Both waits in
hack/e2e-post-install-prep.shran off one 15-minute deadline anchored at the script's start, so the head of the chain ate the window and the tail got whatever was left. Two runs on 2026-08-09 failed that way. The linstor HelmRelease went Ready 452s in, and the Deployment needed a further ~520s because its DB migration lost the apiserver and retried four times. Neither figure passes 900s alone. Their sum does, and the install reported the Deployment as broken.Each wait now gets its own budget, started when the previous link finished. Same shape as the node wait and the MetalLB CRD wait further down the same file. What it buys is that every link gets its whole allowance regardless of what the links ahead of it spent.
The cost is on both sides and I measured it. Worst case goes from one 15m window to 900 + 900 + 300 + 300 = 40m, against the 180m the e2e job carries. A Deployment that never converges is now reported at 1355s instead of 900s, from the 452s above plus the loop's 5s poll. A HelmRelease that never goes Ready is still reported at 900s, which I checked by running the new suite against the pre-change script.
The timeout now dumps the pods and recent events of
cozy-linstor. The same "Deployment not Available" has come from a crash-looping migration init container and from a pod waiting on a client TLS Secret cert-manager had not issued yet, and telling those apart used to need the run's bundle. Events come fromkubectl events, which sorts by last-seen and carries no--sort-byto fail on a key some item lacks. Both reads keep their stderr and are bounded, with the client budget smaller than the outer one so kubectl gets to say why before the kill lands.hack/post-install-prep.batsruns the script end to end against a stub PATH with a virtual clock, so a 15-minute budget elapses in a few hundred forks and the timings are exact. Four of its six tests fail against the pre-change script. The other two guard the bound that stays.Known gaps in the new tests
Dropping the
2>&1from either diagnostic read leaves all six tests green, because the harness catches the script's own stderr into the same file the assertion reads. The property holds in production anyway, since the caller merges both streams into one log, but nothing catches the removal. The test for "one failing read does not suppress the other" needs the pods read to run first; swap the two reads and it passes vacuously. Its comment says so at the point where someone would make that change.Screenshots
Not a UI change.
Downstream repositories
Walked the map against the diff. Two files, both under
hack/. Theccptrigger is about moving or renaming things underhack/or changing what a make target does, and its anchorshack/package.mkandhack/common-envs.mkare untouched.talmandansible-cozystackkey off node prerequisites inhack/e2e-prepare-cluster.bats, a different file.Related
Same family, found while working here and filed instead of folded in. #3707 and #3708 for unbounded reads elsewhere in this script, #3712 for the
--sort-by=.lastTimestampform in fourteen other places, #3713 for the wait loop dropping kubectl's stderr for its whole budget, #3714 for the MetalLB CRD wait failing without a message.Release note
Summary by CodeRabbit
Bug Fixes
Tests