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): gen build cache in merge ci for tiflash #3187

Conversation

purelind
Copy link
Collaborator

Gen build cache in merge ci for tiflash.

  • update base image from centos7 to rockylinux 8 because centos7 end of life
  • update ccache version from 4.5 to 4.10
  • Gen build and ut cache in merged pipelines

Copy link

ti-chi-bot bot commented Oct 29, 2024

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

Pull Request (PR) Title:

feat(tiflash): gen build cache in merge ci for tiflash

PR Description:

Gen build cache in merge ci for tiflash.
* update base image from centos7 to rockylinux 8 because centos7 end of life
* update ccache version from 4.5 to 4.10
* Gen build and ut cache in merged pipelines

Key Changes:

  1. A new Jenkins pipeline job 'pingcap/tiflash/merged_build' is introduced. This job provides a mechanism to generate build cache in merge CI for TiFlash.
  2. New groovy scripts for the pipeline job are added. These scripts define the stages and steps of the pipeline job including debug information collection, source code checkout, cache preparation, project configuration, build, license check, and post-build activities.
  3. The base image for the pipeline job has been updated from CentOS 7 to Rocky Linux 8 due to CentOS 7's end of life.
  4. The ccache version used in the CI/CD pipeline has been updated from 4.5 to 4.10.
  5. The pipeline job is configured to generate both build and unit test (ut) cache.
  6. The Kubernetes namespace for the Jenkins job has been updated from jenkins-tidb to jenkins-tiflash.

Potential Issues:

  1. The PR adds new dependencies (like the specific base image and ccache version). If these dependencies are not available in the CI/CD environment, the pipeline job may fail.
  2. There are hardcoded values in the scripts (e.g., memory and CPU resource requests/limits, NFS server IP, paths). These values may not suit all CI/CD environments and may need to be customized.
  3. The pipeline job uses a specific namespace (jenkins-tiflash). If this namespace does not exist in the Kubernetes cluster where Jenkins runs, the job may fail.
  4. The PR adds TODO comments in the code, indicating areas that may need further work or adjustment.
  5. The merged_build and merged_unit_test scripts have a high degree of complexity and many steps. If any step fails, it may be challenging to diagnose and fix.

Suggestions:

  1. Ensure the new dependencies are available in the CI/CD environment.
  2. Replace hardcoded values with environment variables or Jenkins parameters to make the scripts more flexible.
  3. Ensure the jenkins-tiflash namespace exists in the Kubernetes cluster, or replace it with an existing one.
  4. Address the TODO comments in the code as soon as possible to prevent technical debt.
  5. Consider breaking down the scripts into smaller, reusable parts to improve maintainability and error handling. For example, separate scripts could be used for setup, build, and cleanup stages.

Copy link

ti-chi-bot bot commented Oct 29, 2024

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

Summary of Changes:

  • A new Jenkins pipeline job for building the tiflash project is introduced. This includes configuring the project, building it, checking licenses, and uploading build artifacts.
  • The base image is updated from centos7 to rockylinux 8, which is due to the end of life of centos7.
  • Ccache version is updated from 4.5 to 4.10.
  • Jenkins Job DSL is used to define the pipeline job.
  • The Jenkins pod for this job is configured with necessary resources and environment settings.
  • The build process involves fetching necessary dependencies, setting up ccache and other necessary tools, building the project, and performing post-build tasks.
  • The post-build tasks involve uploading build artifacts and updating build caches.
  • The unit_test script is also updated for the same base image change and ccache version change.

Potential Problems:

  • The namespace for Jenkins job is set as jenkins-tiflash but it's commented as need to adjust namespace after test. It's better to confirm if the namespace setting is appropriate.
  • In the Prepare Cache stage, the ccache configuration is set as read_only, which may affect scenarios where cache needs to be updated.
  • The script for uploading ccache and proxy cache in the Post Build stage is not fully implemented. This may affect the caching mechanism for future builds.
  • The license check is commented as TODO, it should be implemented to ensure the project doesn't have potential license issues.

Suggestions:

  • Confirm the appropriate namespace for the Jenkins job.
  • If the build process needs to update the ccache, the read_only option in ccache configuration should be reconsidered.
  • Implement the scripts for uploading ccache and proxy cache in the Post Build stage to ensure the caching mechanism works.
  • Implement the license check in the License check stage to avoid potential license issues.
  • Double-check all the TODO comments and address them.

Copy link

ti-chi-bot bot commented Oct 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

@ti-chi-bot ti-chi-bot bot merged commit b62ce0e into PingCAP-QE:main Oct 29, 2024
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.

1 participant