feat(dockerfiles): add tidb-operator CI base images with skaffold and workflow support#4764
Conversation
Add detection, build, and publish workflows for tidb-operator Docker images under dockerfiles/ci/tidb-operator/, including the e2e-base and backup-manager-base images.
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR adds new base Docker images for the tidb-operator CI, introduces skaffold configuration for multi-platform builds of these images, and integrates these builds into the GitHub Actions workflows for PR checks and releases. The changes include well-structured Dockerfiles with cache mounts, pinning base images by digest, and improved URL handling for downloads. The GitHub workflows follow existing patterns for modular, matrix-based builds and include caching and multi-arch support. Overall, the implementation is solid but has several areas for improvement in robustness, maintainability, and minor correctness details.
Critical Issues
-
Dockerfile
backup-manager-base: MissingRCLONE_VERSIONARG default value- File:
dockerfiles/ci/tidb-operator/backup-manager-base/Dockerfile - The
RCLONE_VERSIONARG is referenced in the curl URL but never declared or given a default. This will cause build failures unless passed explicitly. - Suggestion: Add a default value for
RCLONE_VERSIONsimilar toSHUSH_VERSIONfor consistent builds, e.g.:ARG RCLONE_VERSION=v1.71.2
- File:
-
Dockerfile
e2e-base:AWS_ARCHvariable casing inconsistency- File:
dockerfiles/ci/tidb-operator/e2e-base/Dockerfilelines ~21-27 - The shell script sets
aws_arch(lowercase), but the curl URL uses${AWS_ARCH}(uppercase), which is unset, causing the AWS CLI download URL to be invalid. - Suggestion: Use consistent casing for the variable, e.g.:
aws_arch=... # lowercase ... curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-${aws_arch}.zip" -o awscliv2.zip
- File:
-
GitHub Actions: Skipping push in PR workflow but no explicit cache usage
- The PR workflow builds images with
--push=falsebut passes--cache-artifactsto skaffold. If no push happens, the cache might not be persisted or used effectively, potentially causing slower rebuilds. - Suggestion: Ensure local caching is enabled and effective between runs or clarify expectations in comments.
- The PR workflow builds images with
Code Improvements
-
Use of
ADDfor remote URL in Dockerfile- File:
dockerfiles/ci/tidb-operator/e2e-base/Dockerfileline ~30 - Using
ADDto fetch a remote URL is convenient but less explicit thancurl. It also does not allow retry logic or verification. - Suggestion: Consider switching to a
curl+ checksum verification step for better reliability and security, e.g.:RUN curl -fsSL -o /cert-manager.yaml "https://github.com/cert-manager/cert-manager/releases/download/${CERT_MANAGER_VERSION}/cert-manager.yaml"
- File:
-
Dockerfile: Combine related RUN commands for smaller image layers
- Both Dockerfiles have multiple RUN commands that could be combined to reduce image layers and improve caching behavior, e.g., install packages and download tools in one RUN block.
-
Dockerfile: Pinning all external tool versions
kubectl,helm,cert-manager,shush, andrcloneversions should be clearly defined as ARGs with default values for reproducibility (onlyshushversion is declared as ARG).- Suggestion: Add ARG declarations for all external tools' versions with explicit defaults.
-
GitHub Actions: Use latest stable tags for actions
- Several actions use commit SHA pins (e.g.,
actions/checkout@de0fac2e4...). While pinning is good for stability, consider using tags for easier maintenance unless specific bug fixes require pinning. - Suggestion: Evaluate if tags like
actions/checkout@v3suffice.
- Several actions use commit SHA pins (e.g.,
Best Practices
-
Documentation: Missing comments in Dockerfiles
- Both Dockerfiles would benefit from brief explanatory comments about what each major step installs and why (e.g., why certain tools are included).
- Also add a comment for the purpose of these images in the repo or a README in
dockerfiles/ci/tidb-operator/.
-
Testing: No indication of test coverage for new images or workflows
- Ensure PR or release jobs include steps to validate the built images, e.g., by running smoke tests or verifying presence of installed tools.
- Add test workflow triggers or scripts to verify functionality of these images.
-
Naming: Image names could be more descriptive
- For example,
tidb-operator/e2e-baseandtidb-operator/backup-manager-baseare fine but consider adding tags or labels inside Dockerfiles to indicate version or purpose. - Add labels in Dockerfiles for metadata, e.g.:
LABEL maintainer="team@pingcap.com" LABEL version="${KUBECTL_VERSION}" LABEL description="Base image for tidb-operator e2e tests"
- For example,
-
GitHub Workflow: Matrix platform runner selection
- The
skaffold-tidb-operatorjob uses a conditional inruns-onthat’s somewhat complex and could be simplified or documented better. - Consider using matrix
runs-ondirectly or document why arm64 uses a different runner.
- The
-
GitHub Workflow: Explicitly declare all needed permissions
contents: readandpackages: writeare set, but if pushing images to GHCR or other actions require additional scopes, verify and document.
By addressing the above, this PR will be more robust, maintainable, and secure, ensuring smooth CI operations for tidb-operator base images.
…rfile - Remove erroneous semicolon before `&&` continuation - Uncomment ARG RCLONE_VERSION declaration
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces new base Docker images for the tidb-operator CI, including e2e-base and backup-manager-base, along with Skaffold configuration and GitHub Actions workflows for building and releasing these images with multi-platform support. The approach modernizes Dockerfiles by pinning base images to digests, using cache mounts, and simplifying installation steps. The workflows integrate these images into the existing CI ecosystem effectively. Overall, the changes are thoughtfully designed with good reproducibility and CI/CD practices, but some improvements and minor fixes can further enhance robustness and maintainability.
Critical Issues
-
Dockerfile typo in
e2e-base/Dockerfile(Line ~22):curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-${AWS_ARCH}.zip" -o awscliv2.zip; \The variable used in the URL is
${AWS_ARCH}, but the variable declared isaws_arch(lowercase). This will cause the AWS CLI download URL to be malformed and the build to fail.Suggestion: Change
${AWS_ARCH}to${aws_arch}in the URL to use the correct shell variable.
Code Improvements
-
Add explicit
ENVorARGforAWS_ARCHine2e-base/Dockerfile(Line ~18-24):
Currently,aws_archis a shell variable inside aRUNcommand. To improve readability and potential reuse, consider using anARGandENVfor the architecture or at least document this carefully. -
Pin versions for all external downloads to ensure reproducibility:
While you pinned cert-manager and rclone versions, the AWS CLI install uses a direct latest path. Consider pinning the AWS CLI version explicitly or documenting how the version is managed. -
Use
ADDvscurlconsistently:
You useADDto fetchcert-manager.yamlbutcurlfor binaries. WhileADDcan handle URLs, it's generally clearer to usecurlorwgetfor binaries andADDfor static files. Consider consistency and documentation on why one is preferred over the other. -
Add
--no-install-recommendstoapt-get installine2e-baseDockerfile:
This reduces image size by avoiding unnecessary recommended packages:apt-get install -y --no-install-recommends ca-certificates curl git openssl default-mysql-client unzip python3
-
Add
rm -rf /var/lib/apt/lists/*afterapt-get installfor image size reduction:
Even with cache mounts, cleaning apt lists after install is a good practice to keep images lean. -
Use explicit shell form for multi-line RUN commands for readability:
Some RUN commands could be split or documented better for maintainability.
Best Practices
-
Add README or documentation for new Docker images and Skaffold module:
This will help future maintainers and users understand the purpose and usage ofe2e-baseandbackup-manager-baseimages and the newci-tidb-operatorSkaffold module. -
Improve naming consistency in workflows:
The GitHub Actions job names use underscores (skaffold-tidb-operator) while others use hyphens. Choose one style for consistency, preferably hyphens as per GitHub Actions conventions. -
Add tests or verify the new images in the build pipeline:
The workflows build and publish images but do not run any tests on the images. Adding smoke tests or container startup verification would increase confidence. -
Add comments in workflow files describing new jobs and matrix strategy:
This aids readability and maintenance. -
Consider adding explicit
platformsin the Dockerfiles withTARGETARCHargs:
While multi-platform build is configured, ensure Dockerfiles are fully compatible and tested on bothamd64andarm64.
Summary of Suggested Fixes
# dockerfiles/ci/tidb-operator/e2e-base/Dockerfile, lines ~22-24:
RUN set -eux; \
case "${TARGETARCH}" in \
amd64) aws_arch=x86_64 ;; \
arm64) aws_arch=aarch64 ;; \
*) echo "unsupported TARGETARCH=${TARGETARCH}" >&2; exit 1 ;; \
esac; \
curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-${aws_arch}.zip" -o awscliv2.zip; \
unzip awscliv2.zip; \
./aws/install; \
rm -rf aws awscliv2.zip# Add --no-install-recommends and cleanup for apt-get install in e2e-base
RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
--mount=type=cache,target=/var/lib/apt/lists,sharing=locked \
apt-get update && \
apt-get install -y --no-install-recommends ca-certificates curl git openssl default-mysql-client unzip python3 && \
rm -rf /var/lib/apt/lists/*Overall, this PR adds valuable CI infrastructure support for tidb-operator with good practices around reproducibility and multi-platform builds. Addressing the critical AWS CLI variable typo and improving documentation and minor Dockerfile enhancements will significantly increase maintainability and reliability.
aws_arch (lowercase) was set but AWS_ARCH (uppercase) was referenced
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR adds new Dockerfiles for tidb-operator CI base images, improves existing Dockerfiles with better caching and reproducibility, and integrates these images into the CI/CD workflows using Skaffold and GitHub Actions. The approach uses multi-platform builds with cache mounts and pins base images by digest for stability. The changes appear well-structured and aligned with modern container build best practices.
Critical Issues
-
Dockerfile
backup-manager-base: Missing cleanup of dnf cache- File:
dockerfiles/ci/tidb-operator/backup-manager-base/Dockerfile(lines 4-6) - Issue:
dnf installis run with cache mount, but there is no cleanup or explicitdnf clean allafterward. This can cause larger image size due to leftover cache. - Suggestion: Add
dnf clean allafter installation inside the same RUN command to reduce final image size, e.g.:RUN --mount=type=cache,target=/var/cache/dnf,sharing=locked \ dnf install -y ca-certificates bind-utils wget nc unzip && \ dnf clean all
- File:
-
Dockerfile
e2e-base: Missing apt cleanup- File:
dockerfiles/ci/tidb-operator/e2e-base/Dockerfile(lines 6-10) - Issue: Apt caches are mounted for speed, but after installation, the package lists and cache are not cleaned. Without cleanup, image size may bloat.
- Suggestion: Add
rm -rf /var/lib/apt/lists/*and/orapt-get cleanafter install in the same RUN:RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ --mount=type=cache,target=/var/lib/apt/lists,sharing=locked \ apt-get update && \ apt-get install -y ca-certificates curl git openssl default-mysql-client unzip python3 && \ rm -rf /var/lib/apt/lists/*
- File:
-
Dockerfile
e2e-base: AWS CLI install cleanup- File:
dockerfiles/ci/tidb-operator/e2e-base/Dockerfile(lines 15-24) - Issue: The AWS CLI installation unzips and installs but does not remove installation artifacts completely, potentially increasing image size.
- Suggestion: The current cleanup
rm -rf aws awscliv2.zipis good, but verify no hidden temp files remain. Also consider combining commands into one RUN to ensure layer cleanliness.
- File:
-
GitHub Actions workflow: Conditional for
skaffold-tidb-operatorjob- File:
.github/workflows/pull-ci-runtime-images.yamland.github/workflows/release-ci-runtime-images.yaml(lines ~208-260) - Issue: The condition for running tidb-operator skaffold jobs uses
needs.detect-changes.outputs.tidb-operator == 'true' || needs.detect-changes.outputs.skaffold == 'true'. If the detection logic is inaccurate or theskaffoldoutput is overly broad, it could trigger unnecessary builds. - Suggestion: Confirm that the
detect-changesstep reliably detects changes only relevant to tidb-operator and skaffold. Consider more granular detection or caching to avoid redundant builds.
- File:
Code Improvements
-
Consolidate Dockerfile ARG usage for TARGETARCH
- Both Dockerfiles use
ARG TARGETARCHbut do not explicitly set it when building. Document or ensure that CI/build process always passes--build-arg TARGETARCHfor reproducibility. - Suggest adding a comment or README snippet explaining which build args are required.
- Both Dockerfiles use
-
Use consistent versions for pinned tools
cert-managerversion is pinned tov1.15.1ine2e-baseDockerfile, but no explicit pin forrcloneandshushexceptv1.71.2andv1.5.5respectively inbackup-manager-base.- Suggest centralizing version definitions or documenting them for easier updates.
-
Use
COPYinstead ofADDif possible- In
e2e-baseDockerfile,ADDis used to download cert-manager yaml from a URL.ADDsupports URLs but can be less explicit. - Suggest switching to
RUN curl -fsSL ... -o /cert-manager.yamlfor clarity and better caching control, e.g.:RUN curl -fsSL https://github.com/cert-manager/cert-manager/releases/download/${CERT_MANAGER_VERSION}/cert-manager.yaml -o /cert-manager.yaml
- In
-
Skaffold config: Add comments for new module
- The
ci-tidb-operatorconfig indockerfiles/ci/skaffold.yamllacks descriptive comments. - Suggest adding a short comment explaining the purpose of this config and the images it builds for maintainability.
- The
-
GitHub Actions: Use matrix for platform in release workflow
- The release workflow runs only on
ubuntu-24.04forskaffold-tidb-operator(amd64 only). - Suggest extending to use matrix for
linux/amd64andlinux/arm64similar to the pull request workflow to produce multi-arch images on release.
- The release workflow runs only on
Best Practices
-
Documentation for new Dockerfiles and workflows
- No README or documentation update is included to explain the purpose, usage, or build instructions for the new tidb-operator Dockerfiles and Skaffold config.
- Suggest adding docs in
dockerfiles/ci/tidb-operator/README.mdand updating main CI docs to help contributors understand and maintain these images.
-
Testing coverage for new images
- There is no mention of tests validating the built images (e.g., smoke tests, version checks).
- Recommend adding lightweight tests or integration tests in CI to verify that the new images contain expected binaries and versions.
-
Naming consistency in Skaffold image names
- The images are named
tidb-operator/e2e-baseandtidb-operator/backup-manager-base, which is consistent, but consider if-basesuffix is desired in image tags or if semantic tags would be helpful. - Also ensure these names do not conflict with existing registry images.
- The images are named
Summary of actionable fixes:
- Add package manager cache cleanup in Dockerfiles to reduce final image size.
- Confirm and document build ARGs, especially
TARGETARCH. - Switch
ADDURL usage to explicitcurlfor cert-manager.yaml in Dockerfile. - Extend release CI job to build multi-platform images.
- Add documentation for new Dockerfiles and Skaffold config.
- Implement basic image validation tests in CI.
These changes will improve image efficiency, maintainability, and robustness of the new tidb-operator CI base images and their workflows.
|
|
||
| # install kubectl, helm CLIs | ||
| ARG KUBECTL_VERSION=v1.28.5 | ||
| ENV HELM_VERSION=v3.11.0 |
There was a problem hiding this comment.
| ENV HELM_VERSION=v3.11.0 | |
| ARG HELM_VERSION=v3.11.0 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 participant
Changes
New Dockerfiles
tidb-operator/e2e-base/Dockerfile— e2e test base image (kubectl, helm, AWS CLI, cert-manager)tidb-operator/backup-manager-base/Dockerfile— backup manager base image (rclone, shush)Dockerfile Improvements
--mount=type=cacheinstead ofrm -rffor apt/dnf cache cleanupdebian:bookworm-slimto sha256 digest for reproducible buildsjetstacktocert-manager(avoids 301 redirect)ncw/rclonetorclone/rclone(avoids 301 redirect)curl | tarpipeADDinstruction for cert-manager.yaml downloadSkaffold
ci-tidb-operatorconfig withtidb-operator/e2e-baseandtidb-operator/backup-manager-baseartifactsGitHub Actions
pull-ci-runtime-images.yaml— PR verification: addskaffold-tidb-operatorjob (dual-platform build, no push)release-ci-runtime-images.yaml— Release: addskaffold-tidb-operatorjob (QEMU multi-platform build and publish)