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

feat: tiflash and tikv split release 8.2 jobs #3009

Merged

Conversation

purelind
Copy link
Collaborator

@purelind purelind commented Jun 25, 2024

User description

tiflash and tikv split release 8.2 jobs


PR Type

Enhancement, Tests


Description

  • Created folders and added pipeline jobs for TiFlash and TiKV release 8.2.
  • Defined Jenkins pipelines for TiFlash integration and unit tests.
  • Defined Jenkins pipeline for TiKV unit tests.
  • Added Kubernetes pod templates for TiFlash and TiKV jobs.
  • Updated kustomization to include new presubmit jobs for TiFlash and TiKV release 8.2.
  • Added presubmit job configurations for TiFlash and TiKV release 8.2.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
aa_folder.groovy
Create folder for TiFlash release 8.2 pipelines                   

jobs/pingcap/tiflash/release-8.2/aa_folder.groovy

  • Created a new folder for pingcap/tiflash release 8.2 pipelines.
  • Added a description for the folder.
  • +3/-0     
    pull_integration_test.groovy
    Add integration test pipeline for TiFlash release 8.2       

    jobs/pingcap/tiflash/release-8.2/pull_integration_test.groovy

  • Added a new pipeline job for TiFlash release 8.2 integration tests.
  • Configured job parameters, properties, and SCM details.
  • +39/-0   
    pull_unit_test.groovy
    Add unit test pipeline for TiFlash release 8.2                     

    jobs/pingcap/tiflash/release-8.2/pull_unit_test.groovy

  • Added a new pipeline job for TiFlash release 8.2 unit tests.
  • Configured job parameters, properties, and SCM details.
  • +39/-0   
    aa_folder.groovy
    Create folder for TiKV release 8.2 pipelines                         

    jobs/tikv/tikv/release-8.2/aa_folder.groovy

  • Created a new folder for tikv/tikv release 8.2 pipelines.
  • Added a description for the folder.
  • +3/-0     
    pull_unit_test.groovy
    Add unit test pipeline for TiKV release 8.2                           

    jobs/tikv/tikv/release-8.2/pull_unit_test.groovy

  • Added a new pipeline job for TiKV release 8.2 unit tests.
  • Configured job parameters, properties, and SCM details.
  • +39/-0   
    pull_integration_test.groovy
    Define Jenkins pipeline for TiFlash integration tests       

    pipelines/pingcap/tiflash/release-8.2/pull_integration_test.groovy

  • Defined a Jenkins pipeline for TiFlash integration tests.
  • Configured stages for debug info, checkout, cache preparation, build,
    and integration tests.
  • +394/-0 
    pull_unit_test.groovy
    Define Jenkins pipeline for TiFlash unit tests                     

    pipelines/pingcap/tiflash/release-8.2/pull_unit_test.groovy

  • Defined a Jenkins pipeline for TiFlash unit tests.
  • Configured stages for debug info, checkout, cache preparation, build,
    and unit tests.
  • +325/-0 
    pull_unit_test.groovy
    Define Jenkins pipeline for TiKV unit tests                           

    pipelines/tikv/tikv/release-8.2/pull_unit_test.groovy

  • Defined a Jenkins pipeline for TiKV unit tests.
  • Configured stages for debug info, checkout, lint, build, and tests.
  • +203/-0 
    Configuration changes
    7 files
    pod-pull_build.yaml
    Define Kubernetes pod template for TiFlash build jobs       

    pipelines/pingcap/tiflash/release-8.2/pod-pull_build.yaml

  • Defined a Kubernetes pod template for TiFlash build jobs.
  • Configured container specifications and volume mounts.
  • +135/-0 
    pod-pull_integration_test.yaml
    Define Kubernetes pod template for TiFlash integration test jobs

    pipelines/pingcap/tiflash/release-8.2/pod-pull_integration_test.yaml

  • Defined a Kubernetes pod template for TiFlash integration test jobs.
  • Configured container specifications and volume mounts.
  • +92/-0   
    pod-pull_unit-test.yaml
    Define Kubernetes pod template for TiFlash unit test jobs

    pipelines/pingcap/tiflash/release-8.2/pod-pull_unit-test.yaml

  • Defined a Kubernetes pod template for TiFlash unit test jobs.
  • Configured container specifications and volume mounts.
  • +131/-0 
    pod-pull_unit_test.yaml
    Define Kubernetes pod template for TiKV unit test jobs     

    pipelines/tikv/tikv/release-8.2/pod-pull_unit_test.yaml

  • Defined a Kubernetes pod template for TiKV unit test jobs.
  • Configured container specifications and volume mounts.
  • +52/-0   
    kustomization.yaml
    Update kustomization for TiFlash and TiKV release 8.2 presubmits

    prow-jobs/kustomization.yaml

  • Updated kustomization to include new presubmit jobs for TiFlash and
    TiKV release 8.2.
  • +42/-40 
    release-8.2-presubmits.yaml
    Add presubmit job configurations for TiFlash release 8.2 

    prow-jobs/pingcap/tiflash/release-8.2-presubmits.yaml

    • Added presubmit job configurations for TiFlash release 8.2.
    +26/-0   
    release-8.2-presubmits.yaml
    Add presubmit job configurations for TiKV release 8.2       

    prow-jobs/tikv/tikv/release-8.2-presubmits.yaml

    • Added presubmit job configurations for TiKV release 8.2.
    +14/-0   

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

    Copy link

    ti-chi-bot bot commented Jun 25, 2024

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

    The pull request title suggests that this is a feature to split the release jobs for TiFlash and TiKV 8.2.

    Key Changes:

    1. Addition of new groovy scripts for the CI/CD jobs related to TiFlash and TiKV 8.2 release.
    2. New yaml files that define the pod configurations for the Kubernetes jobs related to the release.
    3. The scripts and yaml files are organized under the directory structure jobs/pingcap/tiflash/release-8.2 and jobs/tikv/tikv/release-8.2.
    4. Addition of new presubmits for both TiFlash and TiKV 8.2 release in prow-jobs/pingcap/tiflash/release-8.2-presubmits.yaml and prow-jobs/tikv/tikv/release-8.2-presubmits.yaml.

    Potential Problems:

    1. There are placeholders in the groovy scripts like <your-jenkins-server> which need to be replaced with actual values.
    2. Make sure the directory paths mentioned in the scripts and yaml files exist and have the correct permissions.
    3. Ensure that the new presubmits do not conflict with any existing presubmits.

    Suggestions:

    1. Replace placeholders with actual values.
    2. Check the directory paths and their permissions.
    3. Validate the new presubmits configuration against the existing configuration to avoid conflicts.
    4. Test these scripts and configuration in a controlled environment before merging.

    @qodo-merge-pro qodo-merge-pro bot added enhancement New feature or request Tests labels Jun 25, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The PR introduces a significant amount of new Jenkins pipeline code across multiple files. Each pipeline script involves complex configurations, including environment setups, caching mechanisms, and parallel execution steps. Reviewers should ensure that the logic for handling dependencies, caching, and parallel execution is correctly implemented to avoid potential build and test failures.
    Performance Concerns:
    The new Jenkins jobs include configurations for resource allocation (e.g., CPU, memory) that are quite high. It's crucial to validate that these resource allocations are justified based on the actual needs of the build and test processes to optimize cost and efficiency.
    Code Duplication:
    There appears to be a considerable amount of repeated code across different pipeline scripts, especially in the setup and caching configurations. It might be beneficial to refactor these into shared libraries or templates to reduce duplication and simplify maintenance.

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Add a security context to the runner container to enhance security by dropping all capabilities and running as a non-root user

    Add a securityContext to the runner container to drop all capabilities and run as a
    non-root user, enhancing the security of the container.

    pipelines/pingcap/tiflash/release-8.2/pod-pull_build.yaml [6-13]

     containers:
       - name: runner
         image: "hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6"
         command:
           - "/bin/bash"
           - "-c"
           - "cat"
    +    securityContext:
    +      runAsUser: 1000
    +      runAsGroup: 3000
    +      capabilities:
    +        drop: ["ALL"]
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Adding a security context as suggested would significantly enhance the security posture of the container by enforcing least privilege, which is a critical best practice in container security.

    10
    Possible issue
    Add a condition to check if the JOB_SPEC parameter is provided before reading it

    Consider adding a condition to check if the JOB_SPEC parameter is provided before
    attempting to read it. This will prevent potential runtime errors if the parameter is
    missing.

    pipelines/pingcap/tiflash/release-8.2/pull_integration_test.groovy [11]

    -final REFS = readJSON(text: params.JOB_SPEC).refs
    +final REFS = params.JOB_SPEC ? readJSON(text: params.JOB_SPEC).refs : null
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a null check before reading the JOB_SPEC parameter can prevent runtime errors due to missing parameters, which is crucial for robustness.

    8
    Ensure proxy_commit_hash is not null before using it in the Prepare Cache stage

    Add a check to ensure that proxy_commit_hash is not null before using it in the Prepare
    Cache stage to avoid potential null pointer exceptions.

    pipelines/pingcap/tiflash/release-8.2/pull_integration_test.groovy [128]

    -proxy_cache_ready = fileExists("/home/jenkins/agent/proxy-cache/${proxy_commit_hash}-amd64-linux-llvm")
    +proxy_cache_ready = proxy_commit_hash ? fileExists("/home/jenkins/agent/proxy-cache/${proxy_commit_hash}-amd64-linux-llvm") : false
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Checking for null before using proxy_commit_hash prevents potential null pointer exceptions, which is critical for the stability of the script.

    8
    Safety
    Add safety checks to ensure sh commands are executed in the correct directory

    The sh commands in the "Checkout" stage that remove all files and directories (rm -rf ./*)
    and change ownership (chown 1000:1000 -R ./) could potentially cause issues if they are
    executed in the wrong directory. Consider adding a safety check to ensure the commands are
    executed in the correct directory.

    pipelines/pingcap/tiflash/release-8.2/pull_unit_test.groovy [64-88]

    -sh "rm -rf ./*"
    +sh """
    +if [ "\$(pwd)" == "${WORKSPACE}/tiflash" ]; then
    +    rm -rf ./*
    +else
    +    echo "Not in the expected directory. Aborting."
    +    exit 1
    +fi
    +"""
     ...
     sh """
    -chown 1000:1000 -R ./
    +if [ "\$(pwd)" == "${WORKSPACE}/tiflash" ]; then
    +    chown 1000:1000 -R ./
    +else
    +    echo "Not in the expected directory. Aborting."
    +    exit 1
    +fi
     """
     
    Suggestion importance[1-10]: 8

    Why: Adding safety checks before executing potentially destructive commands like rm -rf and chown is crucial to prevent accidental data loss or permission issues, especially in a CI/CD pipeline where such mistakes can be costly.

    8
    Enhancement
    Remove the unnecessary tty field from the runner container configuration

    The tty field is set to true for the runner container, but the command is set to cat,
    which might not require a TTY. Consider removing the tty field if it's not necessary.

    pipelines/pingcap/tiflash/release-8.2/pod-pull_unit-test.yaml [7-9]

     command:
       - "cat"
    -tty: true
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies an unnecessary tty setting for a container running a simple 'cat' command, which can enhance the configuration's clarity and correctness.

    8
    Enable tty for the dockerd container to support interactive commands

    Consider setting tty to true for the dockerd container to ensure it can handle interactive
    commands if needed.

    pipelines/pingcap/tiflash/release-8.2/pod-pull_integration_test.yaml [17]

    -tty: false
    +tty: true
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Changing tty to true can enhance the interactivity of the container, which might be beneficial depending on the use case. This is a valid suggestion but not critical.

    6
    Best practice
    Add error handling for the ks3util command to provide informative error messages if it fails

    Add error handling for the ks3util command to provide more informative error messages in
    case the command fails.

    pipelines/pingcap/tiflash/release-8.2/pull_integration_test.groovy [62]

    -sh "ks3util -c \$KS3UTIL_CONF cp -f ks3://ee-fileserver/download/cicd/daily-cache-code/src-tiflash.tar.gz src-tiflash.tar.gz"
    +sh """
    +if ! ks3util -c \$KS3UTIL_CONF cp -f ks3://ee-fileserver/download/cicd/daily-cache-code/src-tiflash.tar.gz src-tiflash.tar.gz; then
    +    echo "Failed to download src-tiflash.tar.gz from ks3"
    +    exit 1
    +fi
    +"""
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling for the ks3util command enhances the script's reliability by providing clear feedback on failures, improving maintainability.

    7
    Add a readiness probe to the docker container to ensure it is ready before accepting traffic

    Add a readinessProbe to the docker container to ensure it is ready before accepting
    traffic.

    pipelines/pingcap/tiflash/release-8.2/pod-pull_integration_test.yaml [35]

     name: "docker"
    +readinessProbe:
    +  httpGet:
    +    path: /healthz
    +    port: 2375
    +  initialDelaySeconds: 5
    +  periodSeconds: 10
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a readiness probe is a best practice to ensure the container is fully ready before it starts serving traffic, enhancing the reliability of the deployment.

    7
    Add a timeout to the sh step in the Debug info stage to prevent it from hanging indefinitely

    Add a timeout to the sh step in the Debug info stage to prevent it from hanging
    indefinitely.

    pipelines/pingcap/tiflash/release-8.2/pull_integration_test.groovy [37-43]

     sh label: 'Debug info', script: """
     printenv
     echo "-------------------------"
     go env
     echo "-------------------------"
     echo "debug command: kubectl -n ${K8S_NAMESPACE} exec -ti ${NODE_NAME} bash"
    -"""
    +""", timeout: 5
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Implementing a timeout is a good practice to avoid indefinite hangs, but it's a relatively minor improvement in the context of the entire script.

    6
    Reliability
    Add a liveness probe to the dockerd container to ensure it is restarted if it becomes unresponsive

    Add a livenessProbe to the dockerd container to automatically restart it if it becomes
    unresponsive.

    pipelines/pingcap/tiflash/release-8.2/pod-pull_integration_test.yaml [7]

     name: "dockerd"
    +livenessProbe:
    +  exec:
    +    command:
    +    - cat
    +    - /tmp/healthy
    +  initialDelaySeconds: 15
    +  periodSeconds: 20
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Implementing a liveness probe is crucial for automatically handling cases where the container becomes unresponsive, thus improving the overall reliability.

    7
    Performance
    Optimize the sh command to extract the ccache tar file directly from the source path

    The sh command in the "Prepare Cache" stage that copies the ccache tar file could be
    optimized by using tar -xf directly from the source path to avoid unnecessary cp and cd
    commands.

    pipelines/pingcap/tiflash/release-8.2/pull_unit_test.groovy [105-106]

    -cp -r \$ccache_tar_file ccache.tar
    -tar -xf ccache.tar
    +tar -xf \$ccache_tar_file -C /tmp
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: This suggestion improves performance by reducing unnecessary file operations. Direct extraction using tar is more efficient and reduces the risk of intermediate errors.

    6
    Optimize the sh command to move the tikv directory and create a symbolic link without changing directories

    The sh command in the "Checkout" stage that moves the tikv directory and creates a
    symbolic link could be optimized by using mv and ln commands directly without changing
    directories.

    pipelines/tikv/tikv/release-8.2/pull_unit_test.groovy [69-71]

     mv ./tikv \$HOME/tikv-src
    -cd \$HOME/tikv-src
     ln -s \$HOME/tikv-target \$HOME/tikv-src/target
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion correctly identifies an unnecessary directory change, which simplifies the script slightly. However, the impact on performance or maintainability is minimal, hence the moderate score.

    6
    Combine the two cargo nextest run commands into a single command with multiple partitions

    The sh command in the "Test" stage that runs cargo nextest run twice could be optimized by
    combining the two runs into a single command with multiple partitions.

    pipelines/tikv/tikv/release-8.2/pull_unit_test.groovy [174-187]

    -if cargo nextest run -P ci --binaries-metadata test-binaries.json --cargo-metadata test-metadata.json --partition count:1/2 ${EXTRA_NEXTEST_ARGS}; then
    -    echo "test pass"
    -else
    -    gdb -c core.* -batch -ex "info threads" -ex "thread apply all bt"
    -    exit 1
    -fi
    -if cargo nextest run -P ci --binaries-metadata test-binaries.json --cargo-metadata test-metadata.json --partition count:2/2 ${EXTRA_NEXTEST_ARGS}; then
    +if cargo nextest run -P ci --binaries-metadata test-binaries.json --cargo-metadata test-metadata.json --partition count:1/2 ${EXTRA_NEXTEST_ARGS} && \
    +   cargo nextest run -P ci --binaries-metadata test-binaries.json --cargo-metadata test-metadata.json --partition count:2/2 ${EXTRA_NEXTEST_ARGS}; then
         echo "test pass"
     else
         gdb -c core.* -batch -ex "info threads" -ex "thread apply all bt"
         exit 1
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While the suggestion to combine the commands could streamline the script, it does not account for the need to handle failures between test partitions distinctly, which might be intentional for debugging or logging purposes.

    5
    Maintainability
    Uncomment the priority property if prioritization is needed for the job

    The priority property is commented out. If prioritization is needed, uncomment and set the
    appropriate priority level. Otherwise, remove the commented line to keep the code clean.

    jobs/pingcap/tiflash/release-8.2/pull_integration_test.groovy [14]

    -// priority(0) // 0 fast than 1
    +priority(0) // 0 fast than 1
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion correctly identifies a commented-out configuration line that could be activated for functionality or removed for cleanliness, thus improving the maintainability of the code.

    6
    Use a ConfigMap for the DOCKER_HOST environment variable to manage configurations more effectively

    Consider using a ConfigMap for the environment variable DOCKER_HOST to manage
    configurations more effectively.

    pipelines/pingcap/tiflash/release-8.2/pod-pull_integration_test.yaml [31-32]

     - name: "DOCKER_HOST"
    -value: "tcp://localhost:2375"
    +  valueFrom:
    +    configMapKeyRef:
    +      name: docker-config
    +      key: DOCKER_HOST
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a ConfigMap for environment variables enhances maintainability and flexibility in configuration management, though it's not critical for functionality.

    6

    @ti-chi-bot ti-chi-bot bot removed the lgtm label Jun 25, 2024
    Copy link

    ti-chi-bot bot commented Jun 25, 2024

    [LGTM Timeline notifier]

    Timeline:

    • 2024-06-25 08:21:31.904605799 +0000 UTC m=+707818.390094631: ☑️ agreed by wuhuizuo.
    • 2024-06-25 09:46:24.635359955 +0000 UTC m=+712911.120848787: ✖️🔁 reset by purelind.

    Copy link

    ti-chi-bot bot commented Jun 25, 2024

    New changes are detected. LGTM label has been removed.

    Copy link

    ti-chi-bot bot commented Jun 25, 2024

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

    This Pull Request introduces enhancements and tests by splitting the release 8.2 jobs for TiFlash and TiKV in the PingCAP QE CI.

    Key changes include:

    1. Creation of new folders and pipeline jobs for the respective TiFlash and TiKV release 8.2.
    2. Definition of Jenkins pipelines for TiFlash and TiKV integration and unit tests.
    3. Addition of Kubernetes pod templates for TiFlash and TiKV jobs.
    4. Update of kustomization to include new presubmit jobs for TiFlash and TiKV release 8.2.
    5. Addition of presubmit job configurations for TiFlash and TiKV release 8.2.

    Potential issues:

    1. There are no apparent issues with the PR. However, it's crucial to ensure the new pipeline jobs, Kubernetes pod templates, and presubmit job configurations are correctly set up and adhere to the required standards and practices.

    Fixing suggestions:

    1. Ensure all new files and changes follow the naming conventions and structuring standards used in the project.
    2. After merging, monitor the CI jobs to ensure they run as expected and fix any issues that may arise.
    3. Have another CI/CD engineer review the changes for a second opinion or to catch any potential issues that might have been missed.

    # struct ref: https://pkg.go.dev/k8s.io/test-infra/prow/config#Presubmit
    presubmits:
    tikv/tikv:
    - name: tikv/tikv/release-8.2/pull_unit_test
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    we should disable the old job to watch on release-8.2 branch.

    Copy link

    ti-chi-bot bot commented Jun 26, 2024

    [APPROVALNOTIFIER] This PR is APPROVED

    This pull-request has been approved by: wuhuizuo

    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 Jun 26, 2024

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

    The pull request mainly does the following:

    1. Splits the release 8.2 jobs for the TiFlash and TiKV repositories.
    2. Set up Jenkins pipelines for integration and unit tests.
    3. Defines Kubernetes pod templates for the jobs.
    4. Updates the Kustomize configuration to include the new jobs.
    5. Adds presubmit job configurations for the TiFlash and TiKV release 8.2.

    Key files:

    • aa_folder.groovy: Creates new folders for TiFlash and TiKV release 8.2 pipelines.
    • pull_integration_test.groovy and pull_unit_test.groovy: Adds new pipeline jobs for TiFlash and TiKV release 8.2 integration and unit tests.
    • pod-pull_build.yaml, pod-pull_integration_test.yaml, and pod-pull_unit_test.yaml: Defines Kubernetes pod templates for the jobs.
    • kustomization.yaml: Updates the Kustomize configuration to include the new jobs.
    • release-8.2-presubmits.yaml: Adds presubmit job configurations for the TiFlash and TiKV release 8.2.

    Potential problems:

    • The new pipelines and jobs are not tested yet. There may be configuration errors or compatibility issues.
    • The Kubernetes pod templates may not match the actual resource requirements of the jobs.

    Suggestions:

    • You should test the new pipelines and jobs in a controlled environment before merging.
    • Review the Kubernetes pod templates and adjust the resource requests and limits as necessary.
    • If there are any problems, you should revert the changes and fix the issues before resubmitting the pull request.

    @purelind purelind added the lgtm label Jun 26, 2024
    @ti-chi-bot ti-chi-bot bot merged commit d03db1d into PingCAP-QE:main Jun 26, 2024
    2 of 3 checks passed
    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