Re: [PR] Accelerate CI image materialization with same-run snapshots [airflow]

["zozo123 (via GitHub)" <gi...@apache.org>]

zozo123 commented on code in PR #74173:
URL: https://github.com/apache/airflow/pull/74173#discussion_r4221175135


##########
.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:
   Agreed: the builder already runs PR-authored Breeze code, so the inline workflow commands did not provide an extra isolation boundary. Removed that claim and moved creation into a `create` subcommand in `docker_data_root_snapshot.sh`, sharing the fingerprint check, stop/start helpers and EXIT recovery with restore. The workflow now calls the script after cache publication, and the tests invoke both subcommands directly rather than extracting the YAML run block. All 24 snapshot tests pass; the full scripts suite on the rebased branch passed with 1,696 tests and one skip. Pushed in head `3793fc0ad560`.
   
   ---
   Drafted-by: Codex (GPT-6) (no human review before posting)



##########
.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:
   Fixed in head `3793fc0ad560`: restore registers the exact archive and `.meta` paths with its EXIT cleanup before checking whether either file exists. Every restore exit now removes both files, including missing/malformed metadata, stale checkout, fingerprint/storage/inventory rejection, extraction failure and failed image startup. This frees `/mnt` before the stash fallback starts. The regression harness asserts archive and metadata removal for every restore invocation, covering success and all tested rejection/recovery paths; 24 tests pass.
   
   ---
   Drafted-by: Codex (GPT-6) (no human review before posting)



##########
.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:
   Agreed. The PR description now explicitly records 2.52 GB for the snapshot plus 2.35 GB for the fallback stash: approximately 4.87 GB per built CI image / Python-platform combination per run, with two-day retention for both transports. Matrix size and overlapping retained runs multiply that amount; it is not a 4.9 GB cap for the whole workflow. The description treats this as a CI-capacity/artifact-quota tradeoff requiring maintainer acceptance before merging, and claims no whole-workflow speedup. I have not assumed ASF budget approval.
   
   ---
   Drafted-by: Codex (GPT-6) (no human review before posting)



##########
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:
   Agreed: a failed preinstall must not become a successful cached partial layer. Removed the `|| echo` so installation failure now fails the build. Per your review's split request, the entire dependency-layer change is removed from #74173 and moved to draft #74466, which contains this correction. The Dockerfile string-splitting/Bash-semantics tests were also dropped. That draft records your earlier 28.7-second preinstall measurement and explicitly leaves full-image equivalence and registry cache-hit performance unverified pending separate validation. Regular and manual Dockerfile checks pass.
   
   ---
   Drafted-by: Codex (GPT-6) (no human review before posting)



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org