Preempting: do not delete the victim if it just exits in WaitingPods #100325

cwdsuzhou · 2021-03-17T06:20:43Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Do not delete the victim if it just exits in WaitingPods.

We do deleting pods to free resources. But if a pod exists in WaitingPods, it actually does not scheduled. So we just need reject it to free the resources from NodeInfo.

Which issue(s) this PR fixes:

Fixes #100235

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-03-17T06:20:50Z

@cwdsuzhou: 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.

cwdsuzhou · 2021-03-17T06:21:05Z

/assign @Huang-Wei

cwdsuzhou · 2021-03-17T06:21:18Z

/sig scheduling

lala123912 · 2021-03-17T08:41:55Z

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

+		} else {
+			if err := util.DeletePod(cs, victim); err != nil {
+				klog.ErrorS(err, "preempting pod", "pod", klog.KObj(victim))
+				return framework.AsStatus(err)


preempting ->Preempting

cwdsuzhou · 2021-03-17T08:55:53Z

/retest

cwdsuzhou · 2021-03-17T09:43:17Z

/retest

cwdsuzhou · 2021-03-17T14:02:27Z

/retest

alculquicondor · 2021-03-17T14:43:07Z

/assign @Huang-Wei

alculquicondor · 2021-03-17T14:46:00Z

Logic makes sense to me, but leaving review to Wei.

cwdsuzhou · 2021-03-18T02:03:55Z

/retest

cwdsuzhou · 2021-03-18T02:28:43Z

/retest

Huang-Wei

Thanks @cwdsuzhou . Some comments below.

Huang-Wei · 2021-03-18T02:09:50Z

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

+		} else {
+			if err := util.DeletePod(cs, victim); err != nil {


nit:

Suggested change

} else {

if err := util.DeletePod(cs, victim); err != nil {

} else if err := util.DeletePod(cs, victim); err != nil {

Huang-Wei · 2021-03-18T02:10:12Z

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

+		} else {
+			if err := util.DeletePod(cs, victim); err != nil {
+				klog.ErrorS(err, "preempting pod", "pod", klog.KObj(victim))
+				return framework.AsStatus(err)


Huang-Wei · 2021-03-18T02:25:37Z

test/integration/scheduler/framework_test.go

 	}); err != nil {
-		t.Error("Expected the waiting pod to get preempted and deleted")
+		t.Error("Expected the waiting pod to get preempted")
 	}



Can you add a step to call API server to verify the waited Pod is not deleted physically?

is there another test for non-permit case were we do verify that the pod is deleted?

we can add another low-priority regular Pod and let it be running, so that the preemptor needs to preempt two pods, then we can verify one is removed from waitingMap, and the other is deleted physically.

Huang-Wei · 2021-03-18T02:36:31Z

test/integration/scheduler/framework_test.go

+		w := false
+		permitPlugin.fh.IterateOverWaitingPods(func(wp framework.WaitingPod) { w = true })
+		return !w, nil


(Probably a bit picky) Can we change the timeout of TestPermitPlugin from 10 seconds to 30 seconds; otherwise if the preemption logic lasts more than 10 seconds, we cannot tell if the waitingPod is rejected by preemption logic or the timer.

https://github.com/kubernetes/kubernetes/blob/master/test/integration/scheduler/framework_test.go#L453
https://github.com/kubernetes/kubernetes/blob/master/test/integration/scheduler/framework_test.go#L1852

the timeout is already 30s

I see. I checked the UT...

cwdsuzhou · 2021-03-18T02:58:46Z

PR addressed

cwdsuzhou · 2021-03-18T04:04:43Z

/retest

Huang-Wei · 2021-03-18T05:34:18Z

PR addressed

And this : #100325 (comment) :)

cwdsuzhou · 2021-03-18T06:18:54Z

PR addressed

And this : #100325 (comment) :)

Have added a check

alculquicondor · 2021-03-18T13:53:31Z

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

 		if waitingPod := fh.GetWaitingPod(victim.UID); waitingPod != nil {
 			waitingPod.Reject(pluginName, "preempted")
+		} else if err := util.DeletePod(cs, victim); err != nil {
+			klog.ErrorS(err, "Preempting pod", "pod", klog.KObj(victim))


should we add the preemptor?

alculquicondor · 2021-03-18T13:55:00Z

test/integration/scheduler/framework_test.go

 	}); err != nil {
-		t.Error("Expected the waiting pod to get preempted and deleted")
+		t.Error("Expected the waiting pod to get preempted")
 	}



is there another test for non-permit case were we do verify that the pod is deleted?

Huang-Wei · 2021-03-18T18:12:27Z

test/integration/scheduler/framework_test.go

+		if _, err := getPod(testCtx.ClientSet, waitingPod.Name, waitingPod.Namespace); err != nil {
+			return false, nil
+		}


I meant to move this to L1896 as we don't need to run the API check inside this loop - only check it once.

Huang-Wei

Some nits. LGTM otherwise.

Huang-Wei · 2021-03-22T17:50:18Z

test/integration/scheduler/framework_test.go

+	wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
+		pod, err := getPod(testCtx.ClientSet, runningPod.Name, runningPod.Namespace)
+		if err != nil {
+			return false, nil
+		}
+		if len(pod.Spec.NodeName) != 0 {
+			return true, nil
+		}
+		return false, nil
+	})


