Split the longest e2e tests into concurrent sub-tests - #8254
Open
Amaury Chamayou (achamayou) wants to merge 7 commits into
Open
Split the longest e2e tests into concurrent sub-tests#8254Amaury Chamayou (achamayou) wants to merge 7 commits into
Amaury Chamayou (achamayou) wants to merge 7 commits into
Conversation
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-ci-runtime-under-15-min
branch
from
September 1, 2026 15:10
ec099c3 to
addf9b6
Compare
Amaury Chamayou (achamayou)
marked this pull request as ready for review
September 1, 2026 16:46
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 1, 2026 16:46
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces CI critical-path time by splitting several long-running end-to-end (e2e) tests into smaller sub-tests that can run concurrently within each existing ctest bucket, and by making partitioning infrastructure safe under concurrent execution.
Changes:
- Split
schema_testbackup snapshot download work into multiple concurrent sub-tests. - Hoist two reconfiguration join-snapshot checks into their own concurrent sub-tests (including IPv6 variants).
- Replace
ConcurrentRunner’s fixed-batch threading with a bounded thread-pool model, and update iptables partitioning to use per-instance chains with locking for thread-safety.
Custom instructions used:
.github/copilot-instructions.md
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/schema.py | Registers multiple backup-snapshot download sub-tests instead of one serial sub-test. |
| tests/reconfiguration.py | Removes tail-serial join-snapshot calls from run_all, adds IPv6 wrappers with shared availability check. |
| tests/partitions_test.py | Splits partitions test into concurrent groups, each with its own partitioned network, and caps concurrency. |
| tests/nodes.py | Registers join-snapshot checks as separate sub-tests to run concurrently with other nodes tests. |
| tests/infra/runner.py | Implements bounded concurrency via ThreadPoolExecutor and changes sub-test tracking to support pooling. |
| tests/infra/partitions.py | Makes iptables partitioning concurrency-safe via per-instance chains and a global lock, plus adds cleanup-all support. |
| tests/e2e_operations.py | Refactors backup-snapshot download runner to execute grouped test lists, enabling concurrent grouping upstream. |
| tests/cleanup_iptables.py | Updates cleanup tool to dump/cleanup all leftover per-instance chains. |
Suppressed comments (2)
tests/infra/runner.py:294
- FAILURES is a module-level list populated by threading.excepthook. ConcurrentRunner.run() appends FAILURES into its local failures list but never clears FAILURES at the start of a run, so a previous run's background-thread failure can make a later run fail even if all its sub-tests pass.
Clear FAILURES at the start of run() to make repeated runs in one process reliable.
LOG.configure(**config)
tests = self.tests
tests/infra/partitions.py:127
- Typo in log message: "[unamed]" should be "[unnamed]".
LOG.info(f'Dropping rules "{self.name or "[unamed]"}"')
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The two virtual e2e jobs are the CI critical path, at 15.2 min (bucket_b) and 15.5 min (bucket_c plus partitions) against a 15 min target. Each pays ~5 min of fixed overhead, so the tests have to fit well under 10 min. ctest runs the tests in a bucket one after another, so a bucket costs the sum of its tests' wall times, and each test costs its longest sub-test. Measuring node process lifetimes from the logs of run 33500039436 shows the time is concentrated in a few long serial chains rather than spread out. `partitions` was the worst: 282s, and the only e2e test not using ConcurrentRunner at all. One network ran fourteen tests in sequence, then two further networks ran one after the other. Split it into eight sub-tests, each on its own network, keeping the original relative order within each group. `nodes_test` ran `run_join_old_snapshot` (22s) and `run_join_no_snapshot_against_original_primary` (7s) sequentially at the end of `reconfiguration.run_all`, once for IPv4 and once for IPv6. They each build their own single-node network and share no state with the main chain, so they become four sub-tests of their own. `schema_test` was paced by `download-snapshot` at 104s of its 105s total, a single network running five backup-snapshot-fetch tests in sequence. Each finds the primary and issues its own transactions, so split them into three groups. To run several partitioned networks at once, `Partitioner` now owns a private iptables chain named for the pid and an instance counter, rather than sharing a global `CCF-TEST` chain that every new instance flushed. Its rules already matched only its own nodes' addresses and ports, so co-existing chains do not interfere. libiptc reads, modifies and writes the whole table, so mutations take a process-wide lock. `cleanup_iptables.py` gains `cleanup_all` to remove leftovers from a killed run. ConcurrentRunner now runs sub-tests in a bounded pool rather than in fixed batches, and caps concurrency at half the cores by default. Batching made a cap expensive, because a single long sub-test held back everything queued behind it; a pool starts the next sub-test as soon as any finishes, so the cap costs nothing when a critical path dominates. This is what keeps the extra networks from overwhelming the runner. `partitions` asks for a lower cap, since tests that deliberately isolate nodes and wait for elections are the least tolerant of not getting CPU promptly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24ad6978-a80f-45bc-b73b-268aeaf2b30e
Two problems from the previous commit's CI run. Job A failed ruff's import-order check on tests/infra/runner.py and tests/infra/partitions.py. `infra.*` belongs in its own first-party block after the third-party one, as it was before. This was self-inflicted: ruff was run over a directory holding only the changed files, where `infra` does not resolve as first-party, so its autofix merged the blocks and then agreed with itself. Job C failed with `basic` timing out waiting for an election, 75s after dropping its partition, while three other partitioned networks were running. iptc.easy operates on a table object that is shared process-wide and is committed and refreshed on every call, so interleaved calls from several threads lose updates: one thread's refresh can discard another's pending change, leaving stale DROP rules in place. That is exactly what a node which never rejoins an election looks like. The lock was previously taken per rule, which leaves a partition observable half-applied and does nothing about a concurrent refresh landing mid-set. Hold it across the whole set of rules for a partition, an isolation and a drop, so each appears and disappears in one step. Lower the partitions concurrency from 4 to 2 as well, since these groups are the least tolerant in the suite both of iptables contention and of not getting CPU promptly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24ad6978-a80f-45bc-b73b-268aeaf2b30e
The first measurement of the bounded pool showed recovery_test going from 166.9s to 184.4s. It has nineteen sub-tests and ran unbounded before, so a cap of half the cores was throttling a test that was already comfortable: eight concurrent sub-tests is below what it was doing, not above. The cap exists to bound growth from newly split tests, not to tighten what already worked. Set it to one per core, which leaves recovery_test and the other large pools running as they did, and keep the explicit lower value in partitions_test, whose sub-tests are the ones that actually need it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24ad6978-a80f-45bc-b73b-268aeaf2b30e
Two governance_test nodes outlived their test by about 8.5 minutes in CI, logging roughly 660 lines a minute on a join retry loop through the rest of the bucket. `test_all_members` stops the network it is given and recovers into a new one, which it returns. `gov` rebound its local `network` to that, but the enclosing `infra.network.network` context manager still owns the original, so nothing ever stopped the recovered network. The nodes that leaked were the ones `test_no_quote` and `test_node_data` deliberately add without ever trusting, which is why they sat retrying rather than exiting. Give `gov` explicit ownership of the recovered network and stop it in a finally block. The node logs of the leaked nodes were clean, so this reaps them without newly failing on anything. `partitions_test.run_recovery_elections` has the same shape: `test_recovery_elections` stops both the original and its intermediate network but returns a third that nobody stopped. Fix it the same way, which matters more now that the partitions groups run concurrently and a stray node is a neighbour of a live test rather than of an idle runner. The pattern is wider than these two: any test that recovers into a new network and returns it leaks unless its caller takes ownership, and recovery.py does this in several places. Left alone here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24ad6978-a80f-45bc-b73b-268aeaf2b30e
The iptables chain-name limit was enforced with an assert, which python -O strips, turning a violation into an obscure iptables failure later. Raise instead. ConcurrentRunner.tests was a class attribute, so two runners in one process would share accumulated sub-tests. Move it into __init__ so each instance owns its own list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24ad6978-a80f-45bc-b73b-268aeaf2b30e
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-ci-runtime-under-15-min
branch
from
September 1, 2026 17:22
7de6971 to
9494d70
Compare
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
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.
Summary
The two virtual e2e jobs were the CI critical path. This PR changes no buckets and no workflows — ctest still runs each bucket serially. The tests inside it just stop being serial.
Measured across three consecutive green runs (33527819778, 33531420031, 33537384996):
Critical path: 15.5 -> 14.2-14.6 min. Run-to-run variance is around +/-0.6 min per job; the 14.4 for job C above is mostly that, its
Build Debugstep alone took 3.48 min against a typical 2.9.Where the time went
A bucket costs the sum of its tests' wall times, and each test costs its longest sub-test. Measuring node process lifetimes from the logs of run 33500039436 showed the time concentrated in a few long serial chains:
partitionsConcurrentRunnerat allnodes_testschema_testChanges
partitions. One network ran fourteen tests in sequence, then two further networks ran one after the other. Now eight sub-tests, each on its own network, preserving the original relative order within each group.nodes_test.run_join_old_snapshot(22s) andrun_join_no_snapshot_against_original_primary(7s) ran sequentially at the end ofreconfiguration.run_all, once for IPv4 and once for IPv6. Each builds its own single-node network and shares no state with the main chain, so they become four sub-tests of their own.schema_test. Paced almost entirely bydownload-snapshot: one network running five backup-snapshot-fetch tests in sequence. Each finds the primary and issues its own transactions, so they split into three groups.Partitionermade safe for concurrent networks. It now owns a private iptables chain named for the pid and an instance counter, instead of a globalCCF-TESTchain that every new instance flushed. Its DROP rules already matched only its own nodes' addresses and ports, so co-existing chains do not interfere.iptc.easyoperates on a table object that is shared process-wide and committed and refreshed on every call, so interleaved calls from several threads lose updates; all access takes a lock, held across a whole set of rules so a partition appears and disappears in one step.cleanup_iptables.pygainscleanup_allfor leftovers from a killed run.ConcurrentRunneruses a bounded pool. It previously ran sub-tests in fixed batches, joining each batch before starting the next, which made a cap expensive: one long sub-test held back everything queued behind it. A pool starts the next sub-test as soon as any finishes, so the cap costs nothing when a critical path dominates. The default is one per core, a ceiling on new growth rather than a tightening of what already worked.partitions_testpasses a lower value, since its groups are the least tolerant of both CPU contention and iptables contention.Leaked networks reaped. Two
governance_testnodes outlived their test by ~8.5 min, logging ~660 lines/min on a join retry loop through the rest of the bucket.test_all_membersstops the network it is given and recovers into a new one which it returns, butgovonly rebound its local variable: the enclosing context manager still owned the original, so nothing stopped the recovered network. The nodes that leaked were the onestest_no_quoteandtest_node_datadeliberately add without ever trusting, which is why they sat retrying rather than exiting.govnow owns the recovered network and stops it in afinally.partitions_test.run_recovery_electionshad the same shape and is fixed the same way, which matters more now those groups run concurrently. Cost:governance_test53.3s -> 57.7s,partitionsunchanged at 157.6s.This pattern is wider than these two — any test that recovers into a new network and returns it leaks unless its caller takes ownership, and
recovery.pydoes it in several places. Left alone here.What the intermediate runs taught us
All caught and fixed in this PR rather than guessed at:
recovery_testslower (166.9s -> 184.4s): it has nineteen sub-tests and ran unbounded before, so the cap throttled a test that was already comfortable. Raised to one per core;recovery_testis back to 164.4s.basictiming out waiting for an election 75s after dropping its partition, which is what stale DROP rules look like. Locking per rule was not enough given the shared, auto-refreshing iptables table; the lock now spans a whole rule set, and the groups run two at a time.Coverage and testing
partitionsgroups make exactly the sametest_*calls, in the same order, as the oldrun(), and that both standalone checks are still registered.run_allkeeps its tests minus the two hoisted networks, thatnodes_testregisters the four hoisted sub-tests alongside its original five, that the threedownload-snapshotgroups make the same fivetest_*calls in the same order, and thatgovandrun_recovery_electionsstill call exactly what they did before.ConcurrentRunnerpool against stubbed infrastructure: cap respected and reached, all sub-tests run, thread names still carry the sub-test name (the log format depends on it), failures propagate naming the sub-test,--regexand-Nstill work, a long sub-test does not stall the queue behind it, and two runners in one process do not share sub-tests.ValueErrorrather than asserting, so it survivespython -O.Why this is close to the floor for the current job layout
Job A carries ~12.4 min of work unrelated to e2e buckets, dominated by the Debug build with clang-tidy at 7.9 min. Jobs B and C each pay ~5.35 min of fixed overhead. With the remaining e2e work spread across the three, the best achievable maximum is about 13.8 min.
So further bucket work is worth at most ~0.4 min, and the critical path cannot go much below ~13.9 min without one of:
Remaining test-side headroom
bucket_bis the constraint, and most of it is four tests each bounded by one long sub-test:recovery_test(164s, ~123s sub-test),nodes_test(138s, ~131sreconfiguration.run_all),e2e_logging_http2(73s) andschema_test(73s).Next candidates, deliberately left out:
e2e_logging.do_main_tests, a ~35-test serial chain pacing thecppsub-test of bothe2e_loggingande2e_logging_http2, so splitting it would help both buckets. Buttest_historical_query_rangeandtest_historical_receiptsread transactions accumulated by earlier tests in the chain, so it needs those dependencies untangled rather than a contiguous cut. Worth ~0.3 min, since job A caps the benefit.reconfiguration.run_all. Its node count drifts 3 -> 4 -> 3 -> 2 across the sequence and later tests carry@reqs.at_least_n_nodesconstraints that depend on that drift, so it needs per-group node counts rather than a contiguous cut.