Repository: kubernetes/kubernetes (language: go)

Issue:
Don't clear nominatedNodeName in preemption if no candidates are found, because it is cleared later anyway
Now, if we want to try a preemption of a pod and no candidates are found, `nominatedNodeName` is cleared for a pod in the preemption code:

https://github.com/kubernetes/kubernetes/blob/88dfcb225d41326113990e87b11137641c121a32/pkg/scheduler/framework/preemption/preemption.go#L316-L324

But, because we return `framework.NewPostFilterResultWithNominatedNode("")` anyway when there are no candidates:

https://github.com/kubernetes/kubernetes/blob/88dfcb225d41326113990e87b11137641c121a32/pkg/scheduler/framework/preemption/preemption.go#L263-L276

The nominatedNodeName will be cleared in the `handleSchedulingFailure` in the scheduling cycle:

https://github.com/kubernetes/kubernetes/blob/88dfcb225d41326113990e87b11137641c121a32/pkg/scheduler/schedule_one.go#L1088-L1096

So, we can remove the `ClearNominatedNodeName` in `findCandidates`, because it's a duplicated effort.

Reducing number of API calls could improve performance.

/kind feature
/sig scheduling


Discussion / hints:
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.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
/cc @dom4ha @sanposhiho 
+1
Yes, definitely. The scheduling part should not have any api calls and the ones in `handleSchedulingFailure` we will make async.

Fix patch:
diff --git a/pkg/scheduler/framework/preemption/preemption.go b/pkg/scheduler/framework/preemption/preemption.go
index d23c18595ccd4..edaaad94c08c7 100644
--- a/pkg/scheduler/framework/preemption/preemption.go
+++ b/pkg/scheduler/framework/preemption/preemption.go
@@ -315,11 +315,6 @@ func (ev *Evaluator) findCandidates(ctx context.Context, state *framework.CycleS
 	}
 	if len(potentialNodes) == 0 {
 		logger.V(3).Info("Preemption will not help schedule pod on any node", "pod", klog.KObj(pod))
-		// In this case, we should clean-up any existing nominated node name of the pod.
-		if err := util.ClearNominatedNodeName(ctx, ev.Handler.ClientSet(), pod); err != nil {
-			logger.Error(err, "Could not clear the nominatedNodeName field of pod", "pod", klog.KObj(pod))
-			// We do not return as this error is not critical.
-		}
 		return nil, framework.NewDefaultNodeToStatus(), nil
 	}
 


Test patch:
diff --git a/test/integration/scheduler/preemption/preemption_test.go b/test/integration/scheduler/preemption/preemption_test.go
index a1b6acb7458a1..0b0bb0fd38e68 100644
--- a/test/integration/scheduler/preemption/preemption_test.go
+++ b/test/integration/scheduler/preemption/preemption_test.go
@@ -1526,14 +1526,16 @@ func newAlwaysFail(_ context.Context, _ runtime.Object, _ framework.Handle) (fra
 // properly in different scenarios.
 func TestNominatedNodeCleanUp(t *testing.T) {
 	tests := []struct {
-		name         string
+		name string
+		// Initial nodes to simulate special conditions.
+		initNodes    []*v1.Node
 		nodeCapacity map[v1.ResourceName]string
 		// A slice of pods to be created in batch.
 		podsToCreate [][]*v1.Pod
 		// Each postCheck function is run after each batch of pods' creation.
 		postChecks []func(ctx context.Context, cs clientset.Interface, pod *v1.Pod) error
 		// Delete the fake node or not. Optional.
-		deleteNode bool
+		deleteFakeNode bool
 		// Pods to be deleted. Optional.
 		podNamesToDelete []string
 
@@ -1600,8 +1602,39 @@ func TestNominatedNodeCleanUp(t *testing.T) {
 				testutils.WaitForPodToSchedule,
 				testutils.WaitForNominatedNodeName,
 			},
-			// Delete the node to simulate an ErrNoNodesAvailable error.
-			deleteNode:       true,
+			// Delete the fake node to simulate an ErrNoNodesAvailable error.
+			deleteFakeNode:   true,
+			podNamesToDelete: []string{"low"},
+		},
+		{
+			name: "mid-priority pod preempts low-priority pod at the beginning, but could not find candidates after the nominated node is deleted",
+			// Create a taint node to simulate the `UnschedulableAndUnresolvable` condition in `findCandidates` during preemption.
+			initNodes: []*v1.Node{
+				st.MakeNode().Name("taint-node").Capacity(map[v1.ResourceName]string{v1.ResourceCPU: "1"}).
+					Taints([]v1.Taint{
+						{
+							Key:    "taint-node",
+							Value:  "true",
+							Effect: v1.TaintEffectNoSchedule,
+						},
+					}).
+					Obj(),
+			},
+			nodeCapacity: map[v1.ResourceName]string{v1.ResourceCPU: "1"},
+			podsToCreate: [][]*v1.Pod{
+				{
+					st.MakePod().Name("low").Priority(lowPriority).Req(map[v1.ResourceName]string{v1.ResourceCPU: "1"}).Obj(),
+				},
+				{
+					st.MakePod().Name("medium").Priority(mediumPriority).Req(map[v1.ResourceName]string{v1.ResourceCPU: "1"}).Obj(),
+				},
+			},
+			postChecks: []func(ctx context.Context, cs clientset.Interface, pod *v1.Pod) error{
+				testutils.WaitForPodToSchedule,
+				testutils.WaitForNominatedNodeName,
+			},
+			// Delete the fake node to trigger the `UnschedulableAndUnresolvable` condition in `findCandidates`.
+			deleteFakeNode:   true,
 			podNamesToDelete: []string{"low"},
 		},
 		{
@@ -1648,6 +1681,12 @@ func TestNominatedNodeCleanUp(t *testing.T) {
 				)
 
 				cs, ns := testCtx.ClientSet, testCtx.NS.Name
+				for _, node := range tt.initNodes {
+					if _, err := createNode(cs, node); err != nil {
+						t.Fatalf("Error creating initial node %v: %v", node.Name, err)
+					}
+				}
+
 				// Create a node with the specified capacity.
 				nodeName := "fake-node"
 				if _, err := createNode(cs, st.MakeNode().Name(nodeName).Capacity(tt.nodeCapacity).Obj()); err != nil {
@@ -1672,8 +1711,8 @@ func TestNominatedNodeCleanUp(t *testing.T) {
 					}
 				}
 
-				// Delete the node if necessary.
-				if tt.deleteNode {
+				// Delete the fake node if necessary.
+				if tt.deleteFakeNode {
 					if err := cs.CoreV1().Nodes().Delete(testCtx.Ctx, nodeName, *metav1.NewDeleteOptions(0)); err != nil {
 						t.Fatalf("Node %v cannot be deleted: %v", nodeName, err)
 					}