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(pingcap/tiflash): add refactored jobs for release-8.1 #3064

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented Aug 7, 2024

Fixes #3063

Signed-off-by: wuhuizuo wuhuizuo@126.com

Copy link

ti-chi-bot bot commented Aug 7, 2024

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

The pull request is titled "feat(pingcap/tiflash): add refactored jobs for release-8.1"

The main changes in this pull request are:

  1. Added new Jenkins job definitions for the "release-8.1" of the "pingcap/tiflash" repository. These new jobs include "pull_unit_test" and "pull_integration_test".

  2. Added new Jenkins pipeline definitions for the "release-8.1" of the "pingcap/tiflash" repository. These pipelines include steps for running unit tests and integration tests.

  3. Added new Kubernetes Pod definitions for the "release-8.1" of the "pingcap/tiflash" repository. These Pods define the environment for running the Jenkins jobs.

  4. Updated the "prow-jobs/kustomization.yaml" file to include the new job and pipeline definitions.

Potential problems:

  1. Environment Variables: The environment variables used in the jobs and pipelines are not defined in this pull request. This could cause errors if they are not set correctly in the Jenkins and/or Kubernetes environment.

  2. Branch Filtering: The branches for which the jobs should be triggered are defined using regular expressions. If the branch names in the repository do not match these patterns, the jobs will not be triggered.

  3. Code Quality Checks: There seem to be no code quality checks (such as linting or static code analysis) included in the jobs or pipelines.

Fixing suggestions:

  1. Ensure that all required environment variables are defined in the Jenkins and/or Kubernetes environment.

  2. Verify that the branch name patterns correctly match the branch names in the repository.

  3. Consider adding code quality checks to the jobs or pipelines to ensure the quality of the code.

Fixes #3063

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the fix/add-tiflash-8.1-pipelines branch from d0dc33f to 0a04b03 Compare August 7, 2024 03:48
Copy link

ti-chi-bot bot commented Aug 7, 2024

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

The pull request introduces new jobs for the release-8.1 branch of the pingcap/tiflash repository. Here's a summary of the changes and potential issues:

Summary

  • Adds a new folder for pipelines of pingcap/tiflash repo for version 8.1.
  • Creates new Jenkins jobs for Integration Test and Unit Test for the release-8.1 branch.
  • Defines new Kubernetes pod configuration for the build process and tests.
  • Refactors the pipeline scripts for the release-8.1 branch.

Potential Issues

  • Directly using a hard-coded IP address 10.2.12.82 for the NFS server in the Kubernetes pod definition may cause problems if the server IP changes or is not accessible from some environments. It would be better to use a domain name or a Kubernetes Service for this.

  • In the script for run-gtest.sh, the parallelism value is set to 12. This could lead to high resource usage if the test runner machine does not have enough resources.

  • There are commented lines like // priority(0) // 0 fast than 1 and // TODO: remove this default container in the Jenkins job definition scripts. These may need to be addressed or removed.

Suggestions

  • Replace the hard-coded NFS server IP with a more reliable method of location, such as a domain name or a Kubernetes Service.

  • Make sure the parallelism value is appropriate for the test runner machine's resources.

  • Address or remove the commented lines in the Jenkins job definition scripts. If they are not needed, it's better to clean them to avoid confusion for future maintenance.

Comment on lines +39 to +52
- name: jnlp # TODO: remove this default container
image: "jenkins/inbound-agent:3148.v532a_7e715ee3-1"
resources:
requests:
cpu: "500m"
memory: "500Mi"
limits:
cpu: "500m"
memory: "500Mi"
volumeMounts:
- mountPath: "/home/jenkins/agent/rust"
name: "volume-0"
readOnly: false
- mountPath: "/home/jenkins/agent/ccache"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@purelind we should remove this container. It just a commander container which Jenkins will append it automatically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a cache presence check in the pipeline that will definitely be executed in the jnlp container, so it cannot be removed for now.

wuhuizuo and others added 2 commits August 9, 2024 11:40
Co-authored-by: Purelind <purelind@users.noreply.github.com>
Co-authored-by: Purelind <purelind@users.noreply.github.com>
Copy link

ti-chi-bot bot commented Aug 9, 2024

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

Key changes:

  1. A new file jobs/pingcap/tiflash/release-8.1/aa_folder.groovy is added.
  2. Two new files jobs/pingcap/tiflash/release-8.1/pull_integration_test.groovy and jobs/pingcap/tiflash/release-8.1/pull_unit_test.groovy are added.
  3. Four new files pipelines/pingcap/tiflash/release-8.1/pod-pull_build.yaml, pipelines/pingcap/tiflash/release-8.1/pod-pull_integration_test.yaml, pipelines/pingcap/tiflash/release-8.1/pod-pull_unit-test.yaml, and pipelines/pingcap/tiflash/release-8.1/pull_integration_test.groovy are added.
  4. prow-jobs/kustomization.yaml is updated to include the new files.
  5. New CI/CD pipelines are added for the pingcap/tiflash repository for the release-8.1 branch.

Potential problems:

  1. The pull request does not have a proper description detailing the changes made. It's always good to have a comprehensive summary of changes for reviewers to understand what has been done.
  2. There are no tests included in the PR to verify the changes.

