Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support to set qps and burst via env variable #2755

Merged
merged 5 commits into from
Jun 16, 2023

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Jun 15, 2023

Currently some configuration such as kube-api-burst and kube-api-qps must be set via command line option.

Knative deployments usually do not have the command line option, the command line option is unusual and so knative/operator also does not support the API.

Hence This patch allows to set client config via env variable.

Release Note

env variable `KUBE_API_QPS` and `KUBE_API_BURST` are available. They are same options for `--kube-api-burst` and `--kube-api-qps`

nak3 added 2 commits June 15, 2023 18:46
Currently some configuration such as `kube-api-burst` and
`kube-api-qps` must be set via command line option.

Knative deployments usually do not have the command line option,
the command line option is unusual and so knative/operator also does
not support the API.

Hence This patch allows to set client config via env variable.
@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 15, 2023
@knative-prow knative-prow bot requested review from aslom and krsna-m June 15, 2023 10:04
@nak3
Copy link
Contributor Author

nak3 commented Jun 15, 2023

/cc @dprotaso @skonto

@knative-prow knative-prow bot requested review from dprotaso and skonto June 15, 2023 10:06
@skonto
Copy link
Contributor

skonto commented Jun 15, 2023

LGTM thanks @nak3!

fs.Float64Var(&c.QPS, "kube-api-qps", float64(envVarOrDefault("KUBE_API_QPS", 0)), "Maximum QPS to the server from the client.")
}

func envVarOrDefault(key string, val int) int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we dont have such a func elsewhere in the code base?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think there is no util function like this.

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Patch coverage: 54.54% and project coverage change: -0.32 ⚠️

Comparison is base (4dbc312) 82.06% compared to head (1586464) 81.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2755      +/-   ##
==========================================
- Coverage   82.06%   81.75%   -0.32%     
==========================================
  Files         166      167       +1     
  Lines       10150    10201      +51     
==========================================
+ Hits         8330     8340      +10     
- Misses       1576     1614      +38     
- Partials      244      247       +3     
Impacted Files Coverage Δ
apis/duck/v1/kresource_type.go 91.30% <0.00%> (-8.70%) ⬇️
environment/client_config.go 32.65% <66.66%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nak3
Copy link
Contributor Author

nak3 commented Jun 15, 2023

/hold

I think I found one bug...

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023
@nak3
Copy link
Contributor Author

nak3 commented Jun 15, 2023

/hold cancel

Fixed.

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023
@dprotaso
Copy link
Member

dprotaso commented Jun 15, 2023

/hold

We actually want to drop these settings from our controllers since K8s supports API priority and fairness now - https://kubernetes.io/docs/concepts/cluster-administration/flow-control/

So we should disable all client side rate limiting

Some context:
https://kubernetes.slack.com/archives/C0EG7JC6T/p1680796032246709?thread_ts=1680791299.631439&cid=C0EG7JC6T

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023
@nak3
Copy link
Contributor Author

nak3 commented Jun 16, 2023

Ah, thank you. Then, let's defer to #2756

@nak3 nak3 closed this Jun 16, 2023
@nak3 nak3 reopened this Jun 16, 2023
@dprotaso
Copy link
Member

TestMetricsExport flake

/retest

@dprotaso
Copy link
Member

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2023
@knative-prow
Copy link

knative-prow bot commented Jun 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dprotaso
Copy link
Member

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2023
@knative-prow knative-prow bot merged commit eb63a40 into knative:main Jun 16, 2023
40 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants