Merge pull request #109245 from alculquicondor/fix-nominate

Fix: abort nominating a pod that was already scheduled to a node

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -870,9 +870,14 @@ func (npm *nominator) add(pi *framework.PodInfo, nominatingInfo *framework.Nomin
 	}
 
 	if npm.podLister != nil {
-		// If the pod is not alive, don't contain it.
+		// If the pod was removed or if it was already scheduled, don't nominate it.
-		if _, err := npm.podLister.Pods(pi.Pod.Namespace).Get(pi.Pod.Name); err != nil {
+		updatedPod, err := npm.podLister.Pods(pi.Pod.Namespace).Get(pi.Pod.Name)
-			klog.V(4).InfoS("Pod doesn't exist in podLister, aborting adding it to the nominator", "pod", klog.KObj(pi.Pod))
+		if err != nil {
+			klog.V(4).InfoS("Pod doesn't exist in podLister, aborted adding it to the nominator", "pod", klog.KObj(pi.Pod))
+			return
+		}
+		if updatedPod.Spec.NodeName != "" {
+			klog.V(4).InfoS("Pod is already scheduled to a node, aborted adding it to the nominator", "pod", klog.KObj(pi.Pod), "node", updatedPod.Spec.NodeName)
 			return
 		}
 	}

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -54,11 +54,11 @@ var (
 	TestEvent    = framework.ClusterEvent{Resource: "test"}
 	NodeAllEvent = framework.ClusterEvent{Resource: framework.Node, ActionType: framework.All}
 	EmptyEvent   = framework.ClusterEvent{}
-)
 
-var lowPriority, midPriority, highPriority = int32(0), int32(100), int32(1000)
+	lowPriority, midPriority, highPriority = int32(0), int32(100), int32(1000)
-var mediumPriority = (lowPriority + highPriority) / 2
+	mediumPriority                         = (lowPriority + highPriority) / 2
-var highPriorityPodInfo, highPriNominatedPodInfo, medPriorityPodInfo, unschedulablePodInfo = framework.NewPodInfo(&v1.Pod{
+
+	highPriorityPodInfo = framework.NewPodInfo(&v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "hpp",
 			Namespace: "ns1",
@@ -67,8 +67,8 @@ var highPriorityPodInfo, highPriNominatedPodInfo, medPriorityPodInfo, unschedula
 		Spec: v1.PodSpec{
 			Priority: &highPriority,
 		},
-}),
+	})
-	framework.NewPodInfo(&v1.Pod{
+	highPriNominatedPodInfo = framework.NewPodInfo(&v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "hpp",
 			Namespace: "ns1",
@@ -80,8 +80,8 @@ var highPriorityPodInfo, highPriNominatedPodInfo, medPriorityPodInfo, unschedula
 		Status: v1.PodStatus{
 			NominatedNodeName: "node1",
 		},
-	}),
+	})
-	framework.NewPodInfo(&v1.Pod{
+	medPriorityPodInfo = framework.NewPodInfo(&v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "mpp",
 			Namespace: "ns2",
@@ -96,8 +96,8 @@ var highPriorityPodInfo, highPriNominatedPodInfo, medPriorityPodInfo, unschedula
 		Status: v1.PodStatus{
 			NominatedNodeName: "node1",
 		},
-	}),
+	})
-	framework.NewPodInfo(&v1.Pod{
+	unschedulablePodInfo = framework.NewPodInfo(&v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "up",
 			Namespace: "ns1",
@@ -120,6 +120,24 @@ var highPriorityPodInfo, highPriNominatedPodInfo, medPriorityPodInfo, unschedula
 			NominatedNodeName: "node1",
 		},
 	})
+	nonExistentPodInfo = framework.NewPodInfo(&v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "ne",
+			Namespace: "ns1",
+			UID:       "nens1",
+		},
+	})
+	scheduledPodInfo = framework.NewPodInfo(&v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "sp",
+			Namespace: "ns1",
+			UID:       "spns1",
+		},
+		Spec: v1.PodSpec{
+			NodeName: "foo",
+		},
+	})
+)
 
 func getUnschedulablePod(p *PriorityQueue, pod *v1.Pod) *v1.Pod {
 	pInfo := p.unschedulablePods.get(pod)
@@ -837,7 +855,7 @@ func TestPriorityQueue_PendingPods(t *testing.T) {
 }
 
 func TestPriorityQueue_UpdateNominatedPodForNode(t *testing.T) {
-	objs := []runtime.Object{medPriorityPodInfo.Pod, unschedulablePodInfo.Pod, highPriorityPodInfo.Pod}
+	objs := []runtime.Object{medPriorityPodInfo.Pod, unschedulablePodInfo.Pod, highPriorityPodInfo.Pod, scheduledPodInfo.Pod}
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	q := NewTestQueueWithObjects(ctx, newDefaultQueueSort(), objs)
@@ -892,6 +910,21 @@ func TestPriorityQueue_UpdateNominatedPodForNode(t *testing.T) {
 		t.Errorf("Unexpected diff after updating pods (-want, +got):\n%s", diff)
 	}
 
+	// Attempt to nominate a pod that was deleted from the informer cache.
+	// Nothing should change.
+	q.AddNominatedPod(nonExistentPodInfo, &framework.NominatingInfo{NominatingMode: framework.ModeOverride, NominatedNodeName: "node1"})
+	if diff := cmp.Diff(q.PodNominator, expectedNominatedPods, cmp.AllowUnexported(nominator{}), cmpopts.IgnoreFields(nominator{}, "podLister", "RWMutex")); diff != "" {
+		t.Errorf("Unexpected diff after nominating a deleted pod (-want, +got):\n%s", diff)
+	}
+	// Attempt to nominate a pod that was already scheduled in the informer cache.
+	// Nothing should change.
+	scheduledPodCopy := scheduledPodInfo.Pod.DeepCopy()
+	scheduledPodInfo.Pod.Spec.NodeName = ""
+	q.AddNominatedPod(framework.NewPodInfo(scheduledPodCopy), &framework.NominatingInfo{NominatingMode: framework.ModeOverride, NominatedNodeName: "node1"})
+	if diff := cmp.Diff(q.PodNominator, expectedNominatedPods, cmp.AllowUnexported(nominator{}), cmpopts.IgnoreFields(nominator{}, "podLister", "RWMutex")); diff != "" {
+		t.Errorf("Unexpected diff after nominating a scheduled pod (-want, +got):\n%s", diff)
+	}
+
 	// Delete a nominated pod that doesn't have nominatedNodeName in the pod
 	// object. It should be deleted.
 	q.DeleteNominatedPodIfExists(highPriorityPodInfo.Pod)