Prevent scheduler crashing in default preemption plugin #101560

yuanchen8911 · 2021-04-28T05:33:29Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

FindCandidates or CallExtenders in defaultpreemption plugin can return victim without pods (e.g., because of issues in Filter plugins or extenders). As a result, the scheduler will panic and crash when accessing an non-existing pod. This PR adds additional checks to prevent it from crashing the scheduler.

Which issue(s) this PR fixes:

Fixes #101548

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2021-04-28T05:33:36Z

@yuanchen8911: 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/test-infra repository.

yuanchen8911 · 2021-04-28T05:34:19Z

/cc @Huang-Wei @ahg-g

yuanchen8911 · 2021-04-28T05:34:27Z

/sig scheduling

Huang-Wei

As mentioned in #101548 (comment), I'd like to understand the root cause, and then come up with all defensive checking at once in this PR.

Huang-Wei · 2021-04-28T22:28:10Z

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

+	var (
+		victims         *extenderv1.Victims
+		found           bool
+		latestStartTime *metav1.Time
+	)
+	if victims, found = nodesToVictims[minNodes2[0]]; found {
+		latestStartTime = util.GetEarliestPodStartTime(victims)
+	}


This is neater:

var latestStartTime *metav1.Time if victims, found := nodesToVictims[minNodes2[0]]; found { latestStartTime = util.GetEarliestPodStartTime(victims) }

yuanchen8911 · 2021-04-30T03:12:07Z

/retest

yuanchen8911 · 2021-05-01T19:40:19Z

Note that there are test cases that return candidates with empty victims. It makes the simple change if fits && len(pods) ==0 in dryRunPreemption fail the tests.

--- FAIL: TestDryRunPreemption (0.11s)
    --- FAIL: TestDryRunPreemption/a_pod_that_fits_with_no_preemption (0.00s)
        default_preemption_test.go:1059: cycle 0: unexpected candidates (-want, +got):   []defaultpreemption.Candidate{
            - 	&defaultpreemption.candidate{victims: &v1.Victims{}, name: "node1"},
            - 	&defaultpreemption.candidate{victims: &v1.Victims{}, name: "node2"},
              }
    --- FAIL: TestDryRunPreemption/a_pod_that_fits_on_one_node_with_no_preemption (0.00s)
        default_preemption_test.go:1059: cycle 0: unexpected candidates (-want, +got):   []defaultpreemption.Candidate{
            - 	&defaultpreemption.candidate{victims: &v1.Victims{}, name: "node1"},
              }

yuanchen8911 · 2021-05-01T20:24:53Z

/retest

yuanchen8911 · 2021-05-02T17:21:43Z

@Huang-Wei updated the description and PR. PTAL!

Huang-Wei · 2021-05-03T20:18:51Z

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

