Configurable MaxConcurrentReconciles #151
Conversation
Signed-off-by: vaspahomov <vas2142553@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vaspahomov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughA new configuration option for maximum concurrent reconciles was introduced for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant NodeMaintenanceReconciler
participant ControllerRuntime
User->>Main: Start application with flags
Main->>Main: Parse max-concurrent-reconciles flag
Main->>NodeMaintenanceReconciler: Initialize with MaxConcurrentReconciles value
NodeMaintenanceReconciler->>ControllerRuntime: SetupWithManager (with MaxConcurrentReconciles)
ControllerRuntime-->>NodeMaintenanceReconciler: Controller runs with specified concurrency
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi @vaspahomov. Thanks for your PR. I'm waiting for a medik8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
main.go (1)
72-76: Validate user input and re-phrase the flag description
maxConcurrentReconcilesmay be set to 0 or a negative value via the CLI, which controller-runtime will silently coerce to the default (1). A quick sanity-check makes the behaviour explicit and avoids user confusion.
While touching the flag, a slightly clearer description helps:- flag.IntVar(&maxConcurrentReconciles, "max-concurrent-reconciles", 1, "The number of max concurrent reconciles") + flag.IntVar(&maxConcurrentReconciles, "max-concurrent-reconciles", 1, + "Maximum number of NodeMaintenance reconciles that may run concurrently (minimum 1)")Add a guard straight after
flag.Parse():flag.Parse() + +if maxConcurrentReconciles < 1 { + setupLog.Info("max-concurrent-reconciles must be >= 1 – falling back to 1", + "providedValue", maxConcurrentReconciles) + maxConcurrentReconciles = 1 +}Also applies to: 83-84
controllers/nodemaintenance_controller.go (1)
71-73: Minor: document acceptable range forMaxConcurrentReconcilesThe code happily accepts
0, which controller-runtime treats as default (1). A short comment (or the validation suggested inmain.go) would make this implicit behaviour explicit and prevent misconfiguration later.Also applies to: 78-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controllers/controllers_suite_test.go(1 hunks)controllers/nodemaintenance_controller.go(2 hunks)main.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
main.go (2)
pkg/utils/validation.go (1)
NewOpenshiftValidator(15-21)controllers/nodemaintenance_controller.go (1)
NodeMaintenanceReconciler(64-73)
🔇 Additional comments (2)
controllers/controllers_suite_test.go (1)
94-101: Tests: explicit concurrency setting is good 👍Hard-coding
MaxConcurrentReconciles: 1keeps the test suite deterministic and mirrors the default controller-runtime behaviour, so the addition looks correct.main.go (1)
127-134: Wiring the parsed flag into the reconciler looks correctThe new field is correctly initialised and passed through, so the controller receives the requested concurrency level.
|
some first thoughts...
|
|
I agree that test code is essential here to verify that nothing is messed up for concurrent NMO reconciles |
|
/test 4.18-openshift-e2e |
Why we need this PR
Default value for controller-manager for MaxConcurrentReconciles is 1.
If we are going to start n parallel
NodeMaintenances at the same time. We'll handle last one aftern * dtime. Where d is duration of reconcile.In current model the longest reconcile is more then 30s
That means - if we are starting ~50 parallel NodeMaintenances and each of them are failed to finish immediately (some blocking PDB exists) we we'll came up with the last one only after ~30mins.
Changes made
Added cli arg to override
MaxConcurrentReconcilesWhich issue(s) this PR fixes
Test plan
Summary by CodeRabbit