Add --burst-limit option for client-side throttling limit configuration #10842

isutton · 2022-04-07T15:38:11Z

What this PR does / why we need it:

Client-side throttling seems to be an issue in larger environments such as OpenShift clusters, where it is common to have several hundreds CRDs out-of-the-box.

From this view point, it is fair that clients should be able to fine tune this accordingly should the environment they work on evolves, which is currently not possible, and quite frustrating.

This change introduces the --burst-limit option to helm (and its counterpart HELM_BURST_LIMIT environment variable) to address that issue, allowing clients to properly tune their client usage as their environment evolves.

Special notes for your reviewer:

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

Closes #10560

yxxhero · 2022-04-08T06:57:15Z

@isutton Please create a issue this PR. Thanks very much.

yxxhero · 2022-04-08T07:19:01Z

--default-burst-limit --> --burst-limit or --kube-burst-limit
when --burst-limit not set. it will be set defaultBurstLimit.

isutton · 2022-04-08T13:02:43Z

--default-burst-limit --> --burst-limit or --kube-burst-limit
when --burst-limit not set. it will be set defaultBurstLimit.

Fixed in b2ba745.

isutton · 2022-04-08T13:09:33Z

@isutton Please create a issue this PR. Thanks very much.

This PR closes #10560.

pkg/cli/environment.go

yxxhero · 2022-04-08T14:21:37Z

unittest?

isutton · 2022-04-08T14:31:32Z

unittest?

There are no unit tests as the value is only kept to be forwarded to the client using the appropriate API; additionally there are evidences through the change in oc that this value works reasonably well with OpenShift out-of-the-box.

yxxhero · 2022-04-09T00:19:06Z

cmd/helm/root.go

@@ -67,6 +67,7 @@ Environment variables:
 | $HELM_KUBEASUSER                   | set the Username to impersonate for the operation.                                |
 | $HELM_KUBECONTEXT                  | set the name of the kubeconfig context.                                           |
 | $HELM_KUBETOKEN                    | set the Bearer KubeToken used for authentication.                                 |
+| $HELM_DEFAULT_BURST_LIMIT          | set the default burst limit in the case the server contains many CRDs             |


HELM_DEFAULT_BURST_LIMIT --> HELM_BURST_LIMIT is right?

This has been fixed.

yxxhero · 2022-04-09T00:52:16Z

add your test case into TestEnvSettings @isutton

yxxhero · 2022-04-09T00:52:44Z

pkg/cli/environment_test.go

isutton · 2022-04-11T10:28:04Z

add your test case into TestEnvSettings @isutton

TestEnvSettings has been amended to consider HELM_BURST_LIMIT environment variable and --burst-limit option.

yxxhero · 2022-04-12T00:10:44Z

@isutton I think all is ok. need a core maintainer to review this.

yxxhero · 2022-04-12T00:10:44Z

@isutton I think all is ok. need a core maintainer to review this.

sabre1041

This looks good. A few comments

sabre1041 · 2022-05-16T20:37:47Z

cmd/helm/root.go

@@ -67,6 +67,7 @@ Environment variables:
 | $HELM_KUBEASUSER                   | set the Username to impersonate for the operation.                                |
 | $HELM_KUBECONTEXT                  | set the name of the kubeconfig context.                                           |
 | $HELM_KUBETOKEN                    | set the Bearer KubeToken used for authentication.                                 |
+| $HELM_BURST_LIMIT                  | set the default burst limit in the case the server contains many CRDs             |


Let's both call out the default value as well how it can be disabled (-1)

Fixed in 14daf80

sabre1041 · 2022-05-16T20:43:02Z

pkg/cli/environment.go


 	"helm.sh/helm/v3/pkg/helmpath"
 )

 // defaultMaxHistory sets the maximum number of releases to 0: unlimited
 const defaultMaxHistory = 10

+// defaultBurstLimit sets the default client-side throttling limit
+const defaultBurstLimit = 250


Recommend setting the value to 100 as the default. The default in the Kubernetes rest client is 10 and 250 seems to be a dramatic change

Fixed in 3760853

