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

bugfix(scheduler): Preemption picks wrong victim node with higher priority pod on it after #128307 #129137

Closed
wants to merge 1 commit into from
Closed

bugfix(scheduler): Preemption picks wrong victim node with higher priority pod on it after #128307 #129137

wants to merge 1 commit into from
+7 −7

Conversation

mochizuki875
Copy link
Member

@mochizuki875 mochizuki875 commented Dec 10, 2024 •
edited
Loading

What type of PR is this?

/sig scheduling
/kind bug

What this PR does / why we need it:

As described in #129136, appropriate victim is not selected when preemption is triggered considering PDB even after #128307 has been merged.

According to here, the number of PDB violations should be counted for each node(1), and minimum highest priority victim is picked if the number of PDB violations is the same(2).

kubernetes/pkg/scheduler/framework/preemption/preemption.go

Lines 411 to 424 in 1148e5e

// pickOneNodeForPreemption chooses one node among the given nodes.
// It assumes pods in each map entry are ordered by decreasing priority.
// If the scoreFuns is not empty, It picks a node based on score scoreFuns returns.
// If the scoreFuns is empty,
// It picks a node based on the following criteria:
// 1. A node with minimum number of PDB violations.
// 2. A node with minimum highest priority victim is picked.
// 3. Ties are broken by sum of priorities of all victims.
// 4. If there are still ties, node with the minimum number of victims is picked.
// 5. If there are still ties, node with the latest start time of all highest priority victims is picked.
// 6. If there are still ties, the first such node is picked (sort of randomly).
// The 'minNodes1' and 'minNodes2' are being reused here to save the memory
// allocation and garbage collection time.
func pickOneNodeForPreemption(logger klog.Logger, nodesToVictims map[string]*extenderv1.Victims, scoreFuncs []func(node string) int64) string {

However, the number of PDB violations(numViolatingVictim) is only counted up when fits=false.
This means low priority pod which violates PDB may not be counted as numViolatingVictim.

kubernetes/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go

Lines 234 to 240 in a499fac

for _, p := range violatingVictims {
if fits, err := reprievePod(p); err != nil {
return nil, 0, framework.AsStatus(err)
} else if !fits {
numViolatingVictim++
}
}

Which issue(s) this PR fixes:

fix: #129136
related: #128307

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed a suboptimal scheduler preemption behavior where potential preemption victims were violating Pod Disruption Budgets.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.32 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.32.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Dec 10 06:08:10 UTC 2024.

@k8s-ci-robot k8s-ci-robot added 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/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 10, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 10, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mochizuki875
Once this PR has been reviewed and has the lgtm label, please assign sanposhiho for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@mochizuki875 mochizuki875 mentioned this pull request Dec 10, 2024
Open
@mochizuki875
Copy link
Member Author

close.
ref #129136 (comment)

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

@AxeZhan AxeZhan Awaiting requested review from AxeZhan

@damemi damemi Awaiting requested review from damemi

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Preemption picks wrong victim node with higher priority pod on it after #128307
2 participants
@mochizuki875 @k8s-ci-robot