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

Custom burst limit settings don't apply to discovery #13128

Closed
evanfoster opened this issue Jun 18, 2024 · 0 comments · Fixed by #13129
Closed

Custom burst limit settings don't apply to discovery #13128

evanfoster opened this issue Jun 18, 2024 · 0 comments · Fixed by #13129
Labels
bug Categorizes issue or PR as related to a bug.

Comments

@evanfoster
Copy link

evanfoster commented Jun 18, 2024

Helm currently has the --burst-limit/$HELM_BURST_LIMIT options, which allows users to give Helm more headroom when making bursty API requests. I've seen it said that setting --burst-limit will help with rate limiting issues on clusters with large numbers of CRDs, since the number of requests required for discovery goes up as the number of CRDs increases.

Unfortunately, this is not the case:

config.Burst = env.BurstLimit

Here, Helm sets k8s.io/client-go/rest.Config.Burst. When genericclioptions instantiates a discovery client, it overrides Config.Burst with ConfigFlags.discoveryBurst: https://github.com/kubernetes/kubernetes/blob/3f9b79fc119d064d00939f91567b48d9ada7dc43/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go#L281

This means that no matter what --burst-limit is set to, generricclioptions (and by extension, Helm) will only allow a burst limit of 300 for discovery. I found this because I was helping someone deal with client-side throttling on a large cluster.

There are two ways this issue could be addressed:

  1. Add a separate --discovery-burst-limit/$HELM_DISCOVERY_BURST_LIMIT flag that would independently set generricclioptions.ConfigFlags.discoveryBurst
  2. Set generricclioptions.ConfigFlags.discoveryBurst to the value of --burst-limit

The benefit to the first approach is flexibility. However, I don't know if we even need that flexibility (there's talk of removing client-side throttling now that APF is a thing). The second approach is nice because it's a very small change and makes the --burst-limit flag meet people's expectations. To me, the second approach seems to be more inline with the principle of least astonishment. Here's the diff:

diff --git a/pkg/cli/environment.go b/pkg/cli/environment.go
index ba103252..d5101c68 100644
--- a/pkg/cli/environment.go
+++ b/pkg/cli/environment.go
@@ -112,7 +112,7 @@ func New() *EnvSettings {
 	env.Debug, _ = strconv.ParseBool(os.Getenv("HELM_DEBUG"))
 
 	// bind to kubernetes config flags
-	env.config = &genericclioptions.ConfigFlags{
+	config := &genericclioptions.ConfigFlags{
 		Namespace:        &env.namespace,
 		Context:          &env.KubeContext,
 		BearerToken:      &env.KubeToken,
@@ -133,6 +133,8 @@ func New() *EnvSettings {
 			return config
 		},
 	}
+	env.config = config.WithDiscoveryBurst(env.BurstLimit)
+
 	return env
 }

I'll be submitting a PR shortly based off of this diff.

EDIT: I see a flaw in my approach. The above diff won't be exactly what I'm submitting, but it represents the intent at least.

@mattfarina mattfarina added the bug Categorizes issue or PR as related to a bug. label Jun 20, 2024
mattfarina pushed a commit that referenced this issue Jul 10, 2024
When --burst-limit/$HELM_BURST_LIMIT is set, the specified value is not
currently used for the discovery client instantiated by
genericclioptions. This change sets genericclioptions.discoveryBurst to
the value of --burst-limit, meaning it should now be possible to fix
client-side throttling issues encountered by the discovery client.

This value is only configured if --burst-limit is actually set. If
--burst-limit is set to the default value, then discoveryBurst should be
left at its default of 300.

Closes #13128

Signed-off-by: Evan Foster <[email protected]>
(cherry picked from commit 69362df)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants