-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
clarification for ResourceQuota concept #42685
Conversation
try to resolve #42605 |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Co-authored-by: Tim Bannister <tim@scalefactory.com>
Co-authored-by: Tim Bannister <tim@scalefactory.com>
You can set quota for the total number of certain resources of all standard, | ||
namespaced resource types using the following syntax: | ||
You can set quota for **the total number of one particular resource** | ||
for certain resources of all standard, namespaced resource types using the following syntax: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this all object kinds or only some? The new wording seems contradictory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it brings more confusion. I switch to another expression to see if it is better.
You can set quota for the total number of certain resources of all standard, | ||
namespaced resource types using the following syntax: | ||
You can set quota for **the total number of one particular resource** of all standard, namespaced resource types | ||
using the following syntax: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My experiments showed that the <resource>
in count/<resource>
can be anything.
It can be something that doesn't exist at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ... this doen't have to be a "standard" resource type, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the API accepts any resource name in that field, this could mean:
- you can set any ResourceQuota, even for an API the cluster doesn't yet have
- if the cluster already has that resource kind, you cannot add a new resource that would exceed the quota
- if the cluster gains that resource kind, the quota starts to be enforced
- you can set any ResourceQuota, even for an API the cluster doesn't yet have
- if the cluster already has that resource kind, you cannot add a new resource that would exceed the quota
- if the cluster gains that resource kind, the quota starts to be enforced
- however, all of the above only includes built-in APIs and CustomResourceDefinitions. An APIService backed by an aggregated API server can fail to enforce a ResourceQuota without being seen as defective.
- you can set any ResourceQuota, even for an API the cluster doesn't yet have
- however, there is an internal list of API kinds where enforcement happens
- if you set a ResourceQuota for an different resource kind, the ResourceQuota is not enforced, and the lack of enforcement is silent
Let's be clear on which of those two three cases applies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 1st case is the correct answer, but has to double confirm this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's case 2 (very well considered). Though the "default" aggregated apiserver (APIService) enforces quota.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'standard' is indeed ambiguous, better to clearly list out: built-in API and CRD
👋 @pegasas . |
for my view, this has resolved all comments and pending merging. |
I suggest that you get a technical review of the changes. |
@kubernetes/sig-api-machinery-misc can you either review this, or let us know which SIG should do the technical review? |
Ping @kubernetes/sig-api-machinery-pr-reviews : PTAL and advise if y'all are the right folx to be reviewing the PR? |
@kubernetes/sig-api-machinery-leads : Could you please assign someone to look at this? It has been pending in our queue for a very long time & we'd appreciate some input. |
Slack channel message sent on #sig-api-machinery |
@yliaog from the pool of your generosity, would you mind taking a look to this one? |
/assign @yliaog |
@@ -168,13 +168,14 @@ Here is an example set of resources users may want to put under object count quo | |||
The same syntax can be used for custom resources. | |||
For example, to create a quota on a `widgets` custom resource in the `example.com` API group, use `count/widgets.example.com`. | |||
|
|||
When using `count/*` resource quota, an object is charged against the quota if it exists in server storage. | |||
When using such a resource quota (only available for some object kinds), an object is charged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps indicating that only some APIServices might be excluded. It's nearly all resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pegasas : Whenever you have some cycles, would you be okay making this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Correct this one to "nearly for all resources".
You can set quota for the total number of certain resources of all standard, | ||
namespaced resource types using the following syntax: | ||
You can set quota for **the total number of one particular resource** of all standard, namespaced resource types | ||
using the following syntax: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'standard' is indeed ambiguous, better to clearly list out: built-in API and CRD
@@ -168,13 +168,14 @@ Here is an example set of resources users may want to put under object count quo | |||
The same syntax can be used for custom resources. | |||
For example, to create a quota on a `widgets` custom resource in the `example.com` API group, use `count/widgets.example.com`. | |||
|
|||
When using `count/*` resource quota, an object is charged against the quota if it exists in server storage. | |||
When using such a resource quota (nearly for all object kinds), an object is charged | |||
against the quota if the control plane has a stored copy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if the control plane has a stored copy." is not very clear what this means, may be something like: "if the object kind exists (is defined) in the control plane"
/lgtm |
LGTM label has been added. Git tree hash: cd70079092371991feccb1a0177c3135f3291920
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of recommend a tweak, because the changes so far don't cover https://github.com/kubernetes/website/pull/42685/files#r1546282734
Co-authored-by: Tim Bannister <tim@scalefactory.com>
Co-authored-by: Tim Bannister <tim@scalefactory.com>
sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: afd1a8067e2d6ad7873a0d657fa09fbc3dea364a
|
Thank you for addressing comments and thank you all for reviews |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: reylejano 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 |
It seems owner @whitebear009 has solved #1 by whitebeard10#2.
I've mades some minor change on #1
For #2, my suggestion is we should add a link to below paragraph.
Then count/pod and pod would be clear.
In my opinion, we should use
pods
in resource quota yaml, and counts/pods for directly use kubectl.