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

clarification for ResourceQuota concept #42685

Merged
merged 9 commits into from
Apr 22, 2024
Merged

Conversation

pegasas
Copy link
Contributor

@pegasas pegasas commented Aug 23, 2023

  1. count/* — it's precieved as a wildcard mask as if it contains all the resources such as a pods, services, persistentvolumeclaims. But this doesn’t not work that way. Consider rewording or adding some information about the meaning, maybe as a warning block or as a note block.

2. quota for the total number of certain resources and generic object count quota means the same, so there could be two syntaxes for the same object, for example count/pods and pods. I found the only difference — you cannot set any quota in count/-less syntax except what mentioned in the table. The problem is the wording — it looks like you can specify something different using different syntaxes, but in fact it's the same things with some limitations for count/-less syntax. Consider rewording to highlight that this is just another syntax for the same resources (if I'm not mistaken).

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 23, 2023
@pegasas
Copy link
Contributor Author

pegasas commented Aug 23, 2023

try to resolve #42605

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2023
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Aug 23, 2023
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Aug 23, 2023
@netlify
Copy link

netlify bot commented Aug 23, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit efed6fd
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/660ca1d71235490008ac3906
😎 Deploy Preview https://deploy-preview-42685--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

pegasas and others added 2 commits August 23, 2023 15:00
Co-authored-by: Tim Bannister <tim@scalefactory.com>
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 24, 2023
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:
Copy link
Contributor

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.

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

@sftim sftim Oct 9, 2023

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:

  1. 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
  2. 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.
  3. 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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

@kbhawkey
Copy link
Contributor

👋 @pegasas .
Checking if you are actively working on your pull request, or if you can close it?
Thanks

@pegasas
Copy link
Contributor Author

pegasas commented Dec 20, 2023

👋 @pegasas . Checking if you are actively working on your pull request, or if you can close it? Thanks

for my view, this has resolved all comments and pending merging.

@kbhawkey
Copy link
Contributor

I suggest that you get a technical review of the changes.

@sftim
Copy link
Contributor

sftim commented Jan 2, 2024

@kubernetes/sig-api-machinery-misc can you either review this, or let us know which SIG should do the technical review?

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 2, 2024
@divya-mohan0209
Copy link
Contributor

Ping @kubernetes/sig-api-machinery-pr-reviews : PTAL and advise if y'all are the right folx to be reviewing the PR?

@divya-mohan0209
Copy link
Contributor

@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.

@divya-mohan0209
Copy link
Contributor

Slack channel message sent on #sig-api-machinery

@fedebongio
Copy link

@yliaog from the pool of your generosity, would you mind taking a look to this one?

@yliaog
Copy link
Contributor

yliaog commented Apr 1, 2024

/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
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:
Copy link
Contributor

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.
Copy link
Contributor

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"

@yliaog
Copy link
Contributor

yliaog commented Apr 2, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: cd70079092371991feccb1a0177c3135f3291920

Copy link
Contributor

@sftim sftim left a 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

content/en/docs/concepts/policy/resource-quotas.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/resource-quotas.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024
@k8s-ci-robot k8s-ci-robot requested a review from yliaog April 3, 2024 00:24
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@pegasas
Copy link
Contributor Author

pegasas commented Apr 3, 2024

I kind of recommend a tweak, because the changes so far don't cover https://github.com/kubernetes/website/pull/42685/files#r1546282734

sure.
i commit these changes.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: afd1a8067e2d6ad7873a0d657fa09fbc3dea364a

@reylejano
Copy link
Member

Thank you for addressing comments and thank you all for reviews
/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2024
@k8s-ci-robot k8s-ci-robot merged commit 34ac45c into kubernetes:main Apr 22, 2024
6 checks passed
@pegasas pegasas deleted the quota branch April 22, 2024 20:57
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants