ci: shared per-run build + nightly-warmed builder image for PR CI - #418
ci: shared per-run build + nightly-warmed builder image for PR CI#418fengttt wants to merge 6 commits into
Conversation
PR CI compiles the same head commit twice: the standalone pessimistic job builds natively with -cover and the compose proxy job repeats the whole build (thirdparties + full Go graph) inside docker. Add a build-mo.yaml reusable workflow that builds the -cover mo-service once and uploads the self-contained runtime tree (mo-service, lib/, dict/, cgo .so) as a run-scoped artifact. Both BVT workflows gain an optional build_artifact input: when set they download the artifact, verify its head sha and build flavor, and skip their own build; on any miss or mismatch they build from source exactly as before, so this lands safely before the matrixone entrypoint passes the input. The compose job assembles its runtime image from the artifact via optools/images/Dockerfile.prebuilt when the head ships it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every PR build recompiled all PR-invariant inputs from scratch: the Go module graph, the full Go build cache, and the C thirdparties. Add a ci-builder job to the nightly image workflow that bakes those into matrixorigin/matrixone:ci-builder (Dockerfile.ci-builder in matrixone), and teach build-mo.yaml to mount the PR checkout into that image and run make build there: only packages the PR changed recompile, and go build incrementally downloads modules newer than the baked cache, so a stale image only costs speed, never correctness. Measured locally: unchanged tree ~20s, worst-case exported-API change to pkg/container/types ~54s, versus ~10 minutes cold. Building inside the ubuntu-22.04 toolchain container also makes the binary match the matrixorigin/ubuntu:22.04 runtime image glibc by construction. When the image pull fails (not yet published, registry outage) the job falls back to the existing from-source build on the runner, so this is safe to land before the first nightly image exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The UT job (ci.yaml) and coverage-UT job compile the full tree from scratch every run: ~10 minutes of go-test compilation plus a C thirdparties build. The nightly ci-builder image now warms exactly those compile flavors (-race -tags matrixone_test, and covermode=set with the verbatim coverpkg list), and the Go build cache is location-independent for pure-Go packages, so both jobs gain a best-effort seed step: pull the image, extract the build/module caches into the runner's cache paths, and stash the prebuilt thirdparties for restoration after make clean (which wipes them; go clean -testcache only invalidates cached test results, never compiled objects). prepare_coverage_cgo.sh honors the same MO_PREBUILT_THIRDPARTIES seed so make cgo stops rebuilding the C thirdparties. The module cache extracts via a tar stream with --no-same-permissions because its read-only directory layout breaks plain docker cp. Every seed path is guarded and continue-on-error: no docker, no image, or a failed pull just leaves the job building cold as today. Also give the shared build job environment: ci so the optional Docker Hub credentials resolve, and extend the ci-builder nightly timeout for the two extra warm flavors. Verified locally against a checkout at a different path with the host toolchain: full ./... race-flavor test compile in 3m18s warm (1m11s rerun floor), make cgo in ~11s with seeded thirdparties. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extend the nightly ci-builder to both CI architectures via a build matrix, published per-arch and combined under one multi-arch manifest tag so every consumer pulls matrixorigin/matrixone:ci-builder and gets its own architecture. The arm64 leg warms what the SCA job compiles (plain build + the full static-check pass), and the SCA job gains the same guarded seed step as the UT jobs, additionally restoring golangci-lint's analysis cache. Mirror all ci-builder tags to ACR (registry.cn-shanghai.aliyuncs.com) and make every pull site prefer the ACR mirror: the SCA and coverage-UT runners are self-hosted in Shanghai/Guangzhou where Docker Hub is slow or unreliable. The ACR attempt is time-bounded (300s) so a slow cross-region pull from GitHub-hosted runners cannot eat the savings, with Docker Hub as the fallback and a cold build as the final fallback. Both registry logins are optional secrets and best-effort. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes at head 176f2059. The shared-once build direction is sound, but the producer/cache owner still has three correctness and security blockers.
-
[P0] Keep untrusted PR compilation outside the privileged credential boundary. The current matrixone caller is a
pull_request_targetworkflow usingsecrets: inherit. This reusable job addsenvironment: ci, checks out the fork head with the supplied token and default persisted credentials, logs into registries, and then executes the PR-controlledMakefile. The host cold path can directly read runner credential state, and the shared build does not need privileged secrets to validate a public PR. Please run head compilation in an unprivilegedpull_requestcontext with no inherited/environment secrets andpersist-credentials: false, or split trusted cache preparation from untrusted compilation so credentials and untrusted code never share a runner. The CI workflow should make that trust contract explicit rather than relying on every caller to remember it. -
[P1] Do not restore
thirdparties/installwithout exact input compatibility.build-mo.yaml,ci.yaml, andcoverage-ut.yamlcopy the nightly native tree into the PR checkout unconditionally. The thirdparty file targets do not depend on all source archives, Makefiles, flags, compiler/base-image identity, target OS/arch, and branch state, so a PR changing native inputs can test the nightly main implementation and go falsely green. The smallest safe first rollout is to seed only Go module/build caches. If native reuse is retained, require an exact manifest/fingerprint covering every native input and toolchain dimension and rebuild on any mismatch. -
[P1] Close fallback inside the shared-build producer. The caller makes both BVT consumers hard
needsdependents. If the cacheddocker runor builder image itself is stale/broken, this job fails and both consumers are skipped; their missing-artifact fallback is unreachable. Treat the cached/container build as best effort, remove all partial outputs after failure, set up the checkout Go version, and run the existing clean host build. Publish only after one path succeeds; a real cold compile failure should still fail once and block both consumers.
Before calling the speedup proven, please also report end-to-end measurements including compressed builder-image size and pull time on both GitHub ubuntu-22.04 westus and the China self-hosted pools. Bound both ACR and Docker Hub pulls; today ACR may consume 300 seconds and the Docker Hub fallback has no explicit bound, so a cache outage can make the optimized path slower than the baseline.
Suggested acceptance closure: fork PR cannot access inherited/environment/checkout/registry credentials; native-input change and divergent release branch force a native rebuild; forced cached-build failure completes through a clean cold build; produced artifact is SHA/flavor/platform-validated and smoke-tested by both standalone and compose consumers.
aptend
left a comment
There was a problem hiding this comment.
Requesting changes at head 176f205.
-
[P0] Keep untrusted PR code outside the privileged credential boundary. The caller is pull_request_target with secrets: inherit; this workflow checks out the PR head, logs into registries, and then executes the PR-controlled Makefile. Move PR compilation to an unprivileged pull_request workflow with no inherited/environment secrets and persist-credentials: false, or strictly separate trusted cache preparation from untrusted compilation.
-
[P1] Do not restore thirdparties/install without exact compatibility validation. The native targets lack source/config/toolchain prerequisites, so a PR changing native archives, headers, flags, or a release branch diverging from main can test the nightly main artifacts and falsely pass. Seed only Go caches, or require an exact fingerprint covering native inputs, branch, toolchain, OS/arch, base image, and build flags.
-
[P2] Close fallback inside the shared-build producer. MatrixOne#27077 makes both BVT consumers hard needs dependents, so a producer failure skips them before their cold-build fallback can run. Treat cached/container failures as best effort, clean partial outputs, and retry once with a clean host build while preserving genuine PR compile failures.
Please also isolate and clean Docker credentials; raw docker login persists credentials in runner Docker state that later PR-controlled steps can use.
…ache, contained fallback Review fixes for #418 / matrixone#27077 (XuPeng-SH, aptend, LeftHandCold): [P0] No credential may share a runner with PR-controlled code. The shared build job drops environment: ci and every registry secret; all ci-builder pulls (build-mo and the three seed steps) are now anonymous and the checkout no longer persists its token. If anonymous ACR pulls are not permitted, provision a pull-only credential at the runner/node level via infrastructure — never via workflow-level docker login, which persists credentials in runner docker state that later PR-controlled steps can read. [P1] The prebuilt thirdparties/install tree now restores only on an exact fingerprint match (schema, os, arch, git rev-parse HEAD:thirdparties — the tree hash covers every archive, header, the Makefile with its flags, and the download script) at all four consumers; any mismatch clears the seed and builds native deps from source. Go module/build caches remain best-effort acceleration, which is always correctness-safe. [P2] Fallback now lives inside the producer: the cached container build is best-effort (continue-on-error) with ownership repair and a loader smoke test; on any failure the job sets up the checkout's Go toolchain and runs the existing make clean cold build, and packaging (with a final env -u LD_LIBRARY_PATH smoke test) runs after either path. A consumer skip via needs now only happens on a genuine compile failure, which is the intended behavior. Also per review: every runtime DSO ships under lib/ ($ORIGIN/lib), the prebuilt-cgo/ contract is gone, and the Docker Hub fallback pull is time-bounded like the ACR attempt so a registry outage cannot make the optimized path slower than the cold baseline (layers are content- addressed, so a timed-out ACR attempt still counts toward the Docker Hub attempt). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks @XuPeng-SH @aptend — all points addressed at head 8b18024 (with matrixone-side e417582532): [P0] Credential boundary. The shared build job now holds no credentials at all: [P1] Native cache compatibility. [P2] Fallback containment. The cached container build is now best-effort inside the producer: Pull bounds. Both the ACR and Docker Hub pulls are bounded at 300s each. Layers are content-addressed, so a timed-out ACR attempt still counts toward the Docker Hub attempt of the same image; worst case a full registry outage adds a bounded ~10 min before the cold path, typical is 1–3 min. Artifact contract. All runtime DSOs consolidated under Measurements. Local: incremental 🤖 Generated with Claude Code |
aptend
left a comment
There was a problem hiding this comment.
Requesting changes at head 8b18024 after re-review.
[P1] Preserve the cached build's failure status before running the loader smoke test. The step starts with set -uo pipefail (without -e), captures docker run in status, and evaluates [ "${status}" -eq 0 ], but a false test does not stop the script. If make build produces mo-service and then fails a later validation, the subsequent ./mo-service -h can succeed, making the entire step successful. The host cold-build fallback is then skipped and the artifact is published despite the failed build target.
Please explicitly exit on nonzero status after the ownership repair, for example:
if [ "${status}" -ne 0 ]; then
exit "${status}"
fiThe previous credential and native-cache blockers are otherwise addressed at this head: registry pulls are anonymous with non-persisted checkout credentials, and native restoration is fingerprint-gated.
…ge producer, locality-routed pulls, Go-caches-only host seeding Re-review fixes for #418 / matrixone#27077: [P1, aptend] The cached-build step ran without set -e, so its bare status test could not stop the script: a failed docker run whose leftover mo-service still passed the loader smoke test would mark the step successful, skip the cold fallback, and publish an artifact from a failed build. Explicitly exit with the docker run status right after the ownership repair. [P1, XuPeng-SH] The producer no longer accepts any caller secret: the TOKEN_ACTION declaration and checkout token are gone (public anonymous checkout with the job's read-only default token), and the workflow caps permissions to contents: read so the trust contract is enforced here rather than remembered by callers. The matrixone caller drops secrets: inherit in tandem. [P1, XuPeng-SH] Route registry pulls by runner locality via RUNNER_ENVIRONMENT: GitHub-hosted runners try Docker Hub first and the Shanghai ACR second, self-hosted pools the reverse; each attempt stays bounded at 300s and completed layers carry across attempts. When both fail, prune partially pulled layers so they cannot starve the cold build of disk. [P2, XuPeng-SH] Host jobs (UT, SCA, coverage-UT) now seed Go module/build caches ONLY — the smaller safe first rollout. Reusing prebuilt native libraries outside the builder container would need a producer/consumer toolchain-ABI fingerprint (CC, cmake, base-image identity), which schema/os/arch/tree-hash does not capture across different runner images. The container path in this workflow keeps fingerprint-gated native reuse because producer and consumer toolchains are the same image by construction. prepare_coverage_cgo.sh reverts to its original form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re-review round addressed at head 2c39853 (matrixone side ad96d0c882): [P1] Cached-build failure propagation (@aptend, head 8b18024). Confirmed — without [P1] Producer credential boundary (@XuPeng-SH #2). The producer now accepts no caller secrets at all: the [P1] Registry routing & disk (@XuPeng-SH #3). Pulls now route by [P2] Native-cache toolchain dimensions (@XuPeng-SH #4). Adopted your smaller safe first rollout: UT/SCA/coverage-UT now seed Go module/build caches only — the content-addressed Go cache is always correctness-safe — and native deps keep building from source on those hosts; Process/canary (@XuPeng-SH #1). Agreed: #27077 stays draft; after this PR merges I'll dispatch the image workflow, then run a canary on the exact #27077 head — including a forced cached-build failure — and post per-pool compressed-size/pull/build timings plus cache-hit/miss/registry-unavailable outcomes here before it leaves draft. 🤖 Generated with Claude Code |
What this PR does / why we need it:
Cuts the repeated cold compilation out of MatrixOne PR CI. Tracking issue: matrixorigin/matrixone#27076.
build-mo.yaml(new reusable workflow) — builds the-covermo-service once per run and uploads the self-contained runtime tree (~99MB artifact). It builds inside the nightlyci-builderimage when reachable (only PR-changed packages recompile; the binary links against the ubuntu-22.04 toolchain, matching the runtime image glibc by construction), and falls back to a from-source runner build otherwise.e2e-standalone-parallelande2e-compose-parallelaccept an optionalbuild_artifactinput: download, verify head sha + flavor, and skip their own ~10-minute builds; any mismatch falls back to building from source.optools/images/Dockerfile.prebuilt(matrixone side) assembles the compose runtime image from the artifact in seconds.-race -tags matrixone_test;-covermode=setwith the verbatim coverpkg list; the full static-check pass on arm64). Guarded,continue-on-errorseed steps extract the Go build/module caches (and golangci-lint's cache for SCA) onto the runner; the Go build cache is location-independent for pure-Go packages so tests keep running on the host exactly as today.prepare_coverage_cgo.shhonors a seeded thirdparties dir somake cgostops rebuilding the C thirdparties.ci-builderimages — added toimage-build.yaml: amd64+arm64 matrix, combined under one manifest tag, pushed to Docker Hub and mirrored to ACR; every pull site prefers the ACR mirror (time-bounded) because the SCA/coverage runners are in Shanghai/Guangzhou. Skipped on release events and on branches without the Dockerfile.Merge order: this PR is a safe no-op on its own (all new inputs optional, all seeds degrade to cold builds) and MUST merge before the matrixone-side PR that wires
entrypoint.yamltobuild-mo.yaml.Measured locally: incremental
-coverbuild 20s–1min vs ~10min cold; full race-flavor test compile ~3min warm;make cgo~11s with seeded thirdparties.🤖 Generated with Claude Code