potiuk commented on code in PR #74173: URL: https://github.com/apache/airflow/pull/74173#discussion_r4307907858
########## scripts/ci/docker_data_root_snapshot.sh: ########## @@ -0,0 +1,161 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Moves the CI image between jobs as a copy of the Docker data directory that holds it, so a job +# restores it with one extraction instead of `docker image load` unpacking and checksumming every +# layer again. +# +# docker_data_root_snapshot.sh create SNAPSHOT_FILE +# Creates the snapshot after the builder publishes its caches. +# docker_data_root_snapshot.sh restore SNAPSHOT_FILE +# Restores the image. Incompatible snapshots leave the daemon untouched. Failed +# materialization cleans partial image state and restarts Docker for the normal stash path. +set -euo pipefail + +DATA_ROOT="/var/lib/docker" +DAEMON_STOPPED=false +RESTORE_IN_PROGRESS=false +RESTORE_FILE="" + +function cleanup_daemon() { + local result=$? + trap - EXIT + if [[ -n "${RESTORE_FILE}" ]]; then + rm -f "${RESTORE_FILE}" "${RESTORE_FILE}.meta" + fi + if [[ "${RESTORE_IN_PROGRESS}" == true ]]; then + stop_daemon + remove_image_store + start_daemon + elif [[ "${DAEMON_STOPPED}" == true ]]; then + start_daemon + fi + exit "${result}" +} +trap cleanup_daemon EXIT + +function check_supported_daemon() { + local daemon=$1 + local _ driver root + read -r _ driver _ root <<< "${daemon}" + if [[ "${driver}" != overlay2 || "${root}" != "${DATA_ROOT}" ]]; then + echo "::warning::Unsupported Docker image store; skipping snapshot: ${daemon}" + exit 3 + fi +} + +# A data directory is only readable by the daemon version and storage driver that wrote it. +function daemon_fingerprint() { + docker info --format '{{.ServerVersion}} {{.Driver}} {{.Architecture}} {{.DockerRootDir}}' +} + +function stop_daemon() { + DAEMON_STOPPED=true + sudo systemctl stop docker.socket docker +} + +function start_daemon() { + sudo systemctl start docker + DAEMON_STOPPED=false +} + +function remove_image_store() { + sudo rm -rf "${DATA_ROOT}/image" "${DATA_ROOT}/overlay2" +} + +function create_snapshot() { + local snapshot_file="${1}" + local daemon image_id + daemon="$(daemon_fingerprint)" + check_supported_daemon "${daemon}" + 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" + stop_daemon + sudo tar --directory "${DATA_ROOT}" --xattrs --acls --numeric-owner \ + --create --file - image overlay2 \ + | zstd -3 -T0 --quiet --force -o "${snapshot_file}" + start_daemon +} + +function restore_snapshot() { + local snapshot_file="${1}" + RESTORE_FILE="${snapshot_file}" + local -a meta + local image_id snapshot_fingerprint daemon existing_images existing_containers + if [[ ! -f "${snapshot_file}" || ! -f "${snapshot_file}.meta" ]]; then + echo "No snapshot at ${snapshot_file}" + exit 2 + fi + read -r -a meta < "${snapshot_file}.meta" + if [[ ${#meta[@]} != 6 ]]; then + echo "Invalid snapshot metadata" + exit 3 + fi + if [[ "${meta[5]}" != "$(git rev-parse HEAD)" ]]; then + echo "Snapshot belongs to a different checkout revision" + exit 3 + fi + image_id="${meta[4]}" + snapshot_fingerprint="${meta[*]:0:4}" + daemon="$(daemon_fingerprint)" + check_supported_daemon "${daemon}" + if [[ "${snapshot_fingerprint}" != "${daemon}" ]]; then + echo "::warning::Snapshot daemon fingerprint mismatch: '${snapshot_fingerprint}' != '${daemon}'" + exit 3 + fi + existing_images="$(docker images --all --quiet)" + existing_containers="$(docker ps --all --quiet)" + if [[ -n "${existing_images}" || -n "${existing_containers}" ]]; then + echo "The daemon already holds images, which restoring the snapshot would drop" + exit 3 + fi + stop_daemon + RESTORE_IN_PROGRESS=true + remove_image_store + zstd -d -T0 --quiet --stdout "${snapshot_file}" \ + | sudo tar --directory "${DATA_ROOT}" --xattrs --acls --numeric-owner --extract --file - Review Comment: **Extended attributes are dropped on restore.** GNU tar with `--xattrs` but no `--xattrs-include` restores only `user.*` attributes. I checked in an `ubuntu:24.04` container: a binary with `cap_net_raw=ep` archived with these flags comes back with no capabilities, and adding `--xattrs-include='*'` on extract brings `security.capability` back. overlay2 keeps its opaque-directory markers in `trusted.overlay.opaque`, which are filtered the same way, so a directory that a layer deleted and recreated shows the lower layers' contents again. ```suggestion | sudo tar --directory "${DATA_ROOT}" --xattrs --xattrs-include='*' --acls --numeric-owner --extract --file - ``` ########## scripts/ci/docker_data_root_snapshot.sh: ########## @@ -0,0 +1,161 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Moves the CI image between jobs as a copy of the Docker data directory that holds it, so a job +# restores it with one extraction instead of `docker image load` unpacking and checksumming every +# layer again. +# +# docker_data_root_snapshot.sh create SNAPSHOT_FILE +# Creates the snapshot after the builder publishes its caches. +# docker_data_root_snapshot.sh restore SNAPSHOT_FILE +# Restores the image. Incompatible snapshots leave the daemon untouched. Failed +# materialization cleans partial image state and restarts Docker for the normal stash path. +set -euo pipefail + +DATA_ROOT="/var/lib/docker" +DAEMON_STOPPED=false +RESTORE_IN_PROGRESS=false +RESTORE_FILE="" + +function cleanup_daemon() { + local result=$? + trap - EXIT + if [[ -n "${RESTORE_FILE}" ]]; then + rm -f "${RESTORE_FILE}" "${RESTORE_FILE}.meta" + fi + if [[ "${RESTORE_IN_PROGRESS}" == true ]]; then + stop_daemon + remove_image_store + start_daemon + elif [[ "${DAEMON_STOPPED}" == true ]]; then + start_daemon + fi + exit "${result}" +} +trap cleanup_daemon EXIT + +function check_supported_daemon() { + local daemon=$1 + local _ driver root + read -r _ driver _ root <<< "${daemon}" + if [[ "${driver}" != overlay2 || "${root}" != "${DATA_ROOT}" ]]; then + echo "::warning::Unsupported Docker image store; skipping snapshot: ${daemon}" + exit 3 + fi +} + +# A data directory is only readable by the daemon version and storage driver that wrote it. +function daemon_fingerprint() { + docker info --format '{{.ServerVersion}} {{.Driver}} {{.Architecture}} {{.DockerRootDir}}' +} + +function stop_daemon() { + DAEMON_STOPPED=true + sudo systemctl stop docker.socket docker +} + +function start_daemon() { + sudo systemctl start docker + DAEMON_STOPPED=false +} + +function remove_image_store() { + sudo rm -rf "${DATA_ROOT}/image" "${DATA_ROOT}/overlay2" +} + +function create_snapshot() { + local snapshot_file="${1}" + local daemon image_id + daemon="$(daemon_fingerprint)" + check_supported_daemon "${daemon}" + 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" + stop_daemon + sudo tar --directory "${DATA_ROOT}" --xattrs --acls --numeric-owner \ + --create --file - image overlay2 \ + | zstd -3 -T0 --quiet --force -o "${snapshot_file}" + start_daemon +} + +function restore_snapshot() { + local snapshot_file="${1}" + RESTORE_FILE="${snapshot_file}" + local -a meta + local image_id snapshot_fingerprint daemon existing_images existing_containers + if [[ ! -f "${snapshot_file}" || ! -f "${snapshot_file}.meta" ]]; then + echo "No snapshot at ${snapshot_file}" + exit 2 + fi + read -r -a meta < "${snapshot_file}.meta" + if [[ ${#meta[@]} != 6 ]]; then + echo "Invalid snapshot metadata" + exit 3 + fi + if [[ "${meta[5]}" != "$(git rev-parse HEAD)" ]]; then + echo "Snapshot belongs to a different checkout revision" + exit 3 + fi + image_id="${meta[4]}" + snapshot_fingerprint="${meta[*]:0:4}" + daemon="$(daemon_fingerprint)" + check_supported_daemon "${daemon}" + if [[ "${snapshot_fingerprint}" != "${daemon}" ]]; then + echo "::warning::Snapshot daemon fingerprint mismatch: '${snapshot_fingerprint}' != '${daemon}'" + exit 3 + fi + existing_images="$(docker images --all --quiet)" + existing_containers="$(docker ps --all --quiet)" + if [[ -n "${existing_images}" || -n "${existing_containers}" ]]; then + echo "The daemon already holds images, which restoring the snapshot would drop" + exit 3 + fi + stop_daemon + RESTORE_IN_PROGRESS=true + remove_image_store + zstd -d -T0 --quiet --stdout "${snapshot_file}" \ + | sudo tar --directory "${DATA_ROOT}" --xattrs --acls --numeric-owner --extract --file - + start_daemon + if ! docker run --rm --entrypoint /bin/bash "${image_id}" -c true; then Review Comment: `bash -c true` still passes with the attributes above missing, so a damaged restore never falls back to the stash. It would be worth checking more after the restore, for example comparing `docker image inspect` with the meta and checking a file that is known to be removed in a later layer. ########## .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 Review Comment: **The multi-GB snapshot is downloaded before the cheap checks** (empty daemon, daemon fingerprint, driver). When any of them rejects it, the job then downloads the stash as well. Uploading the `.meta` as a separate small artifact and checking it first would avoid that. ########## .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-v1-${{ 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 == '' Review Comment: When no snapshot exists (builder skipped, create failed, retention expired), `download-artifact` still runs and adds an error annotation in each of the ~30 jobs using this action, even though they stay green. Gating it on the builder's snapshot step output would avoid the noise. ########## scripts/ci/docker_data_root_snapshot.sh: ########## @@ -0,0 +1,161 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Moves the CI image between jobs as a copy of the Docker data directory that holds it, so a job +# restores it with one extraction instead of `docker image load` unpacking and checksumming every +# layer again. +# +# docker_data_root_snapshot.sh create SNAPSHOT_FILE +# Creates the snapshot after the builder publishes its caches. +# docker_data_root_snapshot.sh restore SNAPSHOT_FILE +# Restores the image. Incompatible snapshots leave the daemon untouched. Failed +# materialization cleans partial image state and restarts Docker for the normal stash path. +set -euo pipefail + +DATA_ROOT="/var/lib/docker" +DAEMON_STOPPED=false +RESTORE_IN_PROGRESS=false +RESTORE_FILE="" + +function cleanup_daemon() { + local result=$? + trap - EXIT + if [[ -n "${RESTORE_FILE}" ]]; then + rm -f "${RESTORE_FILE}" "${RESTORE_FILE}.meta" + fi + if [[ "${RESTORE_IN_PROGRESS}" == true ]]; then + stop_daemon + remove_image_store + start_daemon + elif [[ "${DAEMON_STOPPED}" == true ]]; then + start_daemon + fi + exit "${result}" +} +trap cleanup_daemon EXIT + +function check_supported_daemon() { + local daemon=$1 + local _ driver root + read -r _ driver _ root <<< "${daemon}" + if [[ "${driver}" != overlay2 || "${root}" != "${DATA_ROOT}" ]]; then Review Comment: With the containerd image store (Docker 29+), the driver is no longer `overlay2`, so every create and restore exits 3 with only a warning, while the builder still pays for pruning and the upload. A hard signal, or skipping the upload in that case, would make that visible. ########## .github/workflows/ci-image-build.yml: ########## @@ -401,3 +401,35 @@ 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. + - 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: | + ./scripts/ci/docker_data_root_snapshot.sh create "${SNAPSHOT_FILE}" + 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 }}" Review Comment: Every PR run now uploads a second multi-GB copy of each CI image (per Python × platform) as a run artifact, on top of the stash. That is part of the cost the measurements above should weigh. ########## .github/actions/prepare_breeze_and_image/action.yml: ########## @@ -98,3 +126,4 @@ runs: run: > breeze ${IMAGE_TYPE}-image load --platform "${PLATFORM}" --python "${PYTHON}" --image-file-dir "/mnt" shell: bash + if: steps.snapshot.outputs.restored != 'true' Review Comment: A successful restore skips `breeze ci-image load`, and with it breeze's environment checks and the image diagnostics in the logs, so problems that load would catch surface later and less clearly. ########## .github/workflows/ci-image-build.yml: ########## @@ -401,3 +401,35 @@ 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. + - 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: | + ./scripts/ci/docker_data_root_snapshot.sh create "${SNAPSHOT_FILE}" + 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-v1-${{ 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' Review Comment: **Re-runs use a stale snapshot.** Without `overwrite: true`, a re-run of the builder fails here with 409, `continue-on-error` hides it, and consumers keep restoring attempt 1's snapshot because the commit matches. ```suggestion if-no-files-found: 'error' overwrite: true ``` ########## .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-v1-${{ 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 Review Comment: **No timeout on the unpack step**, so a hang in `systemctl stop/start docker` or in the extraction blocks the job until its overall timeout. A half-restored data-root cannot fall back to the stash anyway, so failing fast is the best this step can do (the create step has 5 minutes). ```suggestion id: snapshot timeout-minutes: 10 ``` -- 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]
