-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Description
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:
Line 127 in a2a324e
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:
- Add a separate
--discovery-burst-limit
/$HELM_DISCOVERY_BURST_LIMIT
flag that would independently setgenerricclioptions.ConfigFlags.discoveryBurst
- 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.