fix(pipelines): align check2 pod resources to 24Gi/3C across release branches - #5199
Conversation
…branches Align the ghpr_check2/pull_check2 golang container resources for release-6.5, release-7.1, release-7.5 and release-8.1 with latest and release-8.5 (requests == limits == 24Gi/3C). release-7.5 was reduced to 12Gi/3C by #4982 and is now the tightest among the same-named jobs. Its real-tikv import tests hit PD TSO timeouts and bazel test timeouts; 24Gi is the value already validated for the heavy bazel targets. release-8.5 already uses 24Gi/3C, so it is unchanged.
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
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 standardizes the golang container resource requests and limits for the check2 pods across multiple release branches (release-6.5, release-7.1, release-7.5, release-8.1) to 24Gi memory and 3 CPU cores, aligning them with the latest and release-8.5 branches. The approach is a straightforward YAML configuration update with no changes to image or other pod specs. The changes are clear, well-scoped, and justified by observed test failures due to insufficient resources in some branches. Overall, the PR is well-done and focused.
Code Improvements
-
Maintainability: Centralize resource definitions
-
Where: All modified files (
pipelines/pingcap/tidb/release-*.5/pod-ghpr_check2.yaml) -
Issue: The resource specs are duplicated across multiple release branches' pod YAML files.
-
Why: This duplication increases maintenance burden and risk of divergence again in the future.
-
Suggestion: Consider templating or using a shared base manifest or a kustomize/helm overlay with a common resource block for
check2pods to avoid manual sync across branches.Example approach (conceptual):
# base/check2-resources.yaml resources: requests: memory: 24Gi cpu: "3" limits: memory: 24Gi cpu: "3"
Then overlay this in each branch's pod definition to reduce duplication.
-
-
Explicit CPU as string vs integer
- Where: All changed files, CPU is
"3"(string) - Issue: Kubernetes accepts CPU as a string or number; consistent type helps avoid confusion.
- Why: Current usage is consistent, but if other manifests use integer, consider aligning.
- Suggestion: Confirm consistency in CPU specification format across all pod specs. If all others use string, keep as is.
- Where: All changed files, CPU is
Best Practices
-
Add comments explaining the resource values
-
Where: At the resource block in each YAML (
pod-ghpr_check2.yaml) -
Issue: While the PR description explains the rationale, the YAML files themselves lack comments.
-
Why: Adding comments directly in the YAML improves long-term clarity for maintainers.
-
Suggestion: Add a brief comment above the resources block indicating why these resource values were chosen, e.g.:
# Set to 24Gi memory and 3 CPU cores to prevent bazel test timeouts and PD TSO issues observed at lower memory. resources: requests: memory: 24Gi cpu: "3" limits: memory: 24Gi cpu: "3"
-
-
Consider adding or updating test coverage
- Where: CI pipeline definitions or test suites (not shown here)
- Issue: No indication if resource config changes are validated by automated tests.
- Why: Validating resource configs prevent regressions in resource allocations.
- Suggestion: If not present, create or update tests that validate pod resource requests/limits for critical pods in CI pipelines.
Critical Issues
None identified. The changes are limited to resource requests/limits and do not affect pod logic or security.
Summary: This PR effectively resolves resource inconsistencies impacting test stability. To further improve maintainability, consider centralizing resource configs and adding inline comments. Adding or verifying automated tests for these changes would also help prevent future regressions.
|
/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 |
1 participant
Why
The check2 (
ghpr_check2/pull_check2) pod resources diverge across release branches, andrelease-7.5is the tightest:#4982 reduced
release-7.5from24Gi/6C req + 32Gi/8C limto12Gi/3C(the only branch whose memory was lowered). Its real-tikv import tests (bazel_importintotest4) then started failing with PD TSO timeouts and bazel test timeouts;12Giis below the~12.3Gipeak that #4982 measured for the heavy bazel targets.What
Align the
golangcontainer ofrelease-6.5,release-7.1,release-7.5andrelease-8.1withlatest/release-8.5:release-8.5already uses24Gi/3C, so it is unchanged.Notes
spec.containers[golang].resourceschanges; no image / volume / affinity changes..ci/verify-k8s-pod-yaml.shon the changed files.Related to the release-7.5 jenkins migration verification in #5136.