nit:

Suggested change

wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {

pod, err := getPod(testCtx.ClientSet, runningPod.Name, runningPod.Namespace)

if err != nil {

return false, nil

}

if len(pod.Spec.NodeName) != 0 {

return true, nil

}

return false, nil

})

wait.Poll(100*time.Millisecond, 30*time.Second, podScheduled(testCtx.ClientSet, runningPod.Name, runningPod.Namespace))

Huang-Wei · 2021-03-22T17:53:18Z

test/integration/scheduler/framework_test.go

-		t.Error("Expected the waiting pod to get preempted and deleted")
+		t.Error("Expected the waiting pod to get preempted")
+	}
+	// check waitingPod not deleted


Suggested change

// check waitingPod not deleted

// Expect the waitingPod to be still present.

Huang-Wei · 2021-03-22T17:53:51Z

test/integration/scheduler/framework_test.go

+	if _, err := getPod(testCtx.ClientSet, waitingPod.Name, waitingPod.Namespace); err != nil {
+		t.Error("Get waiting pod in waiting pod failed.")
+	}
+	// check runningPod deleted


Suggested change

// check runningPod deleted

// Expect the runningPod to be deleted physically.

cwdsuzhou · 2021-03-23T06:38:12Z

Done, thanks

Huang-Wei · 2021-03-23T17:13:48Z

Thanks @cwdsuzhou !

/lgtm
/approve

k8s-ci-robot · 2021-03-23T17:14:12Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]
~~test/integration/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

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 Mar 17, 2021

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Mar 17, 2021

k8s-ci-robot assigned Huang-Wei Mar 17, 2021

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Mar 17, 2021

k8s-ci-robot requested review from alculquicondor and hex108 March 17, 2021 06:21

lala123912 reviewed Mar 17, 2021

View reviewed changes

cwdsuzhou force-pushed the donot_delete_waitingpod branch from 629c046 to 4a8852a Compare March 17, 2021 13:36

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 17, 2021

cwdsuzhou force-pushed the donot_delete_waitingpod branch from 4a8852a to 19295ca Compare March 17, 2021 13:39

Huang-Wei reviewed Mar 18, 2021

View reviewed changes

cwdsuzhou force-pushed the donot_delete_waitingpod branch from 19295ca to 68fc0f6 Compare March 18, 2021 02:58

alculquicondor reviewed Mar 18, 2021

View reviewed changes

Huang-Wei reviewed Mar 18, 2021

View reviewed changes

cwdsuzhou force-pushed the donot_delete_waitingpod branch from 68fc0f6 to 9a05872 Compare March 21, 2021 03:59

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 21, 2021

cwdsuzhou force-pushed the donot_delete_waitingpod branch 5 times, most recently from 4f52cd6 to f780d03 Compare March 21, 2021 08:54

Huang-Wei reviewed Mar 22, 2021

View reviewed changes

Preempting: do not delete the victim if it just exits in WaitingPods

850759e

cwdsuzhou force-pushed the donot_delete_waitingpod branch from f780d03 to 850759e Compare March 23, 2021 06:38

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

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

k8s-ci-robot merged commit 4b94216 into kubernetes:master Apr 9, 2021

k8s-ci-robot added this to the v1.22 milestone Apr 9, 2021

cwdsuzhou deleted the donot_delete_waitingpod branch April 15, 2021 07:36

Huang-Wei mentioned this pull request Jul 19, 2021

Rejection of a waiting pod in scheduler may cause delay of its rescheduling #96478

Closed

	} else {
	if err := util.DeletePod(cs, victim); err != nil {
	} else if err := util.DeletePod(cs, victim); err != nil {

	// check waitingPod not deleted
	// Expect the waitingPod to be still present.

	// check runningPod deleted
	// Expect the runningPod to be deleted physically.

Preempting: do not delete the victim if it just exits in WaitingPods #100325

Preempting: do not delete the victim if it just exits in WaitingPods #100325

Conversation

cwdsuzhou commented Mar 17, 2021

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 Mar 17, 2021

cwdsuzhou commented Mar 17, 2021

cwdsuzhou commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwdsuzhou commented Mar 17, 2021

cwdsuzhou commented Mar 17, 2021

cwdsuzhou commented Mar 17, 2021

alculquicondor commented Mar 17, 2021

alculquicondor commented Mar 17, 2021

cwdsuzhou commented Mar 18, 2021

cwdsuzhou commented Mar 18, 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

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

cwdsuzhou commented Mar 18, 2021

cwdsuzhou commented Mar 18, 2021

Huang-Wei commented Mar 18, 2021

cwdsuzhou commented Mar 18, 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

Choose a reason for hiding this comment

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

cwdsuzhou commented Mar 23, 2021

Huang-Wei commented Mar 23, 2021

k8s-ci-robot commented Mar 23, 2021