ci: smoke-test the computing-unit cluster (master boot + worker join)#6719
ci: smoke-test the computing-unit cluster (master boot + worker join)#6719mengw15 wants to merge 1 commit into
Conversation
Automated Reviewer SuggestionsBased on the
|
3bcebab to
7d0a8fa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6719 +/- ##
============================================
- Coverage 74.65% 74.65% -0.01%
Complexity 3438 3438
============================================
Files 1160 1160
Lines 45793 45793
Branches 5069 5069
============================================
- Hits 34189 34188 -1
Misses 9953 9953
- Partials 1651 1652 +1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ No material benchmark regressions detected🟢 2 better · 🔴 0 worse · ⚪ 13 noise (<±5%) · 0 without baseline
Baseline detailsLatest main
Raw CSVconfig_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,354.00,200,128000,565,0.345,17356.82,23616.18,23616.18
1,100,10,64,20,1672.70,2000,1280000,1196,0.730,82393.19,114640.19,114640.19
2,1000,10,64,20,13923.97,20000,12800000,1436,0.877,698670.09,730690.60,730690.60 |
7d0a8fa to
2a7025a
Compare
mengw15
changed the title
2a7025a to
52e89d2
Compare
Nothing in CI verified that an amber worker node can join a running cluster: the standalone computing-unit-master boot check formed only a 1-node self-seeded cluster, and the packaged computing-unit-worker launcher was never started at all. A regression in the worker's join path -- its Pekko remoting/serialization, seed-node config, or startActorWorker itself -- would break the deployed worker while build, unit tests, and the boot checks all stay green. The worker can't reuse smoke-boot.sh: it runs no HTTP server and binds a random Pekko remote port, so there is no fixed port to probe, and booted standalone it just retries its seed and stays up (so "did not crash" proves nothing). Its real health signal is cluster membership. Replace the standalone master boot step with a single cluster check. smoke-cluster.sh boots a computing-unit-master (non-cluster mode self-seeds pekko://Amber@localhost:2552) and a no-arg computing-unit-worker (joins :2552, skipping the getNodeIpAddress/checkip.amazonaws.com lookup that only fires with --serverAddr, so no external network), then asserts the worker JOINS by waiting for ClusterListener's "2 nodes in the cluster" readiness line on the master -- a deliberate signal, not a crash-scan (contrast apache#6332). Because it boots and waits for the master first, it still catches a apache#6204-class linkage crash on master startup (reported as "master exited before listening"), so the separate smoke-boot master step it supersedes is redundant. It fails fast if either JVM dies and reaps both on exit. Wired into amber-integration after the texera-web boot, reusing the same dist; ubuntu-only. test_smoke_cluster.sh drives the harness end-to-end with fake launchers, asserting the specific verdict message for each outcome so a deleted fast-fail branch turns the test red. Closes apache#6523.
52e89d2 to
6d76dcb
Compare
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 1 must-fix · 5 advisory · 0 polish — one stale comment claims a verification the script deliberately skips; the rest are small robustness/maintainability notes on an otherwise solid, well-tested harness.
Correctness (3)
.github/workflows/build.yml:648— comment claims a self-join verification the script deliberately skips; the PR description carries the same clause (must-fix, see inline).github/scripts/test_smoke_cluster.sh:70— unbounded waiter subshell leaks as an orphan when the happy-path case fails (advisory, see inline).github/scripts/test_smoke_cluster.sh:147— the "never listens" timeout branch is the only verdict branch without a message-asserted case (advisory, see inline)
Design & architecture (3)
.github/scripts/smoke-cluster.sh:57— resolve/probe/reap helpers duplicated from smoke-boot.sh with a cross-pointer on only one of the three (advisory, see inline).github/scripts/smoke-cluster.sh:55— ClusterListener's log line is now a CI contract with no pointer at the Scala site (advisory, see inline).github/scripts/smoke-cluster.sh:135— crash paths drop the child exit code that smoke-boot.sh deliberately reports (advisory, see inline)
Verification trace
Verified the harness's assumptions against source: a no-arg worker seeds pekko://Amber@localhost:2552, binds canonical.port = 0, and never reaches the checkip.amazonaws.com lookup (AmberRuntime.scala:96-112); the grep phrase is a verbatim substring of ClusterListener.scala:145-147's log line; leader-actions-interval = 10s (cluster.conf:69) confirms the rationale for not keying on the master's self-join line — which is also what refutes the build.yml comment's "verifies it self-joins" claim. The infra job's test_*.sh discovery (build.yml:1161) picks up the new suite automatically, and this PR's own CI ran both the real cluster check (amber-integration, ubuntu) and the 6-case suite (infra, ubuntu + macos) green.
| # listen on :8085, and verifies it self-joins its 1-node cluster before the | ||
| # worker is launched, so a #6204-class linkage crash on master startup is |
There was a problem hiding this comment.
This clause claims a verification the script deliberately skips: smoke-cluster.sh's NB (lines 143-148) says it does not wait for the master's own "1 nodes" self-join line because keying on it caused a false failure — the only gate before the worker launches is the port probe. The #6204-class crash is still caught, just by the port-wait. The PR description carries the same stale clause; worth updating both.
| # listen on :8085, and verifies it self-joins its 1-node cluster before the | |
| # worker is launched, so a #6204-class linkage crash on master startup is | |
| # listen on :8085 before launching the worker (deliberately not keying on | |
| # the master's own "1 nodes" self-join line -- see the NB in the script), | |
| # so a #6204-class linkage crash on master startup is |
|
|
||
| PHRASE_JOINED='Now we have 2 nodes in the cluster' | ||
|
|
||
| # Resolve a (globbed) launcher to exactly one executable -- same contract as |
There was a problem hiding this comment.
resolve_launcher, port_open, and the TERM-grace-KILL reap are near-verbatim copies of smoke-boot.sh's, and only this one carries a cross-pointer. A future fix to the probe or the reaper (the ::1-vs-IPv4 fix these comments encode is exactly that class) lands in one script and silently misses the twin. Can we extract a small sourced smoke-lib.sh for the shared trio? That's where I'd lean now that there are two full copies; failing that, matching pointers at port_open and the reap keep the next editor honest.
| join_timeout="${4:-40}" | ||
| master_ready_timeout="${SMOKE_MASTER_READY_TIMEOUT:-60}" | ||
|
|
||
| PHRASE_JOINED='Now we have 2 nodes in the cluster' |
There was a problem hiding this comment.
ClusterListener.scala:145-147 now backs a CI contract, but nothing at the Scala site says so — whoever rewords that log line only finds out from a red CI run (the drift hint helps them recover, but after the fact). A one-line comment at the log statement pointing at this script would close the loop.
| for ((i = 0; i < master_ready_timeout; i++)); do | ||
| if port_open; then ready=1; break; fi | ||
| if ! alive "$master_pid"; then | ||
| echo "::error::smoke-cluster: master exited before listening on :$http_port"; dump_logs; exit 1 |
There was a problem hiding this comment.
smoke-boot.sh reaps the dead child and reports "exited on boot (exit code $code)" — its comment calls the exit code "the signal". Here (and in the worker branch at line 164) the code is dropped, so a log-silent death (an OOM SIGKILL's 137) leaves a 120-line dump with no cause in it. code=0; wait "$master_pid" 2>/dev/null || code=$? before the error line restores it.
| [[ "$self" == y ]] && echo "echo '$PHRASE_SELF'" | ||
| case "$trigger" in | ||
| n) : ;; | ||
| wait:*) echo "( while [ ! -f '${trigger#wait:}' ]; do sleep 0.2; done; echo '$PHRASE_JOINED' ) &" ;; |
There was a problem hiding this comment.
If the happy-path case ever fails with the worker never touching $sig — exactly the mutation this case guards — this waiter outlives the run and busy-polls forever: smoke-cluster's cleanup kills the exec'd python, not this pre-exec fork. Bounding the poll (e.g. for _ in \$(seq 1 600) around it) keeps a red run from leaking an orphan on a dev machine.
| "master readiness log drift -> FAIL (timeout + drift hint)" "readiness log may have changed" \ | ||
| "$work/m_drift" "$work/w_sleep2" "$port" 3 | ||
|
|
||
| # --- master crashes before it listens -> FAIL (assert the pre-listen message) --- |
There was a problem hiding this comment.
The master-alive-but-never-listening branch (smoke-cluster.sh:140, "did not listen ... within") is the only verdict branch without a message-asserted case, and the header's slow-timeout-masking argument applies to it too. A fake master that stays alive without binding anything (an exec sleep 120 launcher, like the workers) plus a small SMOKE_MASTER_READY_TIMEOUT covers it in a few lines.
3 participants
What changes were proposed in this PR?
Nothing in CI verified that an amber worker node can join a running cluster. The standalone
computing-unit-masterboot check (#6377) forms only a 1-node self-seeded cluster,texera-webdoesn't touch clustering, and the packagedcomputing-unit-workerlauncher (bin/computing-unit-worker) is never started at all. A regression in the worker's join path — its Pekko remoting/serialization, seed-node config, orstartActorWorkeritself — would break the deployed worker while build, unit tests, and the boot checks all stay green.The worker can't reuse
smoke-boot.sh:ComputingUnitWorker.mainonly callsAmberRuntime.startActorWorker, which runs no HTTP server and bindspekko.remote.artery.canonical.port = 0(a random ephemeral port), so there is no fixed port to probe. Booted standalone it just retries its seed and stays up, so a "did not crash" check proves nothing — the worker's real health signal is cluster membership..github/scripts/smoke-cluster.sh— a self-contained harness that boots acomputing-unit-master(non-cluster mode self-seedspekko://Amber@localhost:2552and serves Dropwizard on:8085) and a no-argcomputing-unit-worker(defaults its seed tolocalhost:2552and skips thegetNodeIpAddress→checkip.amazonaws.comlookup that only fires with--serverAddr, so no external network), then asserts the worker actually JOINS — waiting forClusterListener'sNow we have 2 nodes in the clusterreadiness line on the master (a deliberate signal, not a crash-scan; contrast smoke-boot.sh false positive: jOOQ "tip of the day" text matches the crash regex and fails boot checks at random #6332). It fails fast if either JVM dies and reaps both on exit.smoke-cluster.shboots the master, waits for it to listen on:8085, and verifies it self-joins its 1-node cluster before launching the worker — so a texera-web fails to start after Hadoop 3.4.3 upgrade: NoClassDefFoundError from pjfanning jersey-json #6204-class linkage crash on master startup is still caught (reported asmaster exited before listening). The separatesmoke-boot.shmaster step (ci: add a computing-unit-master boot smoke test in amber-integration #6377) is a strict subset of this, so it's removed rather than run redundantly. Wired intoamber-integrationafter thetexera-webboot, reusing the amber dist already built there.ubuntu-latestonly..github/scripts/test_smoke_cluster.sh— drives the harness end-to-end with fake launchers (6 cases), asserting the specific verdict message for each outcome so that deleting a fast-fail branch (and letting a case pass only via the slow timeout) turns the test red. The happy path ties the join signal to the worker actually running, so it can't pass if the worker is never launched. Discovered and run by theinfrajob.Any related issues, documentation, discussions?
Closes #6523, raised from the #6377 review (suggested by @Yicong-Huang). Supersedes the standalone
computing-unit-masterboot step from #6377 (this cluster check boots and verifies the master too); complements thetexera-webboot (#6319). Smoke rationale from #6220.How was this PR tested?
amber-integration(ubuntu-latest) runs the new step in this PR: the master and worker boot, the worker joins, and the master logs2 nodes in the cluster.test_smoke_cluster.shpasses 6/6 locally on bash 3.2.57 (macOS) and bash 5; mutation-tested — deleting the worker-alive fast-fail, the master-alive fast-fail, or the worker launch each turns the test red;shellcheckclean; deterministic across repeated runs with no leftover processes.AmberRuntime.startActorMaster/startActorWorker: the non-cluster master self-seedslocalhost:2552, and a no-arg worker joins it while skipping thecheckip.amazonaws.comlookup.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-8)