Repository navigation
ci: build the Azurite and Storage Upgrade engine graphs once - #898
azimafroozeh merged 1 commit into
Conversation
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve, subject to the required CI checks and the Azure integration result. I found no blocking defect at bc4924cf5df7ccec30ec3c9d8bc83d5d2e6519f7.
This PR stops two test jobs from repeatedly compiling the same engine dependencies. Previously, each test owner selected its own packages and features. Those selections changed the dependency build and consumed much of the job's time limit. The jobs now use the existing workspace selection and choose individual test targets within it. The storage-upgrade log checker also accepts the empty results from unrelated, filtered test binaries.
The required contract is complete, failing-on-error qualification on hosted Linux, including runners with no cache. The Azure job must exercise its configured storage and admission paths. The upgrade job must test genuine predecessor data and every required upgrade owner. These obligations matter more than a shorter green job.
The change preserves those obligations by inspection:
- The Azure selection keeps every named owner, exact test filtering, required success lines, and the final rejection of unconfigured skips.
set -euo pipefailstill propagates Cargo failures throughtee. - The upgrade checker still requires every expected case to pass. It accepts only one successful summary with positive coverage. Additional summaries must have zero passed, failed, ignored, and measured tests. Missing cases, ignored cases, and two executing binaries still fail.
- The genuine predecessor build, released CLI checks, required context, and missing-predecessor refusal remain unchanged.
Tradeoffs and liability: this fixes the package-selection cause through Cargo's existing feature rules. It adds no storage mechanism or production API. Azure gains one shared shell selection. Upgrade validation extends its existing owner instead of adding another checker. The 44 net added lines therefore reduce operational liability despite increasing source size.
The broader selection has a cost. --lib compiles 16 workspace test harnesses, although only the named owner executes tests. These jobs now depend on unrelated workspace crates compiling. Azure server and CLI tests also share the workspace failpoint features instead of their former separate selections. Test names remain covered, but the former feature combinations do not. This is an acceptable exchange for one maintained build configuration. Five similar jobs should reuse that configuration, rather than create five new package lists.
Local validation from the clean review checkout:
- All 13 storage-upgrade guard tests passed. I ran the existing multi-summary regression against the old validator: it failed as expected. It passed with the new validator. No source edits were needed.
- Generated Linux-target Cargo build plans for all ten distinct target selections. Each selected engine, Lance, DataFusion, Arrow-array, and Tokio build has the same recursive dependency signature across those plans. The supervisor target does not need the engine. This checks build resolution, not elapsed build time.
- Checked the actual manifests and pinned Lance 11.0.0 feature declarations. The shared engine enables
failpoints, with nodstfeature. The shared Lance build includes the workspace's additional backend features. - Workflow guards, applicable self-tests, spelling, and all 217 documentation files passed.
Exact-head CI passed the workflow guard, Azure integration, and the complete Storage Upgrade Compatibility job. The Azure log shows a 21m53s first build and later target builds of at most 23.80s. It shows one Tokio and one Lance compilation. This supports build reuse on that runner. I did not run Azurite or rebuild the full Rust workspace locally. Cache-independent timing and disk costs remain unverified here. One passing run does not establish a 90-minute bound.
What & why
Azurite Azure Integrationruns its eleven cargo calls under six per-crate package selections, and each selection resolves its own feature graph: four of them carry the Lance stack (the engine withfailpoints, cluster, server and the CLI differ on the engine'sfailpointsand tokio'sio-std, so tokio and everything above it recompiles between them). That is 89 minutes on main's push run and a cancellation at the 90-minute budget on #896's run, still compilingomnigraph-engine.Storage Upgrade Compatibilityruns three-p omnigraph-engine --features failpointsscopes after its--workspacecrossversion scope, a third graph at about 20 minutes warm. Both break the cache rule indocs/dev/ci.md(a second cargo invocation in a job must not change the dependency graph), the same mechanism as #755 and #896.Every cargo call in each job now selects the
Test Workspacepackages,--workspace --exclude omnigraph-gqt --exclude omnigraph-dst --features "$FAILPOINT_FEATURES", and an owner names only its target (--lib, or--test <file>). Each job resolves one graph, the oneTest Workspacealready builds, so a cold run is a single engine build.scripts/check-storage-upgrade-ci.pypins the scope commands, so they move with the workflow; its log validator accepts the all-zero summaries that a workspace selection prints for binaries that filtered to nothing and still requires exactly one summary that executed tests; the four old spellings are self-test rows, so a revert of the selection fails the guard.docs/dev/ci.mddescribes the shared selection in the Storage Upgrade paragraph, the Azurite bullet and the cache rule.Backing issue / RFC
Checklist
docs/dev/ci.mdLocal verification
Run from the branch's tree on macOS, with the
Test Workspaceworld built bycargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-dst --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints --no-run:python3 scripts/check-storage-upgrade-ci.py --self-test— 13 tests OKpython3 scripts/check-storage-upgrade-ci.py— Storage upgrade CI OKpython3 scripts/check-ci-cells.py --self-test— self-test ok, 49 required test names definedpython3 scripts/check-classify-copy.py,scripts/check-workflow-action-pins.py,scripts/check-merge-group-triggers.py --self-test,scripts/check-change-classes.py --self-test— all oktypos .github/workflows/ci.yml docs/dev/ci.md scripts/check-storage-upgrade-ci.py— clean--check-log protocolOK--workspace … --lib db::upgrade::tests) — 16 lib binaries, one executed (50 passed, 384 filtered out), fifteen all-zero summaries;--check-log engineOK--test failpoints azure_write_detached_pin_survives_next_write -- --exact) — bothfailpointsbinaries run, cluster's 0 passed / 8 filtered, engine's 1 passed (skips without Azurite, stillok)--lib tests::contract_suite_azure_when_configured -- --exact) — every lib binary runs filtered, the storage one reportsokscripts/check-docs.py— not run (pinned doc-tooling packages not installed locally)Feature-graph evidence:
cargo tree --locked -e features -f "{p} {f}" --prefix noneper selection ona951b0ea, diffed per package: storage 265 packages, azure-admission 299, engine+failpoints 1184, cluster 1059, server 1142, cli 1174, all pairwise different; the shared selection 1307.Notes for reviewers
failpointsis a test target of both the engine and the cluster crate, so the three engine owners also run the cluster binary at zero matching tests;require_cellreads the engine'sokline.--workspace --libcompiles every member's unit-test harness once (workspace crates only, no dependency rebuild); that is the price of one graph for the two--libowners and the engine scope.