perf: reduce E2E CI runtime via parallel jobs and test tuning#420
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CI workflow now splits unit, cluster, and E2E execution. E2E and service-update tests use shared test settings and subtests, cluster mode-change tests run in parallel, and the related docs and comments were updated. ChangesCI and Test Workflow
E2E Test Behavior
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
.circleci/config.yml (1)
74-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding failure-debug archiving to
test_cluster, matchingtest_e2e.
test_e2earchives debug output and logs on failure (Lines 97-106), buttest_clusterhas no equivalent step, so cluster-test failures will be harder to diagnose from CI artifacts alone.♻️ Proposed addition for debug parity
test_cluster: executor: common steps: - common_setup - run: name: Run cluster tests command: | make docker-swarm-init make start-local-registry make buildx-init make test-cluster CONTROL_PLANE_IMAGE_REPO=172.17.0.1:5000/control-plane TEST_RERUN_FAILS=2 + - store_artifacts: + path: <cluster-debug-output-dir> - store_test_results: path: .🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.circleci/config.yml around lines 74 - 87, Add failure-debug archiving to the test_cluster job so it matches the debug behavior in test_e2e. Update the test_cluster steps in the CircleCI config to include the same kind of on-failure artifact collection used by the test_e2e job, placing it around the cluster test run so CI retains logs/debug output when make test-cluster fails.e2e/db_create_test.go (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the checkpoint conf literal to a shared test helper.
The same
PostgresqlConfmap (checkpoint_timeout: "30s",checkpoint_completion_target: "0") is duplicated here and twice more ine2e/add_replica_test.go. A shared helper/var reduces risk of the values drifting between call sites, since tests likely depend on identical checkpoint tuning to make replication assertions reliably fast.♻️ Proposed refactor (e.g., in a shared e2e helpers file)
+var fastCheckpointConf = map[string]any{ + "checkpoint_timeout": "30s", + "checkpoint_completion_target": "0", +}Then reference
fastCheckpointConfat each call site instead of the inline literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/db_create_test.go` around lines 65 - 68, The checkpoint PostgresqlConf literal is duplicated across this test and the replica tests, so extract it into a shared e2e helper/variable and reuse it at each call site. Define a single shared config such as fastCheckpointConf in the e2e helpers area, then update the relevant setup in db_create_test.go and add_replica_test.go to reference that shared value instead of inline maps. Keep the values identical everywhere so the test tuning stays consistent.e2e/add_replica_test.go (1)
41-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate checkpoint conf literal between create and update requests.
Same concern as
db_create_test.go: the identicalPostgresqlConfmap is repeated in the initial create (lines 41-48) and the update request (lines 88-91). Keeping the create/update specs in sync is intentional here (declarative spec), but a shared constant/var would make that intent explicit and prevent accidental divergence during future edits.Also applies to: 88-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/add_replica_test.go` around lines 41 - 48, The same PostgresqlConf literal is duplicated between the initial create and the later update request in the add replica test, so make the shared checkpoint settings explicit by extracting them into a single named constant or variable and reusing it in both request specs. Update the create/update blocks in the test around the replica spec so they both reference that shared definition, keeping the declarative intent clear and preventing future divergence.e2e/service_provisioning_test.go (2)
305-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover commented-out code.
//Version: "1.0.0",is a dead debug artifact next to the activeVersion: "latest".🧹 Cleanup
- //Version: "1.0.0", Version: "latest",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/service_provisioning_test.go` around lines 305 - 306, Remove the leftover commented-out Version assignment next to the active Version field in the provisioning test setup; the commented `Version: "1.0.0",` is dead code and should be deleted so the struct literal around the active `Version: "latest"` stays clean.
252-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
DatabaseSpecliteral across create/Add/Remove.The same
DatabaseSpecfields (name, users, port, nodes) are repeated three times, differing only inServices.postgrest_test.goalready solves this with apostgrestBaseSpechelper — consider the same pattern here to keep future spec changes (e.g. new required fields) in sync across all three call sites.♻️ Example refactor
func updateDatabaseServiceSpec(host1 string, services []*controlplane.ServiceSpec) *controlplane.DatabaseSpec { return &controlplane.DatabaseSpec{ DatabaseName: "test_update_database_service", DatabaseUsers: []*controlplane.DatabaseUserSpec{ { Username: "admin", Password: pointerTo("testpassword"), DbOwner: pointerTo(true), Attributes: []string{"LOGIN", "SUPERUSER"}, }, }, Port: pointerTo(0), PatroniPort: pointerTo(0), Nodes: []*controlplane.DatabaseNodeSpec{ { Name: "n1", HostIds: []controlplane.Identifier{controlplane.Identifier(host1)}, }, }, Services: services, } }Then callers become
Spec: updateDatabaseServiceSpec(host1, nil),Spec: updateDatabaseServiceSpec(host1, []*controlplane.ServiceSpec{...}), andSpec: updateDatabaseServiceSpec(host1, []*controlplane.ServiceSpec{}).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/service_provisioning_test.go` around lines 252 - 360, The same DatabaseSpec setup is duplicated in the create, Add, and Remove paths, so future schema changes will be easy to miss. Extract the repeated database fields into a helper like updateDatabaseServiceSpec and have the existing CreateDatabaseRequest and db.Update calls reuse it, varying only the Services argument. Use the existing database test setup around db.Update, CreateDatabaseRequest, and ServiceSpec to keep the spec consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.circleci/config.yml:
- Around line 74-87: Add failure-debug archiving to the test_cluster job so it
matches the debug behavior in test_e2e. Update the test_cluster steps in the
CircleCI config to include the same kind of on-failure artifact collection used
by the test_e2e job, placing it around the cluster test run so CI retains
logs/debug output when make test-cluster fails.
In `@e2e/add_replica_test.go`:
- Around line 41-48: The same PostgresqlConf literal is duplicated between the
initial create and the later update request in the add replica test, so make the
shared checkpoint settings explicit by extracting them into a single named
constant or variable and reusing it in both request specs. Update the
create/update blocks in the test around the replica spec so they both reference
that shared definition, keeping the declarative intent clear and preventing
future divergence.
In `@e2e/db_create_test.go`:
- Around line 65-68: The checkpoint PostgresqlConf literal is duplicated across
this test and the replica tests, so extract it into a shared e2e helper/variable
and reuse it at each call site. Define a single shared config such as
fastCheckpointConf in the e2e helpers area, then update the relevant setup in
db_create_test.go and add_replica_test.go to reference that shared value instead
of inline maps. Keep the values identical everywhere so the test tuning stays
consistent.
In `@e2e/service_provisioning_test.go`:
- Around line 305-306: Remove the leftover commented-out Version assignment next
to the active Version field in the provisioning test setup; the commented
`Version: "1.0.0",` is dead code and should be deleted so the struct literal
around the active `Version: "latest"` stays clean.
- Around line 252-360: The same DatabaseSpec setup is duplicated in the create,
Add, and Remove paths, so future schema changes will be easy to miss. Extract
the repeated database fields into a helper like updateDatabaseServiceSpec and
have the existing CreateDatabaseRequest and db.Update calls reuse it, varying
only the Services argument. Use the existing database test setup around
db.Update, CreateDatabaseRequest, and ServiceSpec to keep the spec consistent in
one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 215e7ad3-043a-4600-b3ae-c03869f3a14e
📒 Files selected for processing (5)
.circleci/config.ymle2e/add_replica_test.goe2e/db_create_test.goe2e/postgrest_test.goe2e/service_provisioning_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.circleci/config.yml (1)
93-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
go/install+set_gopath+attach_workspaceboilerplate across three jobs.
unit_lint,test_cluster, andtest_e2eeach repeat the identical 3-step sequence (pluscommon_setupalso pins the same Go version), so the Go version now appears hardcoded in 4 places. Consider extracting a shared command (e.g.attach_project) to avoid version drift when bumping Go.♻️ Suggested consolidation
+ attach_project: + steps: + - go/install: + version: '1.25.8' + - set_gopath + - attach_workspace: + at: /home/circleciThen in each job:
unit_lint: executor: common steps: - - go/install: - version: '1.25.8' - - set_gopath - - attach_workspace: - at: /home/circleci + - attach_project(repeat similarly for
test_clusterandtest_e2e)Also applies to: 107-111, 126-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.circleci/config.yml around lines 93 - 97, The setup for `unit_lint`, `test_cluster`, and `test_e2e` repeats the same `go/install`, `set_gopath`, and `attach_workspace` sequence, and `common_setup` hardcodes the same Go version too. Extract that repeated bootstrap into a shared CircleCI command or reusable step (for example, an `attach_project` command) and have each job invoke it, so the Go version and workspace setup are defined in one place and stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.circleci/config.yml:
- Around line 44-52: The Go workspace path is inconsistent between the setup
cache/workspace and the GOPATH export, so downstream jobs won’t reuse the same
downloaded modules and installed tools. Update the CircleCI config so the
`persist_to_workspace` path, `use_mod_cache`, and `set_gopath`/`GOPATH` all
reference the same workspace directory, keeping the `go_workspace` path
consistent across jobs.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 93-97: The setup for `unit_lint`, `test_cluster`, and `test_e2e`
repeats the same `go/install`, `set_gopath`, and `attach_workspace` sequence,
and `common_setup` hardcodes the same Go version too. Extract that repeated
bootstrap into a shared CircleCI command or reusable step (for example, an
`attach_project` command) and have each job invoke it, so the Go version and
workspace setup are defined in one place and stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff903f68-23ad-465d-b4b7-0b1874e94c0d
📒 Files selected for processing (1)
.circleci/config.yml
| - run: | ||
| # GOPATH must be explicit and identical across every job so that | ||
| # go_workspace/pkg/mod and go_workspace/bin (persisted from the | ||
| # setup job) resolve to the same paths the tools were installed | ||
| # to and the modules were downloaded to. Without this, GOPATH | ||
| # defaults to whatever the machine image's ambient default is, | ||
| # which may not match the persisted workspace paths. | ||
| name: Set GOPATH | ||
| command: echo 'export GOPATH=/home/circleci/.go_workspace' >> $BASH_ENV |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for other/prior references to go_workspace / .go_workspace to confirm the canonical directory name.
rg -n 'go_workspace|GOPATH' .circleci/config.ymlRepository: pgEdge/control-plane
Length of output: 689
🏁 Script executed:
#!/bin/bash
sed -n '30,95p' .circleci/config.ymlRepository: pgEdge/control-plane
Length of output: 2246
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' .circleci/config.ymlRepository: pgEdge/control-plane
Length of output: 6831
Align the Go workspace paths across cache, workspace, and GOPATH. persist_to_workspace restores go_workspace/..., but use_mod_cache and set_gopath point at /home/circleci/.go_workspace/..., so the downstream jobs won’t reuse the modules/tools populated in setup. Use one directory name everywhere.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.circleci/config.yml around lines 44 - 52, The Go workspace path is
inconsistent between the setup cache/workspace and the GOPATH export, so
downstream jobs won’t reuse the same downloaded modules and installed tools.
Update the CircleCI config so the `persist_to_workspace` path, `use_mod_cache`,
and `set_gopath`/`GOPATH` all reference the same workspace directory, keeping
the `go_workspace` path consistent across jobs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.circleci/config.yml (1)
88-90: 🧹 Nitpick | 🔵 TrivialCombined parallelism (3 containers ×
E2E_PARALLEL=4) may increase contention.Each of the 3 parallel containers additionally runs 4 concurrent Go test workers (
E2E_PARALLEL=4), for up to 12 concurrent e2e test executions total across the workflow. If e2e tests share limited local resources (ports, docker-compose network/volumes) within a container this is fine since compose stacks are per-container, but worth a sanity check that no shared external service (e.g. a shared registry, DB, or rate-limited endpoint) is contended by all 3 containers simultaneously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.circleci/config.yml around lines 88 - 90, The test_e2e job is running with both CircleCI parallelism and E2E_PARALLEL, which multiplies total concurrent e2e load and may contend on shared external resources. Review the test_e2e configuration in .circleci/config.yml and either reduce parallelism or make the e2e setup isolate any shared dependencies; check the executor/common job setup and the E2E_PARALLEL usage together so the total concurrency stays safe for any registry, DB, or rate-limited endpoint.Makefile (1)
122-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate
--junitfilehere.gotestsumalready adds--junitfile $@-results.xmlin CI, so this target passes the same flag twice in CircleCI. Drop the explicit flag to avoid redundant CLI args.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 122 - 132, The test-e2e target in Makefile is passing a duplicate --junitfile flag because gotestsum already injects the results file in CI. Remove the explicit --junitfile option from the test-e2e recipe and keep the rest of the gotestsum invocation unchanged so the target no longer forwards the same CLI argument twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.circleci/config.yml:
- Around line 88-115: The e2e test selection step can fail before the skip check
when circleci tests split returns no files, because the TEST_FILES/TEST_NAMES
pipeline in the test_e2e job runs under bash -eo pipefail. Update the “Select
this container's e2e tests” run block to handle the empty split case first, and
only build E2E_RUN from TEST_NAMES when TEST_FILES is non-empty; otherwise leave
TEST_NAMES empty so the later “Run e2e tests with Docker Compose” step can
safely skip. Use the existing TEST_FILES, TEST_NAMES, and E2E_RUN logic in
.circleci/config.yml as the place to apply the guard.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 88-90: The test_e2e job is running with both CircleCI parallelism
and E2E_PARALLEL, which multiplies total concurrent e2e load and may contend on
shared external resources. Review the test_e2e configuration in
.circleci/config.yml and either reduce parallelism or make the e2e setup isolate
any shared dependencies; check the executor/common job setup and the
E2E_PARALLEL usage together so the total concurrency stays safe for any
registry, DB, or rate-limited endpoint.
In `@Makefile`:
- Around line 122-132: The test-e2e target in Makefile is passing a duplicate
--junitfile flag because gotestsum already injects the results file in CI.
Remove the explicit --junitfile option from the test-e2e recipe and keep the
rest of the gotestsum invocation unchanged so the target no longer forwards the
same CLI argument twice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da27184d-0bc7-4a89-8ad1-eeb25e689527
📒 Files selected for processing (2)
.circleci/config.ymlMakefile
Split the monolithic CircleCI test job into 3 concurrent jobs (unit_lint, test_cluster, test_e2e), then split test_e2e itself across 3 parallel containers using circleci tests split. Total CI wall-clock dropped from ~44min to ~18min (~59% reduction). Also: - Tune checkpoint_timeout/checkpoint_completion_target for tests that add a replica, cutting TestAddReplica's runtime by ~37% (measured: 276s -> 173s). - Consolidate TestUpdateDatabaseAddService/RemoveService into one test with Add/Remove subtests sharing a database, since the update behavior (not creation) is what's under test. - Fix flaky-test timeouts: spock.wait_for_sync_event and WAL-replay poll windows were too tight under CI load; the post-host-removal sleep and host-count wait in TestForcedHostRemovalWithDatabase were similarly tight. - Add JUnit output to test-e2e so circleci tests split can balance containers by historical timing instead of file count. PLAT-660
moizpgedge
force-pushed
the
Feat/PLAT-660/Reduce-E2E-runtime-by-consolidating-or-using-parallelization
branch
from
3f729e7 to
35f883a
Compare
moizpgedge
changed the title
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Nice work! This looks like a huge improvement. Sorry for the confusion around the job names - I was only suggesting basic_check as an alternative for the make ci job because it's doing several different types of tests and checks. I also have one very small suggestion in the Makefile, but otherwise, this looks really great!
| jobs: | ||
| test: | ||
| # basic_check_1: unit tests + lint + license check (make ci) | ||
| basic_check_1: |
There was a problem hiding this comment.
I think the name basic_check without the _1 makes sense here. What do you think?
There was a problem hiding this comment.
Changed this to basic_check
because other are renamed.
| path: . | ||
|
|
||
| # basic_check_2: multi-host cluster integration tests (clustertest/) | ||
| basic_check_2: |
There was a problem hiding this comment.
I thought your original name, test_cluster was appropriate here, since this is for the make test-cluster target.
| path: . | ||
|
|
||
| # basic_check_3: end-to-end tests (e2e/), split across 3 parallel containers | ||
| basic_check_3: |
There was a problem hiding this comment.
I think your original name, test_e2e was a better fit.
There was a problem hiding this comment.
sure i can change it
| $(gotestsum) \ | ||
| --format-hide-empty-pkg \ | ||
| --format standard-verbose \ | ||
| --junitfile test-e2e-results.xml \ |
There was a problem hiding this comment.
I don't think this change is necessary. We automatically add the --junitfile argument on line 62 where we set gotestsum. If you look in the log output from the latest CircleCI run, you'll see that this argument is getting set twice now:
/home/circleci/.go_workspace/bin/gotestsum --junitfile test-e2e-results.xml \
--format-hide-empty-pkg \
--format standard-verbose \
--junitfile test-e2e-results.xml \
--rerun-fails=2 \
--rerun-fails-max-failures=4 \
--packages='./e2e/...' \
-- \
-tags=e2e_test -count=1 -timeout=45m -parallel 4 -run "^(TestAddNodeOriginAdvanced|TestCancelDatabaseTask|TestUpdateAddNode23|TestUpdateAddNode12|TestMinorVersionUpgrade|TestPortChange|TestScripts|TestWholeCluster)$" -args -fixture ci -debugRename basic_check_1/2/3 back to basic_check/test_cluster/test_e2e per review - the numbered generic names lost the descriptive mapping to their make targets. Also remove the explicit --junitfile flag in test-e2e, since the gotestsum variable already adds it automatically for CI runs and it was being set twice. PLAT-660
2 participants
PLAT-660 PR Summary
What this does
This PR makes the CI build much faster. It used to take about 44 minutes and now takes about 18 minutes. That is roughly 59 percent faster. The main idea was to run things at the same time instead of one after another, plus some smaller fixes to slow and flaky tests.
What changed
The CI used to run everything in one big job. Now it runs in 3 separate jobs at the same time called Basic check 1, Basic check 2, and Basic check 3.
Basic check 3 (the e2e tests) was the slowest part, so that job itself is now split across 3 machines running at the same time instead of one machine running everything.
Some database tests were waiting on a slow setting called a checkpoint. Lowering that setting made one test almost 40 percent faster. We checked other tests too and found this trick only helps tests that add a replica to an already running database, not tests that create everything fresh, so we only applied it where it actually helps.
Two tests that were doing similar setup work were combined into one test with two steps, since the setup itself was not the important part being tested.
A few tests were flaky because they were not given enough time to finish under load. We gave them more time so they pass reliably instead of randomly failing and needing a retry.
We also fixed a small bug in the Makefile where certain test filters would break if they included special characters.
How we tested it
We ran the full test suite locally against a real cluster and everything passed. We also triggered several real CI runs on this branch to confirm each change actually works, not just in theory. The final run showed all jobs passing in about 18 to 19 minutes.
Things to know
The Basic check 3 split relies on a CircleCI feature that only works inside their actual system, so it could only be tested by running it for real, not locally. The balance between its 3 machines will get better automatically over the next few runs as the system learns how long each test actually takes. Right now if Basic check 1 fails, the other two heavier jobs still run instead of stopping early. This uses a bit more compute time but keeps the speed benefit of running everything at once.