Standard API qps and burst in kubeconfig file. #1629

deads2k · 2020-03-23T21:06:24Z

Standard API qps and burst in kubeconfig file.

This allows consistent configuration of all client binaries's QPS and burst settings. This becomes more important now that API priority and fairness is alpha and we want people to try it.

k8s-ci-robot · 2020-03-23T21:06:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

~~keps/sig-api-machinery/OWNERS~~ [deads2k]

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

k8s-ci-robot · 2020-03-24T14:24:12Z

@deads2k: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-enhancements-verify	`224f926`	link	`/test pull-enhancements-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

yue9944882 · 2020-03-25T06:58:22Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+type AuthInfo struct {
+	// ...
+	// QPS indicates the maximum QPS to the master from this client.
+	// If it's zero, the created RESTClient will use DefaultQPS: 5


should we clarify a negative QPS will turn off the ratelimiting in the doc?

"Set to -1 to turn off rate limiting. A value of 0 means to use the default limit from the client binary."

-1 can be even confusing given that the QPS is a float32. what's supposed to happen it's other negative values than -1?

Agree with others, clarify how to turn it off, clarify what negative numbers do (if not that).

I don't really want to make this super more complicated than necessary, but making this into a union/one-of block might make sense.

MikeSpreitzer · 2020-05-14T02:05:47Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+## Summary
+
+Kubernetes REST clients use a simple token bucket rate limiter to control the pace of requests from a client.
+We will expose the values already present in the [REST config](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L114-L120), via a kubeconfig file inside the client stanza.


Line numbers are not stable in the master branch. For a long-lived document like this, it would be better to reference a particular commit. The following URL references the commit to which the master branch currently points. This URL also skips the staging junk, but still suffices to direct the reader to the intended content.

https://github.com/kubernetes/client-go/blob/d4685ebf697c8e0d9c8ae48db6c0ce34221d0326/rest/config.go#L115-L121

MikeSpreitzer · 2020-05-14T07:24:17Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+## Summary
+
+Kubernetes REST clients use a simple token bucket rate limiter to control the pace of requests from a client.
+We will expose the values already present in the [REST config](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L114-L120), via a kubeconfig file inside the client stanza.


"via a kubeconfig file inside the client stanza" sounds like it is referring to a kubeconfig file that is inside some client stanza --- but I do not think this is what's meant here. I think what's meant is more like "via the user definitions in kubeconfig files". Alternatively, perhaps what is meant is more like "via a kubeconfig file that is added to a ServiceAccount's Secret".

Suggested change

We will expose the values already present in the [REST config](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L114-L120), via a kubeconfig file inside the client stanza.

We will expose the values already present in the [REST config](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L114-L120), in the client stanza of a kubeconfig file.

you mean "a user stanza"?

MikeSpreitzer · 2020-05-14T07:28:43Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+
+## Proposal
+
+Add to the `AuthInfo` section of a kubeconfig.


Why not in the preferences? That would merge just fine and not involve the complicated (and poorly documented) logic involved in merging users --- which does not even meet the need here.

Why not in the preferences? That would merge just fine and not involve the complicated (and poorly documented) logic involved in merging users --- which does not even meet the need here.

That's a dead field.

MikeSpreitzer · 2020-05-14T07:30:10Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+This identity spans all the contexts with different namespaces, so it is a closer logical match.
+You don't generally want different QPS characteristics based on namespace.
+
+```go


Please include a URL or k/k-relative pathname for the file where this mod would apply.

MikeSpreitzer · 2020-05-14T07:31:19Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+### in-cluster kubeconfig overrides
+The in-cluster config is the automatic configuration that exists within pods.
+The client code itself can be written to consume an overrides.kubeconfig file, which will be merged with the standard
+kubeconfig merging rules that we use the env var path today.


The merging rules we have today do not allow adding/modifying fields within a user. The first specification of that user is taken as a whole and no part is overridden.

