ticdc: Add release-8.5 CI jobs for TiCDC integration tests. #3884
Conversation
|
I Skip it since the diff size(88948 bytes > 80000 bytes) is too large |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @tenfyzhong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust continuous integration pipeline for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds CI jobs for the release-8.5 branch. While the overall structure is good, there are several critical copy-paste errors that will likely cause the new CI jobs to fail. The pod definitions for MySQL, Pulsar, and storage integration tests are incorrect as they are missing necessary service containers. The corresponding Jenkins pipeline scripts are also misconfigured. Additionally, some Prow jobs in release-8.5-presubmits.yaml are incorrectly targeting the master branch. I've provided detailed comments and suggestions to address these issues.
| apiVersion: v1 | ||
| kind: Pod | ||
| spec: | ||
| securityContext: | ||
| fsGroup: 1000 | ||
| containers: | ||
| - name: golang | ||
| image: "hub.pingcap.net/jenkins/rocky8_golang-1.23:latest" | ||
| tty: true | ||
| resources: | ||
| limits: | ||
| memory: 16Gi | ||
| cpu: "12" | ||
| - name: net-tool | ||
| image: hub.pingcap.net/jenkins/network-multitool | ||
| tty: true | ||
| resources: | ||
| limits: | ||
| memory: 128Mi | ||
| cpu: 100m | ||
| affinity: | ||
| nodeAffinity: | ||
| requiredDuringSchedulingIgnoredDuringExecution: | ||
| nodeSelectorTerms: | ||
| - matchExpressions: | ||
| - key: kubernetes.io/arch | ||
| operator: In | ||
| values: | ||
| - amd64 |
There was a problem hiding this comment.
This pod specification appears to be a copy of the build pod and is missing the necessary containers for a MySQL integration test, specifically a MySQL database container. The test will likely fail without it.
This issue also applies to the following files, which are all missing their respective service containers:
pod-pull_cdc_mysql_integration_light.yaml(missing MySQL)pod-pull_cdc_pulsar_integration_light.yaml(missing Pulsar)pod-pull_cdc_storage_integration_heavy.yaml(missing S3-like storage)pod-pull_cdc_storage_integration_light.yaml(missing S3-like storage)
Please provide the correct pod specifications for these tests, including all required service containers.
| final POD_TEMPLATE_FILE = 'pipelines/pingcap/ticdc/release-8.5/pod-pull_cdc_mysql_integration_heavy.yaml' | ||
| final REFS = readJSON(text: params.JOB_SPEC).refs | ||
| def skipRemainingStages = false | ||
|
|
||
| pipeline { | ||
| agent { | ||
| kubernetes { | ||
| namespace K8S_NAMESPACE | ||
| yamlFile POD_TEMPLATE_FILE | ||
| defaultContainer 'golang' | ||
| } | ||
| } |
There was a problem hiding this comment.
This pipeline is misconfigured and contains an unused variable.
- The variable
skipRemainingStageson line 11 is defined but never used and should be removed. - The pipeline should use a separate, simpler build pod for the initial stages (
Debug info,Checkout,prepare) and the specific test pod for theTestsstage. This is a common pattern for efficiency and correctness, as seen inpull_cdc_kafka_integration_heavy.groovy. You should definePOD_TEMPLATE_FILE_BUILDand use it for the top-level agent.
These issues also apply to pull_cdc_mysql_integration_light.groovy, pull_cdc_pulsar_integration_light.groovy, pull_cdc_storage_integration_heavy.groovy, and pull_cdc_storage_integration_light.groovy.
final POD_TEMPLATE_FILE = 'pipelines/pingcap/ticdc/release-8.5/pod-pull_cdc_mysql_integration_heavy.yaml'
final POD_TEMPLATE_FILE_BUILD = 'pipelines/pingcap/ticdc/release-8.5/pod-pull_cdc_integration_build.yaml'
final REFS = readJSON(text: params.JOB_SPEC).refs
pipeline {
agent {
kubernetes {
namespace K8S_NAMESPACE
yamlFile POD_TEMPLATE_FILE_BUILD
defaultContainer 'golang'
}
}
| branches: | ||
| - ^master$ |
There was a problem hiding this comment.
This job is configured to run on the master branch, but this file is for release-8.5 presubmits. This is likely a copy-paste error. This job should target the release-8.5 branch by using the *branches anchor defined at the top of the file. This also applies to pull-build, pull-check, and pull-error-log-review jobs in this file.
branches: *branches| securityContext: | ||
| fsGroup: 1000 | ||
| containers: | ||
| - image: wurstmeister/zookeeper |
There was a problem hiding this comment.
For reproducible CI builds, it's best practice to use a specific image tag instead of relying on the implicit :latest tag. Please specify a version for the wurstmeister/zookeeper image. This also applies to the same image in pod-pull_cdc_kafka_integration_light.yaml.
- image: wurstmeister/zookeeper:3.4.6| value: "3306" | ||
| - name: DB_NAME | ||
| value: test | ||
| image: rustinliu/ticdc-canal-json-adapter:latest |
There was a problem hiding this comment.
Using the :latest tag for Docker images in CI can lead to unpredictable behavior and failures if the image is updated. Please use a specific version tag or a digest for the rustinliu/ticdc-canal-json-adapter image to ensure build reproducibility. This also applies to the same image in pod-pull_cdc_kafka_integration_light.yaml.
| agent: jenkins | ||
| # skip_if_only_changed: *skip_if_only_changed | ||
| run_before_merge: true |
There was a problem hiding this comment.
Jenkins presubmit jobs usually require decorate: false to prevent Prow from adding its own pod utilities, which can interfere with the Jenkins agent setup. This job definition is missing it. Please add decorate: false for consistency with the other Jenkins jobs in this file.
agent: jenkins
decorate: false # need add this.
run_before_merge: true
- Add Jenkins jobs for release-8.5 branch of the ticdc repository. - Configure Prow jobs to trigger these Jenkins jobs for the release-8.5 branch. - Include various integration tests (MySQL, Kafka, Pulsar, Storage) for both light and heavy scenarios. - Define Kubernetes pod templates for these jobs. Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
tenfyzhong
force-pushed
the
ticdc-8.5
branch
from
8a02861 to
c007643
Compare
|
I Skip it since the diff size(88948 bytes > 80000 bytes) is too large |
|
@tenfyzhong: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds CI infrastructure for the TiCDC release-8.5 branch by creating comprehensive test job configurations, Jenkins pipelines, and Kubernetes pod templates to run integration tests for MySQL, Kafka, Pulsar, and Storage integrations in both light and heavy configurations.
Key Changes
- Created Prow job configuration for release-8.5 branch with integration test definitions for MySQL, Kafka, Pulsar, and Storage
- Added Jenkins pipeline scripts for each integration test type (light/heavy variants)
- Defined Kubernetes pod templates with appropriate resource allocations for test execution
- Configured Jenkins job DSL files to wire up the CI pipeline infrastructure
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| prow-jobs/pingcap/ticdc/release-8.5-presubmits.yaml | Defines presubmit job configurations for release-8.5 branch including integration test triggers and contexts |
| prow-jobs/kustomization.yaml | Registers the new release-8.5-presubmits.yaml configuration file |
| pipelines/pingcap/ticdc/release-8.5/*.groovy | Jenkins pipeline scripts implementing build, test, and deployment logic for each integration test type |
| pipelines/pingcap/ticdc/release-8.5/pod-*.yaml | Kubernetes pod templates defining container images, resources, and runtime configurations |
| jobs/pingcap/ticdc/release-8.5/*.groovy | Jenkins job DSL definitions linking Prow jobs to pipeline scripts |
| jobs/pingcap/ticdc/release-8.5/aa_folder.groovy | Creates the Jenkins folder structure for release-8.5 jobs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: pull-unit-test | ||
| decorate: true # need add this. | ||
| skip_if_only_changed: *skip_if_only_changed | ||
| branches: | ||
| - ^master$ | ||
| spec: | ||
| containers: | ||
| - name: check | ||
| image: &image ghcr.io/pingcap-qe/ci/base:v2024.10.8-32-ge807718-go1.23 | ||
| command: [bash, -ce] | ||
| args: | ||
| - | | ||
| make unit_test_in_verify_ci | ||
| resources: | ||
| requests: | ||
| memory: 24Gi | ||
| cpu: "12" | ||
| limits: | ||
| memory: 24Gi | ||
| cpu: "12" | ||
|
|
||
| - name: pull-build | ||
| decorate: true # need add this. | ||
| skip_if_only_changed: *skip_if_only_changed | ||
| branches: | ||
| - ^master$ | ||
| spec: | ||
| containers: | ||
| - name: build | ||
| image: *image | ||
| command: [bash, -ce] | ||
| args: | ||
| - | | ||
| make cdc | ||
| resources: &resources | ||
| requests: | ||
| memory: 12Gi | ||
| cpu: "6" | ||
| limits: | ||
| memory: 24Gi | ||
| cpu: "6" | ||
|
|
||
| - name: pull-check | ||
| decorate: true # need add this. | ||
| skip_if_only_changed: *skip_if_only_changed | ||
| branches: | ||
| - ^master$ | ||
| spec: | ||
| containers: | ||
| - name: check | ||
| image: *image | ||
| command: [bash, -ce] | ||
| args: | ||
| - | | ||
| make check | ||
| resources: *resources | ||
|
|
||
| - name: pull-error-log-review | ||
| branches: | ||
| - ^master$ | ||
| decorate: true | ||
| decoration_config: | ||
| timeout: 20m | ||
| skip_cloning: true | ||
| trigger: "(?m)^/test (?:.*? )?pull-error-log-review(?: .*?)?$" | ||
| rerun_command: "/test pull-error-log-review" | ||
| skip_if_only_changed: *skip_if_only_changed | ||
| spec: | ||
| containers: | ||
| - name: check | ||
| image: ghcr.io/pingcap-qe/ci/base:v2024.10.8-32-ge807718-go1.23 | ||
| command: [/bin/bash, -ce] | ||
| args: | ||
| - | | ||
| if [[ "${JOB_TYPE:-}" == "batch" ]]; then | ||
| echo "JOB_TYPE is batch, skipping error log review check" | ||
| exit 0 | ||
| fi | ||
| wget -O /tmp/config.yaml https://raw.githubusercontent.com/pingcap-qe/ci/main/configs/error-log-review/config.yaml | ||
| go run github.com/PingCAP-QE/ci/tools/error-log-review@main \ | ||
| -config /tmp/config.yaml \ | ||
| -pr "${REPO_OWNER}/${REPO_NAME}#${PULL_NUMBER}" | ||
| env: | ||
| - name: GITHUB_API_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: token | ||
| name: github-token | ||
| resources: | ||
| requests: | ||
| cpu: "1" | ||
| memory: 2Gi | ||
| limits: | ||
| cpu: "1" | ||
| memory: 2Gi |
There was a problem hiding this comment.
Branch filter mismatch. The jobs defined for release-8.5 branch (lines 9-77) have branches: *branches which correctly targets ^release-8\.5(\.\d)?$. However, the subsequent jobs (pull-unit-test, pull-build, pull-check, pull-error-log-review) on lines 79-173 are configured with branches: - ^master$. These jobs will never run for the release-8.5 branch. Either these jobs should be removed from this file, or their branch configuration should be updated to match the release-8.5 branch pattern.
| @@ -0,0 +1,154 @@ | |||
| // REF: https://www.jenkins.io/doc/book/pipeline/syntax/#declarative-pipeline | |||
| // Keep small than 400 lines: https://issues.jenkins.io/browse/JENKINS-37984 | |||
| // should triggerd for master branches | |||
There was a problem hiding this comment.
Typo: "triggerd" should be "triggered". The comment says "should triggerd for master branches" but should read "should be triggered for master branches".
| // should triggerd for master branches | |
| // should be triggered for master branches |
| @@ -0,0 +1,154 @@ | |||
| // REF: https://www.jenkins.io/doc/book/pipeline/syntax/#declarative-pipeline | |||
| // Keep small than 400 lines: https://issues.jenkins.io/browse/JENKINS-37984 | |||
| // should triggerd for master branches | |||
There was a problem hiding this comment.
Typo: "triggerd" should be "triggered". The comment says "should triggerd for master branches" but should read "should be triggered for master branches".
| // should triggerd for master branches | |
| // should be triggered for master branches |
| @@ -0,0 +1,153 @@ | |||
| // REF: https://www.jenkins.io/doc/book/pipeline/syntax/#declarative-pipeline | |||
| // Keep small than 400 lines: https://issues.jenkins.io/browse/JENKINS-37984 | |||
| // should triggerd for master branches | |||
There was a problem hiding this comment.
Typo: "triggerd" should be "triggered". The comment says "should triggerd for master branches" but should read "should be triggered for master branches".
| // should triggerd for master branches | |
| // should be triggered for master branches |
| @@ -0,0 +1,151 @@ | |||
| // REF: https://www.jenkins.io/doc/book/pipeline/syntax/#declarative-pipeline | |||
| // Keep small than 400 lines: https://issues.jenkins.io/browse/JENKINS-37984 | |||
| // should triggerd for master branches | |||
There was a problem hiding this comment.
Typo: "triggerd" should be "triggered". The comment says "should triggerd for master branches" but should read "should be triggered for master branches".
| // should triggerd for master branches | |
| // should be triggered for master branches |
| dir("ticdc") { | ||
| cache(path: "./bin", includes: '**/*', key: prow.getCacheKey('binary', REFS, 'cdc-pulsar-integration')) { | ||
| // build cdc, pulsar_consumer, cdc.test for integration test | ||
| // only build binarys if not exist, use the cached binarys if exist |
There was a problem hiding this comment.
Typo: "binarys" should be "binaries". The comment on line 90 says "only build binarys if not exist" but should read "only build binaries if not exist".
| // only build binarys if not exist, use the cached binarys if exist | |
| // only build binaries if not exist, use the cached binaries if exist |
| dir("ticdc") { | ||
| cache(path: "./bin", includes: '**/*', key: prow.getCacheKey('binary', REFS, 'cdc-mysql-integration')) { | ||
| // build cdc, kafka_consumer, storage_consumer, cdc.test for integration test | ||
| // only build binarys if not exist, use the cached binarys if exist |
There was a problem hiding this comment.
Typo: "binarys" should be "binaries". The comment on line 90 says "only build binarys if not exist" but should read "only build binaries if not exist".
| // only build binarys if not exist, use the cached binarys if exist | |
| // only build binaries if not exist, use the cached binaries if exist |
| dir("ticdc") { | ||
| cache(path: "./bin", includes: '**/*', key: prow.getCacheKey('binary', REFS, 'cdc-mysql-integration')) { | ||
| // build cdc, kafka_consumer, storage_consumer, cdc.test for integration test | ||
| // only build binarys if not exist, use the cached binarys if exist |
There was a problem hiding this comment.
Typo: "binarys" should be "binaries". The comment on line 90 says "only build binarys if not exist" but should read "only build binaries if not exist".
| // only build binarys if not exist, use the cached binarys if exist | |
| // only build binaries if not exist, use the cached binaries if exist |
| dir("ticdc") { | ||
| cache(path: "./bin", includes: '**/*', key: prow.getCacheKey('binary', REFS, 'cdc-integration-test')) { | ||
| // build cdc, kafka_consumer, storage_consumer, cdc.test for integration test | ||
| // only build binarys if not exist, use the cached binarys if exist |
There was a problem hiding this comment.
Typo: "binarys" should be "binaries". The comment on line 90 says "only build binarys if not exist" but should read "only build binaries if not exist".
| // only build binarys if not exist, use the cached binarys if exist | |
| // only build binaries if not exist, use the cached binaries if exist |
| dir("ticdc") { | ||
| cache(path: "./bin", includes: '**/*', key: prow.getCacheKey('binary', REFS, 'cdc-integration-test')) { | ||
| // build cdc, kafka_consumer, storage_consumer, cdc.test for integration test | ||
| // only build binarys if not exist, use the cached binarys if exist |
There was a problem hiding this comment.
Typo: "binarys" should be "binaries". The comment on line 90 says "only build binarys if not exist" but should read "only build binaries if not exist".
| // only build binarys if not exist, use the cached binarys if exist | |
| // only build binaries if not exist, use the cached binaries if exist |
1 participant
feat: Add release-8.5 branch for TiCDC CI jobs
This pull request introduces the necessary configurations to enable CI jobs for the
release-8.5branch of the TiCDC repository. This ensures that integration tests are run for this specific release branch, maintaining code quality and stability.The following changes have been made:
pingcap/ticdc/release-8.5has been created in Jenkins to house the jobs for this release branch.release-8.5branch and specific pod templates.release-8.5-presubmits.yaml) has been created. This file defines the presubmit jobs that will be triggered for therelease-8.5branch, including the integration tests.kustomization.yaml: Thekustomization.yamlfile has been updated to include the newrelease-8.5-presubmits.yamlfile, ensuring it's picked up by the Kustomize build process.