search

PingCAP-QE / ci

Continue intergration tests
Apache License 2.0
19 stars 96 forks source link

Update sanitizer tiflash image #3018

Closed yibin87 closed 1 day ago

yibin87 commented 1 day ago

PR Type

enhancement

Description

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

    ti-chi-bot[bot] commented 1 day ago

    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.

    codiumai-pr-agent-pro[bot] commented 1 day ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 1
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review None
    codiumai-pr-agent-pro[bot] commented 1 day ago

    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]](https://github.com/PingCAP-QE/ci/pull/3018/files#diff-cf6e0f80b8d57ff16c04804cccb20aa2048f6041c6c159d041d860881b6fb9aeR75-R78) ```diff 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
    ti-chi-bot[bot] commented 1 day ago

    [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: - ~~[jenkins/OWNERS](https://github.com/PingCAP-QE/ci/blob/main/jenkins/OWNERS)~~ [purelind] 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[bot] commented 1 day ago

    [LGTM Timeline notifier]

    Timeline:

    wuhuizuo commented 1 day ago

    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.

    JaySon-Huang commented 1 day ago

    Ref https://github.com/pingcap/tiflash/issues/7193