Client-side throttling seems to be an issue in larger environments such as OpenShift clusters, where it is common to have several hundreds CRDs out-of-the-box. From this view point, it is fair that clients should be able to fine tune this accordingly should the environment they work on evolves, which is currently not possible, and quite frustrating. This change introduces the --default-burst-limit option to helm (and its counterpart HELM_DEFAULT_BURST_LIMIT environment variable) to address that issue, allowing clients to properly tune their client usage as their environment evolves. Signed-off-by: Igor Sutton <isuttonl@redhat.com>

Signed-off-by: Igor Sutton <isuttonl@redhat.com>

…ions Signed-off-by: Igor Sutton <isuttonl@redhat.com>

sabre1041

LGTM

…on (helm#10842) * feat: add configuration for client-side throttling limit Client-side throttling seems to be an issue in larger environments such as OpenShift clusters, where it is common to have several hundreds CRDs out-of-the-box. From this view point, it is fair that clients should be able to fine tune this accordingly should the environment they work on evolves, which is currently not possible, and quite frustrating. This change introduces the --default-burst-limit option to helm (and its counterpart HELM_DEFAULT_BURST_LIMIT environment variable) to address that issue, allowing clients to properly tune their client usage as their environment evolves. Signed-off-by: Igor Sutton <isuttonl@redhat.com> * chore: change DefaultBurstLimit to BurstLimit Signed-off-by: Igor Sutton <isuttonl@redhat.com> * chore: add HELM_BURST_LIMIT to golden file Signed-off-by: Igor Sutton <isuttonl@redhat.com> * chore: add burst limit tests Signed-off-by: Igor Sutton <isuttonl@redhat.com> * docs: add burst limit default value to documentation Signed-off-by: Igor Sutton <isuttonl@redhat.com> * refactor: change burst limit default value to 100 per review instructions Signed-off-by: Igor Sutton <isuttonl@redhat.com> Signed-off-by: Gong Yongjie <yong-jie.gong@microfocus.com>

…on (helm#10842) * feat: add configuration for client-side throttling limit Client-side throttling seems to be an issue in larger environments such as OpenShift clusters, where it is common to have several hundreds CRDs out-of-the-box. From this view point, it is fair that clients should be able to fine tune this accordingly should the environment they work on evolves, which is currently not possible, and quite frustrating. This change introduces the --default-burst-limit option to helm (and its counterpart HELM_DEFAULT_BURST_LIMIT environment variable) to address that issue, allowing clients to properly tune their client usage as their environment evolves. Signed-off-by: Igor Sutton <isuttonl@redhat.com> * chore: change DefaultBurstLimit to BurstLimit Signed-off-by: Igor Sutton <isuttonl@redhat.com> * chore: add HELM_BURST_LIMIT to golden file Signed-off-by: Igor Sutton <isuttonl@redhat.com> * chore: add burst limit tests Signed-off-by: Igor Sutton <isuttonl@redhat.com> * docs: add burst limit default value to documentation Signed-off-by: Igor Sutton <isuttonl@redhat.com> * refactor: change burst limit default value to 100 per review instructions Signed-off-by: Igor Sutton <isuttonl@redhat.com>

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 7, 2022

isutton force-pushed the configurable-default-burst-limit branch from dbc354e to 7ddea4e Compare April 7, 2022 15:39

isutton force-pushed the configurable-default-burst-limit branch from 75398fc to b2ba745 Compare April 8, 2022 13:06

isutton force-pushed the configurable-default-burst-limit branch from b2ba745 to 651e5d4 Compare April 8, 2022 14:09

pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 8, 2022

yxxhero reviewed Apr 8, 2022

View reviewed changes

pkg/cli/environment.go Outdated Show resolved Hide resolved

isutton changed the title ~~Add --default-burst-limit option for client-side throttling limit configuration~~ Add --burst-limit option for client-side throttling limit configuration Apr 8, 2022

isutton force-pushed the configurable-default-burst-limit branch from 651e5d4 to c8236fb Compare April 8, 2022 14:46

yxxhero reviewed Apr 9, 2022

View reviewed changes

yxxhero added the feature label Apr 9, 2022

isutton force-pushed the configurable-default-burst-limit branch from c8236fb to 606c700 Compare April 11, 2022 10:26

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 11, 2022

isutton force-pushed the configurable-default-burst-limit branch from 606c700 to 0a8e7e4 Compare April 11, 2022 10:26

yxxhero requested review from hickeyma, marckhouzam and adamreese April 15, 2022 03:23

sabre1041 requested changes May 16, 2022

View reviewed changes

isutton force-pushed the configurable-default-burst-limit branch from 13597f9 to 3760853 Compare May 17, 2022 08:31

Igor Sutton added 6 commits May 17, 2022 10:53

chore: change DefaultBurstLimit to BurstLimit

306d6de

Signed-off-by: Igor Sutton <isuttonl@redhat.com>

chore: add HELM_BURST_LIMIT to golden file

af66b72

Signed-off-by: Igor Sutton <isuttonl@redhat.com>

chore: add burst limit tests

313de07

Signed-off-by: Igor Sutton <isuttonl@redhat.com>

docs: add burst limit default value to documentation

14daf80

Signed-off-by: Igor Sutton <isuttonl@redhat.com>

refactor: change burst limit default value to 100 per review instruct…

5392281

…ions Signed-off-by: Igor Sutton <isuttonl@redhat.com>

isutton force-pushed the configurable-default-burst-limit branch from 3760853 to 5392281 Compare May 17, 2022 08:54

yxxhero requested a review from sabre1041 May 17, 2022 10:21

sabre1041 approved these changes May 17, 2022

View reviewed changes

sabre1041 merged commit 823d929 into helm:main May 17, 2022

mattfarina added this to the 3.10.0 milestone May 17, 2022

justenwalker mentioned this pull request May 19, 2022

feat(*): add flags/env for kube api tls overrides #10944

Merged

3 tasks

negz mentioned this pull request Aug 17, 2022

Disable client-side rate-limiting when AP&F is enabled kubernetes/kubernetes#111880

Open

negz mentioned this pull request Aug 27, 2022

Bump default discovery rate limit to 300rps #11280

Closed

dkulchinsky mentioned this pull request Dec 12, 2022

Upgrade Helm pkg dependency that provides $HELM_BURST_LIMIT hashicorp/terraform-provider-helm#1011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --burst-limit option for client-side throttling limit configuration #10842

Add --burst-limit option for client-side throttling limit configuration #10842

isutton commented Apr 7, 2022 •

edited

yxxhero commented Apr 8, 2022

yxxhero commented Apr 8, 2022

isutton commented Apr 8, 2022 •

edited

isutton commented Apr 8, 2022

yxxhero commented Apr 8, 2022

isutton commented Apr 8, 2022

yxxhero Apr 9, 2022

isutton Apr 11, 2022

yxxhero commented Apr 9, 2022

yxxhero commented Apr 9, 2022

isutton commented Apr 11, 2022

yxxhero commented Apr 12, 2022

yxxhero commented Apr 12, 2022

sabre1041 left a comment

sabre1041 May 16, 2022

isutton May 17, 2022

sabre1041 May 16, 2022

isutton May 17, 2022

sabre1041 left a comment

Add --burst-limit option for client-side throttling limit configuration #10842

Add --burst-limit option for client-side throttling limit configuration #10842

Conversation

isutton commented Apr 7, 2022 • edited

yxxhero commented Apr 8, 2022

yxxhero commented Apr 8, 2022

isutton commented Apr 8, 2022 • edited

isutton commented Apr 8, 2022

yxxhero commented Apr 8, 2022

isutton commented Apr 8, 2022

yxxhero Apr 9, 2022

Choose a reason for hiding this comment

isutton Apr 11, 2022

Choose a reason for hiding this comment

yxxhero commented Apr 9, 2022

yxxhero commented Apr 9, 2022

isutton commented Apr 11, 2022

yxxhero commented Apr 12, 2022

yxxhero commented Apr 12, 2022

sabre1041 left a comment

Choose a reason for hiding this comment

sabre1041 May 16, 2022

Choose a reason for hiding this comment

isutton May 17, 2022

Choose a reason for hiding this comment

sabre1041 May 16, 2022

Choose a reason for hiding this comment

isutton May 17, 2022

Choose a reason for hiding this comment

sabre1041 left a comment

Choose a reason for hiding this comment

isutton commented Apr 7, 2022 •

edited

isutton commented Apr 8, 2022 •

edited