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

Clarify ResourceQuota limit for PriorityClass #26273

Conversation

Cweiping
Copy link

According to the priorityClass design document, improve the priorityClass that can be configured by users through Resourcequota.
Reference proposal doc: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/scheduling/pod-priority-resourcequota.md#sample-user-story-1
/kind feature
/language en

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jan 28, 2021
@netlify
Copy link

netlify bot commented Jan 28, 2021

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 188ccc2

https://deploy-preview-26273--kubernetes-io-master-staging.netlify.app

@Cweiping Cweiping force-pushed the feature/clairly_resource_quota_limit_for_PriorityClass branch from 047864a to beb6ea5 Compare January 28, 2021 13:57
@Cweiping
Copy link
Author

Cweiping commented Feb 2, 2021

Done. Please review again.
@ydFu

@Cweiping Cweiping force-pushed the feature/clairly_resource_quota_limit_for_PriorityClass branch from beb6ea5 to 91ecacf Compare February 2, 2021 08:38
@k8s-ci-robot k8s-ci-robot 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 Feb 2, 2021
@Cweiping Cweiping force-pushed the feature/clairly_resource_quota_limit_for_PriorityClass branch from 91ecacf to 3e2e272 Compare February 3, 2021 01:42
@Cweiping

This comment has been minimized.

@ydFu
Copy link
Member

ydFu commented Feb 3, 2021

/lgtm

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

LGTM label has been added.

Git tree hash: c1ccb1280402ff9cb98480ae27bfa89d7a0c72be

@Cweiping
Copy link
Author

Cweiping commented Feb 3, 2021

/assign @tengqm
/assign @stfim

@sftim
Copy link
Contributor

sftim commented Feb 3, 2021

/sig scheduling

I'd like to get technical review on these changes.

@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Feb 3, 2021
@sftim
Copy link
Contributor

sftim commented Feb 3, 2021

(@ydFu - are you providing LGTM on behalf of SIG Scheduling?)


{{< codenew file="policy/priority-class-resourcequota.yaml" >}}

```shell script
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be "shell" rather than "shell script"?

@@ -610,16 +610,23 @@ plugins:
values: ["cluster-services"]
```

Now, "cluster-services" pods will be allowed in only those namespaces where a quota object with a matching `scopeSelector` is present.
For example:
Then, create a corresponding resource quota object in `kube-system` namespace:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Then, create a corresponding resource quota object in `kube-system` namespace:
Then, create a resource quota object in the `kube-system` namespace:

```

In this case, a pod creation will be allowed if:
1. Pod has no priority class and created in any namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Pod has no priority class and created in any namespace.
1. the Pod's `priorityClassName` is not specified, or


In this case, a pod creation will be allowed if:
1. Pod has no priority class and created in any namespace.
2. Pod has priority class other than `cluster-services` and created in any namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2. Pod has priority class other than `cluster-services` and created in any namespace.
1. the Pod's `priorityClassName` is specified to a value other than "`cluster-services`", or

In this case, a pod creation will be allowed if:
1. Pod has no priority class and created in any namespace.
2. Pod has priority class other than `cluster-services` and created in any namespace.
3. Pod has priority class `cluster-services` and created in `kube-system` namespace, and passed resource quota check.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
3. Pod has priority class `cluster-services` and created in `kube-system` namespace, and passed resource quota check.
1. the Pod's `priorityClassName` is set to "`cluster-services`", it is to be created
in the `kube-system` namespace, and it has passed the resource quota check.

1. Pod has no priority class and created in any namespace.
2. Pod has priority class other than `cluster-services` and created in any namespace.
3. Pod has priority class `cluster-services` and created in `kube-system` namespace, and passed resource quota check.
Pod creation will be rejected if pod has priority class `cluster-services` and created in namespace other than `kube-system`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Pod creation will be rejected if pod has priority class `cluster-services` and created in namespace other than `kube-system`.
A Pod creation request is rejected if its `priorityClassName` is set to "`cluster-services`"
and it is to be created in a namespace other than `kube-system`.

@ydFu
Copy link
Member

ydFu commented Feb 4, 2021

(@ydFu - are you providing LGTM on behalf of SIG Scheduling?)

@sftim
Thank you for your question.
I would also like to have more experienced reviewers and approvers provide more professional advice on this PR.
As you said, you will conduct a technical review.
We work together to make the kubernetes/website more perfect.

@Cweiping Cweiping force-pushed the feature/clairly_resource_quota_limit_for_PriorityClass branch from 3e2e272 to 188ccc2 Compare February 9, 2021 08:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021
Copy link
Member

@irvifa irvifa left a comment

Choose a reason for hiding this comment

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

/lgtm
/sig scheduling

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

LGTM label has been added.

Git tree hash: 97b6abf229777050660b21cb8ed11a24f553acf3

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.

/approve

{{< codenew file="policy/priority-class-resourcequota.yaml" >}}

```shell
$ kubectl apply -f https://k8s.io/examples/policy/priority-class-resourcequota.yaml -n kube-system
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
$ kubectl apply -f https://k8s.io/examples/policy/priority-class-resourcequota.yaml -n kube-system
kubectl apply -f https://k8s.io/examples/policy/priority-class-resourcequota.yaml -n kube-system

matchExpressions:
- operator : In
scopeName: PriorityClass
values: ["cluster-services"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a newline at the end of this manifest so that it looks OK no matter what OS you download it on.

1. the Pod's `priorityClassName` is set to `cluster-services`, it is to be created
in the `kube-system` namespace, and it has passed the resource quota check.

A Pod creation request is rejected if its `priorityClassName` is set to `cluster-services`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: What rejects the request?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim, ydFu

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 Feb 9, 2021
@sftim
Copy link
Contributor

sftim commented Feb 9, 2021

/retitle Clarify ResourceQuota limit for PriorityClass

@k8s-ci-robot k8s-ci-robot changed the title Clairly Resourcequota limit for PriorityClass Clarify ResourceQuota limit for PriorityClass Feb 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 539e524 into kubernetes:master Feb 9, 2021
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. kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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.

6 participants