Increase the default QPS and Burst value of the controller manager #1461
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @iawia002! |
k8s-ci-robot
added
the
size/XS
|
This issue is introduced in #1416 . |
|
/cc @shiyan2016 |
|
@yuswift: GitHub didn't allow me to request PR reviews from the following users: shiyan2016. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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/test-infra repository. |
|
/cc @snowplayfire |
|
@yuswift: GitHub didn't allow me to request PR reviews from the following users: snowplayfire. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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/test-infra repository. |
|
friendly ping @hectorj2f |
| @@ -100,8 +100,8 @@ member clusters and do the necessary reconciliation`, | |||
| flags.StringVar(&kubeFedConfig, "kubefed-config", "", "Path to a KubeFedConfig yaml file. Test only.") | |||
| flags.StringVar(&kubeconfig, "kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.") | |||
| flags.StringVar(&masterURL, "master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.") | |||
| flags.Float32Var(&restConfigQPS, "rest-config-qps", 5.0, "Maximum QPS to the api-server from this client.") | |||
There was a problem hiding this comment.
These values come from https://github.com/kubernetes/client-go/blob/master/rest/config.go#L45 as any other client. I suggest you simply adapt them in your deployment to other values using the flags.
There was a problem hiding this comment.
Yes, I understand that, but based on the actual situation, for the kubefed-controller-manager component, 5 and 10 are too small. For most users, it will be more convenient for us to set the default value to a more appropriate value. WDYT?
There was a problem hiding this comment.
These values come from https://github.com/kubernetes/client-go/blob/master/rest/config.go#L45 as any other client. I suggest you simply adapt them in your deployment to other values using the flags.
I'm not sure if there are anyone who are using v0.8.1, after we upgraded to this version, we did encounter the slowing start up problem of controller. IMO, increasing it is necessary for the users who are using v0.8.1.
There was a problem hiding this comment.
I agree that default qps is small. We need to set a value that makes more sense to kubefed controller by default.
It is not easy to get a suitable value. I think we can refer to https://cloud.google.com/kubernetes-engine/docs/best-practices/scalability. It says Clusters larger than 500 nodes use 100 QPS client limit for both components: scheduler and controller manager. Consider kubefed is a manager of multiple clusters. I think this 100 for 500 nodes can be a value as minimum for kubefed. And burst can be double of it: 200.
So, my proposal is:
"rest-config-qps": 100
"rest-config-burst": 200
@yuswift @hectorj2f @iawia002 what do you think?
There was a problem hiding this comment.
So, my proposal is:
"rest-config-qps": 100 "rest-config-burst": 200
Updated.
iawia002
force-pushed
the
update-rest-config-default-value
branch
from
0a8dcac
to
bc4afe5
Compare
|
/test all |
|
@xunpan: No presubmit jobs available for kubernetes-sigs/kubefed@master In 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/test-infra repository. |
|
@hectorj2f |
|
/lgtm |
|
There is no other comments. Let me merge it. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iawia002, xunpan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
I found that the default QPS and Burst values for the rest-client of the kubefed-controller-manager component were configured very low(5 and 10), causing the component to start very slowly:
So I set it to a suitable value.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer: