feat(ci): add buildkit dockerfile test for docker/Dockerfile - #1489
feat(ci): add buildkit dockerfile test for docker/Dockerfile#1489JavaPythonAIForBAT wants to merge 9 commits into
Conversation
Add a buildkit-dockerfile-test workflow that builds docker/Dockerfile on both arm64 and amd64 buildkit runners (no QEMU), using the internal cache service (apt mirror, pypi index, torch wheel). Also parameterize the Dockerfile: - Add APTMIRROR, PIP_INDEX_URL, PYTORCH_INDEX_URL, PIP_TRUSTED_HOST ARGs - Use APTMIRROR cache service for apt when provided, else huaweicloud mirror - Use PIP_INDEX_URL/PYTORCH_INDEX_URL for pip, else huaweicloud/pytorch defaults
github-actions
Bot
added
CICD
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,44 @@ | |||
| name: buildkit-dockerfile-test | |||
There was a problem hiding this comment.
[security · medium]
No explicit permissions are set for the workflow. While this workflow doesn't directly use GITHUB_TOKEN, it's a security best practice to explicitly declare minimal permissions (e.g., permissions: {}) to adhere to principle of least privilege and prevent accidental credential misuse.
Suggestion:
| name: buildkit-dockerfile-test | |
| name: buildkit-dockerfile-test | |
| permissions: {} | |
| on: |
| @@ -0,0 +1,44 @@ | |||
| name: buildkit-dockerfile-test | |||
There was a problem hiding this comment.
[maintainability · medium]
No concurrency group is defined. When the workflow is triggered by multiple pull_request events (e.g., pushes to the same PR or different PRs), redundant builds will run concurrently, wasting runner resources. Add a concurrency block to cancel in-progress runs for the same PR.
Suggestion:
| name: buildkit-dockerfile-test | |
| name: buildkit-dockerfile-test | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| on: |
| jobs: | ||
| build-test: |
There was a problem hiding this comment.
[maintainability · medium]
The job build-test lacks timeout-minutes. Docker builds can occasionally hang or take much longer than expected, especially on self-hosted runners. Without a timeout, a stuck build could consume runner resources indefinitely. Set an appropriate timeout based on expected build duration.
Suggestion:
| jobs: | |
| build-test: | |
| jobs: | |
| build-test: | |
| timeout-minutes: 60 |
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 1 | ||
| persist-credentials: false | ||
|
|
||
| - name: Build and push | ||
| uses: docker/build-push-action@v6 |
There was a problem hiding this comment.
[performance · medium]
The workflow uses docker/build-push-action but does not set up Docker Buildx explicitly and does not configure layer caching (cache-from/cache-to). Without caching, every build rebuilds all layers from scratch, which is slow and resource-intensive. Add a setup-buildx-action step and enable GitHub Actions cache (type=gha) for faster incremental builds.
Suggestion:
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 1 | |
| persist-credentials: false | |
| - name: Build and push | |
| uses: docker/build-push-action@v6 | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 1 | |
| persist-credentials: false | |
| - name: Set up Docker Buildx | |
| uses: docker/setup-buildx-action@v3 | |
| - name: Build and push | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: . | |
| file: docker/Dockerfile | |
| push: true | |
| tags: swr.cn-southwest-2.myhuaweicloud.com/modelfoundry/test-buildkit:triton-${{ matrix.runner_info.arch }}-${{ github.sha }} | |
| build-args: | | |
| CANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:8.5.0-a3-ubuntu22.04-py3.10 | |
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | |
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | |
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | |
| PYTORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpu | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max | |
| provenance: false |
| persist-credentials: false | ||
|
|
||
| - name: Build and push | ||
| uses: docker/build-push-action@v6 |
There was a problem hiding this comment.
[security · high]
Third-party action docker/build-push-action@v6 is pinned to a mutable tag (v6). Tags can be force-pushed or hijacked, making the workflow vulnerable to supply-chain attacks. Pin this action to a full commit SHA (e.g., docker/build-push-action@<sha>) for immutability.
Suggestion:
| uses: docker/build-push-action@v6 | |
| uses: docker/build-push-action@b32b51a8eda65d6793cd0494a773d4f6bcef32dc |
| context: . | ||
| file: docker/Dockerfile | ||
| push: true | ||
| tags: swr.cn-southwest-2.myhuaweicloud.com/modelfoundry/test-buildkit:triton-${{ matrix.runner_info.arch }}-${{ github.sha }} |
There was a problem hiding this comment.
[bug · critical]
The build-push-action step has push: true but there is no prior docker/login-action step to authenticate with the target registry (swr.cn-southwest-2.myhuaweicloud.com). Pushing to a private registry without authentication will fail. Add a login step before build/push, or ensure the credential is available via a pre-configured Docker config on the runner.
Suggestion:
| tags: swr.cn-southwest-2.myhuaweicloud.com/modelfoundry/test-buildkit:triton-${{ matrix.runner_info.arch }}-${{ github.sha }} | |
| # Add before Build and push step: | |
| - name: Login to SWR | |
| uses: docker/login-action@v3 | |
| with: | |
| registry: swr.cn-southwest-2.myhuaweicloud.com | |
| username: ${{ secrets.SWR_USERNAME }} | |
| password: ${{ secrets.SWR_PASSWORD }} |
| build-args: | | ||
| CANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:8.5.0-a3-ubuntu22.04-py3.10 | ||
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | ||
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | ||
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | ||
| PYTORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpu |
There was a problem hiding this comment.
[bug · high]
The workflow runs inside a container (swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:9.0.0-a3-ubuntu22.04-py3.12) and the docker/build-push-action references internal Kubernetes service URLs (cache-service.nginx-pypi-cache.svc.cluster.local) in build-args. These .svc.cluster.local DNS names are only resolvable from within a Kubernetes cluster. If the self-hosted runners are not running inside the same cluster, or if DNS resolution is not properly configured inside the workflow container, the Docker build will fail because it cannot reach these internal endpoints. Consider making these endpoints configurable or ensuring the runner environment has DNS access to the cluster's internal DNS.
Suggestion:
| build-args: | | |
| CANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:8.5.0-a3-ubuntu22.04-py3.10 | |
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | |
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | |
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | |
| PYTORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpu | |
| Consider using secrets or environment-specific variables for these internal endpoints, and add a fallback to public mirrors in the Dockerfile (the Dockerfile already has fallback logic for APTMIRROR). |
| ARG PIP_TRUSTED_HOST="" | ||
|
|
||
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[bug · high]
The sed regex (ports|archive).ubuntu.com does not match security.ubuntu.com. In Ubuntu 22.04's /etc/apt/sources.list, security updates are often served by security.ubuntu.com. When a custom APTMIRROR is provided, these entries will not be rewritten, which could cause package installation failures or fallback to the public internet.
Consider also replacing security.ubuntu.com (and ports.ubuntu.com is already covered).
Suggestion:
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ | |
| sed -Ei "s@(ports|archive|security).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; |
| ARG PIP_TRUSTED_HOST="" | ||
|
|
||
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[bug · medium]
When APTMIRROR ends with a trailing slash (e.g., http://mirror.example.com/), the #http:// prefix stripping leaves a trailing slash, and sed produces http://mirror.example.com//ubuntu.com — a double slash. This can cause apt update failures.
Consider stripping trailing slashes from APTMIRROR, for example:
APTMIRROR="${APTMIRROR%/}" before using it in sed.
Suggestion:
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ | |
| APTMIRROR="${APTMIRROR%/}" && sed -Ei "s@(ports|archive|security).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; |
The /whl/cpu path is a find-links directory for torch wheels, not a general pypi index. Using it as extra-index-url made pip query it for every package (auditwheel, cmake, einops...), causing read timeouts. torch comes from the cann base image, so a separate torch index is not needed.
| name: buildkit-dockerfile-test | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: |
There was a problem hiding this comment.
[security · medium]
The workflow lacks a concurrency group. For pull_request triggers, rapid successive pushes (e.g., force-pushing amended commits) can spawn multiple redundant builds for the same PR. Add a concurrency group keyed on the PR ref to cancel in-progress runs and avoid wasted resources.
Suggestion:
| name: buildkit-dockerfile-test | |
| on: | |
| pull_request: | |
| paths: | |
| name: buildkit-dockerfile-test | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | |
| cancel-in-progress: true | |
| on: | |
| pull_request: | |
| paths: |
| name: buildkit-dockerfile-test | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - '.github/workflows/buildkit-dockerfile-test.yaml' | ||
| - 'docker/Dockerfile' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
[security · low]
No permissions key is declared for the workflow. While the default permissions for pull_request events are read-only (secure), adding an explicit least-privilege permissions: block at the workflow or job level makes the security posture transparent and prevents accidental privilege creep when triggers change.
Suggestion:
| name: buildkit-dockerfile-test | |
| on: | |
| pull_request: | |
| paths: | |
| - '.github/workflows/buildkit-dockerfile-test.yaml' | |
| - 'docker/Dockerfile' | |
| workflow_dispatch: | |
| name: buildkit-dockerfile-test | |
| permissions: | |
| contents: read | |
| on: | |
| pull_request: | |
| paths: | |
| - '.github/workflows/buildkit-dockerfile-test.yaml' | |
| - 'docker/Dockerfile' | |
| workflow_dispatch: |
| on: | ||
| pull_request: | ||
| paths: | ||
| - '.github/workflows/buildkit-dockerfile-test.yaml' | ||
| - 'docker/Dockerfile' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
[security · low]
Triggers pull_request (not pull_request_target) — no dangerous checkout of PR head code with elevated permissions. The paths filter correctly limits runs to only when relevant files change. Good.
| build-test: | ||
| name: "build docker/Dockerfile (${{ matrix.runner_info.arch }})" | ||
| runs-on: ${{ matrix.runner_info.runner }} |
There was a problem hiding this comment.
[security · high]
No timeout-minutes is set for the job. The runners (linux-aarch64-cpu-4, linux-amd64-cpu-4) appear to be self-hosted runners. Without a timeout, a stuck build could run indefinitely, consuming runner resources and potentially racking up costs. Add a reasonable timeout (e.g., 60 minutes) at the job level.
Suggestion:
| build-test: | |
| name: "build docker/Dockerfile (${{ matrix.runner_info.arch }})" | |
| runs-on: ${{ matrix.runner_info.runner }} | |
| build-test: | |
| name: "build docker/Dockerfile (${{ matrix.runner_info.arch }})" | |
| runs-on: ${{ matrix.runner_info.runner }} | |
| timeout-minutes: 60 |
| container: | ||
| image: swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:9.0.0-a3-ubuntu22.04-py3.12 |
There was a problem hiding this comment.
[security · low]
The container image for the job uses a specific tag (cann:9.0.0-a3-ubuntu22.04-py3.12), which is good. However, consider using a digest (@sha256:...) for the container image as well to ensure immutability and reproducibility of the build environment.
Suggestion:
| container: | |
| image: swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:9.0.0-a3-ubuntu22.04-py3.12 | |
| container: | |
| image: swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann@sha256:<digest> |
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | ||
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | ||
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | ||
| provenance: false |
There was a problem hiding this comment.
[security · low]
The provenance: false setting disables BuildKit attestation metadata. While not a security vulnerability per se, disabling provenance means the built image lacks software supply-chain metadata (e.g., build timestamps, source repo info). Consider whether provenance should be enabled for traceability, especially for images pushed to a registry.
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | ||
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | ||
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | ||
| provenance: false |
There was a problem hiding this comment.
[performance · medium]
No cache configuration is set for the build. Docker layer caching (type=gha or type=registry) can significantly speed up repeated builds. Consider adding cache-from: type=gha and cache-to: type=gha,mode=max to leverage GitHub Actions cache for Docker layers, especially since this workflow builds for two architectures.
Suggestion:
| provenance: false | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max | |
| provenance: false |
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[bug · medium]
When APTMIRROR is provided, the sed pattern (ports|archive).ubuntu.com only matches archive.ubuntu.com and ports.ubuntu.com, but does not match security.ubuntu.com. Ubuntu's standard /etc/apt/sources.list typically contains separate entries for security.ubuntu.com (e.g., deb http://security.ubuntu.com/ubuntu jammy-security main restricted). In environments without internet access (like the internal CI network configured in the workflow), this would cause apt operations against security.ubuntu.com to fail. The old fallback path correctly handled both archive.ubuntu.com and security.ubuntu.com — the custom mirror path should do the same.
Suggestion:
| RUN if [ -n "$APTMIRROR" ]; then \ | |
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ | |
| RUN if [ -n "$APTMIRROR" ]; then \ | |
| sed -Ei "s@(ports|archive|security).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[bug · medium]
The shell parameter expansion ${APTMIRROR#http://} only strips the http:// prefix. If APTMIRROR is passed with https:// (e.g., https://internal-mirror.example.com), the prefix won't be stripped, resulting in a malformed URL like http://https://internal-mirror.example.com/ubuntu/.... This makes the build silently produce incorrect apt sources. Consider stripping any protocol prefix to handle both http:// and https:// cases.
Suggestion:
| RUN if [ -n "$APTMIRROR" ]; then \ | |
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ | |
| RUN if [ -n "$APTMIRROR" ]; then \ | |
| mirror_host="${APTMIRROR#*://}" \ | |
| sed -Ei "s@(ports|archive|security).ubuntu.com@${mirror_host}@g" /etc/apt/sources.list; \ |
| RUN mkdir -p /root/.config/pip && \ | ||
| echo -e "[global]\nindex-url = https://repo.huaweicloud.com/repository/pypi/simple\nextra-index-url = https://download.pytorch.org/whl/cpu" > /root/.config/pip/pip.conf | ||
| printf '[global]\nindex-url = %s\n' "$PIP_INDEX_URL" > /root/.config/pip/pip.conf && \ |
There was a problem hiding this comment.
[maintainability · low]
The previous pip.conf configuration included extra-index-url = https://download.pytorch.org/whl/cpu alongside the primary index URL. This line has been removed entirely in the new dynamic configuration. If any packages in requirements.txt or requirements_dev.txt depend on PyTorch packages from that extra index, installations may fail. Please confirm that this removal is intentional — either because the internal proxy now mirrors PyTorch packages, or because PyTorch is no longer a dependency.
torch-npu requires torch==2.7.1+cpu which is only available on the PyTorch CPU index (https://download.pytorch.org/whl/cpu), not on PyPI. Use the real PyTorch URL as extra-index-url, not the cache service (/whl/cpu is a find-links directory, not a proper simple index).
| ARG PIP_TRUSTED_HOST="" | ||
|
|
||
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[maintainability · low]
The ${APTMIRROR#http://} parameter expansion only strips the http:// protocol prefix, but not https://. If a user passes an https:// mirror URL (e.g., https://mirror.internal.com), the stripping will not remove the protocol, resulting in a malformed sources.list entry like deb http://https://mirror.internal.com/....
Suggestion: Use a more robust approach to strip any protocol prefix, or document that only http:// mirrors are supported. For example, you could strip both protocols using bash pattern matching or sed substitution:
mirror=${APTMIRROR#http://}
mirror=${mirror#https://}
sed -Ei "s@(ports|archive).ubuntu.com@${mirror}@g" /etc/apt/sources.list| ARG PIP_TRUSTED_HOST="" | ||
|
|
||
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[bug · high]
When APTMIRROR is set, only archive.ubuntu.com and ports.ubuntu.com are replaced in sources.list, but security.ubuntu.com is not handled. On Ubuntu 22.04 (the base image version), amd64 images have separate security.ubuntu.com entries in sources.list. If the CI/build environment uses an internal mirror without public internet access, the apt update step will fail when trying to reach security.ubuntu.com.
Suggestion: Add a sed command to also replace security.ubuntu.com when APTMIRROR is set, similar to the else branch which handles it:
sed -Ei "s@security[.]ubuntu[.]com@${APTMIRROR#http://}@g" /etc/apt/sources.list| ARG PIP_TRUSTED_HOST="" | ||
|
|
||
| RUN if [ -n "$APTMIRROR" ]; then \ | ||
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ |
There was a problem hiding this comment.
[maintainability · low]
When APTMIRROR is set, the sed command lacks || true (or equivalent error handling), making the build fail if the sed pattern doesn't match any line. The else branch uses 2>/dev/null || true for exactly this reason — the Ubuntu base image may have a different sources.list format or the patterns may not be present. For consistency and robustness, the if-branch should also handle this gracefully.
Suggestion:
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list; \ | |
| sed -Ei "s@(ports|archive).ubuntu.com@${APTMIRROR#http://}@g" /etc/apt/sources.list 2>/dev/null || true |
| printf '[global]\nindex-url = %s\nextra-index-url = %s\n' "$PIP_INDEX_URL" "$PYTORCH_INDEX_URL" > /root/.config/pip/pip.conf && \ | ||
| if [ -n "$PIP_TRUSTED_HOST" ]; then printf 'trusted-host = %s\n' "$PIP_TRUSTED_HOST" >> /root/.config/pip/pip.conf; fi |
There was a problem hiding this comment.
[security · low]
Using printf with format specifiers %s to interpolate build args (PIP_INDEX_URL, PYTORCH_INDEX_URL, PIP_TRUSTED_HOST) is unsafe if these values contain % characters. Though unlikely with current CI values, a future user passing a URL containing % (e.g., %2F for URL-encoded paths) would cause garbled output.
Consider using cat with a heredoc or redirecting echo content to avoid printf format-string interpretation.
Suggestion:
| printf '[global]\nindex-url = %s\nextra-index-url = %s\n' "$PIP_INDEX_URL" "$PYTORCH_INDEX_URL" > /root/.config/pip/pip.conf && \ | |
| if [ -n "$PIP_TRUSTED_HOST" ]; then printf 'trusted-host = %s\n' "$PIP_TRUSTED_HOST" >> /root/.config/pip/pip.conf; fi | |
| { echo '[global]'; echo "index-url = $PIP_INDEX_URL"; echo "extra-index-url = $PYTORCH_INDEX_URL"; } > /root/.config/pip/pip.conf && \ | |
| if [ -n "$PIP_TRUSTED_HOST" ]; then echo "trusted-host = $PIP_TRUSTED_HOST" >> /root/.config/pip/pip.conf; fi |
torch==2.7.1+cpu is only available on the PyTorch CPU index, which is slow to download from download.pytorch.org. Install it in a separate RUN layer using the internal cache service (/whl/cpu, 20min timeout, cached 1 month), and remove the extra-index-url from pip.conf so the requirements install no longer queries the slow torch index for every package. - Replace PYTORCH_INDEX_URL with TORCH_INDEX_URL (cache service) - pip.conf now only has the main index (no extra-index-url) - requirements install relies on torch being pre-installed (torch-npu dependency torch==2.7.1+cpu is satisfied)
--index-url made pip resolve ALL of torch's dependencies (e.g. filelock) against the torch-only /whl/cpu index, which timed out. --find-links only uses the URL as an extra wheel source for torch while resolving dependencies from the main index (pip.conf index-url).
pip's default 15s read timeout is too short when the cache service streams the torch wheel from the slow upstream (download.pytorch.org). Set timeout to 600s in both pip.conf and the torch install command.
Use docker/login-action to authenticate to SWR at build time with GitHub secrets (SWR_USERNAME/SWR_PASSWORD), instead of relying on the pre-configured docker config.json injected via Vault. buildx forwards the client registry credentials to the remote buildkitd via the gRPC session, so no buildkitd config change or restart is needed to add or rotate registry credentials.
7ec10ab to
2643194
Compare
| name: buildkit-dockerfile-test | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - '.github/workflows/buildkit-dockerfile-test.yaml' | ||
| - 'docker/Dockerfile' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
[security · medium]
The workflow lacks a top-level permissions key. Without explicit permissions, the default access level depends on the triggering event and repository context — for pull_request events from the same repository, the default is write-all, which grants broad, unnecessary access. Since this workflow passes secrets (SWR_USERNAME, SWR_PASSWORD) and only needs to check out code, log into a container registry, and push images, it should restrict permissions to the minimum. Other workflows in this repository demonstrate two safe approaches:
permissions: read-all(ci.yml)permissions: {}with per-jobpermissions:blocks (Ascend950-ci.yml)
Recommendation: Add permissions: read-all at the top level, or permissions: {} and grant only contents: read at the job level.
Suggestion:
| name: buildkit-dockerfile-test | |
| on: | |
| pull_request: | |
| paths: | |
| - '.github/workflows/buildkit-dockerfile-test.yaml' | |
| - 'docker/Dockerfile' | |
| workflow_dispatch: | |
| name: buildkit-dockerfile-test | |
| on: | |
| pull_request: | |
| paths: | |
| - '.github/workflows/buildkit-dockerfile-test.yaml' | |
| - 'docker/Dockerfile' | |
| workflow_dispatch: | |
| permissions: read-all |
| on: | ||
| pull_request: | ||
| paths: | ||
| - '.github/workflows/buildkit-dockerfile-test.yaml' | ||
| - 'docker/Dockerfile' | ||
| workflow_dispatch: | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
[maintainability · low]
The workflow does not define a concurrency group. When multiple commits are pushed to the same pull request in quick succession, redundant workflow runs will be triggered and executed in parallel, wasting runner resources. Adding a concurrency group with cancel-in-progress: true ensures only the latest run proceeds and cancels any in-progress runs for the same PR. This pattern is already used in other workflows in this repository (e.g., ci.yml and Ascend950-ci.yml).
Suggestion:
| on: | |
| pull_request: | |
| paths: | |
| - '.github/workflows/buildkit-dockerfile-test.yaml' | |
| - 'docker/Dockerfile' | |
| workflow_dispatch: | |
| jobs: | |
| on: | |
| pull_request: | |
| paths: | |
| - '.github/workflows/buildkit-dockerfile-test.yaml' | |
| - 'docker/Dockerfile' | |
| workflow_dispatch: | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| jobs: |
| jobs: | ||
| build-test: | ||
| name: "build docker/Dockerfile (${{ matrix.runner_info.arch }})" | ||
| runs-on: ${{ matrix.runner_info.runner }} | ||
| container: | ||
| image: swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:9.0.0-a3-ubuntu22.04-py3.12 |
There was a problem hiding this comment.
[bug · medium]
The job build-test runs on self-hosted runners (linux-aarch64-cpu-4, linux-amd64-cpu-4) but does not define timeout-minutes. Without a timeout, a long-running or stuck build (e.g., hanging network call, infinite loop in Docker build) could consume runner resources indefinitely, potentially blocking other jobs and incurring unnecessary costs. Other workflows in this repository set timeouts: Ascend950-ci.yml uses timeout-minutes: 120 for build jobs and timeout-minutes: 360 for test jobs. Consider adding an appropriate timeout based on expected build duration.
Suggestion:
| jobs: | |
| build-test: | |
| name: "build docker/Dockerfile (${{ matrix.runner_info.arch }})" | |
| runs-on: ${{ matrix.runner_info.runner }} | |
| container: | |
| image: swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:9.0.0-a3-ubuntu22.04-py3.12 | |
| jobs: | |
| build-test: | |
| name: "build docker/Dockerfile (${{ matrix.runner_info.arch }})" | |
| runs-on: ${{ matrix.runner_info.runner }} | |
| timeout-minutes: 120 | |
| container: | |
| image: swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:9.0.0-a3-ubuntu22.04-py3.12 |
| - name: Login to SWR | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| registry: swr.cn-southwest-2.myhuaweicloud.com | ||
| username: ${{ secrets.SWR_USERNAME }} | ||
| password: ${{ secrets.SWR_PASSWORD }} | ||
|
|
||
| - name: Build and push | ||
| uses: docker/build-push-action@v6 |
There was a problem hiding this comment.
[security · medium]
The third-party actions docker/login-action@v3 and docker/build-push-action@v6 are referenced using mutable tags (v3, v6). While the repository already uses this pattern consistently, tags can be overwritten by the action owner, creating a supply-chain risk where an updated tag could introduce unexpected behavior or malicious code. Pinning to a full commit SHA eliminates this risk. If maintainability is a concern, consider using Dependabot with a commit-pinning configuration to automate SHA updates.
Suggestion:
| - name: Login to SWR | |
| uses: docker/login-action@v3 | |
| with: | |
| registry: swr.cn-southwest-2.myhuaweicloud.com | |
| username: ${{ secrets.SWR_USERNAME }} | |
| password: ${{ secrets.SWR_PASSWORD }} | |
| - name: Build and push | |
| uses: docker/build-push-action@v6 | |
| - name: Login to SWR | |
| uses: docker/login-action@e92387c33f0dc5b9e5eca8b5e0c36d5e2f39e4c2 # v3 | |
| with: | |
| registry: swr.cn-southwest-2.myhuaweicloud.com | |
| username: ${{ secrets.SWR_USERNAME }} | |
| password: ${{ secrets.SWR_PASSWORD }} | |
| - name: Build and push | |
| uses: docker/build-push-action@ca877d1a5cebe945fa4ff4c6e3a4c7f7e5e2c3b4 # v6 |
| - name: Build and push | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: . | ||
| file: docker/Dockerfile | ||
| push: true | ||
| tags: swr.cn-southwest-2.myhuaweicloud.com/modelfoundry/test-buildkit:triton-${{ matrix.runner_info.arch }}-${{ github.sha }} | ||
| build-args: | | ||
| CANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:8.5.0-a3-ubuntu22.04-py3.10 | ||
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | ||
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | ||
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | ||
| TORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpu | ||
| provenance: false |
There was a problem hiding this comment.
[performance · low]
The build step pushes the image but does not configure Docker layer caching. Without cache-from / cache-to, every run rebuilds all layers from scratch, which increases build time and wastes runner resources. The existing build-docker-image.yml workflow in the same repository already uses GitHub Actions cache for Docker layers:
cache-from: type=gha
cache-to: type=gha,mode=maxAdding similar caching would significantly speed up subsequent builds for the same branch/PR.
Suggestion:
| - name: Build and push | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: . | |
| file: docker/Dockerfile | |
| push: true | |
| tags: swr.cn-southwest-2.myhuaweicloud.com/modelfoundry/test-buildkit:triton-${{ matrix.runner_info.arch }}-${{ github.sha }} | |
| build-args: | | |
| CANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:8.5.0-a3-ubuntu22.04-py3.10 | |
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | |
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | |
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | |
| TORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpu | |
| provenance: false | |
| - name: Build and push | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: . | |
| file: docker/Dockerfile | |
| push: true | |
| tags: swr.cn-southwest-2.myhuaweicloud.com/modelfoundry/test-buildkit:triton-${{ matrix.runner_info.arch }}-${{ github.sha }} | |
| build-args: | | |
| CANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:8.5.0-a3-ubuntu22.04-py3.10 | |
| APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081 | |
| PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simple | |
| PIP_TRUSTED_HOST=cache-service.nginx-pypi-cache.svc.cluster.local | |
| TORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpu | |
| provenance: false | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max |
1 participant
Summary
Add a buildkit-dockerfile-test workflow that builds
docker/Dockerfileon both arm64 and amd64 buildkit runners natively (no QEMU), using the internal cache service for faster and more reliable builds.This is adapted from the same pattern used in vllm-ascend (PR #13188).
Changes
New workflow:
.github/workflows/buildkit-dockerfile-test.yamldocker/Dockerfileand the workflow itselflinux-aarch64-cpu-4-buildkitandlinux-amd64-cpu-4-buildkitrunnersdocker/Dockerfileusingdocker/build-push-actionwith:APTMIRROR=http://cache-service.nginx-pypi-cache.svc.cluster.local:8081PIP_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/pypi/simplePYTORCH_INDEX_URL=http://cache-service.nginx-pypi-cache.svc.cluster.local/whl/cpuCANN_BASE_IMAGE=swr.cn-southwest-2.myhuaweicloud.com/base_image/ascend-ci/cann:...Modified:
docker/DockerfileARG APTMIRROR,ARG PIP_INDEX_URL,ARG PYTORCH_INDEX_URL,ARG PIP_TRUSTED_HOST