shahar1 commented on code in PR #74173:
URL: https://github.com/apache/airflow/pull/74173#discussion_r4210244944
##########
.github/workflows/ci-image-build.yml:
##########
@@ -401,3 +401,70 @@ jobs:
steps.stashed-image.outputs.reusable != 'true'
- name: "Check disk space after build"
run: df -H
+
+ # Snapshot only after cache publication: pruning removes the mount-cache
image.
+ # Keep creation in trusted workflow commands; the configurable checkout
is untrusted.
+ - name: "Snapshot CI image ${{ inputs.platform }}:${{
env.PYTHON_MAJOR_MINOR_VERSION }}"
+ id: snapshot-export
+ continue-on-error: true
+ timeout-minutes: 5
+ env:
+ # Docker is bind-mounted from /mnt on AMD; write the archive on the
root disk.
+ SNAPSHOT_FILE: >-
+ ${{ runner.temp }}/ci-image-snapshot-${{
+ inputs.platform == 'linux/amd64' && 'linux_amd64' || 'linux_arm64'
+ }}-${{ env.PYTHON_MAJOR_MINOR_VERSION }}.tar.zst
+ run: |
+ daemon_stopped=false
+ function restart_docker_on_exit() {
+ local result=$?
+ trap - EXIT
+ if [[ "${daemon_stopped}" == true ]]; then
+ sudo systemctl start docker
+ fi
+ exit "${result}"
+ }
+ trap restart_docker_on_exit EXIT
+ daemon="$(docker info --format \
+ '{{.ServerVersion}} {{.Driver}} {{.Architecture}}
{{.DockerRootDir}}')"
+ read -r _ driver _ root <<< "${daemon}"
+ if [[ "${driver}" != overlay2 || "${root}" != /var/lib/docker ]];
then
+ echo "::warning::Unsupported Docker image store; skipping snapshot
creation: ${daemon}"
+ exit 3
+ fi
+ image_id="$(docker images --quiet --filter
'label=org.apache.airflow.image=airflow-ci' | sort -u)"
+ if [[ -z "${image_id}" || "${image_id}" == *$'\n'* ]]; then
+ echo "Expected exactly one CI image in the daemon, found:
'${image_id}'" >&2
+ exit 1
+ fi
+ docker ps --all --quiet | xargs --no-run-if-empty docker rm --force
>/dev/null
+ docker images --quiet | sort -u | {
+ grep --invert-match --fixed-strings --line-regexp "${image_id}" ||
true
+ } | xargs --no-run-if-empty docker rmi --force >/dev/null
+ docker builder prune --all --force >/dev/null
+ printf '%s %s %s\n' "${daemon}" "${image_id}" "$(git rev-parse
HEAD)" \
+ > "${SNAPSHOT_FILE}.meta"
+ daemon_stopped=true
+ sudo systemctl stop docker.socket docker
+ sudo tar --directory /var/lib/docker --xattrs --acls --numeric-owner
\
+ --create --file - image overlay2 \
+ | zstd -3 -T0 --quiet --force -o "${SNAPSHOT_FILE}"
+ sudo systemctl start docker
+ daemon_stopped=false
+ shell: bash
+ if: >
+ github.event_name == 'pull_request' &&
+ inputs.upload-image-artifact == 'true' && inputs.image-stash-ref ==
'' &&
+ (inputs.checkout-ref == '' || inputs.checkout-ref == github.sha)
+ - name: "Upload CI image snapshot ${{ inputs.platform }}:${{
env.PYTHON_MAJOR_MINOR_VERSION }}"
+ continue-on-error: true
+ uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a
# v7.0.1
+ with:
+ name: >-
+ ci-image-snapshot-v2-${{ env.PYTHON_MAJOR_MINOR_VERSION }}-${{
+ inputs.platform == 'linux/amd64' && 'amd64' || 'arm64' }}
+ path: "${{ runner.temp }}/ci-image-snapshot-*-${{
env.PYTHON_MAJOR_MINOR_VERSION }}.tar.zst*"
+ if-no-files-found: 'error'
+ compression-level: '0'
+ retention-days: '2'
Review Comment:
Both transports are now uploaded per run — this snapshot (2.52 GB) plus the
`ci-image-save-v3-*` stash (2.35 GB) that the fallback still needs — so per-run
artifact storage for the CI image roughly doubles to ~4.9 GB at 2-day
retention. That is the price of having a fallback and I am not arguing against
it, but it is an ASF-quota question rather than a code one, so it should be
stated explicitly for whoever watches that budget.
##########
.github/workflows/ci-image-build.yml:
##########
@@ -401,3 +401,70 @@ jobs:
steps.stashed-image.outputs.reusable != 'true'
- name: "Check disk space after build"
run: df -H
+
+ # Snapshot only after cache publication: pruning removes the mount-cache
image.
+ # Keep creation in trusted workflow commands; the configurable checkout
is untrusted.
Review Comment:
The isolation this comment claims is not there to be had: the create step
only runs when `github.event_name == 'pull_request'`, and in that event this
same job checks out `inputs.checkout-ref` (empty, so the PR merge commit) and
runs `breeze ci-image build` from it at line 160 — PR-authored code already
executes here with the same privileges. The three callers that pass a non-empty
`checkout-ref` (`update-constraints-on-push.yml`, `release-constraints.yml`,
`publish-docs-to-s3.yml`) are all `workflow_dispatch` and so excluded by the
event guard anyway.
What the inlining does cost is a second copy of the fingerprint check, the
stop/start pair and the restart trap, which the restore path already has in
`docker_data_root_snapshot.sh` — and it forces the test to parse this YAML and
`bash -c` the `run:` block to reach the logic. A `create` subcommand in the
script, sharing those helpers, would drop ~45 lines here and let the test call
it directly.
##########
.github/actions/prepare_breeze_and_image/action.yml:
##########
@@ -79,6 +79,33 @@ runs:
run: |
echo "Checking free space!"
df -H
+ - name: "Restore CI image snapshot ${{ inputs.platform }}:${{
inputs.python }}"
+ continue-on-error: true
+ uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c
# v8.0.1
+ with:
+ name: >-
+ ci-image-snapshot-v2-${{ inputs.python }}-${{
+ inputs.platform == 'linux/amd64' && 'amd64' || 'arm64' }}
+ path: "/mnt/"
+ id: restore-snapshot
+ if: >
+ github.event_name == 'pull_request' &&
+ inputs.image-type == 'ci' && inputs.image-stash-ref == ''
+ # Falls back to the image stash below whenever the snapshot does not fit
this runner's daemon.
+ - name: "Unpack CI image snapshot ${{ inputs.platform }}:${{ inputs.python
}}"
+ id: snapshot
+ env:
+ PLATFORM: ${{ inputs.platform }}
+ PYTHON: ${{ inputs.python }}
+ run: |
+ if ./scripts/ci/docker_data_root_snapshot.sh restore \
+ "/mnt/ci-image-snapshot-${PLATFORM//\//_}-${PYTHON}.tar.zst"; then
+ echo "restored=true" >> "${GITHUB_OUTPUT}"
+ else
+ echo "Falling back to loading the image stash"
Review Comment:
On the fallback the 2.52 GB archive stays on `/mnt`: the script only `rm
-f`s it on the success path, and every rejection (`exit 2/3/4`) leaves it
behind. The stash path then downloads another ~2.35 GB to `/mnt` and `docker
image load`s it into the data root, which on AMD is bind-mounted from the same
disk. Worth an `rm -f
"/mnt/ci-image-snapshot-${PLATFORM//\//_}-${PYTHON}.tar.zst"*` in this branch
(or in the script's failure trap) so a rejected snapshot does not eat 2.5 GB of
the disk the fallback needs.
##########
Dockerfile.ci:
##########
@@ -1966,13 +1982,24 @@ RUN bash /scripts/docker/install_packaging_tools.sh
COPY --from=scripts install_airflow_when_building_images.sh /scripts/docker/
+COPY --from=dependency-manifests /dependency-manifests/ ${AIRFLOW_SOURCES}/
+ARG UPGRADE_RANDOM_INDICATOR_STRING=""
+# Only preinstall locked third-party dependencies. The full sync after COPY
still installs
+# workspace packages from the current checkout and retains its existing
resolution fallback.
+RUN
--mount=type=cache,id=ci-$TARGETARCH-$DEPENDENCY_CACHE_EPOCH,target=/root/.cache/
\
+ if [[ -z "${UPGRADE_RANDOM_INDICATOR_STRING}" ]]; then \
+ uv sync --all-packages --frozen --group ci-image
--no-install-workspace \
+ --no-binary-package lxml --no-binary-package xmlsec \
+ --no-python-downloads --no-managed-python || \
+ echo "Dependency preinstallation failed; deferring to the full
source installation"; \
Review Comment:
Swallowing the failure here means a failed preinstall is committed as a
successful, cached layer. Because `main` pushes its cache with `mode=max`, one
transient failure in a `main` build stores a partial `/usr/local` in the
registry cache. Every PR with the same manifests then reuses it until a
`pyproject.toml` or `uv.lock` changes. The full sync below repairs the
environment, so correctness is fine, but the saving is silently gone, and this
`echo` is the only trace in the build log.
This step uses the same `uv.lock` and the same `--frozen` flags as the
authoritative sync, so whatever makes it fail (network, an sdist build) would
also make that sync fail. I would drop the `|| echo` and let the build fail
like any other install step. A `::warning::` would not help here: BuildKit
prefixes every output line with `#50 26.59`, so the runner never treats it as
an annotation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
