Create more test bucket to debug high cpu issues#31377
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR partitions selected Kubernetes and OpenShift tests into CPU-heavy buckets, adds discovery and ChangesCPU-heavy test bucket scheduling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0c146d90-7bd2-11f1-8628-d6f865f51c18-0 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xueqzhan 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/test/ginkgo/cmd_runsuite.go (1)
691-786: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRepeated bucket-execution boilerplate could be extracted into a helper.
The
copyTests → recordTestBucketInterval → q.Execute → EndInterval → log → appendsequence is now duplicated ~14 times in this function (4 new instances added here). Extracting a small helper would reduce duplication and make future bucket additions less error-prone.♻️ Example helper extraction
func executeTestBucket(ctx context.Context, q *parallelTestQueue, recorder monitor.Recorder, name string, tests []*testCase, parallelism int, cfg testOutputConfig, abortFn abortFunc) []*testCase { testsCopy := copyTests(tests) intervalID, startTime := recordTestBucketInterval(recorder, name) q.Execute(ctx, testsCopy, parallelism, cfg, abortFn) recorder.EndInterval(intervalID, time.Now()) logrus.Infof("Completed %s test bucket in %v", name, time.Since(startTime)) return testsCopy }Usage:
- draTestsCopy := copyTests(draTests) - draIntervalID, draStartTime := recordTestBucketInterval(monitorEventRecorder, "DRA") - q.Execute(testCtx, draTestsCopy, parallelism, testOutputConfig, abortFn) - monitorEventRecorder.EndInterval(draIntervalID, time.Now()) - logrus.Infof("Completed DRA test bucket in %v", time.Since(draStartTime)) - tests = append(tests, draTestsCopy...) + tests = append(tests, executeTestBucket(testCtx, q, monitorEventRecorder, "DRA", draTests, parallelism, testOutputConfig, abortFn)...)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/test/ginkgo/cmd_runsuite.go` around lines 691 - 786, The bucket execution flow in this function is repeated many times and should be extracted into a shared helper. Move the copyTests, recordTestBucketInterval, q.Execute, monitorEventRecorder.EndInterval, logrus.Infof, and append sequence into a reusable helper like executeTestBucket, then replace each bucket block (for example Kubernetes, DRA, InPlaceResize, Probing, Storage, Network, Netpol, Builds, OpenShift, DeploymentConfig, MustGather) with calls to it. Keep the helper responsible for returning the copied tests so the caller can continue appending them to tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/test/ginkgo/cmd_runsuite.go`:
- Around line 691-786: The bucket execution flow in this function is repeated
many times and should be extracted into a shared helper. Move the copyTests,
recordTestBucketInterval, q.Execute, monitorEventRecorder.EndInterval,
logrus.Infof, and append sequence into a reusable helper like executeTestBucket,
then replace each bucket block (for example Kubernetes, DRA, InPlaceResize,
Probing, Storage, Network, Netpol, Builds, OpenShift, DeploymentConfig,
MustGather) with calls to it. Keep the helper responsible for returning the
copied tests so the caller can continue appending them to tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 62231f2a-4f19-4df7-9419-d5e1cf9a1f93
📒 Files selected for processing (1)
pkg/test/ginkgo/cmd_runsuite.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview 10 |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a8f9bc00-7bd7-11f1-9f83-ccfaed31241a-0 |
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/96088da0-7caf-11f1-9351-44f6e94c965d-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview 10 |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5569f8e0-7cb1-11f1-8cff-afa80418945c-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5badb0e0-7d63-11f1-8c41-d227074856d9-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5de72620-7d63-11f1-9955-6ab1f637f7c9-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5fcf2000-7d63-11f1-91fc-dc7e147706c0-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f85932f0-7e1e-11f1-9e51-697b9f4b4700-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/34049bb0-7f04-11f1-8417-692a57ad4cdf-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8c73e3f0-7f81-11f1-946f-9c12be0cd70d-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8ef42630-7f81-11f1-8ce5-b6c8f01cae8a-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
1 similar comment
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/90f2dc60-7f81-11f1-8b19-52ee4f204bd8-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/931a7750-7f81-11f1-8f19-257e2412d336-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9477bd60-7f81-11f1-8c44-c2c42c566060-0 |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/96b6d7f0-7f81-11f1-8f52-e72a372c11f5-0 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/test/ginkgo/cmd_runsuite.go (1)
697-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why CPU-heavy buckets are capped at 20, and avoid repeating the literal.
Every other bucket's non-default parallelism choice in this function is explained inline (storage halves parallelism to avoid cloud quota issues, ordered-namespace-deletion runs at 1 due to workqueue backlog sensitivity, netpol runs at 2 due to cluster overload risk, builds halves parallelism for CPU concerns). The two new CPU-heavy buckets introduce
min(20, parallelism)with no such rationale, and the20is duplicated across both call sites, risking drift if one is tuned later without the other.
pkg/test/ginkgo/cmd_runsuite.go#L697-L702: add a short comment explaining whyKubeCPUHeavyis capped at 20 (e.g. limiting concurrent CPU-heavy tests to reduce node CPU pressure), and reference a named constant instead of the literal20.pkg/test/ginkgo/cmd_runsuite.go#L758-L763: use the same named constant introduced above forOpenshiftCPUHeavy, with the shared rationale comment (or a pointer to it).As per coding guidelines,
**/*.goshould "Favor clarity and maintainability over cleverness in Go code. Keep comments minimal and helpful, explaining why rather than what."♻️ Proposed fix
+// cpuHeavyBucketParallelism caps the concurrency of CPU-intensive test buckets +// to reduce node CPU pressure during the debugging window for high-CPU incidents. +const cpuHeavyBucketParallelism = 20 + kubeCPUHeavyTestsCopy := copyTests(kubeCPUHeavyTests) kubeCPUHeavyIntervalID, kubeCPUHeavyStartTime := recordTestBucketInterval(monitorEventRecorder, "KubeCPUHeavy") - q.Execute(testCtx, kubeCPUHeavyTestsCopy, min(20, parallelism), testOutputConfig, abortFn) + q.Execute(testCtx, kubeCPUHeavyTestsCopy, min(cpuHeavyBucketParallelism, parallelism), testOutputConfig, abortFn) // cap parallelism to limit CPU pressure from these heavy testsopenshiftCPUHeavyTestsCopy := copyTests(openshiftCPUHeavyTests) openshiftCPUHeavyIntervalID, openshiftCPUHeavyStartTime := recordTestBucketInterval(monitorEventRecorder, "OpenshiftCPUHeavy") - q.Execute(testCtx, openshiftCPUHeavyTestsCopy, min(20, parallelism), testOutputConfig, abortFn) + q.Execute(testCtx, openshiftCPUHeavyTestsCopy, min(cpuHeavyBucketParallelism, parallelism), testOutputConfig, abortFn) // same cap as KubeCPUHeavy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/test/ginkgo/cmd_runsuite.go` around lines 697 - 702, In pkg/test/ginkgo/cmd_runsuite.go at lines 697-702, define a shared named constant for the CPU-heavy concurrency cap, add a brief rationale comment explaining that the cap of 20 reduces node CPU pressure, and use it in the KubeCPUHeavy min call. At lines 758-763, replace the duplicated literal in the OpenshiftCPUHeavy min call with the same constant and reuse or reference the shared rationale comment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/test/ginkgo/cmd_runsuite.go`:
- Around line 697-702: In pkg/test/ginkgo/cmd_runsuite.go at lines 697-702,
define a shared named constant for the CPU-heavy concurrency cap, add a brief
rationale comment explaining that the cap of 20 reduces node CPU pressure, and
use it in the KubeCPUHeavy min call. At lines 758-763, replace the duplicated
literal in the OpenshiftCPUHeavy min call with the same constant and reuse or
reference the shared rationale comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0b11c0dc-e138-4bbe-8865-38da3aa6c507
📒 Files selected for processing (1)
pkg/test/ginkgo/cmd_runsuite.go
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
2 similar comments
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5b886280-7fbc-11f1-8e20-2cbf8d65e45f-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5e0e2300-7fbc-11f1-8218-81f6912f5e79-0 |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5f7469c0-7fbc-11f1-9ec4-40cd8ee858b7-0 |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/61798890-7fbc-11f1-9177-12c5e824609d-0 |
|
Scheduling required tests: |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-upgrade-fips-rhcos9-techpreview 10 |
|
@xueqzhan: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/72b8a4d0-7fce-11f1-93b1-ee2b7ad1d948-0 |
|
@xueqzhan: The following tests 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 67ed2a9
|
1 participant
/hold
Summary by CodeRabbit