Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
f58c30f to
80d5cb7
Compare
1d49fda to
bf067ab
Compare
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Measured on this PR's own CI run: each CI-image consumer saves ~87 s of image prep, but every consumer now starts ~4 min later, because snapshot-ci-images lives inside the reusable workflow those consumers wait on. Net wall-clock per PR goes up, while runner-minutes go down. One structural change (create the snapshot inside build-ci-images) should turn this into a pure win, so I'd like that resolved before merging.
Snapshot job extends the critical path of every consumer (.github/workflows/ci-image-build.yml:407)
Numbers from this PR's run 37206844037 and a baseline PR run from the same afternoon on the same runner pool, 37210552741, both ubuntu-22.04:
| baseline | this PR | |
|---|---|---|
Build CI linux/amd64 image 3.10 finished |
14:54:13 | 13:55:59 |
| first consumer job started | 14:54:13 (+0 s) | 13:59:55 (+3 min 56 s) |
| consumer CI-image prep, median | 189 s (stash 67 s + load 106 s, n=29) | 102 s (download 59 s + unpack 31 s, n=79) |
Every job in ci-amd.yml that has needs: build-ci-images waits for the whole called workflow, and snapshot-ci-images is part of it; continue-on-error: true changes the conclusion, not the wait. The job spent 178 s of its 229 s restoring and docker image load-ing the stash that build-ci-images had exported seconds earlier, then 25 s snapshotting and 15 s uploading. So the ~87 s saved per consumer is paid for with ~236 s added before any consumer can start. For a PR the end-to-end duration gets roughly 2.5 min longer; what improves is total runner time (79 consumers × 87 s ≈ 115 runner-minutes in this run). The description lists "end-to-end workflow duration" among the metrics to evaluate but does not report it.
Suggestion: create the snapshot as the last step of build-ci-images instead of in a separate job. The image is already in that daemon, so the 178 s reload disappears and the extra critical path shrinks to roughly the 40 s of create + upload. move_docker_to_mnt.sh bind-mounts /mnt/var-lib-docker onto /var/lib/docker, so DockerRootDir still reads /var/lib/docker and check_supported_daemon passes there. Keep the same gate (github.event_name == 'pull_request' && inputs.upload-image-artifact == 'true' && inputs.image-stash-ref == '') and continue-on-error on the step, and place it after "Stash cache mount": create runs docker builder prune --all, which would otherwise wipe the mount cache before it is exported. If the separate job was chosen for isolation reasons, please spell them out in the job comment. In pull_request context the token is read-only either way, and build-ci-images already runs the PR's own sources.
Smaller observations
scripts/ci/docker_data_root_snapshot.sh:53—check_supported_daemonaccepts onlyoverlay2. When GitHub's runner images move to the containerd image store, every snapshot will be rejected, the producer keeps spending its minutes, and nothing surfaces it because the create step iscontinue-on-error. Anecho "::warning::..."on the fingerprint-mismatch and unsupported-store exits would make that visible in the run summary.- The description still says the key is
ci-image-snapshot-v1-*; the code usesci-image-snapshot-v2-. - The script tests drive the real bash through shimmed
docker/sudo/zstd/gitand cover every bail-out path. All 23 pass locally in under a second. Nice approach.
Worth a second look from
This change touches the CI image stash/restore flow; folks with the most context here:
@potiuk— authored 10 of the last 25 commits onci-image-build.ymlandprepare_breeze_and_image/action.yml(the stash andimage-stash-refdesign)@jscheffl— 2 of the last 25 commits on the same files
None of them have been notified — asking any of them for an extra pass is the maintainer's call, and optional.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The findings
below are observations, not blockers; an Apache Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Airflow handles maintainer review:
Contributing guide.
Every job that prepares the CI image runs `docker image load` on the stash, which unpacks and checksums every layer of an 8 GB image again, about two minutes per job. Extracting a copy of the image store into a stopped daemon gives the same image in about half a minute on the same runners. The image stash stays as it is, for other consumers and as the fallback when a snapshot does not fit the daemon a job runs on.
Snapshot failures must not prevent the authoritative stash from loading, and an older branch snapshot must not substitute an image for the current checkout.
Raw daemon snapshots are job handoffs, so publishing them into branch caches unnecessarily exposes later runs to arbitrary checkout refs.
The snapshot script runs checkout code and prunes Docker state. Existing cache publications must finish before that optional execution.
The optional snapshot producer executes checkout code without publishing branch caches; its artifacts are available only to consumers in the same workflow run.
Snapshot producers must not accept arbitrary checkout refs in privileged workflow contexts, and a partially failed Docker stop must still trigger restart cleanup.
Preparing the authoritative image in a second runner adds nearly three minutes before consumers can start. All branch publications must finish before the optional snapshot code prunes Docker state.
The builder can check out a configurable ref before publishing caches. Snapshot execution must read the workflow commit itself rather than trust files left in that working tree.
Executing snapshot code inside the configurable-ref builder introduces a cache-poisoning finding. A separate job preserves the security boundary; its latency cost must remain explicit when measuring the runner-time benefit.
An unavailable daemon must not be mistaken for an empty image store before destructive snapshot materialization.
Reloading the freshly built image in a second job delays every consumer by nearly four minutes. Trusted workflow commands reuse the builder daemon without executing code from the configurable checkout.
d3c3f01 to
6e8fd38
Compare
The builder bind-mounts Docker from /mnt. Reading its image store and writing the compressed archive to that same disk competes for I/O; runner temp keeps the output on the root disk.
zozo123
changed the title
Ordinary source edits invalidate the dependency installation layer because it follows the full checkout copy. Reuse locked third-party dependencies while retaining the complete source installation and its resolution fallback.
Use portable path-preserving shell commands for the BuildKit manifest stage and cover nested paths containing spaces.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the shell required by the manifest extraction stage so the BuildKit RUN command works with Docker's default shell.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
zozo123
changed the title
Restore the current UV and prek versions, propagate manifest discovery and copy failures, and cover failure injection in the extraction tests.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@shahar1 — measured the current
All 79 active CI-image consumers successfully restored the snapshot and skipped the legacy image-load action. Snapshot creation took 73s and upload took 23s; the total workflow numbers above already include that 96s overhead. The critical-path tradeoff is also favorable for the median consumer in this comparison: consumers started 69s later relative to workflow creation, but their faster image preparation meant the median paired job reached the end of preparation 16s earlier. Across all 79 paired consumers, preparation used 110.6 fewer runner-minutes. The total workflow saving is 69.6 minutes after accounting for every executed job, including the builder. Sources: current-head run, attempt 1 · matched baseline run, attempt 1. Measurements use GitHub Actions job/step timestamps and consumer action outcomes. Wall time is workflow creation to the last executed job completing; runner time is the sum of executed job durations. This is one treatment run and one exact-matrix baseline, so the percentages describe this comparison rather than a guaranteed result for every PR. The dependency-layer cache saving is separate and not included as a demonstrated benefit here. Additional repetitions would establish consistency, but the measured result is already positive: faster preparation, less runner time, and a faster completed workflow. |
This branch has not been deployed
4 participants
Summary
This PR reduces repeated CI-image preparation in two independent, conservative layers:
build-ci-imagesbuilder for consumers in the same pull-request workflow run. Consumers restore it when compatible and retain the existing image-stash/load path as fallback.Neither optimization skips image builds or tests, and neither introduces a Bazel dependency.
Image snapshot changes
build-ci-images, after image and mount-cache publication, using the already-built daemon.ci-image-snapshot-v2-*artifacts with two-day retention.overlay2storage.Dependency-layer changes
pyproject.tomlanduv.lockbefore the full checkout.pip checkpath.Safety and fallback
Both optimizations are optional. Missing artifacts, malformed or stale snapshot metadata, incompatible daemon state, unsupported storage, failed extraction, failed Docker recovery, failed image startup, missing lockfiles, upgrade builds, or failed dependency preinstallation fall back to the existing path. The PR does not reduce test coverage.
Measurements and limitations
The earlier separate-producer snapshot design measured consumer preparation at a median of 102 seconds versus 189 seconds for the baseline, but delayed consumers by approximately 236 seconds. The current design creates the snapshot in the builder after cache publication, removing the separate producer reload; the net workflow benefit still requires measurement.
The first current-design run created the snapshot in 74 seconds and uploaded it in 14 seconds. The latest revision writes the archive off the Docker data disk. No guaranteed cache-hit rate, minute saving, or whole-workflow speedup is claimed.
The dependency-layer miniature probe demonstrated the intended cache boundary: source-only changes reused the dependency layer, while manifest and lockfile changes invalidated it. A full Airflow image equivalence/performance run remains environment/CI dependent.
Validation
uv.lock, nested paths including spaces, upgrade bypass, and successful/failed optional preinstallation./bin/sh. CI is rerunning for head490da23a65.Scope
This PR optimizes image materialization and dependency-layer reuse. It does not implement semantic CI-environment identity, skip tests, prune coverage, or make reusable cross-run/fork-produced environment artifacts trusted.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5.5) and Codex (GPT-6), following the Airflow guidelines.