Skip to content

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

@evanfoster

Description

@evanfoster

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugCategorizes issue or PR as related to a bug.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions