refactor(pipelines/pingcap/tidb): replace hub.pingcap.net image refs with public equivalents (D1-D6)#4773
Conversation
…alents Replace all internal-only hub.pingcap.net image references in pipelines/pingcap/tidb/ pod YAMLs for release-6.5, release-6.5-fips, and latest branches with public registry images. D1: Go builder images → ghcr.io/pingcap-qe/ci/jenkins D2: Bazel build images → us-docker.pkg.dev/pingcap-testing-account/internal/test D3: Service sidecar images → docker.io or ghcr.io equivalents D5: Remove DOCKER_REGISTRY_MIRROR env var D6: Docker build tag → us-docker.pkg.dev/.../internal/test/qa/tidb Ref: FLA-242
![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 replaces all internal hub.pingcap.net image references in the pipelines/pingcap/tidb/ directory with public equivalents (ghcr.io, docker.io, and us-docker.pkg.dev) across multiple YAML and Groovy files. The replacements follow established patterns, ensuring compatibility with public registries and removing PingCAP-specific dependencies. Overall, the changes are straightforward, low-risk, and well-documented. However, there are some opportunities to improve maintainability and ensure consistency.
Critical Issues
No critical issues were identified. The changes are primarily updates to image references and removal of unused environment variables, which are low-risk.
Code Improvements
-
Centralize Image References for Maintainability
Files: All YAML files (pipelines/pingcap/tidb/...)
Issue: Image versions (ghcr.io/...andus-docker.pkg.dev/...) are hardcoded across multiple files. This approach can lead to issues with consistency if versions need to be updated in the future.
Suggestion: Define image references as global variables or utilize a centralized configuration file. This way, updates to image versions can be made in a single location. For example:# pipelines/pingcap/tidb/image-config.yaml golang_image: ghcr.io/pingcap-qe/ci/jenkins:v2025.12.7-3-g1c0b8cf-go1.23 docker_image: docker.io/library/docker:20.10.24-dind kafka_image: docker.io/confluentinc/cp-kafka:7.6.2 zookeeper_image: docker.io/confluentinc/cp-zookeeper:7.6.2 mysql_image: docker.io/library/mysql:5.7
Then reference these variables in the pod specs:
containers: - name: golang image: ${golang_image}
-
Remove Deprecated Environment Variables Globally
Files:release-6.5/pod-pull_tiflash_test.yaml, and possibly others
Issue: TheDOCKER_REGISTRY_MIRRORenvironment variable was removed in one file, but this cleanup may need to be applied across other YAML files.
Suggestion: Confirm whether this variable is unused globally and remove it from other pipelines if applicable.
Best Practices
-
Testing Coverage for Image Compatibility
Files: All updated files
Issue: While the new images are claimed to follow established patterns, no specific testing was mentioned beyond ensuring no references tohub.pingcap.netremain. Compatibility testing with the public images should be explicitly documented.
Suggestion: Add a simple smoke test to validate that the public images work as intended for a few critical workflows. For example, automated tests could include pulling the images and verifying their behaviors during CI runs. -
Document the Reason for Image Version Choices
Files: All updated files
Issue: The rationale for specific image tags (e.g.,v2025.12.7-3-g1c0b8cf-go1.23) is unclear. This can hinder future maintainers who may need to update these versions.
Suggestion: Include a comment or documentation detailing why these specific versions were chosen. For example:# Using v2025.12.7-3-g1c0b8cf-go1.23 as it includes Go 1.23 support and all required dependencies for CI tests. image: ghcr.io/pingcap-qe/ci/jenkins:v2025.12.7-3-g1c0b8cf-go1.23
-
Improve Groovy Code Maintainability
Files:release-6.5/pull_tiflash_test.groovy
Issue: The image reference update in the Groovy file is hardcoded, similar to the YAML files.
Suggestion: Use a parameterized approach in the Groovy script to avoid hardcoding image tags. For example:def dockerImageTag = 'us-docker.pkg.dev/pingcap-testing-account/internal/test/qa/tidb:${REFS.base_ref}' sh "docker build -t ${dockerImageTag} -f tidb.Dockerfile ."
Conclusion
The PR is well-executed and achieves its goal of replacing internal image references with public ones. The changes are low-risk, but improvements to maintainability (centralized configuration for image versions) and documentation (image version rationale, testing coverage) would enhance future maintainability and robustness.
wuhuizuo
changed the title
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 pull request refactors Kubernetes pod YAMLs and associated pipeline scripts in the pipelines/pingcap/tidb directory by replacing internal image references (hub.pingcap.net) with public equivalents (ghcr.io, us-docker.pkg.dev, and docker.io). The approach follows established patterns from previous PRs and adheres to public registry standards. The changes are straightforward, but the PR spans multiple files and directories, requiring careful validation of image compatibility and functionality in CI/CD workflows. Overall, the quality of the refactor is solid, but some testing and best practices could be improved.
Critical Issues
-
Image Compatibility Validation:
- Files: All modified YAML files.
- Issue: The new public images may have subtle differences in configuration or dependencies compared to the replaced internal images. There is no indication in the PR description that these images were tested for compatibility beyond a grep search for
hub.pingcap.net. - Solution: Validate the functionality of each pipeline by running all affected CI/CD workflows using the updated images to ensure no regressions are introduced.
-
Removed
DOCKER_REGISTRY_MIRROREnvironment Variable:- File:
pipelines/pingcap/tidb/release-6.5/pod-pull_tiflash_test.yaml, lines 22-23. - Issue: The removal of the
DOCKER_REGISTRY_MIRRORenvironment variable may impact image pull performance or fail in environments relying on the mirror. This change is not justified in the description. - Solution: Confirm that the mirror is no longer required and document why it was removed. If performance concerns arise, consider introducing a fallback strategy.
- File:
Code Improvements
-
Centralize Image Definitions:
- Files: All modified YAML files.
- Issue: The same image tags (e.g.,
ghcr.io/pingcap-qe/ci/jenkins:v2024.10.8-119-g4e56df7-go1.21) are repeated across many files, making updates error-prone and difficult to maintain. - Solution: Use Kubernetes ConfigMaps or Helm templates to centralize image definitions for reuse. For example:
In the
image: "{{ .Values.images.golang }}"
values.yamlfile:images: golang: ghcr.io/pingcap-qe/ci/jenkins:v2024.10.8-119-g4e56df7-go1.21
-
Explicit Image Pull Policies:
- Files: Various pod YAML files.
- Issue: While some containers specify
imagePullPolicy: Always, many do not. This can lead to outdated images being used if a newer tag is pushed. - Solution: Explicitly set
imagePullPolicy: Alwaysfor all containers using mutable tags likelatest.
Best Practices
-
Testing Coverage:
- Files: All modified files.
- Issue: The PR description mentions no tests beyond verifying the absence of
hub.pingcap.net. This is insufficient for ensuring CI/CD pipelines function as expected. - Solution: Run integration tests for all affected pipelines and document the results in the PR description.
-
Documentation for Image Changes:
- Files: All modified files.
- Issue: The rationale for specific image replacements (e.g., switching to
ghcr.ioordocker.io) is not provided. - Solution: Include comments or documentation explaining why each image was chosen and whether it adheres to security and compatibility requirements.
-
Versioning for Images:
- Files: Various pod YAML files.
- Issue: Some public image tags (e.g.,
docker.io/library/mysql:5.7) use a specific version, while others are less specific (ghcr.io/...:latest). Mixing these approaches can lead to unexpected behaviors. - Solution: Use explicit version tags for all images to ensure consistent behavior across environments.
-
Remove Hardcoded Registry References in Scripts:
- File:
pipelines/pingcap/tidb/release-6.5/pull_tiflash_test.groovy, line 73. - Issue: The registry reference (
us-docker.pkg.dev/...) is hardcoded in the Groovy script, making it difficult to update or reuse. - Solution: Externalize the registry URL into a pipeline environment variable or configuration file:
def registry = System.getenv("DOCKER_REGISTRY") ?: "us-docker.pkg.dev/pingcap-testing-account/internal/test" docker.build("${registry}/qa/tidb:${REFS.base_ref}")
- File:
Summary of Changes
- Validate image compatibility with integration testing.
- Centralize image definitions for maintainability.
- Explicitly set
imagePullPolicy: Alwaysto avoid stale images. - Provide documentation for image choices and rationale.
- Standardize versioning for all images to ensure predictability.
pod_label.withCiLabels in release-6.5 pipelines
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 pull request refactors multiple pipeline YAML and Groovy files to replace internal hub.pingcap.net Docker image references with public equivalents hosted on ghcr.io and us-docker.pkg.dev. The approach adheres to established patterns from prior refactoring efforts and ensures compatibility with the EOL branches (release-6.5, release-6.5-fips). Overall, the changes are straightforward but introduce minor risks related to image compatibility and runtime behavior due to the shifted registry sources.
Critical Issues
- File:
pull_tiflash_test.groovy(Line 73)- Issue: The new image tag in the
docker buildcommand (us-docker.pkg.dev/pingcap-testing-account/hub/pingcap/tidb/images/tidb-server:${REFS.base_ref}) introduces a hardcoded path that could break if registry organization changes or ifbase_refresolves to invalid values. - Suggested Solution: Parameterize the registry URL and validate
base_refbefore use. Example:def registryURL = "us-docker.pkg.dev/pingcap-testing-account/hub/pingcap/tidb/images" docker build -t ${registryURL}/tidb-server:${REFS.base_ref} -f tidb.Dockerfile .
- Issue: The new image tag in the
Code Improvements
-
File:
pod-pull_tiflash_test.yaml(Line 22)- Issue: The
DOCKER_REGISTRY_MIRRORenvironment variable was removed entirely without explanation. Some builds may rely on this mirror for faster image pulls. - Suggested Solution: Provide a fallback mechanism or confirm that the mirror is universally unused. Example:
- name: DOCKER_REGISTRY_MIRROR value: ${DOCKER_REGISTRY_MIRROR:-"https://default-mirror.example.com"}
- Issue: The
-
File:
pull_br_integration_test.groovy,pull_common_test.groovy, etc.- Issue: Repeated use of
yaml pod_label.withCiLabels(POD_TEMPLATE_FILE, REFS)is redundant and could benefit from abstraction. - Suggested Solution: Abstract the pattern into a helper function to improve maintainability. Example:
def generatePodYaml(namespace, templateFile, refs) { return pod_label.withCiLabels(templateFile, refs) }
- Issue: Repeated use of
Best Practices
-
Testing Coverage
- Issue: While the description mentions
rg hub\\.pingcap\\.netreturns 0, no explicit runtime or deployment test coverage is described. - Suggested Solution: Add automated tests or logs confirming that public images can be pulled successfully and are compatible with existing pipeline workflows.
- Issue: While the description mentions
-
Documentation
- Issue: The PR does not include documentation updates or comments explaining the rationale behind specific image replacements (e.g., why
docker.io/confluentinc/cp-zookeeper:7.6.2was chosen for Zookeeper). - Suggested Solution: Add inline comments in the YAML files or Groovy scripts to clarify non-obvious changes.
- Issue: The PR does not include documentation updates or comments explaining the rationale behind specific image replacements (e.g., why
-
Naming Conventions
- Issue: The image tags (e.g.,
v2024.10.8-119-g4e56df7-go1.21) are verbose and obfuscate their purpose. This could complicate debugging. - Suggested Solution: Use a concise naming convention with meaningful identifiers, such as
go1.21-latest.
- Issue: The image tags (e.g.,
Conclusion
The PR achieves its goal of replacing internal image references with public ones but introduces minor risks regarding compatibility and mirror removal. Testing and documentation improvements are recommended to ensure long-term reliability. Addressing redundancy in Groovy scripts could improve maintainability significantly.
|
@wuhuizuo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/approve |
|
/test pull-verify-k8s-pod-yaml |
|
/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
Summary
Replace all internal-only
hub.pingcap.netimage references inpipelines/pingcap/tidb/pod YAMLs for release-6.5, release-6.5-fips, and latest with public registry images.Changes
ghcr.io/pingcap-qe/ci/jenkins:*(24 pods across 3 directories)us-docker.pkg.dev/pingcap-testing-account/internal/test/wangweizhen/...(3 pods)DOCKER_REGISTRY_MIRRORenv var frompod-pull_tiflash_test.yamlpull_tiflash_test.groovy→us-docker.pkg.dev/.../internal/test/qa/tidb:${REFS.base_ref}Testing
hub.pingcap.netrefs remain:rg hub\\.pingcap\\.net pipelines/pingcap/tidb/returns 0Risk
Low. All replacements follow established patterns from PR #4763 and existing clean release dirs (8.5+). Images at ghcr.io and us-docker.pkg.dev are immutable. Release-6.5/6.5-fips are EOL branches.