feat: Introduce controller concurrency and Kubernetes API flags#368
feat: Introduce controller concurrency and Kubernetes API flags#368tomergee wants to merge 5 commits intokubernetes-sigs:mainfrom
Conversation
…onfiguration options with corresponding documentation. default values remain the same values as before --kube-api-qps (default 20) --kube-api-burst (default 30) --concurrent-workers (default 1) in cmd/agent-sandbox-controller/main.go Plumbed the new limits into restConfig before setting up the controller manager. Updated SetupWithManager signatures across all controller files.
✅ Deploy Preview for agent-sandbox canceled.
|
|
Hi @tomergee. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
|
/ok-to-test |
|
/ok-to-test |
…onfiguration options with corresponding documentation.
igooch
left a comment
There was a problem hiding this comment.
Should we keep the QPS default the same as controller-runtime? If yes, please also update the documentation as well.
…mPool controllers, and adjust kube API rate limit defaults Added error and logical validation on flag values, updated configuration.md and refernced release manifests
… and SandboxWarmPool controllers, and adjust kube API rate limit defaults.
| if kubeAPIQPS == 0 || kubeAPIQPS < -1 { | ||
| setupLog.Error(nil, "kube-api-qps must be greater than 0, or -1 for no rate limiting") | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Do we ever want it to be -1 ?
Wouldn't it be safer to say this has to be > 0 ?
There was a problem hiding this comment.
-1 is the default value that comes with the controller (before the change)
There was a problem hiding this comment.
-1 means disabling throttling (no limits).
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aditya-shantanu, tomergee 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 |
|
|
||
| mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ | ||
| restConfig := ctrl.GetConfigOrDie() | ||
| restConfig.QPS = float32(kubeAPIQPS) |
There was a problem hiding this comment.
We should define kubeAPIQPS as float instead of int to allow for fractional values like 0.5
| } | ||
| // A logical maximum (too much will create unnecessary load on the API server) | ||
| totalWorkers := sandboxConcurrentWorkers + sandboxClaimConcurrentWorkers + sandboxWarmPoolConcurrentWorkers | ||
| if totalWorkers > 1000 { |
There was a problem hiding this comment.
How is 1000 decided? Do we need to enforce a max now? It might work in a very large cluster with a large master VM.
There was a problem hiding this comment.
What do you think is a logical maximum? we should enforce something? otherwise its just wasting resources and performance...
API flags configuration options with corresponding documentation, default values remain the same values as before:
--concurrent-workers(default: 1): The maximum number of concurrent reconciles for the controllers. Increase this value to process multiple Sandbox and SandboxClaim events in parallel.--kube-api-qps(default: 20): The maximum Queries Per Second (QPS) sent to the Kubernetes API server from the controller.--kube-api-burst(default: 30): The maximum burst for throttle requests to the Kubernetes API server.--sandbox-concurrent-workers(default: 1): The maximum number of concurrent reconciles for the Sandbox controller.--sandbox-claim-concurrent-workers(default: 1): The maximum number of concurrent reconciles for the SandboxClaim controller.--sandbox-warm-pool-concurrent-workers(default: 1): The maximum number of concurrent reconciles for the SandboxWarmPool controller.--kube-api-qps(default: -1 ; no rate limiting): The maximum Queries Per Second (QPS) sent to the Kubernetes API server from the controller.--kube-api-burst(default: 10): The maximum burst for throttle requests to the Kubernetes API server.in cmd/agent-sandbox-controller/main.go
Plumbed the new limits into restConfig before setting up the controller manager. Updated SetupWithManager signatures across all controller files.