fix(pipelines/tiflash): always skip fail-point-tests on release-6.5 - #4852
Conversation
OCI still has tidb-server:release-6.5-failpoint, so pull succeeds and the previous fallback never ran. The image does not expose enableTestAPI, so tidb-ci/fail-point-tests fail. Always use regular TiDB image and skip those suites; keep fullstack and other cases.
![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 fixes integration test failures on the release-6.5 pipeline for TiFlash by always skipping fail-point-tests regardless of whether the failpoint image pull succeeds. The approach replaces a conditional fallback (based on image pull failure) with an unconditional fallback to the regular TiDB image and strips fail-point-tests from the run script unconditionally. The change is targeted, minimal, and improves stability of the pipeline by preventing errors caused by the failpoint image lacking the enableTestAPI. The patch is clear and straightforward.
Critical Issues
None found.
Code Improvements
-
Reduce Redundant Image Pull
Currently, the patch still attempts todocker pull "${TIDB_FAILPOINT_IMAGE}"before falling back. Since the failpoint image is always unusable on release-6.5, consider skipping the pull altogether to save time and network resources. For example:# Skip pulling failpoint image on release-6.5 as it is unusable echo "WARN: skipping pull of failpoint image on release-6.5; using regular TiDB image" TIDB_FAILPOINT_IMAGE="${TIDB_IMAGE}"
-
Improve Sed Pattern Robustness
Thesedcommand removes the exact prefix string:sed -i -e 's#./run-test.sh tidb-ci/fail-point-tests && ##g' ./run.shConsider anchoring or making it more precise to avoid accidental removals if the command appears elsewhere unexpectedly. For example, restrict to lines starting with that prefix:
sed -i -e '/^\.\/run-test.sh tidb-ci\/fail-point-tests && /s#^\.\/run-test.sh tidb-ci/fail-point-tests && ##' ./run.sh -
Add Comments or Logs Explaining Why Fail-Point Tests Are Skipped
In the script, add a comment or log message that clearly states the rationale to aid future maintainers:echo "WARN: skipping fail-point-tests because release-6.5 failpoint image lacks enableTestAPI support"
Best Practices
-
Add a Comment Block Explaining the Entire Fallback Logic
At the top of the pipeline stage or script section, add a brief comment describing the fallback rationale and the known issue with the failpoint images on release-6.5. This improves maintainability. -
Testing & Verification
The PR description mentions manual testing steps. Once merged, consider adding an automated check or test to verify that fail-point-tests are indeed skipped on release-6.5 to prevent regressions. -
Style / Readability
The indentation of theifblock inside thepipelineis inconsistent in the diff. Ensure consistent indentation for readability.
Summary of Suggested Code Change
-# release-6.5: OCI may still publish a :failpoint tag, but it does not expose
-# enableTestAPI (fail-point-tests fail on curl_tidb get fail/.../enableTestAPI).
-# Always use the regular TiDB image and skip suites that need real failpoints.
-timeout 300 docker pull "${TIDB_FAILPOINT_IMAGE}" || true
-echo "WARN: release-6.5 TiDB failpoint image is not usable for IT; falling back to ${TIDB_IMAGE}"
-TIDB_FAILPOINT_IMAGE="${TIDB_IMAGE}"
-if [ -f ./run.sh ] && grep -q 'tidb-ci/fail-point-tests' ./run.sh; then
- echo "WARN: skipping tidb-ci/fail-point-tests"
- sed -i -e 's#./run-test.sh tidb-ci/fail-point-tests && ##g' ./run.sh
-fi
+ # release-6.5: failpoint images are unusable because they lack enableTestAPI.
+ # Skip pulling failpoint image to save time and fall back unconditionally.
+ echo "WARN: skipping pull of failpoint image on release-6.5; using regular TiDB image"
+ TIDB_FAILPOINT_IMAGE="${TIDB_IMAGE}"
+
+ if [ -f ./run.sh ] && grep -q 'tidb-ci/fail-point-tests' ./run.sh; then
+ echo "WARN: skipping tidb-ci/fail-point-tests because failpoint image lacks enableTestAPI"
+ sed -i -e '/^\.\/run-test.sh tidb-ci\/fail-point-tests && /s#^\.\/run-test.sh tidb-ci/fail-point-tests && ##' ./run.sh
+ fiThis change improves clarity, performance, and safety.
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dillon-zheng 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 |
2 participants
Summary
Follow-up after #4849 / #4850.
tidb-ci/cluster.yamlon release-6.5 usesqa/tidb:...-failpoint. The previous logic only skippedfail-point-testswhen the failpoint image pull failed. OCI still hastidb-server:release-6.5-failpoint, so pull succeeds, but the image does not exposeenableTestAPI:Change
TIDB_FAILPOINT_IMAGE→ regularrelease-6.5TiDB image./run-test.sh tidb-ci/fail-point-tests &&fromrun.shfullstack-test, async_grpc, collation, …) still runTest plan
/test pull-integration-teston Release 6.5.6 hotfix 20260720 pingcap/tiflash#10993tidb-ciproceeds to fullstack-test without enableTestAPI error