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.

Sign up for GitHub

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

Jump to bottom

Add PodDisruptionBudget support in pod preemption #56178

Merged
merged 2 commits into from
Nov 23, 2017
Merged

Add PodDisruptionBudget support in pod preemption #56178

merged 2 commits into from
Nov 23, 2017

Conversation

bsalamat
Copy link
Member

What this PR does / why we need it:
This PR adds the logic to make scheduler preemption aware of PodDisruptionBudget. Preemption tries to avoid preempting pods whose PDBs are violated by preemption. If preemption does not find any other pods to preempt, it will preempt pods despite violating their PDBs.

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 #53913

Special notes for your reviewer:

Release note:

Add PodDisruptionBudget support during pod preemption

ref/ #47604

/sig scheduling

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 22, 2017
@bsalamat
Copy link
Member Author

@davidopp This is my PR to add support for PDB. Could you please add milestone?
I am going to add integration tests.

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017
@davidopp davidopp added this to the v1.9 milestone Nov 22, 2017
davidopp
davidopp reviewed Nov 22, 2017
View reviewed changes
plugin/pkg/scheduler/core/generic_scheduler.go Outdated
@@ -776,6 +809,45 @@ func nodePassesExtendersForPreemption(
return true, nil
}

// filterPodsWithPDBViolation groups the given "pods" into two groups of "violatedPos"
Copy link
Member

Choose a reason for hiding this comment

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

violatedPods

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

plugin/pkg/scheduler/core/generic_scheduler.go
func selectVictimsOnNode(
pod *v1.Pod,
meta algorithm.PredicateMetadata,
nodeInfo *schedulercache.NodeInfo,
fitPredicates map[string]algorithm.FitPredicate,
queue SchedulingQueue,
) ([]*v1.Pod, bool) {
pdbs []*policy.PodDisruptionBudget,
) ([]*v1.Pod, int, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Update the function-level comment to explain how you use PDB.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2017
bsalamat
bsalamat commented Nov 22, 2017
View reviewed changes
Copy link
Member Author

@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, @davidopp! Addressed the comments and added more integration tests. No other logic changes. PTAL.

plugin/pkg/scheduler/core/generic_scheduler.go Outdated
@@ -776,6 +809,45 @@ func nodePassesExtendersForPreemption(
return true, nil
}

// filterPodsWithPDBViolation groups the given "pods" into two groups of "violatedPos"
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

plugin/pkg/scheduler/core/generic_scheduler.go
func selectVictimsOnNode(
pod *v1.Pod,
meta algorithm.PredicateMetadata,
nodeInfo *schedulercache.NodeInfo,
fitPredicates map[string]algorithm.FitPredicate,
queue SchedulingQueue,
) ([]*v1.Pod, bool) {
pdbs []*policy.PodDisruptionBudget,
) ([]*v1.Pod, int, bool) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@bsalamat bsalamat changed the title [WIP] Add PodDisruptionBudget support during pod preemption Add PodDisruptionBudget support during pod preemption Nov 22, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2017
@bsalamat bsalamat changed the title Add PodDisruptionBudget support during pod preemption Add PodDisruptionBudget support in pod preemption Nov 22, 2017
davidopp
davidopp reviewed Nov 22, 2017
View reviewed changes
test/integration/scheduler/preemption_test.go Outdated
initPausePod(context.clientSet, &pausePodConfig{
Name: "low-pod3",
Namespace: context.ns.Name,
Priority: &lowPriority,
Copy link
Member

@davidopp davidopp Nov 22, 2017 •
edited

Choose a reason for hiding this comment

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

I'd suggest to make this &mediumPriority so it exactly matches node-1 except for violating more PDBs

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@davidopp
Copy link
Member

Looks great, just had one small comment.

@bsalamat
Copy link
Member Author

Thanks a lot, @davidopp for your prompt review. PTAL.

@davidopp
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, davidopp

Associated issue: 53913

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@bsalamat
Copy link
Member Author

/kind feature
/priority important-soon

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 22, 2017
@davidopp davidopp added status/in-progress priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed milestone/needs-attention priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 22, 2017
@davidopp
Copy link
Member

PR is LGTM and approved, just awaiting auto-merge.

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Current

@bsalamat @davidopp @k82cn @resouer

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/feature: New functionality.
Help

@bsalamat
Copy link
Member Author

I wonder why this has not auto-merged yet.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55952, 49112, 55450, 56178, 56151). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 82c8898 into kubernetes:master Nov 23, 2017
@bsalamat bsalamat deleted the pdb branch November 23, 2017 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidopp davidopp davidopp left review comments

Assignees

@resouer resouer

@davidopp davidopp

@k82cn k82cn

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

Support PodDisruptionBudget during preemption
6 participants
@bsalamat @davidopp @k8s-github-robot @resouer @k82cn @k8s-ci-robot