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:
- Add a separate
--discovery-burst-limit/$HELM_DISCOVERY_BURST_LIMIT flag that would independently set generricclioptions.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.
Helm currently has the
--burst-limit/$HELM_BURST_LIMIToptions, which allows users to give Helm more headroom when making bursty API requests. I've seen it said that setting--burst-limitwill 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:
helm/pkg/cli/environment.go
Line 127 in a2a324e
Here, Helm sets
k8s.io/client-go/rest.Config.Burst. Whengenericclioptionsinstantiates a discovery client, it overridesConfig.BurstwithConfigFlags.discoveryBurst: https://github.com/kubernetes/kubernetes/blob/3f9b79fc119d064d00939f91567b48d9ada7dc43/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go#L281This means that no matter what
--burst-limitis 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:
--discovery-burst-limit/$HELM_DISCOVERY_BURST_LIMITflag that would independently setgenerricclioptions.ConfigFlags.discoveryBurstgenerricclioptions.ConfigFlags.discoveryBurstto the value of--burst-limitThe 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-limitflag meet people's expectations. To me, the second approach seems to be more inline with the principle of least astonishment. Here's the diff: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.