You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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:
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)
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:
helm/pkg/cli/environment.go
Line 127 in a2a324e
Here, Helm sets
k8s.io/client-go/rest.Config.Burst
. Whengenericclioptions
instantiates a discovery client, it overridesConfig.Burst
withConfigFlags.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-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:
--discovery-burst-limit
/$HELM_DISCOVERY_BURST_LIMIT
flag that would independently setgenerricclioptions.ConfigFlags.discoveryBurst
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: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.
The text was updated successfully, but these errors were encountered: