feat(ci): refactor tidb-test latest stage agents#4623
Conversation
|
[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 |
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR aims to refactor multiple pipeline Groovy scripts by transitioning from pipeline-wide Kubernetes agents to stage-specific Kubernetes agents with retries 2, enhancing modularity and error resilience. The workspace handoff behavior is preserved by introducing caching mechanisms in critical stages. While the refactor is comprehensive and aligns with the stated goals, there are minor opportunities to improve maintainability, address redundancy, and streamline certain patterns.
Critical Issues
None identified. The refactor appears to function as intended without introducing bugs, security risks, or performance degradation.
Code Improvements
-
Redundant
agentdefinitions:- Files: All modified Groovy scripts.
- Issue: Many stages share identical
agentconfigurations, which could lead to redundancy and make future updates harder. Maintaining common configurations in a shared variable or function could simplify the scripts. - Solution:
def commonAgentConfig = { kubernetes { namespace K8S_NAMESPACE yamlFile POD_TEMPLATE_FILE retries 2 defaultContainer 'golang' } } ... stage('Checkout') { agent { commonAgentConfig() } options { timeout(time: 5, unit: 'MINUTES') } steps { ... } }
-
Caching keys consistency:
- Files: Multiple files (e.g.,
ghpr_mysql_test/pipeline.groovy,pull_mysql_test_next_gen/pipeline.groovy). - Issue: Cache key generation uses different patterns (
ws/${BUILD_TAG}vs.git/pingcap/...). Ensuring a consistent pattern across all pipelines could reduce confusion and potential cache collisions. - Solution: Standardize cache key generation logic into a helper method or utility function.
- Files: Multiple files (e.g.,
Best Practices
-
Code duplication:
- Files: All modified Groovy scripts.
- Issue: The
agentdefinitions and caching logic are repeated across multiple stages and files. Centralizing these via shared functions or templates could improve maintainability. - Solution: Extract shared logic into reusable Groovy functions or Jenkins shared libraries.
-
Testing and verification:
- Files: Not directly present in the diff.
- Issue: While the PR mentions manual verification steps (
git diff --check), automated tests for pipeline logic changes (e.g., unit tests for shared libraries or dry runs) are not outlined. - Solution: Incorporate automated validation scripts to ensure changes do not break CI workflows. Use tools like
pipeline-linterfor Jenkinsfiles.
-
Missing documentation:
- Files: All modified Groovy scripts.
- Issue: The rationale for specific changes (e.g., why
agent noneis preferred, or whyretries 2is used) is not documented in the scripts. This could make it harder for future developers to understand the logic. - Solution: Add comments at the top of each script or near major changes explaining the refactor and its intent.
Additional Suggestions
- Error handling for caching:
-
Files: Multiple files (e.g.,
pull_mysql_test_next_gen/pipeline.groovy). -
Issue: Cache restore commands (
sh label: 'restore tidb') do not include error handling or fallback mechanisms for cache failures. -
Solution: Add retry logic or fallback behavior for critical caching steps to ensure pipeline resilience.
cache(path: "./", includes: '**/*', key: "git/pingcap/tidb/rev-${REFS.pulls[0].sha}", restoreKeys: ['git/pingcap/tidb/rev-']) { retry(2) { sh label: 'restore tidb', script: 'git rev-parse HEAD || echo "Cache restore failed"' } }
-
By addressing the redundancy and improving documentation, this PR can become more maintainable and easier to understand for future contributors.
|
/hold |
There was a problem hiding this comment.
Code Review
This pull request refactors multiple Jenkins pipelines by moving agent definitions to specific stages, adding retries to Kubernetes configurations, and implementing repository caching. Reviewers identified syntax errors in hardcoded cache keys and recommended using prow.getCacheKey for improved consistency and robustness.
| } | ||
| steps { | ||
| dir("tidb") { | ||
| cache(path: "./", includes: '**/*', key: "git/pingcap/tidb/rev-${REFS.pulls[0].sha}}", restoreKeys: ['git/pingcap/tidb/rev-']) { |
There was a problem hiding this comment.
There is a typo in the cache key: an extra closing brace } at the end of the string interpolation ${REFS.pulls[0].sha}}. For better consistency and robustness, it is recommended to use prow.getCacheKey('git', REFS) instead of hardcoding the key. Please also check and fix the same typo in the Checkout stage (line 30).
cache(path: "./", includes: '**/*', key: prow.getCacheKey('git', REFS), restoreKeys: prow.getRestoreKeys('git', REFS)) {
References
- In Groovy pipeline scripts for this repository, the REFS.repo variable evaluates to the string 'tidb', making it interchangeable with the hardcoded value.
| } | ||
| } | ||
| dir("tidb-test") { | ||
| cache(path: "./", includes: '**/*', key: "git/PingCAP-QE/tidb-test/rev-${REFS.pulls[0].sha}}", restoreKeys: ['git/PingCAP-QE/tidb-test/rev-']) { |
There was a problem hiding this comment.
There is a typo in the cache key: an extra closing brace } at the end of the string interpolation ${REFS.pulls[0].sha}}. For better consistency and robustness (especially for batch builds), it is recommended to use prow.getCacheKey('git', REFS) instead of hardcoding the key. Please also check and fix the same typo in the Checkout stage (line 39).
cache(path: "./", includes: '**/*', key: prow.getCacheKey('git', REFS), restoreKeys: prow.getRestoreKeys('git', REFS)) {
References
- In Groovy pipeline scripts for this repository, the REFS.repo variable evaluates to the string 'tidb', making it interchangeable with the hardcoded value.
Summary
pipelines/pingcap-qe/tidb-test/latest/*/pipeline.groovyfiles from pipeline-top Kubernetes agents toagent noneplus stage or matrix-local Kubernetes agentsretries 2to every stage or matrix Kubernetes agent in that batchPrepare,Build, and the standalone Tiproxy connector test stage so the refactor keeps the previous workspace handoff behaviorVerification
git -C ci diff --checkpipelines/pingcap-qe/tidb-test/latestagent { kubernetes { ... } }blocks in that batch now includesretries 2