Skip to content

Separate pod priority from preemption#62243

Merged
k8s-github-robot merged 2 commits intokubernetes:masterfrom
resouer:fix-62068
Apr 19, 2018
Merged

Separate pod priority from preemption#62243
k8s-github-robot merged 2 commits intokubernetes:masterfrom
resouer:fix-62068

Conversation

@resouer
Copy link
Copy Markdown
Contributor

@resouer resouer commented Apr 8, 2018 •
edited
Loading

What this PR does / why we need it:
Users request to split priority and preemption feature gate so they can use priority separately.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62068

Special notes for your reviewer:

I kept use ENABLE_POD_PRIORITY as ENV name for gce cluster scripts for backward compatibility reason. Please let me know if other approach is preffered.

This is a potential break change as existing clusters will be affected, we may need to include this in 1.11 maybe?

TODO: update this doc https://kubernetes.io/docs/concepts/configuration/pod-priority-preemption/

[Update] Usage: in config file for scheduler:

apiVersion: componentconfig/v1alpha1
kind: KubeSchedulerConfiguration
...
disablePreemption: true

Release note:

Split PodPriority and PodPreemption feature gate

@resouer resouer requested a review from bsalamat April 8, 2018 03:26
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 8, 2018
@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 8, 2018
@CaoShuFeng
Copy link
Copy Markdown
Contributor

CaoShuFeng commented Apr 10, 2018 •
edited
Loading

This is a potential break change as existing clusters will be affected, we may need to include this in 1.11 maybe?

What about we do not introduce a new featrue gate, but add a new argument to kube-controller-manager kube-scheduler, to disable preemption?
like

--disable-pod-preemption

bsalamat
bsalamat reviewed Apr 10, 2018
View reviewed changes
pkg/features/kube_features.go Outdated
Copy link
Copy Markdown
Member

@bsalamat bsalamat Apr 10, 2018

Choose a reason for hiding this comment

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

Enabling preemption without enabling priority is wrong configuration. I think we should check and throw error if it happens. I don't know if we have any standard location in our codebase to check for such dependencies of feature gates.

Copy link
Copy Markdown
Contributor Author

@resouer resouer Apr 11, 2018

Choose a reason for hiding this comment

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

Thanks, I will make it a scheduler configuration.

@bsalamat
Copy link
Copy Markdown
Member

bsalamat commented Apr 10, 2018

What about we do not introduce a new featrue gate, but add a new argument to kube-controller-manager, to disable preemption?

I like this idea. We can then consider preemption enabled only when pod priority is enabled and preemption is not explicitly disabled. It prevents bad configurations where priority is disabled and preemption is enabled.

@resouer
Copy link
Copy Markdown
Contributor Author

resouer commented Apr 10, 2018 •
edited
Loading

What about we do not introduce a new feature gate, but add a new argument to kube-controller-manager, to disable preemption?

Sorry, but ... why kube-controller-manager? shouldn't this be part of scheduler configure?

@CaoShuFeng
Copy link
Copy Markdown
Contributor

CaoShuFeng commented Apr 11, 2018

Sorry, but ... why kube-controller-manager? shouldn't this be part of scheduler configure?

kube-scheduler, I wrote it incorrectly. 😄

@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 13, 2018
@bsalamat
Copy link
Copy Markdown
Member

bsalamat commented Apr 13, 2018

Thanks, Harry. Looks generally good, but could you please add a test for the case that preemption is disabled?

@resouer
Copy link
Copy Markdown
Contributor Author

resouer commented Apr 13, 2018

@bsalamat Thanks for review, will do.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 15, 2018
@resouer
Copy link
Copy Markdown
Contributor Author

resouer commented Apr 15, 2018

/test pull-kubernetes-e2e-kops-aws

bsalamat
bsalamat reviewed Apr 16, 2018
View reviewed changes
test/integration/scheduler/preemption_test.go Outdated
Copy link
Copy Markdown
Member

@bsalamat bsalamat Apr 16, 2018

Choose a reason for hiding this comment

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

Please remove lines 337 to 343. We don't need node labels here.

test/integration/scheduler/preemption_test.go Outdated
Copy link
Copy Markdown
Member

@bsalamat bsalamat Apr 16, 2018

Choose a reason for hiding this comment

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

This causes the test to take a long time (30 seconds for each pod) before succeeding and has the potential of timing out when there are more pods in the test. Please use waitForPodUnschedulable to check that preemptor is marked unschedulable and then you can use waitForNominatedNodeName on the preemptor to ensure that its NominatedNodeName is never set. I'd suggest creating a variation of waitForNominatedNodeName that takes a timeout. Then you can use the new variation and wait only several seconds to ensure that the nominated node name is not set.

Copy link
Copy Markdown
Contributor Author

@resouer resouer Apr 17, 2018

Choose a reason for hiding this comment

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

Thanks for pointing out. Just fixed.

@resouer resouer force-pushed the fix-62068 branch 2 times, most recently from 46adcb0 to 65491bf Compare April 17, 2018 21:19
bsalamat
bsalamat reviewed Apr 18, 2018
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.

Thanks, @resouer! Looks good. Just a minor comment.

test/integration/scheduler/util.go Outdated
Copy link
Copy Markdown
Member

@bsalamat bsalamat Apr 18, 2018

Choose a reason for hiding this comment

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

s/create/creates/
Please remove "with given informer factory, custom name and pod informer" from the comment. They don't add any value to the comment and need to be updated when function args change.

bsalamat
bsalamat reviewed Apr 18, 2018
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

Thanks, @resouer!

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

resouer commented Apr 18, 2018

/assign @timothysc

for test approval

@resouer
Copy link
Copy Markdown
Contributor Author

resouer commented Apr 18, 2018

/assign @mikedanese

For component config approval

@resouer resouer changed the title Separate pod priority from preemption in feature gate Separate pod priority from preemption Apr 19, 2018
@mikedanese
Copy link
Copy Markdown
Member

mikedanese commented Apr 19, 2018

/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Apr 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, mikedanese, resouer

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

@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 19, 2018
@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Apr 19, 2018

Automatic merge from submit-queue (batch tested with PRs 59592, 62308, 62523, 62635, 62243). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 1e39d68 into kubernetes:master Apr 19, 2018
@resouer resouer deleted the fix-62068 branch April 19, 2018 21:51
@bsalamat bsalamat added this to the v1.11 milestone Apr 19, 2018
@bsalamat bsalamat added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 19, 2018
@resouer resouer mentioned this pull request Apr 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aveshagarwal aveshagarwal Awaiting requested review from aveshagarwal

@bowei bowei Awaiting requested review from bowei

@gmarek gmarek Awaiting requested review from gmarek

@jbeda jbeda Awaiting requested review from jbeda

@ravisantoshgudimetla ravisantoshgudimetla Awaiting requested review from ravisantoshgudimetla

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@vishh vishh Awaiting requested review from vishh

1 more reviewer

@bsalamat bsalamat bsalamat left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@timothysc timothysc

@mikedanese mikedanese

@bsalamat bsalamat

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/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. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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

v1.11

Development

Successfully merging this pull request may close these issues.

Priority Scheduling w/o Preemption

7 participants

@resouer @CaoShuFeng @bsalamat @mikedanese @k8s-ci-robot @k8s-github-robot @timothysc