fix(pipelines/tiflash): adjust pull_unit_test pod resources and ccache location - #5064
fix(pipelines/tiflash): adjust pull_unit_test pod resources and ccache location#5064wuhuizuo wants to merge 1 commit into
Conversation
…e location Remove custom workspace and move ccache to the workspace volume, switch pod resource requests to limits and drop the unused tmp emptyDir volume.
|
[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 improves the reliability of the tiflash pull_unit_test pipeline by moving the ccache directory to a persistent workspace volume, removing unnecessary workspace overrides, and adjusting pod resource definitions. The approach is straightforward and focuses on configuration changes to pod templates and pipeline scripts. Overall, the changes are concise and align with the stated goals, but a few improvements can be made regarding resource specification clarity and error handling.
Code Improvements
-
Resource Limits vs Requests (pod-pull_unit-test.yaml, lines ~15-20):
The PR changesrequeststolimitsfor CPU and memory. This can cause pods to be evicted or fail to schedule if the cluster cannot guarantee these limits. Usually,requestsspecify guaranteed resources, whilelimitscap usage.- Why: Using
limitswithoutrequestscan lead to unstable scheduling or OOM kills. - Suggestion: Specify both
requestsandlimitsto ensure stable scheduling and resource capping, for example:resources: requests: memory: "24Gi" cpu: "6" limits: memory: "24Gi" cpu: "6"
This approach balances resource guarantees and control.
- Why: Using
-
Remove commented-out ephemeral-storage request (pod-pull_unit-test.yaml, line ~17):
If ephemeral-storage limits are required, consider adding explicitrequestsandlimitsfor ephemeral-storage to avoid pod eviction due to disk pressure. -
customWorkspaceremoval (pull_unit_test.groovy, line ~18):
RemovingcustomWorkspaceis reasonable for consistency, but validate that no downstream steps rely on the old path/home/jenkins/agent/workspace/tiflash-build-common.- Suggestion: Add a quick check or comment to ensure no hardcoded paths are broken.
-
Shell script quoting style (pull_unit_test.groovy, lines ~49-58):
The change from triple double quotes (""") to triple single quotes (''') is fine, but consider consistent style across the repo. Also, the use of$WORKSPACEinside single quotes may not expand in some Groovy contexts.- Why: Single quotes prevent variable interpolation in Groovy strings.
- Suggestion: Use double quotes or explicitly expand the variable, e.g.:
sh label: "config ccache", script: """ ccache -o cache_dir="\$WORKSPACE/.ccache" ... """
Or use Groovy string interpolation if supported.
-
Error handling for ccache commands (pull_unit_test.groovy, lines ~49-58):
The script runs multipleccacheconfiguration commands. Consider adding error checking or fail-fast behavior if any command fails.- Suggestion: Use
set -eor chain commands with&&, e.g.:set -e ccache -o cache_dir="$WORKSPACE/.ccache" && ccache -o max_size=2G && ...
This prevents silent failures.
- Suggestion: Use
Best Practices
-
Documentation update for workspace change (pull_unit_test.groovy, line ~18):
SincecustomWorkspaceis removed, add a brief comment explaining that the default workspace volume is now used for consistency and persistence. -
Testing coverage:
Ensure that pipeline runs are tested after this change to verify that ccache caching works correctly and resources are respected. This may not be in the code but should be part of the PR validation. -
Comment on resource rationale (pod-pull_unit-test.yaml, lines ~12-20):
Add comments explaining why the resource limits are set to these values to help future maintainers understand the reasoning behind these settings.
Summary of actionable suggestions
# pipelines/pingcap/tiflash/latest/pod-pull_unit-test.yaml (resource block)
resources:
requests:
memory: "24Gi"
cpu: "6"
limits:
memory: "24Gi"
cpu: "6"
# Add ephemeral-storage requests/limits if necessary// pipelines/pingcap/tiflash/latest/pull_unit_test.groovy (ccache config block)
sh label: "config ccache", script: """
set -e
ccache -o cache_dir="\$WORKSPACE/.ccache" &&
ccache -o max_size=2G &&
ccache -o hash_dir=false &&
ccache -o compression=true &&
ccache -o compression_level=6 &&
ccache -o read_only=false &&
ccache -z
"""// pipelines/pingcap/tiflash/latest/pull_unit_test.groovy (near line 18)
// Removed customWorkspace to standardize workspace volume usage for persistent caching and resource managementAddressing these points will improve stability, maintainability, and clarity of the pipeline configuration.
Jenkins Replay Status
Summary: success=0, failure=1, building=1 |
What problem does this PR solve?
Adjust the tiflash pull_unit_test pipeline so the job can run reliably:
/tmpto the workspace volume ($WORKSPACE/.ccache), so the cache survives the pod and is not limited by pod resource constraints.customWorkspaceoverride so the default workspace volume is used consistently.requeststolimitsand drop the unusedtmpemptyDir volume.What is changed and how it works?
pipelines/pingcap/tiflash/latest/pull_unit_test.groovypipelines/pingcap/tiflash/latest/pod-pull_unit-test.yamlRelated changes
None.
Check List