Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set reasonable QPS/Burst for resources RESTMapper client #4370

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Mar 2, 2022

Description of the change

Sets reasonable QPS and Burst for the client config used only in the discovery of the REST API (by the RESTMapper).

Before this change, registering the resources plugin took around 8 seconds due to client throttling:

I0302 00:40:47.539843       1 plugins.go:137] Successfully registered plugin "/plugins/fluxv2-packages/fluxv2-packages-v1alpha1-plugin.so"
I0302 00:40:48.318594       1 request.go:597] Waited for 109.310691ms due to client-side throttling,
 not priority and fairness, request: GET:https://10.96.0.1:443/apis/networking.k8s.io/v1beta1
...
I0302 00:40:56.919228       1 request.go:665] Waited for 8.611529015s due to client-side throttling,
 not priority and fairness, request: GET:https://10.96.0.1:443/apis/kustomize.toolkit.fluxcd.io/v1beta1

I0302 00:40:56.923634       1 plugins.go:137] Successfully registered plugin "/plugins/resources/resources-v1alpha1-plugin.so"
...
I0302 00:40:56.924955       1 server.go:158] Starting server on :50051

With this change, it's under 500ms (with no warnings of throttling):

I0302 05:39:33.515837       1 plugins.go:137] Successfully registered plugin "/plugins/fluxv2-packages/fluxv2-packages-v1alpha1-plugin.so"
I0302 05:39:34.010957       1 plugins.go:137] Successfully registered plugin "/plugins/resources/resources-v1alpha1-plugin.so"

Benefits

Faster startup time for the kubeapps-apis server (see #4329).

Possible drawbacks

Initial hammer of the k8s API server when the rest mapper initialises, but don't see this as an issue.

Applicable issues

Additional information

I did investigate whether we could use the client-getter to create a client using the chart configured defaults, but the client-getter is very careful to restrict itself to user requests, enforcing a user token, ensuring any service account token from the service is not used, etc., which I do not think we should change. Given that this client is used only for generating the rest mapper, I ended up going for simple constants.

@absoludity absoludity mentioned this pull request Mar 2, 2022
2 tasks
Comment on lines 37 to 38
DISCOVERY_CLIENT_QPS = 50.0
DISCOVERY_CLIENT_BURST = 100
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Would it make sense to have them as environment variables, defaulted to those values if not present? Don't remember if we are using env variables for this kind of setup. I was thinking in the cases of environments where K8s resources are scarce (e.g. Docker desktop extension or small clusters).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to provide these vars externally. I mean, we're already passing QPS and burst in the chart, so it is just a matter of using it, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite. I did look at it, but it'll require changing the arguments of the grpc registration function. I (personally) don't see any need to be able to modify them, since unlike the user client, they're only used on startup when initializing the RESTMapper.

But I'm not against passing them through... it's just a matter of updating all the plugins to match. Happy to do so if you both think it's better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might even be a chance to replace the (growing) args to the grpc registration function with a struct so that we don't need to update the function signature each time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might even be a chance to replace the (growing) args to the grpc registration function with a struct so that we don't need to update the function signature each time.

+1, that would be nice and give flexibility to the registration process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've updated this PR, rebasing on #4372 so it's clear as an example how we can update the args to the plugin registration without having to modify other plugins necessarily. PTAL :)

Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity force-pushed the 4329-apis-crashloopbackoff branch from 75fd3ce to 2a3f661 Compare March 2, 2022 23:28
@absoludity absoludity changed the base branch from main to 4329-update-registration-signature March 2, 2022 23:28
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you! I do like this approach rather than hardcoding the defaults as constants. Especially, when it comes to rate limitations. This way we ensure Kubeapps can be adjusted for fitting each cluster's performance needs.

Comment on lines +50 to +51
ClientQPS float32
ClientBurst int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, so these values are already been set by the kube-api-qps and kube-api-burst CLI flags we added in the past, aren't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if we're passing them in, I didn't see a reason to use different flags for this client than those already passed in for the user client, so populate the values from the existing flags.

@absoludity absoludity deleted the branch vmware-tanzu:4329-update-registration-signature March 7, 2022 01:48
@absoludity absoludity closed this Mar 7, 2022
@absoludity
Copy link
Contributor Author

Landed at #4386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants