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

[feat] ability to customize kube client qps #101

Merged

Conversation

DerekTBrown
Copy link
Contributor

@DerekTBrown DerekTBrown commented Jan 9, 2025

What does this PR do?

  • This PR extends the gatewayapi plugin to permit increasing Kube Client QPS and Burst values.
  • This PR also adds documentation of this feature.

@DerekTBrown DerekTBrown closed this Jan 9, 2025
@DerekTBrown DerekTBrown reopened this Jan 10, 2025
@DerekTBrown DerekTBrown force-pushed the derektbrown_add_qps_opts branch from 863e03a to 89591d4 Compare January 10, 2025 00:16
@DerekTBrown DerekTBrown force-pushed the derektbrown_add_qps_opts branch from 89591d4 to eaac42c Compare January 10, 2025 00:22
@kostis-codefresh
Copy link
Collaborator

Hello and many thanks for this.

I am curious. Is this solving an issue that you have in production usage right now? Or your suggestion is a best practice that you recommend?

I am wondering though whether this change must happen centrally in the main Argo Rollouts controller so that all plugins get this capability out of the box.

Signed-off-by: Derek Brown <[email protected]>
@DerekTBrown DerekTBrown force-pushed the derektbrown_add_qps_opts branch from 43eaa7f to c6c45f4 Compare January 13, 2025 16:00
@DerekTBrown
Copy link
Contributor Author

I am curious. Is this solving an issue that you have in production usage right now? Or your suggestion is a best practice that you recommend?

TL;DR; This is solving an issue we are seeing in our real environments.

We are in the initial phases of adopting the Gateway API. We started observing issues with Argo Rollouts for services where we added HTTPRoute configuration. pprof on the main Argo Rollouts process showed us that the Rollout worker goroutines were stalled on go-plugin RPC calls. I then monkey-patched pprof into the Gateway API plugin, and observed that the plugin was stalling on the k8s client ratelimiter. Raising the QPS and burst values helps resolve that issue.

@DerekTBrown
Copy link
Contributor Author

I am wondering though whether this change must happen centrally in the main Argo Rollouts controller so that all plugins get this capability out of the box.

The Argo Rollouts controller is a separate process/binary from any of the plugins, so it isn't currently possible to make this change within the controller in a way that would propagate to the plugins.

If the overall project were trying to achieve something like this within the plugins interface, it would need to create a standard interface of command-line flags or config options that are passed into plugins. To my knowledge, this doesn't exist. Even if it did exist, the implementation of this flag in particular would be problematic, because the QPS ratelimits would be effectively double whatever I provide as a command-line option (since the controller and the plugin are separate binaries, the ratelimiters are separate).

As an aside, I started a discussion about the limitations of the plugin architecture here: argoproj/argo-rollouts#4042 (comment).

@kostis-codefresh
Copy link
Collaborator

kostis-codefresh commented Jan 13, 2025

While this PR is great for the plugin, if you have the time I think it would be also useful if you also create a PR against the main rollouts controller and add code that passes down the same parameters to the plugin IF they have already been defined in the controller level
https://github.com/argoproj/argo-rollouts/blob/00b57266b9c93d29d21d897e5a7a17fc82ab2e6a/cmd/rollouts-controller/main.go#L294

Also we would need to update the docs and make it clear to the users that the QPS setting is only for the plugin. If somebody is using multiple plugins or has a different setting in the controller, right now each kubeconfig object will have it own parameters. (the doubling problem you mention). You are right that we cannot fix this, but we can document it .

@kostis-codefresh kostis-codefresh self-requested a review January 13, 2025 16:37
@kostis-codefresh kostis-codefresh self-assigned this Jan 13, 2025
@kostis-codefresh kostis-codefresh merged commit 44ab8fd into argoproj-labs:main Jan 13, 2025
6 checks passed
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.

2 participants