Fix suggestions:

  1. Add a detailed description to the pull request detailing what changes have been made and why they were necessary.
  2. Add tests to verify the new pipelines are working as expected.
  3. It would also be good to have another developer familiar with this repository to review the changes.

Copy link

ti-chi-bot bot commented Aug 9, 2024

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

The pull request introduces new jobs for the pingcap/tiflash repository for the release-8.1 branch.

Key Changes:

  1. A new folder pingcap/tiflash/release-8.1 is created with the description "Folder for pipelines of pingcap/tiflash repo for v8.1".
  2. New Jenkins pipeline jobs pull_integration_test and pull_unit_test are added for the release-8.1 branch.
  3. New pod templates pod-pull_build.yaml, pod-pull_integration_test.yaml and pod-pull_unit-test.yaml are created for Kubernetes.
  4. New pipeline scripts pull_integration_test.groovy and pull_unit_test.groovy are added.

Potential Problems:

  1. No unit tests or integration tests are included in the PR to verify these changes.
  2. The PR lacks a detailed description about the changes made which makes it difficult to understand the context and the impact of the changes.

Fixing Suggestions:

  1. Please add tests to validate the changes.
  2. Please update the PR description with more details about the changes and their impact.

Co-authored-by: Purelind <purelind@users.noreply.github.com>
Copy link

ti-chi-bot bot commented Aug 9, 2024

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

The pull request is for adding new jobs for the release-8.1 of the pingcap/tiflash project.

Key changes:

  1. The PR adds a new folder pingcap/tiflash/release-8.1 for the pipelines and jobs related to the release-8.1.

  2. The PR includes two new groovy scripts: pull_integration_test.groovy and pull_unit_test.groovy under pingcap/tiflash/release-8.1 for the integration and unit tests respectively.

  3. New YAML files: pod-pull_build.yaml, pod-pull_integration_test.yaml, and pod-pull_unit-test.yaml under pingcap/tiflash/release-8.1 are added for Kubernetes pod specifications.

  4. A new file release-8.1-presubmits.yaml is added under prow-jobs/pingcap/tiflash, it contains the presubmit job configurations for the release-8.1.

Potential problems:

  1. Lack of documentation: There is no added documentation or comments in the scripts and configuration files.

  2. Missing testing: There is no indication if the new scripts and configurations have been tested and are working as expected.

Suggestions:

  1. Add comments/documentation in the scripts and configuration files to explain what they are for and how they work.

  2. Test the new scripts and configurations in a controlled environment before merging the changes to the main branch.

  3. Perform a code review for the Groovy and YAML scripts to ensure there are no syntax errors or logical issues.

  4. After merging, monitor the CI/CD pipeline to ensure these new jobs are running and completing successfully. If any errors occur, debug and fix them promptly.

  5. Ensure the new jobs do not significantly increase the overall time taken for the CI/CD pipeline to complete. If they do, consider optimizing the jobs or running some of them in parallel.

Co-authored-by: Purelind <purelind@users.noreply.github.com>
Copy link

ti-chi-bot bot commented Aug 9, 2024

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

The pull request adds new refactored jobs for release-8.1 of the pingcap/tiflash project. The main changes are:

  • It adds a new Groovy files for the jobs: aa_folder.groovy, pull_integration_test.groovy, and pull_unit_test.groovy. These files define the structure and properties of the Jenkins jobs.

  • It also adds new yaml files for the pipelines: pod-pull_build.yaml, pod-pull_integration_test.yaml, pod-pull_unit-test.yaml, pull_integration_test.groovy and pull_unit_test.groovy. These files define the specs of the pod used for the Jenkins jobs and the scripts to run during the pipeline execution.

Potential problems:

  1. There is no validation to check if the added scripts (pull_integration_test.groovy and pull_unit_test.groovy) have the correct syntax and structure.

  2. The scripts are referencing a specific branch (main) and a specific version of the tiflash-llvm-base image (amd64-llvmorg-14.0.6). This could lead to problems if these versions become outdated or are deleted.

  3. The scripts are using hardcoded values for certain parameters (like memory and CPU requests), which might not be suitable for all environments.

Fixing suggestions:

  1. Add a validation step to check the syntax and structure of the added Groovy and yaml scripts.

  2. Instead of hardcoding the branch and image version, consider using a variable or a configuration file so it can be easily updated.

  3. Consider using environment variables or a configuration file for the resource requests to make the scripts more flexible.

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

@ti-chi-bot ti-chi-bot bot added the lgtm label Aug 12, 2024
Copy link

ti-chi-bot bot commented Aug 12, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-08-12 07:08:32.804311854 +0000 UTC m=+165397.507781494: ☑️ agreed by purelind.

Copy link

ti-chi-bot bot commented Aug 12, 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

@ti-chi-bot ti-chi-bot bot added the approved label Aug 12, 2024
@ti-chi-bot ti-chi-bot bot merged commit 4ccea2f into main Aug 12, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/add-tiflash-8.1-pipelines branch August 12, 2024 07:16
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.

[TiFlash@release-8.1] Support more integration pipeline in VerfiyCI of tidb lts branches
2 participants