fix(pipelines): fallback when release-6.5 tidb failpoint image missing#4845
fix(pipelines): fallback when release-6.5 tidb failpoint image missing#4845kumailf wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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 enhances the TiFlash release-6.5 integration test pipeline by adding a fallback mechanism when the tidb-server:release-6.5-failpoint image is missing from the OCI registry. It attempts to pull the failpoint image, and on failure, switches to the regular release-6.5 TiDB image and disables failpoint tests that require the failpoint image. The approach is straightforward and localized, with clear logging and minimal disruption to other test suites. The patch is concise and addresses the immediate issue effectively.
Critical Issues
-
Shell script robustness in
pull_integration_test.groovy(lines 267-279)
The conditional logic for fallback and sed replacement is done inline in a shell snippet embedded in the Groovy script. This may cause subtle issues:- The check
grep -q 'tidb-ci/fail-point-tests' ./run.shassumes the run.sh file exists and contains that literal string exactly. - The
sedcommand removes./run-test.sh tidb-ci/fail-point-tests &&but does not cover other possible invocation patterns or trailing spaces. - If
./run.shdoes not exist or has different formatting, this fallback may silently fail, resulting in fail-point-tests still running and causing failures.
Suggestion:
- Add an explicit existence check for
./run.shbefore grepping and modifying. - Use a more robust pattern to disable fail-point-tests, for example commenting out or renaming the relevant block, or a more general regex to cover variants.
- Log errors if
sedfails or no changes were made, to help diagnose fallback failures.
- The check
Code Improvements
-
Improve readability and maintainability by extracting fallback logic into a separate shell script or Groovy function
Embedding complex shell logic inline reduces readability and makes future modifications harder. Consider moving this logic into a dedicated shell script checked into the repo or a Groovy function with clear input/output, and invoke it from the pipeline. -
Better error handling for
docker pullcommands
Currently, only the failpoint image pull is checked for failure with fallback. Consider adding error handling or logging for the otherdocker pullcommands to catch transient failures or image issues early. -
Avoid modifying
./run.shin-place without version control backup
Directly modifying scripts in-place during CI can be risky and may cause unexpected side effects if the pipeline is retried or run concurrently. Consider copyingrun.shto a temp file and modifying that, or pass environment variables to disable fail-point-tests instead of sed replacements if possible.
Best Practices
-
Add comments explaining why failpoint images are no longer published upstream
While the PR description mentions this, adding a brief comment in the code near the fallback logic will help future maintainers understand the context. -
Add testing or validation steps for the fallback behavior
The test plan indicates manual steps post-merge. Consider adding automated tests or pipeline stages that simulate missing failpoint images to verify fallback and skipping of fail-point-tests work as expected. -
Consistent logging format
Use a consistent prefix or log level indicator when echoing warnings, e.g.,[WARN]orWARNING:instead of justWARN:to align with other logs. -
Shell style: use
&&instead of nestedifwhere possible
For example, instead of the nested if and grep, consider:if [ -f ./run.sh ] && grep -q 'tidb-ci/fail-point-tests' ./run.sh; then ... fi
(This is already done correctly, so just highlight it as good style.)
Summary of action items:
- Add explicit existence checks and error logging around modifying
./run.sh - Consider extracting fallback logic to a dedicated script/function
- Add comments about upstream failpoint image deprecation in the code
- Improve robustness of sed pattern for disabling fail-point-tests
- Consider safer ways to disable fail-point-tests (env var or test skip flags)
- Add automated tests or pipeline validation for fallback scenario
- Add error handling/logging for other docker pull commands
These changes will increase the reliability, maintainability, and transparency of the fallback mechanism introduced in this PR.
Summary
tidb-server:release-6.5-failpointis missing from the OCI registry, fall back to the regularrelease-6.5TiDB image sodocker-composecan start.tidb-ci/fail-point-testsin that fallback path, since those suites require a real failpoint TiDB binary.release-6.5pull_integration_test(e.g. Release 6.5.6 hotfix 20260720 pingcap/tiflash#10993) after upstream stopped publishing 6.5 failpoint images.Test plan
/test pull-integration-teston a release-6.5 TiFlash PRMade with Cursor