Repository navigation
ci: run Linux Go jobs on ARC default - #196
Conversation
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Runner-level evidence read from this head's own CI runs (push 41226779662, pull_request 41226801488) and the last green main run on ubuntu-latest (40441617252).
Reviewed head: 75501b7
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit: tests / native-codex]
.github/workflows/ci.yml:192— movingtestto ARCdefaultturns the BLO-47285 shared-port veth guard into a guard that always passes.- The step's own comment (
ci.yml:301-303) and the test (listen_multicast_sharedport_linux_test.go:72-74) both say it only catches a regression on Linux 6.9+, where UDP early demux marks the socket prefetched. Below 6.9 the kernel delivers correctly with or without the fix, so the test passes either way. - Measured: this head's
testjob on ARC ran on6.8.0-147-generic. Its control line readsIP_MULTICAST_ALL=0, udp_early_demux on: per-socket packets [200 0 0 0 0], which is correct delivery with no misdelivery to catch. The last greenmainrun onubuntu-latestran on6.17.0-1022-azure, where the same control showed the misdelivery ([0 0 0 0 200]). So the step is green both times, but only the hosted run was actually testing anything. - Fix: keep
test(or just the veth step, split into its own job) onubuntu-latest, or make the step fail whenuname -ris below 6.9 so a non-testing kernel shows red instead of green. The other five moved jobs (race,cgo-test,mobile-typecheck,cross-build,iwa-unit) have nothing kernel-dependent that I found. The PR body's "no host OS features" claim holds for those five, but not for thisteststep.
- The step's own comment (
Suggestions (2)
- [pr-review-toolkit: comments]
.github/workflows/ci.yml:31— the header comment says "all four jobs are ubuntu-latest"; that is stale after this change. - [pr-review-toolkit: comments]
.github/workflows/ci.yml:450— "Rust is preinstalled on GitHub-hosted ubuntu runners" now describes a runnercgo-testno longer uses; thecommand -v cargoassertion is still the right guard, just reword the rationale to the ARC image.
Strengths
- Small, mechanical diff limited to
runs-on. The setup actions stay, so the jobs do not depend on what happens to be installed on the runner image. - The workflow's "prove it ran" guards (PASS-line greps, file-selection asserts) worked as intended: they would turn a skip on the new runner red. On ARC the sudo + user/net-namespace veth step, IPv6 loopback, and multicast-on-
loguards all reported PASS.mobile-typecheck,iwa-unit,race,cross-buildandtestare green on both runs;cgo-testwas still in progress at review time.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
75501b7 to
5914d77
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior finding checked against .github/workflows/ci.yml fetched at this exact head.
Reviewed head: 5914d77
Looks good. test stays on ubuntu-latest, so the BLO-47285 veth guard still runs on a 6.9+ kernel where it can actually catch a regression. The other five jobs move to ARC default, and none of them depend on the kernel.
Prior Findings Dispositioned (1)
- prior:75501b7 important 1 — fixed —
.github/workflows/ci.yml:192-194—testis back onruns-on: ubuntu-latest, and a comment says why: the shared-port veth guard needs a privileged host network and Linux 6.9+. ARC runs on6.8.0-147-generic, where the guard passes with or without the fix. The moved jobs arerace,cgo-test,mobile-typecheck,cross-buildandiwa-unit(lines 321, 432, 954, 980, 1001).
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [pr-review-toolkit: comments]
.github/workflows/ci.yml:31— the header still says "all four jobs are ubuntu-latest". Now onlytestis. - [pr-review-toolkit: comments]
.github/workflows/ci.yml:452— "Rust is preinstalled on GitHub-hosted ubuntu runners" describes a runnercgo-testno longer uses. Thecommand -v cargoassertion is still the right guard; only the rationale needs updating for the ARC image.
Strengths
- The diff only touches
runs-onand keeps the setup actions, so the moved jobs don't rely on whatever happens to be installed on the runner image. - The new comment on
testrecords the kernel constraint, so a later sweep that moves everything to ARC won't silently make the guard always pass. - At review time
race,cgo-test,mobile-typecheck,cross-buildandtestwere still running at this head. The merge gate covers them.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Summary
Move the Linux-only Go/Rust/Node CI jobs to the existing ARC
defaultpool:testracecgo-testmobile-typecheckcross-buildiwa-unitThe macOS workflow and hosted-only/public-vantage, Docker, and release jobs remain unchanged.
The ARC runner image provides Go, Rust, Node/npm, and the setup actions remain in place. These jobs require no services, host OS features, public network vantage, or publish credentials.
Validation
actionlintwith the repository's customdefaultrunner label configured (existing shellcheck findings are pre-existing and ignored by the check)git diff --checkgo test ./...was started but exceeds the local validation window; CI should run the full matrix.