Skip to content

Add non-preemptible policy to PriorityClasses#78844

Closed
xychu wants to merge 1 commit intokubernetes:masterfrom
xychu:more-priority-policies
Closed

Add non-preemptible policy to PriorityClasses#78844
xychu wants to merge 1 commit intokubernetes:masterfrom
xychu:more-priority-policies

Conversation

@xychu
Copy link
Copy Markdown
Contributor

@xychu xychu commented Jun 10, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
As we've have non-preempting(#74614) policy, for some cases, users may need non-preemptible PriorityClasses as well.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

	// PreemptLowerPriority means that pod can preempt other pods with lower priority.
	PreemptLowerPriority PreemptionPolicy = "PreemptLowerPriority"
	// PreemptNever means that pod never preempts other pods with lower priority.
	PreemptNever PreemptionPolicy = "Never"
	// NonPreemptible means that pod can not be preempted by other pods with higher priority.
	NonPreemptible PreemptionPolicy = "NonPreemptible"
	// NonPreemptiblePreemptNever means that pod can not be preempted by other pods with higher priority and never preempts other pods with lower priority.
	NonPreemptiblePreemptNever PreemptionPolicy = "NonPreemptiblePreemptNever"

Need advice on the naming part.
Does this PR introduce a user-facing change?:

Add non-preemptible policy to PriorityClass

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jun 10, 2019

Hi @xychu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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

k8s-ci-robot commented Jun 10, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xychu
To complete the pull request process, please assign erictune, ravisantoshgudimetla
You can assign the PR to them by writing /assign @erictune @ravisantoshgudimetla in a comment when ready.

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

Details 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 kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 10, 2019
@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Jun 10, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

draveness
draveness reviewed Jun 10, 2019
View reviewed changes
pkg/apis/core/types.go
PreemptLowerPriority PreemptionPolicy = "PreemptLowerPriority"
// PreemptNever means that pod never preempts other pods with lower priority.
PreemptNever PreemptionPolicy = "Never"
// NonPreemptible means that pod can not be preempted by other pods with higher priority.
Copy link
Copy Markdown
Contributor

@draveness draveness Jun 10, 2019

Choose a reason for hiding this comment

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

I think it's better to open a KEP first and have a thorough discussion about why we need it and how to implement it. Please take a look at https://github.com/kubernetes/enhancements

@wgliang
Copy link
Copy Markdown
Contributor

wgliang commented Jun 11, 2019

@xychu Let's start with KEP.

@denkensk
Copy link
Copy Markdown
Member

denkensk commented Jun 11, 2019 •
edited
Loading

Thanks @xychu May need more use case in this issue kubernetes/enhancements#902.
And add a pr in this
https://github.com/kubernetes/enhancements/blob/master/keps/sig-scheduling/20190317-non-preempting-priorityclass.md

@xychu
Copy link
Copy Markdown
Contributor Author

xychu commented Jun 11, 2019

@draveness @denkensk @wgliang thanks for your review. I'll update the KEP in kubernetes/enhancements#902 first. Will submit a pr later.

@xychu xychu mentioned this pull request Jun 11, 2019
Closed
@xychu
Copy link
Copy Markdown
Contributor Author

xychu commented Jun 11, 2019

KEP pr: kubernetes/enhancements#1096

@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Sep 9, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 9, 2019
@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Oct 9, 2019

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 9, 2019
@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Nov 8, 2019

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 8, 2019

@fejta-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brendandburns brendandburns Awaiting requested review from brendandburns

@mikedanese mikedanese Awaiting requested review from mikedanese

1 more reviewer

@draveness draveness draveness left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@xychu @k8s-ci-robot @fejta-bot @wgliang @denkensk @draveness