ci: run and lint the live-chaos harness, path-gated - #254
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Both findings were right, and the P1 one caught a claim I had written into the comment without checking. P1 — filter closure. I asserted that a change to The closure is the whole workspace, and the omissions were the load-bearing ones: Now This does mean the suite runs on most Rust changes (~5 min). That is the honest cost of it being the only thing validating replication and fencing against real processes. P3 — upload gating. Correct, and the reasoning is exactly right: Answering the open question from the PR description: the namespace step works. This PR's own CI run executed all 13 scenarios in 1m43s, all passing — including 05, 05b, 06 and 07, which cannot run on macOS and had therefore never executed anywhere. The I checked the raw job log rather than trusting the green check, because 1m43s looked too fast. It is genuine: scenario 03 really does bring up three metadata nodes, catch up a learner, change membership and kill a node in 1.5s on this hardware, with sub-second Raft timers. My local machine is simply slower. That check also turned up a defect in my own change: the log artifact collected nothing, because scenarios delete workdirs they generate and |
f5fbbff to
217f974
Compare
fd9fd19 to
46dda2c
Compare
217f974 to
e19ecef
Compare
The live-chaos harness drives real processes over real TLS — metadata Raft, replication, fencing, promotion, failover — and it has never run in CI. No workflow referenced it. It executed only when someone ran it by hand. That gap is not theoretical. Scenario 09 sat red on main from #236, scenario 10 from #237, a data node presented a metadata node's certificate to the admin endpoint across three issues, and scenario 01 failed on a scheduling race that appeared and vanished between runs. Every one of those was found by running the suite manually this week, not by CI, and each had been on main for weeks. The harness was not linted either: the `shell` filter listed `docker/*.sh` and `docker/tests/**`, so editing `lib.sh` — which all thirteen scenarios source — triggered nothing at all. Both are now covered: * `shell` gains `scripts/live-chaos/**`, and a second shellcheck step lints the harness with `-x -P scripts/live-chaos/scenarios` so the scenarios' dynamic `source` resolves. Without `-P`, every scenario reports SC1091 plus spurious "may not be assigned" warnings for variables lib.sh does define — noise that trains people to ignore the job. The tree is clean under it. * A new `chaos` filter and `live-chaos` job run the suite. Scoped, not blanket: only the harness itself and the four crates whose binaries it executes can change a scenario's outcome, so a docs or dashboard edit does not pay for a cluster. Debug binaries, because these scenarios assert correctness rather than throughput. Unprivileged user and mount namespaces are enabled and then PROBED. Ubuntu 24.04 restricts unprivileged userns by default, and scenarios 05, 05b, and 06 need it. The probe does not skip on failure: these scenarios fail with a remediation message rather than skipping, so a runner without namespaces turns the job red honestly instead of reporting success for three scenarios that never ran — which is exactly the silent gap this job exists to close. Per-scenario logs upload on failure. A failing scenario names a log file, and without the artifact that diagnosis dies with the runner. Enabled only now because the suite had to be green first: #251, #252, and #253 fixed the three real failures. Local state is 9 of 13 passing, with the four remaining failures all requiring Linux facilities absent on macOS — `unshare` for 05, 05b, 06 and a clock shim for 07 — which is what this job exists to exercise. Scenario 01 was run five times after the race fix: 5 of 5.
`pull_request: branches: [main]` meant a stacked PR ran no CI whatsoever. This project stacks slices — each bases on the one below — so those PRs carried a single check, an AI reviewer's comment, and nothing else. They could be reviewed, approved, and merged with no build, no tests, and no lint. Nor did merging the parent repair it. Retargeting a PR's base fires the `edited` activity type, which is not in the default `pull_request` event set, so the retargeted PR did not reliably re-trigger CI either. The first real signal arrived on push to main — after the merge, on the branch that is supposed to stay green. The path filters already scope each run, so the added cost is only the jobs a stacked slice actually needs. This is the same shape as the gap the previous commit closes: a check that exists and is trusted, but never runs on the change it is meant to guard.
The upload step collected nothing. Scenarios delete workdirs they generated themselves, so by the time the step ran the `/tmp/vtop-chaos.*` tree was gone — and `if-no-files-found: ignore` turned that into a silent success. The first run confirmed it: the job passed and produced no artifact at all. A diagnostic that appears to exist and is absent exactly when it is needed is worse than none, because it stops anyone from looking for a real one. Supplying CHAOS_WORKDIR fixes both halves: the logs land in a known path, and because the harness only cleans up directories it generated itself, providing one suppresses the cleanup. `if-no-files-found` becomes `error`, so a future break in this wiring fails the job instead of quietly returning to collecting nothing. Upload is now `if: failure()` — the logs are for diagnosing a failed run, and uploading them on every green run is storage for nobody. Verified by running a scenario with CHAOS_WORKDIR set and confirming the log tree survives.
…he upload correctly
Two review findings, both correct.
The chaos filter named four crates and asserted that nothing else could change
a scenario's outcome. That assertion was false. The two binaries this job runs
pull in every workspace crate:
cargo tree -p vtop-node --edges normal
→ broker, log, meta, observe, protocol
cargo tree -p vtop-cli --no-default-features --edges normal
→ adapters, broker, core, log, meta, observe, protocol, state, upload
The omissions were not marginal. vtop-log owns the segment format the
durability and recovery scenarios assert on, and vtop-protocol owns the
produce/fetch wire every scenario speaks — either could have broken the suite
with this job never running, which is precisely the hole this job exists to
close. The filter is now `crates/**`, derived from the closure rather than
from a guess about it.
That means the suite runs on most Rust changes. That is the honest cost of it
being the only thing validating replication and fencing against real
processes; a cheaper gate with a hole in it is what was just removed.
The log upload was gated on `failure()`, which is also true when the namespace
probe or the build failed. In those cases the suite never reached a scenario,
so no logs exist, and `if-no-files-found: error` would stack a spurious "no
files found" on top of the actual cause. It now keys on the run step's own
outcome, so the empty-artifact signal only fires once a run was actually
attempted — which is the only situation in which it means anything.
46dda2c to
1ac27f6
Compare
1 participant
Top of the stack: #250 → #251 → #252 → #253 → this. Bases retarget automatically as each merges.
The live-chaos harness drives real processes over real TLS — metadata Raft, replication, fencing, promotion, failover — and it has never run in CI. No workflow referenced it. It ran only when someone ran it by hand.
What that cost
Every one of these was found by running the suite manually this week, and each had been sitting on
main:vtopctl meta statusfails on any fresh node)The harness wasn't linted either: the
shellfilter listeddocker/*.shanddocker/tests/**, so editinglib.sh— which all thirteen scenarios source — triggered nothing.What this adds
Lint.
shellgainsscripts/live-chaos/**, plus a second shellcheck step using-x -P scripts/live-chaos/scenariosso the scenarios' dynamicsource "$(dirname …)/../lib.sh"resolves.-Pis load-bearing: without it every scenario reports SC1091 and spurious "may not be assigned" warnings for variableslib.shdoes define — noise that trains people to ignore the job. The tree is clean under it as of #253.Run. A
chaosfilter andlive-chaosjob. Scoped, not blanket: only the harness and the four crates whose binaries it executes (vtop-meta,vtop-broker,vtop-node,vtop-cli) can change a scenario's outcome, so a docs or dashboard edit doesn't pay for a cluster. Debug binaries — these scenarios assert correctness, not throughput.Two deliberate choices worth reviewing
Namespaces are enabled, then probed — and the probe does not skip. Ubuntu 24.04 (which
ubuntu-latestnow is) restricts unprivileged userns by default, and 05/05b/06 need it. Those scenarios fail with a remediation message rather than skipping, so a runner without namespaces turns this job red honestly instead of reporting success for three scenarios that never ran. Reporting green on unrun scenarios is the exact failure mode this job exists to close, so I did not want a skip path.Logs upload on failure. A failing scenario names a log file; without the artifact that diagnosis dies with the runner.
Verification
actionlint(repo's pinned image): cleanunsharefor 05/05b/06, a clock shim for 07 — which is precisely what this job is for. I expect this job to be the first time those four have ever executed.Note
This lands last deliberately: enabling CI on a failing suite just makes
mainred. #251, #252, and #253 fix the three real failures first.I have not been able to verify the namespace step on a real runner from here — that is the one thing this PR's own CI run will answer.
Summary by cubic
Run the live-chaos harness in CI and lint it, path-gated to relevant changes, and run CI on every pull request. This catches cluster regressions early on stacked PRs and keeps the harness scripts clean.
chaospath filter and alive-chaosjob that runsscripts/live-chaos/run-all.sh; now triggers onscripts/live-chaos/**,crates/**,Cargo.toml,Cargo.lock,.cargo/**,rust-toolchain*, and.github/workflows/ci.yml; builds debug binaries.CHAOS_WORKDIRso logs persist, upload only if the run step fails, and fail the job if the artifact is empty.shellfilter toscripts/live-chaos/**and added a shellcheck step with-x -P scripts/live-chaos/scenariosto resolve dynamic sources.Written for commit 1ac27f6. Summary will update on new commits.