CNTRLPLANE-3042: Parallelize make verify targets for faster local development#8051
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Makefile verification flow: Sequence Diagram(s)mermaid Make->>Parallel: $(MAKE) -j verify-parallel Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @PoornimaSingour. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
106-106: Consider limiting parallel job count.Using
$(MAKE) -jwithout a number spawns unlimited parallel jobs, which can exhaust system resources on machines with many cores.Suggested fix to limit parallelism
verify: generate update - $(MAKE) -j verify-parallel + $(MAKE) -j$$(nproc) verify-parallel $(MAKE) verify-git-cleanAlternatively, use a fixed reasonable limit like
-j8or let the user control it via an environment variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 106, The Makefile invokes "$(MAKE) -j verify-parallel" which uses unlimited parallel jobs; change this invocation to bound parallelism by using a configurable job count variable (e.g., use "$(MAKE) -j$(JOBS) verify-parallel" or "$(MAKE) -j${PARALLEL:-8} verify-parallel"), add a default value for JOBS/PARALLEL if not set, and document that users can override the variable via the environment to control the parallel job limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 106-107: The two Makefile recipe lines that invoke "$(MAKE) -j
verify-parallel" and "$(MAKE) verify-git-clean" are indented with spaces instead
of a tab; replace the leading spaces on those recipe lines with a single tab
character so make recognizes them as commands (ensure the lines starting with
"$(MAKE) -j verify-parallel" and "$(MAKE) verify-git-clean" begin with a tab,
not spaces).
---
Nitpick comments:
In `@Makefile`:
- Line 106: The Makefile invokes "$(MAKE) -j verify-parallel" which uses
unlimited parallel jobs; change this invocation to bound parallelism by using a
configurable job count variable (e.g., use "$(MAKE) -j$(JOBS) verify-parallel"
or "$(MAKE) -j${PARALLEL:-8} verify-parallel"), add a default value for
JOBS/PARALLEL if not set, and document that users can override the variable via
the environment to control the parallel job limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 29ef1339-49ce-4e0d-b045-07b572520420
📒 Files selected for processing (1)
Makefile
PoornimaSingour
force-pushed
the
fix-verify-targets
branch
from
fc79a60 to
5ec1376
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
105-107: Consider--output-syncfor cleaner parallel output.As noted by a reviewer, when running verification tools in parallel, output from different targets may interleave, making failures harder to diagnose. Using
$(MAKE) -j --output-sync=target verify-parallelwould buffer each target's output and print it atomically.Suggested change
verify: generate update - $(MAKE) -j verify-parallel + $(MAKE) -j --output-sync=target verify-parallel $(MAKE) verify-git-clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 105 - 107, The verify target currently invokes parallel verification with "$(MAKE) -j verify-parallel" which can interleave outputs; change the invocation in the verify recipe to include GNU make's output syncing flag so each subtarget's output is buffered and printed atomically (use "--output-sync=target" with the $(MAKE) call that runs verify-parallel). Update the verify recipe where "$(MAKE) -j verify-parallel" is used to call "$(MAKE) -j --output-sync=target verify-parallel" so parallel output is clean and failures are easier to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 105-107: The verify target currently invokes parallel verification
with "$(MAKE) -j verify-parallel" which can interleave outputs; change the
invocation in the verify recipe to include GNU make's output syncing flag so
each subtarget's output is buffered and printed atomically (use
"--output-sync=target" with the $(MAKE) call that runs verify-parallel). Update
the verify recipe where "$(MAKE) -j verify-parallel" is used to call "$(MAKE) -j
--output-sync=target verify-parallel" so parallel output is clean and failures
are easier to read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 03d76fe6-4138-4482-84e1-b90b6f26b990
📒 Files selected for processing (1)
Makefile
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
PoornimaSingour
force-pushed
the
fix-verify-targets
branch
from
6c6ea2e to
eb1e1c9
Compare
PoornimaSingour
force-pushed
the
fix-verify-targets
branch
from
7e7e2c4 to
643715e
Compare
openshift-ci
bot
added
the
needs-rebase
PoornimaSingour
force-pushed
the
fix-verify-targets
branch
2 times, most recently
from
7b58c63 to
0891ec7
Compare
openshift-ci
bot
removed
the
needs-rebase
|
/retest-required |
|
/approve |
openshift-ci
bot
added
the
approved
|
Scheduling tests matching the |
|
/lgtm cancel Letting the lgtm to @bryan-cox |
|
@PoornimaSingour: This pull request references CNTRLPLANE-3042 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@jparrill: This pull request references CNTRLPLANE-3042 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Test Resultse2e-aws
e2e-aks
|
Makefile
Outdated
| git diff --exit-code HEAD -- | ||
| $(eval STATUS = $(shell git status -s)) | ||
| $(if $(strip $(STATUS)),$(error untracked files detected: ${STATUS})) | ||
| verify-ci: generate update staticcheck fmt vet verify-git-clean |
There was a problem hiding this comment.
This should be able to be dropped now.
Makefile
Outdated
| $(if $(strip $(STATUS)),$(error untracked files detected: ${STATUS})) | ||
|
|
||
| .PHONY: verify-parallel | ||
| verify-parallel: staticcheck fmt vet verify-codespell lint cpo-container-sync run-gitlint |
There was a problem hiding this comment.
I would suggest to run verify-codespell lint cpo-container-sync run-gitlint in parallel and stack staticcheck and fmt together with generate update. That is what we are doing in the GitHub actions now.
Restructure the Makefile verify targets to improve local development workflow: - Extract git cleanliness checks into verify-git-clean target - Create verify-parallel target for independent verification tasks (verify-codespell, lint, cpo-container-sync, run-gitlint) that can run concurrently - Run staticcheck, fmt, and vet sequentially before parallel tasks to ensure code generation completes first - Remove verify-ci target (no longer needed - CI workflows run checks individually) This enables faster local verification by parallelizing independent tasks while maintaining proper dependency ordering. Signed-off-by: Poornima Singour <PoornimaSingour@users.noreply.github.com> Commit-Message-Assisted-by: Claude (via Claude Code)
PoornimaSingour
force-pushed
the
fix-verify-targets
branch
from
51fb79f to
952d9bd
Compare
|
/verified by tests |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/close |
|
@bryan-cox: Closed this PR. DetailsIn response to this:
Instructions 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. |
|
/open |
|
/reopen |
|
@bryan-cox: Reopened this PR. DetailsIn response to this:
Instructions 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. |
|
@PoornimaSingour: This pull request references CNTRLPLANE-3042 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8051 +/- ##
=======================================
Coverage ? 26.83%
=======================================
Files ? 1090
Lines ? 105229
Branches ? 0
=======================================
Hits ? 28242
Misses ? 74559
Partials ? 2428 🚀 New features to boost your workflow:
|
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, jparrill, PoornimaSingour 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 |
|
/test e2e-aks |
|
@PoornimaSingour: all tests passed! 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. |
openshift-merge-bot
bot
merged commit a9b3373
into
openshift:main
6 participants
What this PR does / why we need it:
Currently
verifyruns all targets sequentially:Generate —-> update —-> staticcheck —-> fmt —-> vet —-> verify-codespell—-> lint—-> cpo-container-sync —-> run-gitlint.
This takes too much time. To fix this we introduced verify-parallel which execute
fmt vet verify-codespell lint cpo-container-sync run-gitlinttask parallelly and verify run generate & update task. Also we have created another section for git clean check which was being used twice in verify as well as in verify-cli.Which issue(s) this PR fixes:
Fixes : https://redhat.atlassian.net/browse/CNTRLPLANE-3042
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit