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 |
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: |
|
@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. |
|
/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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/test/ginkgo/cmd_runsuite.go (1)
649-686: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a table-driven approach for bucket bookkeeping/execution.
Each new bucket (DRA, InPlaceResize, Probing, PodGeneration, CRD, DeploymentConfig) requires the same five hand-written steps: store original, duplicate in the count loop, add to
expectedTestCount, copy+execute+record-interval, append totests. With ~15 buckets now following this identical shape, it's easy to miss a step when adding a bucket (e.g., forgetting to add a length toexpectedTestCount), and the function body is growing very large. A slice of{name string; tests []*testCase}processed through shared helper functions would reduce this boilerplate and the chance of a bucket being partially wired.As per coding guidelines,
**/*.goshould "favor clarity and maintainability over cleverness"; the current copy-paste growth pattern is starting to work against that.type testBucket struct { name string tests []*testCase } // helper used both for count-mode duplication and execution func executeBucket(ctx context.Context, q *parallelTestQueue, b *testBucket, parallelism int, cfg *testOutputConfig, abortFn abortFunc, recorder monitorapi.Recorder) []*testCase { cp := copyTests(b.tests) id, start := recordTestBucketInterval(recorder, b.name) q.Execute(ctx, cp, parallelism, cfg, abortFn) recorder.EndInterval(id, time.Now()) logrus.Infof("Completed %s test bucket in %v", b.name, time.Since(start)) return cp }Also applies to: 718-752, 807-813
🤖 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 649 - 686, Refactor the bucket bookkeeping and execution in the surrounding suite-building flow into a table-driven collection of named test buckets, covering all existing groups including DRA, InPlaceResize, Probing, PodGeneration, CRD, and DeploymentConfig. Reuse shared logic for count-mode duplication, expected-test-count accumulation, execution/interval recording, and appending results so every bucket follows the same path and cannot be partially wired; preserve each bucket’s current name and execution behavior.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 649-686: Refactor the bucket bookkeeping and execution in the
surrounding suite-building flow into a table-driven collection of named test
buckets, covering all existing groups including DRA, InPlaceResize, Probing,
PodGeneration, CRD, and DeploymentConfig. Reuse shared logic for count-mode
duplication, expected-test-count accumulation, execution/interval recording, and
appending results so every bucket follows the same path and cannot be partially
wired; preserve each bucket’s current name and execution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4b331ff1-091e-482f-9cf1-4ba6886cbd8e
📒 Files selected for processing (1)
pkg/test/ginkgo/cmd_runsuite.go
|
/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/d76adda0-7ec8-11f1-9b85-2275827518b2-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/3139ced0-7ef7-11f1-824e-022f1670a508-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/3484db20-7ef7-11f1-9ab6-5266e13ab59e-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/faf41b80-7f02-11f1-8e47-d3e9f541726c-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/fd4844b0-7f02-11f1-998c-9c2351de6d23-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/32473440-7f04-11f1-9d8e-a0b5a804e7b6-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/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: |
1 participant
/hold
Summary by CodeRabbit