+			var victims extenderv1.Victims
+			// do not create victims without pods
+			if len(pods) != 0 {
+				victims = extenderv1.Victims{
+					Pods:             pods,
+					NumPDBViolations: int64(numPDBViolations),
+				}


We should treat len(pods) == 0 as an internal error, right? Why not just do if status.IsSuccess() && len(pods) != 0 in L338?

That's my original version but the unit test failed as there are test cases that return empty victims with fit= true. Please see my comment. #101560 (comment) Shall I update the unit test file instead?

yes, please update those tests.

should status.IsSuccess() && len(pods) == 0 be considered an error or just ignored?

let's craft it as an internal error.

Huang-Wei · 2021-05-03T20:21:25Z

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

@@ -404,8 +408,14 @@ func CallExtenders(extenders []framework.Extender, pod *v1.Pod, nodeLister frame

 	var newCandidates []Candidate
 	for nodeName := range victimsMap {
+		// check if victims.Pods is empty
+		victims := victimsMap[nodeName]
+		if len(victims.Pods) == 0 {


I'd prefer to run the check around L391: if an extender returns illegal result and extender.IsIgnorable() == false, return it as an error.

I'd prefer to run the check around L391: if an extender returns illegal result and extender.IsIgnorable() == false, return it as an error.

Check the validness of nodeNameToVictims returned by PrecessPreemption before L399?

for nodeName := range nodeNameToVictims { // check if victims.Pods is empty victims := victimsMap[nodeName] if len(victims.Pods) == 0 { delete(victims, nodeName) } }

before L399, like this:

for nodeName, victims := range nodeNameToVictims { if victims != nil && len(victims.Pods) != 0 { continue } if extender.IsIgnorable() { delete(nodeNameToVictims, nodeName) // klog.... continue } else { return nil, framework.AsStatus(/* build an internal err */) } }

yuanchen8911 · 2021-05-04T01:22:03Z

Updated the PR as suggested, please review it. @Huang-Wei Thanks!

Huang-Wei

Some nits. LGTM otherwise.

Huang-Wei · 2021-05-04T01:50:26Z

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

+			return
+		}
+		if status.IsSuccess() && len(pods) == 0 {
+			status = framework.NewStatus(framework.Unschedulable, fmt.Sprintf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))


Suggested change

status = framework.NewStatus(framework.Unschedulable, fmt.Sprintf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))

status = framework.AsStatus(fmt.Errorf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))

Huang-Wei · 2021-05-04T01:53:02Z

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

@@ -391,6 +394,18 @@ func CallExtenders(extenders []framework.Extender, pod *v1.Pod, nodeLister frame
 			}
 			return nil, framework.AsStatus(err)
 		}
+		// Check if the returned victims are valid.
+		for nodeName, victims := range nodeNameToVictims {
+			if victims != nil && len(victims.Pods) == 0 {


Perfer if victims == nil || len(victims.Pods) == 0 so that we can catch nil victims.

Huang-Wei · 2021-05-04T05:21:02Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go

+			if s == nil || len(s.Name()) == 0 {
+				return
+			}


You can remove the first sub-test "No node needs preemption".

deleted the test case.

yuanchen8911 · 2021-05-04T13:58:29Z

@ahg-g could you review it when you get a chance? Thanks.

xujyan · 2021-05-04T20:03:38Z

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

+			return
+		}
+		if status.IsSuccess() && len(pods) == 0 {
+			status = framework.AsStatus(fmt.Errorf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))


Should the error msg be more explicit: should not happen: the pod fits with no victim pods? Same below.

Does the error status (set in L358) indicate "it should not happen"?

yuanchen8911 · 2021-05-04T20:12:26Z

/retest

Huang-Wei · 2021-05-04T21:58:41Z

/approve
leaving /lgtm to @ahg-g

k8s-ci-robot · 2021-05-04T21:59:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, yuanchen8911

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [Huang-Wei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahg-g · 2021-05-04T14:53:30Z

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

+			if victims == nil || len(victims.Pods) == 0 {
+				if extender.IsIgnorable() {
+					delete(nodeNameToVictims, nodeName)
+					klog.Warningf("Ignoring victims without pods on node %q returned by extender", nodeName)


warnings aren't recommended anymore, either error or info. In this case info probably fits better.

warnings aren't recommended anymore, either error or info. In this case info probably fits better.

Changed it to InfoS.

ahg-g · 2021-05-05T00:19:16Z

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

+			return
+		}
+		if status.IsSuccess() && len(pods) == 0 {
+			status = framework.AsStatus(fmt.Errorf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))


right, be more explicit, like "expected at least one victim on node $q"

right, be more explicit, like "expected at least one victim on node $q"

Updated the error msg. as suggested.

Huang-Wei · 2021-05-05T16:31:31Z

/release-note-none

ahg-g · 2021-05-05T16:36:48Z

/lgtm

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 Apr 28, 2021

k8s-ci-robot requested review from liu-cong and wgliang April 28, 2021 05:34

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 28, 2021

k8s-ci-robot requested review from ahg-g and Huang-Wei April 28, 2021 05:34

yuanchen8911 mentioned this pull request Apr 28, 2021

kube-scheduler crashes and restarts with panics in DefaultPreemption plugin #101548

Closed

yuanchen8911 changed the title ~~Add additonal checks to prevent scheduler crashing because of bugs in…~~ Add checks to prevent scheduler crashing caused by custom plugins Apr 28, 2021

Huang-Wei reviewed Apr 28, 2021

View reviewed changes

yuanchen8911 force-pushed the master branch from 3cc6b4e to c054e3a Compare April 29, 2021 17:38

yuanchen8911 force-pushed the master branch from c054e3a to 381368e Compare May 1, 2021 18:33

yuanchen8911 force-pushed the master branch from 381368e to 3d0eebd Compare May 1, 2021 19:42

yuanchen8911 requested a review from Huang-Wei May 1, 2021 19:43

yuanchen8911 changed the title ~~Add checks to prevent scheduler crashing caused by custom plugins~~ Add checks to prevent scheduler crashing in defaultpreemption plugin May 2, 2021

Huang-Wei reviewed May 3, 2021

View reviewed changes

yuanchen8911 requested a review from Huang-Wei May 3, 2021 20:45

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 4, 2021

Huang-Wei reviewed May 4, 2021

View reviewed changes

yuanchen8911 force-pushed the master branch from 110d2f0 to 785b666 Compare May 4, 2021 03:27

yuanchen8911 requested a review from Huang-Wei May 4, 2021 03:27

yuanchen8911 changed the title ~~Add checks to prevent scheduler crashing in defaultpreemption plugin~~ Prevent scheduler crashing in default preemption plugin May 4, 2021

Huang-Wei reviewed May 4, 2021

View reviewed changes

yuanchen8911 force-pushed the master branch 2 times, most recently from 84086b9 to 9c22e00 Compare May 4, 2021 19:10

xujyan reviewed May 4, 2021

View reviewed changes

yuanchen8911 requested a review from Huang-Wei May 4, 2021 20:12

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2021

ahg-g reviewed May 5, 2021

View reviewed changes

yuanchen8911 force-pushed the master branch from 9c22e00 to 5a0916d Compare May 5, 2021 02:27

Prevent scheduler crashing in default preemption

fce28db

yuanchen8911 force-pushed the master branch from 5a0916d to fce28db Compare May 5, 2021 02:30

yuanchen8911 requested a review from ahg-g May 5, 2021 03:38

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 5, 2021

k8s-ci-robot assigned ahg-g May 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021

k8s-ci-robot merged commit aab19d4 into kubernetes:master May 5, 2021

k8s-ci-robot added this to the v1.22 milestone May 5, 2021

machine424 mentioned this pull request Jul 31, 2022

Kube scheduler panic topolvm/topolvm#544

Closed

	status = framework.NewStatus(framework.Unschedulable, fmt.Sprintf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))
	status = framework.AsStatus(fmt.Errorf("invalid victims on node %q returned by selectVictimsOnNode", nodeInfoCopy.Node().Name))

Prevent scheduler crashing in default preemption plugin #101560

Prevent scheduler crashing in default preemption plugin #101560

Conversation

yuanchen8911 commented Apr 28, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Apr 28, 2021

yuanchen8911 commented Apr 28, 2021

yuanchen8911 commented Apr 28, 2021

Huang-Wei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanchen8911 commented Apr 30, 2021

yuanchen8911 commented May 1, 2021

yuanchen8911 commented May 1, 2021

yuanchen8911 commented May 2, 2021

Choose a reason for hiding this comment

yuanchen8911 May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanchen8911 commented May 4, 2021

Huang-Wei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanchen8911 commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanchen8911 commented May 4, 2021

Huang-Wei commented May 4, 2021

k8s-ci-robot commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented May 5, 2021

ahg-g commented May 5, 2021

yuanchen8911 commented Apr 28, 2021 •

edited

Loading

yuanchen8911 May 3, 2021 •

edited

Loading