Skip to content

add non-preempting option to PriorityClasses#74614

Merged
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
denkensk:no-preempting-priority
May 31, 2019
Merged

add non-preempting option to PriorityClasses#74614
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
denkensk:no-preempting-priority

Conversation

@denkensk
Copy link
Copy Markdown
Member

@denkensk denkensk commented Feb 26, 2019 •
edited
Loading

What type of PR is this?
/kind feature
/priority important-soon

What this PR does / why we need it:
Adds a NonPrempting field to the PriorityClass. If set on a class, it will continue to be prioritized above queued pods of a lesser class, but will not preempt running pods.

Which issue(s) this PR fixes:
Fixes #67671
kubernetes/enhancements#902

Add  NonPrempting field to the PriorityClass.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 26, 2019
@k8s-ci-robot k8s-ci-robot added area/kubectl 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/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 26, 2019
@cblecker cblecker removed their request for review February 27, 2019 00:11
@denkensk denkensk force-pushed the no-preempting-priority branch 3 times, most recently from faccb20 to 23151be Compare March 1, 2019 06:52
bsalamat
bsalamat reviewed Mar 2, 2019
View reviewed changes
yastij
yastij reviewed Mar 4, 2019
View reviewed changes
pkg/apis/scheduling/v1beta1/register.go Outdated
// generated functions takes place in the generated files. The separation
// makes the code compile even when the generated files are missing.
localSchemeBuilder.Register(RegisterDefaults)
localSchemeBuilder.Register(addDefaultingFuncs)
Copy link
Copy Markdown
Member

@yastij yastij Mar 4, 2019

Choose a reason for hiding this comment

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

same here, the regitration for defaults should be under v1 ?

@liggitt liggitt removed sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Mar 6, 2019
@denkensk denkensk changed the title [WIP] No preempting priority [WIP] add non-preempting option to PriorityClasses Mar 21, 2019
@denkensk denkensk force-pushed the no-preempting-priority branch from 23151be to cfc3970 Compare March 28, 2019 03:56
@k8s-ci-robot k8s-ci-robot added area/test sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Mar 28, 2019
@denkensk denkensk force-pushed the no-preempting-priority branch from cfc3970 to dadb9fd Compare March 28, 2019 03:56
@liggitt liggitt added this to the v1.15 milestone May 28, 2019
@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 28, 2019

/retest

liggitt
liggitt reviewed May 28, 2019
View reviewed changes
@denkensk denkensk force-pushed the no-preempting-priority branch from 0cf6f8d to 53c142c Compare May 28, 2019 18:38
bsalamat
bsalamat reviewed May 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

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

/lgtm

Only a minor comment. Please squash commits where appropriate.

@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 29, 2019

Squash commits. @bsalamat @liggitt PTAL? Thank you

@timmycarr
Copy link
Copy Markdown

timmycarr commented May 29, 2019

Hi! Friendly reminder from your release team: we are starting the code freeze for 1.15 tomorrow EOD. Seeing some good progress here, just checking in to see if this is still planned for the 1.15 cycle? See that milestone added a couple of days ago. Assuming that's the case. Can I get a thumbs up?

bsalamat
bsalamat reviewed May 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks, @denkensk!

liggitt
liggitt reviewed May 29, 2019
View reviewed changes
@vllry
Copy link
Copy Markdown
Contributor

vllry commented May 30, 2019

@denkensk do you have time to get the final changes in?

@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 30, 2019

@denkensk do you have time to get the final changes in?

I am working on it now and will update later.

@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 30, 2019

/retest

@liggitt liggitt mentioned this pull request May 30, 2019
Merged
7 tasks
@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 30, 2019

/retest

liggitt
liggitt reviewed May 30, 2019
View reviewed changes
pkg/apis/scheduling/validation/validation.go Outdated
Copy link
Copy Markdown
Member

@liggitt liggitt May 30, 2019

Choose a reason for hiding this comment

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

"preemptionPolicy", since this is shown to the user and should match the serialized field name

pkg/apis/scheduling/validation/validation.go Outdated
Copy link
Copy Markdown
Member

@liggitt liggitt May 30, 2019

Choose a reason for hiding this comment

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

"preemptionPolicy", since this is shown to the user and should match the serialized field name

pkg/kubectl/cmd/create/create_priorityclass.go Outdated
Copy link
Copy Markdown
Member

@liggitt liggitt May 30, 2019

Choose a reason for hiding this comment

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

don't specify a default for alpha fields

@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 30, 2019

verify test requires single-line invocations of SetFeatureGateDuringTest:

Invalid invocations of featuregatetesting.SetFeatureGateDuringTest():
./pkg/apis/scheduling/v1/defaults_test.go:60:	defer featuregatetesting.SetFeatureGateDuringTest(nil, utilfeature.DefaultFeatureGate,
./pkg/apis/scheduling/v1beta1/defaults_test.go:60:	defer featuregatetesting.SetFeatureGateDuringTest(nil, utilfeature.DefaultFeatureGate,
./pkg/apis/scheduling/v1alpha1/defaults_test.go:60:	defer featuregatetesting.SetFeatureGateDuringTest(nil, utilfeature.DefaultFeatureGate,

Always make a deferred call to the returned function to ensure the feature gate is reset:
  defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.<FeatureName>, <value>)()

@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 30, 2019

a few last comments and the verify test fixup, then this LGTM. thanks for all the work

@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 30, 2019

/approve

API changes look good. needs a squash then lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented May 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, denkensk, liggitt

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

The pull request process is described 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

@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 30, 2019

/lgtm

@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 30, 2019

/retest

@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 30, 2019

/test pull-kubernetes-e2e-gce-100-performance

1 similar comment
@BenTheElder
Copy link
Copy Markdown
Member

BenTheElder commented May 30, 2019

/test pull-kubernetes-e2e-gce-100-performance

denkensk and others added 2 commits May 31, 2019 12:37
@denkensk @vllry
non-preempting-priorityclass
5c9438c 
Co-authored-by: Vallery Lancey <vallery@zeitgeistlabs.io>
Co-authored-by: Tan shanshan <tan.shanshan@zte.com.cn>
@denkensk
change preempting to PreemptionPolicy
52f3380
@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 31, 2019

Just rebase in "pkg/features/kube_features.go" @bsalamat @liggitt need lgtm again?

@vllry
Copy link
Copy Markdown
Contributor

vllry commented May 31, 2019

/lgtm

@denkensk
Copy link
Copy Markdown
Member Author

denkensk commented May 31, 2019

/retest

This was referenced Jun 10, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@cheftako cheftako Awaiting requested review from cheftako

+3 more reviewers

@yastij yastij yastij left review comments

@vllry vllry vllry left review comments

@bsalamat bsalamat bsalamat left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@liggitt liggitt

@apelisse apelisse

@yastij yastij

@vllry vllry

@bsalamat bsalamat

Labels

api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubectl area/kubelet area/test 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

API Reviews
Status: API review completed, 1.15

Milestone

v1.15

Development

Successfully merging this pull request may close these issues.

Non-preempting PriorityClass

10 participants

@denkensk @bsalamat @yastij @fejta-bot @apelisse @vllry @liggitt @timmycarr @k8s-ci-robot @BenTheElder