Skip to content

fix(pipelines): fallback when release-6.5 tidb failpoint image missing#4845

Closed
kumailf wants to merge 1 commit into
mainfrom
fix/tiflash-release-6.5-tidb-failpoint-fallback
Closed

fix(pipelines): fallback when release-6.5 tidb failpoint image missing#4845
kumailf wants to merge 1 commit into
mainfrom
fix/tiflash-release-6.5-tidb-failpoint-fallback

Conversation

@kumailf

@kumailf kumailf commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • When tidb-server:release-6.5-failpoint is missing from the OCI registry, fall back to the regular release-6.5 TiDB image so docker-compose can start.
  • Skip tidb-ci/fail-point-tests in that fallback path, since those suites require a real failpoint TiDB binary.
  • Unblocks upstream TiFlash release-6.5 pull_integration_test (e.g. Release 6.5.6 hotfix 20260720 pingcap/tiflash#10993) after upstream stopped publishing 6.5 failpoint images.

Test plan

  • Merge and re-run /test pull-integration-test on a release-6.5 TiFlash PR
  • Confirm logs show WARN fallback + skipping fail-point-tests when failpoint tag is missing
  • Confirm other suites (fullstack / async_grpc / collation) still run

Made with Cursor

@ti-chi-bot

ti-chi-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jayson-huang for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.sh assumes the run.sh file exists and contains that literal string exactly.
    • The sed command removes ./run-test.sh tidb-ci/fail-point-tests && but does not cover other possible invocation patterns or trailing spaces.
    • If ./run.sh does 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.sh before 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 sed fails or no changes were made, to help diagnose fallback failures.

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 pull commands
    Currently, only the failpoint image pull is checked for failure with fallback. Consider adding error handling or logging for the other docker pull commands to catch transient failures or image issues early.

  • Avoid modifying ./run.sh in-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 copying run.sh to 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] or WARNING: instead of just WARN: to align with other logs.

  • Shell style: use && instead of nested if where 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.

@ti-chi-bot ti-chi-bot Bot added the size/S label Jul 22, 2026
@kumailf kumailf changed the title pipelines(tiflash): fallback when release-6.5 tidb failpoint image missing fix(pipelines): fallback when release-6.5 tidb failpoint image missing Jul 22, 2026
@wuhuizuo wuhuizuo closed this Jul 22, 2026
@wuhuizuo
wuhuizuo deleted the fix/tiflash-release-6.5-tidb-failpoint-fallback branch July 22, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants