add options to tune concurrency, qps and burst#287
Conversation
kubernetes-prow
Bot
added
the
kind/feature
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ajaysundark 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 |
kubernetes-prow
Bot
added
approved
kubernetes-prow
Bot
added
the
size/M
| var metricsCertDir string | ||
| var leaderElectionNamespace string | ||
| var enableNodeStateMetrics bool | ||
| var kubeAPIQPS float64 |
There was a problem hiding this comment.
Given that these vars are increasing, I think we can keep them in vars() block outside main for better code organization?
| "Maximum queries per second to the API server from this client. "+ | ||
| "Raise together with --kube-api-burst on large clusters.") | ||
| flag.IntVar(&kubeAPIBurst, "kube-api-burst", 30, | ||
| "Maximum burst for throttle between requests to the API server.") |
There was a problem hiding this comment.
isn't this makes it simpler "maximum number of queries that should be allowed in one burst"
There was a problem hiding this comment.
is there any specific reason for keeping 20 and 30 or is it just to get started.
There was a problem hiding this comment.
These are the current defaults set in controller runtime. I think we could adjust this after our experiments. Maybe I could add a comment to clarify these random numbers here
There was a problem hiding this comment.
Yeah, That would help, I looked at the default and saw its 5 and 10 https://github.com/kubernetes/client-go/blob/f16383b964b3519812bac4daf8f48fc5a529ae0f/rest/config.go#L118-L127, I am I looking at wrong place?
There was a problem hiding this comment.
hmm.. I referred kcm default config here: https://kubernetes.io/docs/reference/command-line-tools-reference/kube-controller-manager.
looks like the controller-runtime had these hardcoded as defaults until recently: kubernetes-sigs/controller-runtime@ab40409
There was a problem hiding this comment.
Regarding the magic numbers, could we have them as constants?
|
|
||
| mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ | ||
| restConfig := ctrl.GetConfigOrDie() | ||
| restConfig.QPS = float32(kubeAPIQPS) |
There was a problem hiding this comment.
why can't we declare it directly as float32?
|
I took this branch for a spin and verified that the tuning works. I ran a manual test with a 1000-node kwok cluster without any parameters and NRC added all the taints in about 2m55s: 
The same manual test with 
I'm still learning how to query Prometheus, but I think I got this right. **Just picked these numbers randomly. |
3 participants
Description
Add flags to the controller to tune controller-runtime knobs for scalability
Related Issue
None
Type of Change
/kind feature
Testing
Manual test runs
Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?
Yes, adds command-flags to tune concurrency, qps and burst -- https://cluster-api.sigs.k8s.io/developer/core/tuning#runtime-tuning-options
Relevant for our ongoing scalability work.