ci(prow): migrate tikv/tikv release-8.1 jobs to target jenkins - #5156
Conversation
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 updates the release-8.1-presubmits.yaml for the tikv/tikv repo by flipping the labels.master value from "1" to "0" on Jenkins agent jobs. This change redirects these jobs from the current Jenkins master to the target Jenkins, aligning with the migration plan described in issues #4964, #4947, and #4942. The changes are minimal and straightforward, focusing solely on the label update, and the overall quality of the change is good.
Code Improvements
-
Clarify the meaning of
labels.master- File:
prow-jobs/tikv/tikv/release-8.1-presubmits.yaml, lines ~11 and ~22 - Issue: The label
masterwith values"1"and"0"is not self-explanatory. It would be helpful to add a comment explaining whatmaster: "1"andmaster: "0"specifically mean in this context (e.g., which Jenkins instance each value corresponds to). - Suggestion: Add inline comments like:
labels: master: "0" # directs job to the target Jenkins (not the current master)
- File:
-
Check impact on job scheduling and concurrency
- Since this label change affects job routing, ensure that the target Jenkins environment is ready and capable of handling these jobs to avoid broken builds or scheduling delays. Add a note in the PR description or documentation confirming this readiness.
Best Practices
-
Add a note or link to related documentation
- File:
prow-jobs/tikv/tikv/release-8.1-presubmits.yamlor PR description - Issue: The PR description references related issues, but it can improve clarity by linking to documentation or README explaining the label usage and migration steps for maintainers unfamiliar with this CI setup.
- Suggestion: Include a short pointer like:
For more details on Jenkins agent migration and label usage, see
docs/ci/jenkins-migration.md.
- File:
-
Test coverage / Validation
- Although this is a config-only change, consider if there are existing presubmit tests or dry-run validations that ensure these jobs get scheduled correctly after the label flip. If not, recommend adding or running such validations before merging.
No critical issues identified given the limited scope of the change. The PR effectively accomplishes its goal with minimal risk.
Jenkins Migration VerificationMigrating jenkins-agent Prow jobs to the to Jenkins (
Summary: success=0 submitted=0 failed=1 infra-fail=0 skipped=1 dry-run=0 total=2 |
Flip labels.master "1" -> "0" for the jenkins-agent jobs in prow-jobs/tikv/tikv/release-8.1-presubmits.yaml so they are scheduled on the to Jenkins. Part of #4964
62b939f to
b2ab6eb
Compare
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 updates the prow-jobs/tikv/tikv/release-8.1-presubmits.yaml to migrate the release-8.1 Jenkins presubmit jobs from the old Jenkins instance (labels.master: "1") to the target Jenkins (labels.master: "0"). The change is straightforward, flipping a label value for two jobs. The overall change is minimal and clear, with no apparent structural issues.
Code Improvements
-
Clarify the meaning of the
masterlabel- File:
prow-jobs/tikv/tikv/release-8.1-presubmits.yaml - Lines: ~10-30
- Issue: The label key
masterwith values"1"or"0"is not self-explanatory. It’s not clear from the snippet what these values signify (e.g., old vs new Jenkins) without external context. - Suggestion: Add a comment above or inline explaining what
master: "0"vsmaster: "1"means in terms of Jenkins targeting, e.g.:labels: # master: "0" schedules job on target Jenkins; "1" schedules on old Jenkins master: "0"
This improves maintainability and clarity for future readers.
- File:
-
Consistency in comment style and clarity
- The comment
# need add this.is unclear English and repeated for both jobs. - Consider rephrasing to something clearer like:
decorate: false # Required to disable decoration for Jenkins jobs
- This makes intent explicit.
- The comment
Best Practices
-
Testing and Validation
- Since these are config changes to presubmit jobs, verify that these jobs have been tested or validated on the target Jenkins after this change.
- Adding a note in the PR description or a comment about successful test runs or rollout status would improve confidence.
-
Documentation
No critical issues or bugs are evident given the limited scope. The key improvement is increased clarity around the master label and improving comment quality.
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@wuhuizuo: 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. |
|
Merging with This PR only flips What the verify actually reports
Root cause of the dominant failure
Hard failures observed (all 3 retries exhausted):
The second recurring failure, Why the target side fails more often
All the failing tests are timing/scheduling sensitive, and both sides run the same pod template from this repository, so a pod-spec difference cannot explain the asymmetry. Resources were explicitly tested and ruled out. An A/B replay of the same Merge evidenceTarget-side build Follow-ups (do not block this migration)
|
Part of #4964.
Summary
Flip
labels.master"1"->"0"for the jenkins-agent jobs inprow-jobs/tikv/tikv/release-8.1-presubmits.yamlso they are scheduled on the to Jenkins.Part of #4947 / #4942