Refactor Dockerfiles to use ARG for base images and add ACR sync work…#4952
Refactor Dockerfiles to use ARG for base images and add ACR sync work…#4952maxmartin-cgi wants to merge 23 commits into
Conversation
Unit Test Results768 tests 768 ✅ 11s ⏱️ Results for commit 5f25c64. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR reduces CI/CD failures caused by Docker Hub rate limiting by enabling Docker builds to optionally pull common base images from an Azure Container Registry (ACR) mirror instead of directly from Docker Hub.
Changes:
- Refactors multiple Dockerfiles to parameterize their base images via
ARG, enabling base image source overrides at build time. - Updates the Docker image build GitHub Actions workflow to pass base-image build args derived from the
ACR_BASE_IMAGE_PREFIXvariable. - Adds a new scheduled/manual GitHub Actions workflow to sync (mirror) selected upstream base images into ACR, and documents the new configuration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
templates/workspace_services/guacamole/guacamole-server/docker/Dockerfile |
Parameterizes Maven + guacd base images via ARG for ACR mirroring. |
templates/workspace_services/gitea/docker/Dockerfile |
Parameterizes the upstream Gitea base image repo via ARG. |
templates/shared_services/gitea/docker/Dockerfile |
Parameterizes the upstream Gitea base image repo via ARG. |
resource_processor/vmss_porter/Dockerfile |
Parameterizes Python base image via ARG. |
api_app/Dockerfile |
Parameterizes Python base image via ARG. |
.github/workflows/build_docker_images.yml |
Passes build args to select builds so they can use mirrored base images when configured. |
.github/workflows/sync_acr_base_images.yml |
New workflow to periodically import selected upstream base images into ACR. |
docs/tre-admins/setup-instructions/workflows.md |
Documents ACR_BASE_IMAGE_PREFIX and the new ACR base image sync workflow. |
docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md |
Documents ACR_BASE_IMAGE_PREFIX and the new ACR base image sync workflow. |
…github.com/microsoft/AzureTRE into maxmartin-cgi/utilise-acr-for-docker-pulls
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/29093980082 (with refid (in response to this comment from @maxmartin-cgi) |
maxmartin-cgi
added
workflows
|
/test-extended 72ed8ef |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/29253755500 (with refid (in response to this comment from @maxmartin-cgi) |
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/29320243407 (with refid (in response to this comment from @maxmartin-cgi) |
marrobi
left a comment
marrobi
left a comment
There was a problem hiding this comment.
Before I do a manual review, here's Opus' thoughts:
1. build_docker_images.yml — workspace gitea build not updated
The build_docker_images.yml only passes GITEA_BASE_IMAGE_REPO for templates/shared_services/gitea. The templates/workspace_services/gitea/docker/Dockerfile also has the same GITEA_BASE_IMAGE_REPO ARG added, but the corresponding build step in the workflow doesn't pass the build arg. It will silently fall back to Docker Hub on every run.
2. Dockerfile.tmpl files not covered
templates/shared_services/gitea/Dockerfile.tmpl and templates/workspace_services/gitea/Dockerfile.tmpl both use FROM debian:bookworm-slim (which is in the sync list) but are not parameterised. If the intent is to cover these, they need the same ARG treatment. If not, the debian entries in the sync workflow's IMAGES array are unused and should be removed to avoid confusion.
3. Silent failure on digest fetch swallows errors
In sync_acr_base_images.yml, both docker buildx imagetools inspect calls use || true, which means any transient error (rate limit, network blip) silently results in an empty digest and triggers a full az acr import --force rather than failing the step. This could mask real problems. Consider removing || true and instead checking exit codes explicitly, or at minimum logging a warning when a digest is empty before proceeding.
4. Inconsistent ARG design — Gitea vs others
For Python, Maven, and guacd the full image reference including tag is passed as a single ARG (e.g. PYTHON_BASE_IMAGE=python:3.12-slim-bookworm). For Gitea the repo and tag are split (GITEA_BASE_IMAGE_REPO + GITEA_TAG). This is inconsistent. Consider aligning to one pattern.
5. Documentation: incorrect file path prefix
Both cicd-pre-deployment-steps.md and workflows.md reference workflow paths as /.github/workflows/... — the leading / should be dropped to match GitHub conventions: .github/workflows/....
6. "Emit repository variable hint" step adds no value
The final step in sync_acr_base_images.yml just echoes a string to the log. It doesn't set the variable, open an issue, or do anything actionable. Consider removing it or replacing it with something meaningful (e.g. a job summary using $GITHUB_STEP_SUMMARY).
|
/test-extended |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/sync_acr_base_images.yml:66
- The sync list does not include
python:3.13-slim-bookworm, buttemplates/shared_services/certs/Dockerfile.tmpluses that tag by default. Sincetemplates/shared_services/certs/is built/published in CI (e.g.deploy_tre_reusable.yml), this bundle will still pull from Docker Hub and can still be affected by rate limits even when ACR mirrors are otherwise enabled.
IMAGES=(
"docker.io/library/python|mirror/library/python|3.12-slim-bookworm"
"docker.io/library/maven|mirror/library/maven|3.9-eclipse-temurin-17-alpine"
"docker.io/guacamole/guacd|mirror/guacamole/guacd|1.6.0"
"docker.io/gitea/gitea|mirror/gitea/gitea|1.15"
|
/test |
|
🤖 pr-bot 🤖 🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/29760396474 (with refid (in response to this comment from @maxmartin-cgi) |
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/29847195977 (with refid (in response to this comment from @maxmartin-cgi) |
| ${DOCKER_BUILD_COMMAND} --build-arg BUILDKIT_INLINE_CACHE=1 \ | ||
| -t "${FULL_IMAGE_NAME_PREFIX}/${image_name}:${version}" \ | ||
| "${docker_cache[@]}" -f "${docker_file}" "${docker_context}" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/sync_acr_base_images.yml:70
- The mirror list doesn’t currently include
python:3.13-slim-bookworm, buttemplates/shared_services/certs/Dockerfile.tmpldefaultsPYTHON_TAGto3.13-slim-bookworm. If ACR mirroring is intended to reduce Docker Hub pulls consistently, this image/tag should be mirrored as well (or the certs template should align to the mirrored Python version).
IMAGES=(
"docker.io/library/python|mirror/library/python|3.12-slim-bookworm"
"docker.io/library/maven|mirror/library/maven|3.9-eclipse-temurin-17-alpine"
"docker.io/guacamole/guacd|mirror/guacamole/guacd|1.6.0"
"docker.io/gitea/gitea|mirror/gitea/gitea|1.15"
"docker.io/gitea/gitea|mirror/gitea/gitea|1.17.3"
"mcr.microsoft.com/azure-functions/python|mirror/mcr/azure-functions/python|4-python3.12"
"docker.io/library/debian|mirror/library/debian|bookworm-slim"
"docker.io/library/debian|mirror/library/debian|bullseye-slim"
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/build_docker_images.yml:63
use_acr_mirrorsis enabled based only on variables/secrets being non-empty. IfACR_BASE_IMAGE_PREFIXis accidentally set to a different registry thanACR_NAME, the workflow will still log into${{ secrets.ACR_NAME }}but try to pull base images from the mismatched prefix, causing hard-to-diagnose auth/pull failures. Consider additionally verifying thatACR_BASE_IMAGE_PREFIXmatches the configuredACR_NAMEbefore enabling mirrors.
if [[ -n "${ACR_BASE_IMAGE_PREFIX}" &&
-n "${ACR_NAME}" &&
-n "${AZURE_CLIENT_ID}" &&
-n "${AZURE_TENANT_ID}" &&
-n "${AZURE_SUBSCRIPTION_ID}" ]]; then
|
/test |
|
🤖 pr-bot 🤖 🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/30010349504 (with refid (in response to this comment from @maxmartin-cgi) |
…flow
Resolves #4917
What is being addressed
Pipeline runs fail with a 429: TOOMANYREQUESTS error whenever we hit rate limits on Docker Hub. This blocks all development and CICD pipeline runs until that limit expires. Here's an example.
How is this addressed