Repository navigation
test(e2e): hold the wait-budget guard to a start margin across four suites - #4468
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe wait-budget guard now covers MariaDB, MongoDB, RabbitMQ, and ClickHouse. It checks database waits against startup and install-timeout floors with a 60-second start margin. It also checks Chainsaw operation slack after reached waits. ChangesE2E wait-budget guard
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to This change only tightens test-time checks and updates documentation, so it does not affect production behavior. No actionable merge-blocking issue was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. Verified the full guard suite (28 tests) passes on the PR head and on a test-merge with current main, where recent rabbitmq backup-demo changes (#4458, #4459) made this a "green apart, red together" candidate; the byte-identity copy check (test 11) stays green too.
The four per-suite floor/slack checks bind against real values. Each op's remainder equals its MBW_SUITES constant exactly (mariadb/rabbitmq 900, mongodb 1200 with SKIP_RESTORE, clickhouse 2700), and the mongodb/rabbitmq/clickhouse charts carry no startupProbe, so the "-" startup-floor column is a real fact and not a skipped case. Reverting the new margin predicate (-ge floor+margin back to -gt floor) reds the fixture tests 4 and 15, which confirms the start-margin semantics are held by fixtures and not just decorative on the live 660s waits.
NIT (non-blocking): the file is still named mariadb-first-boot-waits.bats and keeps the MBW_ prefix while now holding a four-suite contract; a rename is optional churn and the header already generalises the contract.
The wait-budget guard accepted a wait one second over the install timeout, although the wait's clock starts before the install's by the Application reconcile, the chart fetch and the helm-controller queue. A wait lowered from 660 to 601 passed every test and brought back the bare assert timeout the budgets exist to avoid. A wait now has to clear its floor by a named 60s margin. Nothing in the tree bounds those three delays, so the value is policy: the headroom every current wait already carries. The sizing guide in docs/agents/e2e-testing.md asked only for "strictly above", which the guard now rejects; it points at the margin instead of restating a number. The mongodb, rabbitmq and clickhouse walkthrough waits were raised for the same reason as mariadb's, and their Chainsaw ops grown with them, but nothing held either. Each suite is now a row in one table and gets its own floor and op tests, so a regression reds only its own suite. Floors come from the api-server install-timeout default and, for mariadb, the chart's startup probe; each suite's RD is checked for an override instead of being assumed not to carry one. Each op keeps the remainder it had before the raise over every HelmRelease wait it reaches, the bucket one included, so raising a wait and its op together keeps the remainder instead of widening the slack. The mongodb target behind SKIP_RESTORE=1 is not reached and is left out until the op stops passing it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
08d89df to
94b328c
Compare
…ase they wait on (#4471) ## What this PR does Stacked on #4468, which holds the raised backup waits to a start margin; review that one first. This PR's own diff is the six commits on top of it. Most HR-Ready waits in the E2E suites gave up before the release they were waiting on could fail its install. cozystack-api gives every release it generates a 600s install timeout, and the Chainsaw asserts in front of those releases sat at 5 or 6 minutes. When an install genuinely failed, the suite reported a bare assert timeout against a release that was still installing, and the reason the install failed, which the release records when its own timeout fires, was never reached. Red runs failed just as often as they should; they only said less. I raised every HR-Ready wait above the install timeout of its own release, by the 60s start margin #4468 introduces: - Chainsaw asserts on releases cozystack-api generates go to 11m, across bucket, clickhouse, etcd, external-dns, foundationdb, harbor, mongodb, openbao, opensearch, postgres, qdrant, rabbitmq, redis, valkey and vminstance. - Asserts on nested `<name>-system` releases take their floor from the template that renders them: 11m for external-dns, openbao and qdrant (10m `spec.timeout`), 16m for `harbor-test-system` (15m). `harbor-test` sat exactly on its 600s floor, which expires first because the assert's clock starts before the install's; it goes to 11m too. - The kuberture Package release takes its 10m from cozystack-operator and goes to 11m. - In the backup walkthroughs, the bucket waits go from 300s to 660s in clickhouse, kafka, mariadb, mongodb, postgres, rabbitmq and redis, and the two postgres waits go from 360s to 660s. The kafka bucket wait passed no budget and silently took the helper's 300s default, which is why earlier scans for literal budgets missed it. Each Chainsaw op that runs a walkthrough grows by exactly what its waits gained (clickhouse 78m, kafka 46m, mariadb 48m, mongodb 42m, postgres 66m, rabbitmq 48m, redis 36m), so a failure at any later wait keeps the headroom it had and is still reported by the script instead of being cut off by the op. Two waits stay as they are, on purpose: - kafka: `kafka-rd` sets a 7h install timeout, which no budget inside the 215-minute E2E job can clear. The kafka assert and the Kafka waits in the kafka and kafka-metadata walkthroughs keep their numbers, and the assert now says why in-line. - `vm-instance-test`: `vm-instance-rd` disables the install wait, so its release goes Ready as soon as helm has applied and there is no floor to clear. Earlier lists of this problem included it by mistake. The cost falls on red runs only: a suite whose release never goes Ready now spends up to six more minutes before it reports, and in exchange the report carries the release's own reason. Green runs are unchanged. Suites run one after another in a single job capped at 215 minutes, so a systemic failure that reddens many suites now reaches that cap sooner, and a job cancelled at the cap loses its log and `cozyreport.tgz`. I took that trade because a failed install that says why is worth more than a red run that ends a few minutes earlier. `docs/agents/e2e-testing.md` stopped being true in several places once the waits were raised: it described most of the tree as under the floor, mariadb as the only suite raised, the walkthrough bucket waits as under it, and harbor's asserts as sitting on and under theirs. I rewrote those sentences, and the one that pointed at "every wait above the band" of 5-6m, kept the start-margin wording from #4468, and added that a walkthrough wait passing no budget takes the helper's default. The comments in `hack/mariadb-first-boot-waits.bats` that quoted the old op and bucket figures are restated; its existing checks and constants are unchanged, because every op grew by exactly its bucket delta and each remainder still sits at exactly its constant. I also added postgres to the table that file holds the walkthroughs to, so its raised waits and its op cannot drift back apart unnoticed, and a test that holds each table suite's bucket wait to the bucket release's floor, read from the api-server default with `bucket-rd` checked for an override; the op check alone could not catch a bucket wait dropping back, since a lower wait only widens the slack. redis stays out: its walkthrough waits on no redis HelmRelease, only on the bucket and on the sentinel master, so the table's floor check has nothing to read there. How I checked it, since E2E cannot run locally: - I re-ran the enumeration from the sizing section of `docs/agents/e2e-testing.md` over every `hack/e2e-chainsaw/*/chainsaw-test.yaml` and every `wait_hr_ready` call under `examples/backups/`, literal or default, and held each against its own release's floor. Every `Ready=True` assert and every `wait_hr_ready` call a suite drives now clears its floor, except the kafka ones named above. The opt-in etcd round-trip uses bare `kubectl wait hr` calls at 300s instead of that helper; they are listed in #4470. The `Ready=False` assert on the rejected redis upgrade is not a readiness wait and is left out. - `make bats-unit-mariadb-first-boot-waits` passes, 31 of 31. For the new postgres row, putting the op back to 50m fails only the postgres slack test, and a postgres wait at 659s fails only the postgres floor test. Setting any one table suite's bucket wait to 659s fails only the bucket test, naming only that suite, and an install-timeout override on `bucket-rd` fails it too. I ran the mutations under plain `bats` so a fail-fast runner could not hide a second failure. - `hack/select-e2e.sh` over this diff selects bucket, clickhouse, etcd, external-dns, foundationdb, harbor, kafka, kuberture, mariadb, mongodb, openbao, opensearch, postgres, qdrant, rabbitmq, redis, valkey and vminstance. Out of scope, and tracked in #4470: the tenant addon waits in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` (an edit there escalates the whole E2E run), the redis and kafka-metadata `wait_hr_ready` helpers that print no reason on timeout, and the opt-in etcd round-trip's bare `kubectl wait` calls. Known and left for later, none of them changed here: - Only the mariadb Chainsaw asserts and the walkthroughs in the guard's table are held by a test. The other raised asserts, and the redis and kafka bucket waits, rest on literals with in-line reasons; #4470 covers extending the guard. - The Kafka waits in `examples/backups/kafka/04-create-kafka.sh`, `07-restore-to-copy.sh` and `kafka-metadata/run-all.sh` stay at 300s against the 7h floor without an in-line reason at the call site. The doc's rule asks that of Chainsaw asserts only, and the kafka Chainsaw assert carries it. The postgres waits are the ones #3748 is about. Their budget now clears the install-timeout floor and a test holds it there; what a CNPG first boot costs on this platform is still unmeasured, so that issue stays open. Closes #3771 Closes #3753 ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff. It touches E2E suites, backup walkthrough scripts, a unit-lane bats file and an agent doc, none of which any downstream repository restates. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(tests): HelmRelease readiness waits in the E2E suites and backup walkthroughs now outlast the install timeout of the release they wait on, so a failed install reports its own reason instead of a bare timeout. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Extended readiness and backup round-trip test timeouts across multiple services to accommodate slower startup and provisioning. * Added checks to verify that bucket readiness waits meet configured installation-timeout requirements. * **Documentation** * Updated testing guidance to clarify how readiness waits relate to release-specific timeouts and overall test budgets. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
hack/mariadb-first-boot-waits.batslet two regressions of the wait budgets it guards pass green. This closes both. The change is test and doc only: no wait, op or chart value moves.A wait now has to clear its floor by a named
MBW_START_MARGINof 60s, not by one second. The wait's clock starts before the install's by the Application reconcile, the chart artifact fetch and the helm-controller queue. Nothing in the tree bounds those three, so the value is a policy choice and the header says so. It equals the headroom every current wait already carries (660 against a 600s floor), which is why no value has to change. A fixture pins 660 against 600 as passing and 659 and 601 as refused. The sizing guide indocs/agents/e2e-testing.mdasked only for "strictly above" the floor, which the guard now rejects, so it now asks for a start margin and points at the constant instead of restating the number.The mongodb, rabbitmq and clickhouse walkthrough waits and their Chainsaw ops are now held to the same contract as mariadb, through one table with a row per suite. Each suite gets its own floor test and its own op test, so a regression turns only that suite's tests red. The floor comes from the api-server's install-timeout default, and each suite's ApplicationDefinition is checked for an override. The startup-probe half is computed only for mariadb, since the other three charts set no
startupProbe. Each op must keep the remainder it had before its waits were raised, and that raise preserved it: 900s for mariadb and rabbitmq, 1200s for mongodb, 2700s for clickhouse. The remainder is taken over every HelmRelease wait the op reaches, the bucket wait included, so raising a wait together with its op keeps the remainder instead of turning the raise into slack. The mongodb target wait sits behind theSKIP_RESTORE=1its op passes, so it is not reached and is left out; the test turns red once the op stops passing it.The Chainsaw HR-Ready asserts of mongodb, rabbitmq and clickhouse (6m) stay out of scope. They sit under the floor by per-suite choice, and the header says so.
Verification:
make bats-unit-mariadb-first-boot-waitspasses, and the same file passes under dash throughhack/cozytest.sh, as CI runs it.SKIP_RESTORE=1dropped from the mongodb op: only the mongodb op test fails.examples/backups/clickhouse/: the clickhouse floor and op tests fail, because the waits are read from every script in the directory rather than from a listed subset.sort -u, fails only the floor fixture test.MBW_START_MARGINset to 0: only the two fixture tests that encode the margin fail. Deleting the constant fails every test that reads it, as an unbound variable.hack/select-e2e.shselects no suite for this diff.Left for later:
hack/mariadb-first-boot-waits.batsnow guards four suites. I kept the name because renaming it changes its make target and every reference to it, which belongs in a change of its own.Closes #4418
Screenshots
Not a UI change.
Downstream repositories
Release note
Summary by CodeRabbit