Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sanitizer tiflash image #3018

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

yibin87
Copy link
Contributor

@yibin87 yibin87 commented Jul 1, 2024

PR Type

enhancement


Description

  • Updated the builder container image in the TiFlash sanitizer daily pipeline to hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6.

Changes walkthrough 📝

Relevant files
Enhancement
tiflash-sanitizer-daily.groovy
Update `builder` container image in TiFlash sanitizer pipeline

jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy

  • Updated the builder container image to
    hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6.
  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Signed-off-by: yibin <huyibin@pingcap.com>
    Copy link

    ti-chi-bot bot commented Jul 1, 2024

    I have already done a preliminary review for you, and I hope to help you do a better job.

    Based on the diff provided, this pull request only updates the image used for the builder container in the tiflash-sanitizer-daily pipeline from hub.pingcap.net/tiflash/tiflash-llvm-base:amd64 to hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6.

    As for potential problems, there might be compatibility issues with the new image that could affect the pipeline's overall performance. It is also possible that the new image has a larger size than the previous one, which could negatively impact the pipeline's speed.

    As for fixing suggestions, I would recommend running the pipeline with the new image to make sure that it is compatible and that the pipeline's performance is not affected. It would also be a good idea to monitor the pipeline's speed to see if there is any significant difference from before. Additionally, it would be helpful to document the changes made in the pull request to make it easier for other team members to understand what was modified.

    @ti-chi-bot ti-chi-bot bot requested review from purelind and wuhuizuo July 1, 2024 03:06
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added the enhancement New feature or request label Jul 1, 2024
    @ti-chi-bot ti-chi-bot bot added the size/XS label Jul 1, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 1
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review None

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Add missing environment variables to the new container configuration

    Consider adding environment variables (envVars) to the new containerTemplate for
    consistency with the previous configuration. This ensures that any necessary environment
    settings are preserved in the new container configuration.

    jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy [75-78]

     containerTemplate(name: 'builder', image: 'hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6',
    -                  alwaysPullImage: true, ttyEnabled: true, command: 'cat',
    +                  alwaysPullImage: true, envVars: [
    +                      envVar(key: 'DOCKER_HOST', value: 'tcp://localhost:2375')
    +                  ], ttyEnabled: true, command: 'cat',
                       resourceRequestCpu: '10000m', resourceRequestMemory: '32Gi',
                       resourceLimitCpu: '20000m', resourceLimitMemory: '64Gi')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion ensures consistency and maintains necessary environment settings in the new container configuration, which is crucial for the correct operation of the container.

    9

    Copy link
    Collaborator

    @purelind purelind left a comment

    Choose a reason for hiding this comment

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

    /lgtm

    Copy link

    ti-chi-bot bot commented Jul 1, 2024

    [APPROVALNOTIFIER] This PR is APPROVED

    This pull-request has been approved by: purelind

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

    The pull request process is described here

    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

    Copy link

    ti-chi-bot bot commented Jul 1, 2024

    [LGTM Timeline notifier]

    Timeline:

    • 2024-07-01 03:08:14.018191878 +0000 UTC m=+1207420.503680710: ☑️ agreed by purelind.

    @wuhuizuo
    Copy link
    Collaborator

    wuhuizuo commented Jul 1, 2024

    PR Code Suggestions ✨

    Category Suggestion                                                                                                                                    Score
    Maintainability
    Add missing environment variables to the new container configuration
    Consider adding environment variables (envVars) to the new containerTemplate for consistency with the previous configuration. This ensures that any necessary environment settings are preserved in the new container configuration.

    jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy [75-78]

     containerTemplate(name: 'builder', image: 'hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6',
    -                  alwaysPullImage: true, ttyEnabled: true, command: 'cat',
    +                  alwaysPullImage: true, envVars: [
    +                      envVar(key: 'DOCKER_HOST', value: 'tcp://localhost:2375')
    +                  ], ttyEnabled: true, command: 'cat',
                       resourceRequestCpu: '10000m', resourceRequestMemory: '32Gi',
                       resourceLimitCpu: '20000m', resourceLimitMemory: '64Gi')
     
    • Apply this suggestion

    Suggestion importance[1-10]: 9
    Why: The suggestion ensures consistency and maintains necessary environment settings in the new container configuration, which is crucial for the correct operation of the container.

    9

    No, the second container will not use docker.

    @ti-chi-bot ti-chi-bot bot merged commit e631557 into PingCAP-QE:main Jul 1, 2024
    1 of 2 checks passed
    @JaySon-Huang
    Copy link
    Contributor

    Ref pingcap/tiflash#7193

    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.

    4 participants