Repository navigation
Conversation
f58c30f to
80d5cb7
Compare
1d49fda to
bf067ab
Compare
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.
d3c3f01 to
6e8fd38
Compare
|
@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. |
shahar1
left a comment
There was a problem hiding this comment.
The serialization blocker from my first round is gone. I re-measured it, and your numbers in the comment above agree with mine. Since then the PR has also grown a second, independent optimization (the Dockerfile dependency layer), so this round covers both.
Snapshot: consumers no longer wait
Run 37226560360 (head c16d395b5f, AMD, 3.10):
Build CI linux/amd64 image 3.10finished at 19:10:13; the first dependent job started at 19:10:14. That is a 1 s gap, against 3 min 56 s in the previous revision.- The snapshot steps cost the builder ~86 s: create 19:08:45 → 19:09:59, upload finalized 19:10:11 (2.52 GB).
- Consumer image prep: download 72 s + unpack 48 s = 120 s, against stash 67 s +
docker image load106 s = 189 s in baseline run 37210552741.
Your run pair (127 s vs 210 s per consumer, −69.6 runner-minutes, 3 s builder-to-consumer gap) says the same thing. The −87 s wall-time figure compares two different PRs from one run each, so on a 51-minute workflow I would read it as "turnaround unchanged", not as a speed-up. The PR description now states this honestly ("no whole-workflow speedup is claimed"), thank you.
So the snapshot is a capacity win: roughly 70–90 runner-minutes per full AMD run, paid for with ~86 s of serial builder time and a second 2.5 GB artifact per Python version. That is a CI-budget call rather than a code question, so I would like @potiuk and/or @jscheffl to weigh in before this merges. The implementation itself I am happy with: the fallback is safe, failure handling brings back a running daemon, and test_docker_data_root_snapshot.py covers the interesting branches, including the "create after the cache publishers" ordering.
Dependency layer: please split it into its own PR
The dependency-manifests stage plus the --no-install-workspace preinstall is a separate change with a separate risk profile: it touches Dockerfile.ci, which every CI and local Breeze build uses, while the snapshot touches only PR workflows. It also cannot show its benefit in this PR. The layer can only be reused once main pushes it to the ghcr.io/.../cache-linux-amd64 registry cache. In this PR's build (job 111537879924) it missed, as expected: the preinstall took 28.7 s (789 packages) and the full sync took 19.3 s (only the 141 workspace packages). On a cache hit the builder therefore saves ~29 s. Unlike the snapshot, that saving is on the critical path, so it would win back part of the 86 s the snapshot costs.
That makes it worth having, but as its own PR with its own measurement: a PR that touches only sources, built after the layer has landed in the main cache. Then each change can be reviewed, reverted and backported on its own. The mechanism itself looks right to me: COPY --from keys on file contents, so the bind-mount stage re-running on every build does not invalidate it. The comment on install_airflow_when_building_images.sh ("usually incremental small set of packages") becomes true again. One inline comment on the || echo.
Nits
ci-image-snapshot-v2-...: there is no v1 of this artifact, so the-v2suffix will puzzle the next reader (unless it avoids a name cached from an earlier revision of this branch).test_ci_dependency_layer.pycuts theRUNbodies out ofDockerfile.ciby string splitting and runs them underbash. Two of its three tests check thatfind/cpfailures propagate undererrexitand thata || bexits 0, which is bash semantics rather than this change. If anyone reformats the Dockerfile, the tests break with anIndexError. The property worth testing is that a source-only change reuses the layer, and only a real build shows that. If this moves to its own PR, I would drop these tests and put the build measurement in the PR description.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an 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 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.
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.
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>
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>
f53486a to
3793fc0
Compare
…age-data-root-snapshot
There has never been an earlier version of this artifact on main, so the -v2 suffix left over from earlier branch revisions would puzzle the next reader. Snapshot artifacts are scoped to the workflow run that uploads them and kept for two days, so no consumer can see an artifact under the old name and no compatibility shim is needed. A test now checks that the upload and restore steps use the same artifact name, so the two cannot drift apart silently.
A snapshot is only downloaded by consumer jobs of the run that uploaded it. Once it expires, a re-run restores the image stash instead, which keeps its two-day retention. One day halves the storage the snapshot adds on top of the stash.
|
@shahar1 ready for another look. Your round-2 points are addressed at
@potiuk @jscheffl, shahar1 asked for your call on the CI budget before this merges. These figures are per built CI image (Python/platform) per PR run, from run 37804108397:
Is that tradeoff acceptable? CodeQL also reports one Drafted-by: Claude Code (Opus 5.5) (no human review before posting) |
shahar1
left a comment
There was a problem hiding this comment.
All four round-3 points are fixed at 2fe6e121, and I found no further correctness issues in the script, the workflow wiring or the tests; what remains is not code: the CI-budget call that the PR body asks potiuk and jscheffl for, and dismissing CodeQL alert 670, which is the same actions/cache-poisoning/poisonable-step pattern as alerts 624–627 on this job's earlier steps that were already dismissed on main as false positives.
Smaller observations
.github/actions/prepare_breeze_and_image/action.yml— Heads-up, not a blocker: #74889 (mine) touches the same two files and, on self-hosted RunsOn runners, moves the image stash from GitHub artifacts to S3. The snapshot here always goes throughactions/upload-artifact/download-artifact, so on that path it would trade an in-region S3 fetch for a 2.5 GB GitHub-artifact download per consumer. The script's fingerprint andDockerRootDirchecks keep it safe there (it falls back to the stash), so the question is only whether it still pays off. Whichever PR lands second should either route the snapshot through the same stash action or limit it to GitHub-hosted runners; I am happy to do that on my side if this one lands first.- See inline comments on
scripts/tests/ci/test_docker_data_root_snapshot.py:253.
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.
Co-authored-by: Yossi Eliaz <zozo123@users.noreply.github.com>
potiuk
left a comment
There was a problem hiding this comment.
Restoring the Docker data-root instead of docker load is a nice idea. A few things need fixing first (inline), most importantly the dropped extended attributes, which silently change the image the tests run against.
Could you also share some measurements of the actual savings from real CI runs? For example: docker load time vs. snapshot restore time per consumer job, the extra time the builder spends on prune + create + upload, the snapshot size per image, and how often consumers actually accept the snapshot rather than falling back. Ideally from a few PR runs, so we can see the net effect across the ~30 consumer jobs against the extra upload and artifact storage.
One general point: the fast path only exists in prepare_breeze_and_image, while prepare_single_ci_image still loads from the stash. Putting the snapshot-or-stash decision into one shared loader would avoid making every later fix twice.
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
…download - Archive and extract xattrs in every namespace: GNU tar restores only user.* by default, which dropped overlay2's trusted.overlay.opaque and file capabilities, so a restored image could differ from the built one. - Upload the snapshot with overwrite, plus a small metadata artifact that is uploaded even when the export fails; it reads "unavailable" until the archive is complete, so a rerun never restores an earlier attempt. - Check the metadata against the checkout and daemon before downloading the snapshot, and bound extraction with a timeout that falls back to the stash after cleaning partial state. - Load images for prepare_breeze_and_image and prepare_single_ci_image through one shared load_image action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Record the image ID and a hash of its merged file tree when creating the snapshot and compare both after restoring: a restore that lost an opaque directory marker keeps the image ID but changes the tree, so it now falls back to the stash instead of passing `bash -c true`. - Restore through `breeze ci-image load --snapshot-file`, keeping breeze's environment checks, image check, diagnostics and rebuild marker. - Bound the daemon stop and start as well as the extraction. - Download the snapshot metadata by pattern, which adds no error annotation when the builder made no snapshot. - Limit snapshots to GitHub-hosted runners, where self-hosted runners already restore the stash from their own storage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y the unavailable marker Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Every point is addressed in the commits below, with details in the inline replies.
The first version of the file-tree check was broken. On 08c9943 (run 39159493825), all 79 eligible consumers rejected the snapshot and loaded the stash. The run passed, but it saved nothing. The hash sorted the 265k-entry listing inside the container, and Measurements
The 26 PROD-image jobs in each run are not eligible. Net
CI on 6da2d0f: all checks are green (135 passed, none failing), and CodeQL reports no alerts. It no longer reports the Needs a maintainer: only a decision on the runner-time vs. wall-clock and storage trade above. @shahar1: snapshots are limited to GitHub-hosted runners ( |
Sorting the CI image's 265k-entry listing inside the container spilled to temporary files in /tmp while find was still walking it, so every run listed different files and every consumer rejected the snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The builder now calls `breeze ci-image save --snapshot-file`, mirroring the `breeze ci-image load --snapshot-file` that consumers restore with, so both sides of the snapshot go through breeze and its environment checks.
Summary
Reduce repeated CI image materialization by publishing an optional Docker data-directory snapshot from the existing builder. Consumers in the same pull-request workflow run restore it when it fits their daemon; otherwise they use the existing image stash and
breeze ci-image load. Image builds and test coverage are unchanged.How it works
breeze ci-image save --snapshot-file, which callsscripts/ci/docker_data_root_snapshot.sh create./var/lib/docker/{image,overlay2}with--xattrs --xattrs-include='*', so overlay2'strusted.overlay.opaquemarkers and file capabilities survive.unavailableuntil the archive is complete, and it is uploaded even when the export fails.overwrite: true, so a re-run never restores an earlier attempt's snapshot.load_imageaction, used by bothprepare_breeze_and_imageandprepare_single_ci_image, does this in order:pattern, so a missing snapshot adds no annotation.checkof the revision, the daemon fingerprint and storage driver, and that the daemon holds no images.breeze ci-image load --snapshot-file, which keeps breeze's environment checks and diagnostics.timeout-minutes, so the script bounds each step itself: daemon stop and start at 60 s each, extraction at 300 s, and the check at 60 s.pull_requestevents on GitHub-hosted runners.Measurements and artifact budget
The measurements come from two full PR runs of the final design: 39190862270 on 6dabcd9 and 39199861265 on 6da2d0f. The baseline is 237 consumer jobs from three full-CI PR runs without the snapshot: 39141869252, 39143583838 and 39150120169. Durations are the composite-step
duration_msvalues from the job logs.breeze ci-image load103 sNet per consumer job: ready 29–59 s sooner at the median. Unpacking replaces the 103 s
docker loadwith 40–47 s, but the snapshot download is slower than the stash download and gives back part of that.Net per run: the builder finishes 84–100 s later, and every consumer starts that much later. Each consumer therefore finishes about 40–55 s later in wall-clock time. Across the 79 consumers, the run uses about 37–76 fewer runner-minutes. So this saves runner time, not wall-clock time.
Storage: both transports are kept: the snapshot is 2.55 GB and the stash 2.38 GB per Python × platform per run, about 4.9 GB together. The snapshot is kept for one day, compared with two for the stash, so it adds about 2.55 GB-days per combination per run. Matrix size and overlapping runs multiply this. Maintainers responsible for ASF artifact quota and CI capacity need to accept this trade-off before merging.
Scope and validation
ci-image saveandci-image loadwith--snapshot-file, including the exit codes when a snapshot cannot be created or restored. All 22 tests intest_ci_image_commands.pypass.--xattrs-includeis rejected: the image ID stays the same, but the file tree differs.sortinside the container.sortwrote temporary files into/tmpwhilefindwas still walking it, so every consumer rejected the snapshot (run 39159493825). The listing is now sorted on the host.pull_requestevents.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.