The merging rules we have today do not allow adding/modifying fields within a user. The first specification of that user is taken as a whole and no part is overridden.

No, we merge all of it: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/clientcmd/loader.go#L226-L252

You're probably used to selection, but merging is also a thing.

Yes, I looked at that code. You see I raised issues with the poor comment on that function (kubernetes/kubernetes#91092) and the poor user-facing documentation about this (kubernetes/website#20967).

The code in question does not itself answer the question, it just makes a two-handed pile of invocations of mergo.MergeWithOverwrite. The comments on that suggest that merging will be done, but the implementation does not do that. The implementation does not simply look at each layer of structure independently. In the case of a map, the behavior depends on the type of each value. In the case of a map entry whose value is a struct or pointer, the implementation gets to https://github.com/imdario/mergo/blob/v0.3.5/merge.go#L143 --- which replaces the whole value without recursing into it.

I tested, and indeed found that when merging two kubeconfig files that define different fields for a certain user, the result is only the fields of the first kubeconfig file to define that user.

MikeSpreitzer · 2020-05-14T07:36:01Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+}
+```
+
+### in-cluster kubeconfig overrides


The vision here is not entirely clear. I suspect the idea is that the Secret of a ServiceAccount would be extended to have a data member named overrides.kubeconfig containing a kubeconfig file that specifies the QPS and Burst and nothing else, and that anyone who wants to exercise this control has to create a controller that modifies all the ServiceAccount Secret objects. Have I got this right? If so, that's pretty painful to use and requires a controller with extreme privilege.

Have I got this right? If so, that's pretty painful to use and requires a controller with extreme privilege.

Yeah, but anything we do is going to require mutation of that same secret. It isn't to be taken lightly in general. This is about making it possible, I don't think easy is needed.

This QPS limit has to be a property of the service account, so this does seem necessary.

Maybe it's not the only model though, I'll make another comment.

MikeSpreitzer · 2020-05-14T07:36:48Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+### Graduation Criteria
+
+The API itself is well-known since near the beginning of kube.
+We are simply exposing existing values, it will enter as GA.


There is significant novelty here, in the override mechanism.

There is significant novelty here, in the override mechanism.

I don't think so. I built this pre-1.0 so the merging code already exists. This is just about the expression of what merges.

Things are different today, I hear every mother say.

lavalamp · 2020-05-14T21:15:06Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+
+1. Connecting to the kube-apiserver to retrieve a value for default QPS and burst.
+   No default value is going to be good across all workloads.
+   It's also mechanicaly weird to create a client to get a client.


I agree this is a bit weird, but it wouldn't have to be a separate request-- we could make apiserver set a header with its desired rate limit, and the client just obeys it after the first request. This might be simpler?

I am actually liking this (return a header) more the longer I think about it. Since we have to recompile all clients anyway...

After more thought I think this is too heavyweight (on API changes -- but so is the main proposal), and it's easy to believe that not all clients would want the same rate limit.

(So I guess this would still be in consideration if it were just about turning off the rate limit, not about setting it.)

lavalamp · 2020-05-14T21:16:11Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+
+## Implementation History
+
+## Drawbacks


Do other, non-go clients parse kubeconfig files?

I assume repeated invocations of kubectl would not respect the limits in aggregate?

I think this is aimed at long-lived clients rather than kubectl. I would not expect client-side rate limits to be relevant to kubectl.

I would not expect that either, but users who see the settings in their kubeconfig files might.

This proposal allows new content in the kubeconfig files that users hold, but does not require it in general nor in the motivating scenario in particular.

lavalamp · 2020-05-14T21:18:10Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+
+### Goals
+
+1. Allow easy and consistent configuration for the existing client-side rate limiting.


Is this our goal, or is our goal to get rid of it completely?

Is this necessary only transitionally, or forever?

lavalamp · 2020-05-14T21:19:51Z

keps/sig-api-machinery/1630-kubeconfig-qps-burst/README.md

+
+## Drawbacks
+
+## Alternatives


Another alternative might be to plumb these via env var. That'd be gross, but very direct.

The go team tends to do things this way, e.g. for disabling their home grown DNS resolver, and also for disabling HTTP2 I think.

The benefit of this is that it only requires one component to change, no API is baked in, we can remove the env var without breaking people who are setting it.

The downside is that it's not very discoverable (but maybe that's OK since we mostly want to use it to run tests).

Anyone setting this should also look at the client's metrics to verify that it's not spending any time blocked on the QPS limiter (I think there's already a metric).

lavalamp · 2020-05-29T20:50:26Z

@deads2k, @liggitt, and I have agreed on the "pre-deprecated, global env var" approach. That is, we'll make the client library check for an environment variable and avoid installing the rate limiter if it is set to some value. The purpose of doing it this way is to avoid committing to an API since we aren't sure it'll be useful for a long period of time. So, we will do this with the env var already deprecated and remove it without warning.

Since this won't be a user-visible feature we don't plan on making a KEP for this.

deads2k · 2020-06-01T15:30:44Z

@deads2k, @liggitt, and I have agreed on the "pre-deprecated, global env var" approach. That is, we'll make the client library check for an environment variable and avoid installing the rate limiter if it is set to some value. The purpose of doing it this way is to avoid committing to an API since we aren't sure it'll be useful for a long period of time. So, we will do this with the env var already deprecated and remove it without warning.

Since this won't be a user-visible feature we don't plan on making a KEP for this.

Agreed

/close

k8s-ci-robot · 2020-06-01T15:30:57Z

@deads2k: Closed this PR.

In response to this:

@deads2k, @liggitt, and I have agreed on the "pre-deprecated, global env var" approach. That is, we'll make the client library check for an environment variable and avoid installing the rate limiter if it is set to some value. The purpose of doing it this way is to avoid committing to an API since we aren't sure it'll be useful for a long period of time. So, we will do this with the env var already deprecated and remove it without warning.

Since this won't be a user-visible feature we don't plan on making a KEP for this.

Agreed

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 23, 2020

k8s-ci-robot requested review from dims and lavalamp March 23, 2020 21:06

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 23, 2020

make API QPS and burst configureable via kubeconfig

224f926

deads2k force-pushed the api-burst-qps branch from 229e6af to 224f926 Compare March 24, 2020 14:22

deads2k changed the title ~~[wip] make API QPS and burst configureable via kubeconfig~~ Standard API qps and burst in kubeconfig file. Mar 24, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2020

deads2k mentioned this pull request Mar 24, 2020

standardize API qps and burst in kubeconfig #1630

Closed

yue9944882 reviewed Mar 25, 2020

View reviewed changes

deads2k mentioned this pull request Mar 25, 2020

Documenting APIPriorityAndFairness beta criteria #1632

Merged

deads2k mentioned this pull request May 13, 2020

[wip] Standard API qps and burst in kubeconfig file kubernetes/kubernetes#91067

Closed

MikeSpreitzer reviewed May 14, 2020

View reviewed changes

lavalamp reviewed May 14, 2020

View reviewed changes

k8s-ci-robot closed this Jun 1, 2020

	We will expose the values already present in the [REST config](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L114-L120), via a kubeconfig file inside the client stanza.
	We will expose the values already present in the [REST config](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L114-L120), in the client stanza of a kubeconfig file.


		### Goals

		1. Allow easy and consistent configuration for the existing client-side rate limiting.

Standard API qps and burst in kubeconfig file. #1629

Standard API qps and burst in kubeconfig file. #1629

Conversation

deads2k commented Mar 23, 2020 • edited Loading

k8s-ci-robot commented Mar 23, 2020

k8s-ci-robot commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented May 29, 2020

deads2k commented Jun 1, 2020

k8s-ci-robot commented Jun 1, 2020

deads2k commented Mar 23, 2020 •

edited

Loading

MikeSpreitzer May 15, 2020 •

edited

Loading