From 429457e184c10196ed2c0bbd8184e67b67ff50b3 Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Fri, 1 Apr 2022 15:21:10 -0400 Subject: [PATCH] Fix: abort nominating a pod that was already scheduled to a node Change-Id: Iacb8530769e7a93e3bc8384cf51d7a8fd9a192e1 --- .../internal/queue/scheduling_queue.go | 11 ++- .../internal/queue/scheduling_queue_test.go | 71 ++++++++++++++----- 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/pkg/scheduler/internal/queue/scheduling_queue.go b/pkg/scheduler/internal/queue/scheduling_queue.go index 0557dcb3d95..957b27b93c3 100644 --- a/pkg/scheduler/internal/queue/scheduling_queue.go +++ b/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 _, err := npm.podLister.Pods(pi.Pod.Namespace).Get(pi.Pod.Name); err != nil { - klog.V(4).InfoS("Pod doesn't exist in podLister, aborting adding it to the nominator", "pod", klog.KObj(pi.Pod)) + // If the pod was removed or if it was already scheduled, don't nominate it. + updatedPod, err := npm.podLister.Pods(pi.Pod.Namespace).Get(pi.Pod.Name) + 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 } } diff --git a/pkg/scheduler/internal/queue/scheduling_queue_test.go b/pkg/scheduler/internal/queue/scheduling_queue_test.go index 71d7b6612ee..09ca4b281fa 100644 --- a/pkg/scheduler/internal/queue/scheduling_queue_test.go +++ b/pkg/scheduler/internal/queue/scheduling_queue_test.go @@ -54,21 +54,21 @@ 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) -var mediumPriority = (lowPriority + highPriority) / 2 -var highPriorityPodInfo, highPriNominatedPodInfo, medPriorityPodInfo, unschedulablePodInfo = framework.NewPodInfo(&v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hpp", - Namespace: "ns1", - UID: "hppns1", - }, - Spec: v1.PodSpec{ - Priority: &highPriority, - }, -}), - framework.NewPodInfo(&v1.Pod{ + lowPriority, midPriority, highPriority = int32(0), int32(100), int32(1000) + mediumPriority = (lowPriority + highPriority) / 2 + + highPriorityPodInfo = framework.NewPodInfo(&v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hpp", + Namespace: "ns1", + UID: "hppns1", + }, + Spec: v1.PodSpec{ + Priority: &highPriority, + }, + }) + 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)