Repository navigation
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.
d3c3f01 to
6e8fd38
Compare
zozo123
changed the title
zozo123
changed the title
|
@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
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.
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.
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
zozo123
changed the title
…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) |
This branch has not been deployed
4 participants
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 compatible and otherwise use the existing image stash/load path. Image builds and test coverage are unchanged.
Snapshot creation runs after image and mount-cache publication, using the already-built daemon. Both
createandrestorelive indocker_data_root_snapshot.shand share compatibility checks and daemon recovery. This job already executes PR-authored build code; calling the script does not claim an additional isolation boundary.The archive is created in runner temporary storage, off the AMD Docker data disk. Restore checks checkout revision, daemon fingerprint, image ID, empty image/container inventories, and
overlay2storage. Every restore exit removes the downloaded archive and metadata, including rejection and materialization failure, before the stash fallback needs/mntspace.Snapshots remain PR-only, use the
ci-image-snapshot-v1-*artifact name, and are kept for one day. They are downloaded only within the run that uploaded them; once one expires, a re-run restores the stash instead. For the same reason the rename from the-v2name used by earlier branch revisions needs no compatibility handling. Missing or incompatible snapshots fall back to the existing image stash.Measurements and artifact budget
Run 37804108397 (head
3793fc0ad5, AMD, Python 3.11, artifact still named-v2at that head):Prepare breeze & CI image; all 79 restored the snapshot and none fell back to the stash.Six same-day stash-path PR runs, each with 79–80 consumers, created between 15:25 and 16:09 UTC (37800664384, 37806442129, 37806452025, 37806477584, 37806492097, 37806501383), had per-run image-preparation medians of 172–334 s. Their stash download medians were 45–211 s, and
breeze ci-image loadmedians were 103–109 s.The unpack (38 s) is consistently shorter than the load it replaces (103–109 s). Download medians ranged from 45 to 211 s across the baseline runs, and the snapshot download median (151 s) falls inside that range, so this run's overall preparation median also sits inside the baseline range. These are single runs, not a controlled measurement; workflow turnaround is treated as unchanged.
Both transports are retained: in that run the snapshot artifact is 2.53 GB (2,526,037,035 bytes) and the fallback stash is 2.36 GB (2,357,622,713 bytes), totaling 4.88 GB per built CI image / Python-platform combination per run, roughly double the previous image transport storage. That run built one CI image (linux/amd64, Python 3.11), so its CI image transport total was 4.88 GB. The stash keeps its two-day retention and the snapshot is kept for one day, so the snapshot adds about 2.53 GB-days of artifact storage per combination per run, half of what two-day retention would add. This is not a fixed 4.88 GB cap for an entire matrix run; matrix size and overlapping retained runs multiply the storage cost. Maintainers responsible for ASF artifact quota and CI capacity need to accept this tradeoff before merging.
Scope and validation
The independent Dockerfile dependency-layer optimization has been removed from this PR and moved to draft #74466. There is no cross-run or fork-produced trusted environment cache, test skipping, or Bazel dependency.
main: 1,702 passed, 1 skipped.prekhooks on the changed files passed.3793fc0ad5. Since then the branch has only renamed the artifact to-v1, added the name test, cut the snapshot's retention to one day, and mergedmain.actions/cache-poisoning/poisonable-stepalert, on the snapshot step. It is the same rule thatmainalready reports on the four steps after this job's checkout; GitHub reports it as one new alert at the snapshot step's location. It is a false positive for apull_request-only step that writes no cache. Details and a local CodeQL comparison are here; the open alerts are listed here.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.