From 801afbf888f098ef543e2c9f9e18e83d26838c50 Mon Sep 17 00:00:00 2001 From: AxeZhan Date: Fri, 29 Dec 2023 14:50:21 +0800 Subject: [PATCH] refactor TestPriorityQueue_Update --- .../internal/queue/scheduling_queue_test.go | 219 ++++++++++-------- 1 file changed, 125 insertions(+), 94 deletions(-) diff --git a/pkg/scheduler/internal/queue/scheduling_queue_test.go b/pkg/scheduler/internal/queue/scheduling_queue_test.go index 0d4ed88af75..e097d617073 100644 --- a/pkg/scheduler/internal/queue/scheduling_queue_test.go +++ b/pkg/scheduler/internal/queue/scheduling_queue_test.go @@ -862,105 +862,136 @@ func TestPriorityQueue_Pop(t *testing.T) { } func TestPriorityQueue_Update(t *testing.T) { - objs := []runtime.Object{highPriorityPodInfo.Pod, unschedulablePodInfo.Pod, medPriorityPodInfo.Pod} c := testingclock.NewFakeClock(time.Now()) - logger, ctx := ktesting.NewTestContext(t) - ctx, cancel := context.WithCancel(ctx) - defer cancel() - q := NewTestQueueWithObjects(ctx, newDefaultQueueSort(), objs, WithClock(c)) - // add highPriorityPodInfo to activeQ. - q.Update(logger, nil, highPriorityPodInfo.Pod) - if _, exists, _ := q.activeQ.Get(newQueuedPodInfoForLookup(highPriorityPodInfo.Pod)); !exists { - t.Errorf("Expected %v to be added to activeQ.", highPriorityPodInfo.Pod.Name) - } - if len(q.nominator.nominatedPods) != 0 { - t.Errorf("Expected nomindatePods to be empty: %v", q.nominator) - } - // Update highPriorityPodInfo and add a nominatedNodeName to it. - q.Update(logger, highPriorityPodInfo.Pod, highPriNominatedPodInfo.Pod) - if q.activeQ.Len() != 1 { - t.Error("Expected only one item in activeQ.") - } - if len(q.nominator.nominatedPods) != 1 { - t.Errorf("Expected one item in nomindatePods map: %v", q.nominator) - } - // Updating an unschedulable pod which is not in any of the two queues, should - // add the pod to activeQ. - q.Update(logger, unschedulablePodInfo.Pod, unschedulablePodInfo.Pod) - if _, exists, _ := q.activeQ.Get(newQueuedPodInfoForLookup(unschedulablePodInfo.Pod)); !exists { - t.Errorf("Expected %v to be added to activeQ.", unschedulablePodInfo.Pod.Name) - } - // Updating a pod that is already in activeQ, should not change it. - q.Update(logger, unschedulablePodInfo.Pod, unschedulablePodInfo.Pod) - if len(q.unschedulablePods.podInfoMap) != 0 { - t.Error("Expected unschedulablePods to be empty.") - } - if _, exists, _ := q.activeQ.Get(newQueuedPodInfoForLookup(unschedulablePodInfo.Pod)); !exists { - t.Errorf("Expected: %v to be added to activeQ.", unschedulablePodInfo.Pod.Name) - } - if p, err := q.Pop(logger); err != nil || p.Pod != highPriNominatedPodInfo.Pod { - t.Errorf("Expected: %v after Pop, but got: %v", highPriorityPodInfo.Pod.Name, p.Pod.Name) + + tests := []struct { + name string + wantQ string + // wantAddedToNominated is whether a Pod from the test case should be registered as a nominated Pod in the nominator. + wantAddedToNominated bool + // prepareFunc is the function called to prepare pods in the queue before the test case calls Update(). + // This function returns three values; + // - oldPod/newPod: each test will call Update() with these oldPod and newPod. + prepareFunc func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) + }{ + { + name: "add highPriorityPodInfo to activeQ", + wantQ: activeQ, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + return nil, highPriorityPodInfo.Pod + }, + }, + { + name: "Update pod that didn't exist in the queue", + wantQ: activeQ, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + updatedPod := medPriorityPodInfo.Pod.DeepCopy() + updatedPod.Annotations["foo"] = "test" + return medPriorityPodInfo.Pod, updatedPod + }, + }, + { + name: "Update highPriorityPodInfo and add a nominatedNodeName to it", + wantQ: activeQ, + wantAddedToNominated: true, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + return highPriorityPodInfo.Pod, highPriNominatedPodInfo.Pod + }, + }, + { + name: "When updating a pod that is already in activeQ, the pod should remain in activeQ after Update()", + wantQ: activeQ, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + err := q.Update(logger, nil, highPriorityPodInfo.Pod) + if err != nil { + t.Errorf("add pod %s error: %v", highPriorityPodInfo.Pod.Name, err) + } + return highPriorityPodInfo.Pod, highPriorityPodInfo.Pod + }, + }, + { + name: "When updating a pod that is in backoff queue and is still backing off, it will be updated in backoff queue", + wantQ: backoffQ, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + podInfo := q.newQueuedPodInfo(medPriorityPodInfo.Pod) + if err := q.podBackoffQ.Add(podInfo); err != nil { + t.Errorf("adding pod to backoff queue error: %v", err) + } + return podInfo.Pod, podInfo.Pod + }, + }, + { + name: "when updating a pod which is in unschedulable queue and is backing off, it will be moved to backoff queue", + wantQ: backoffQ, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + q.unschedulablePods.addOrUpdate(q.newQueuedPodInfo(medPriorityPodInfo.Pod, "plugin")) + updatedPod := medPriorityPodInfo.Pod.DeepCopy() + updatedPod.Annotations["foo"] = "test" + return medPriorityPodInfo.Pod, updatedPod + }, + }, + { + name: "when updating a pod which is in unschedulable queue and is not backing off, it will be moved to active queue", + wantQ: activeQ, + prepareFunc: func(t *testing.T, logger klog.Logger, q *PriorityQueue) (oldPod, newPod *v1.Pod) { + q.unschedulablePods.addOrUpdate(q.newQueuedPodInfo(medPriorityPodInfo.Pod, "plugin")) + updatedPod := medPriorityPodInfo.Pod.DeepCopy() + updatedPod.Annotations["foo"] = "test1" + // Move clock by podInitialBackoffDuration, so that pods in the unschedulablePods would pass the backing off, + // and the pods will be moved into activeQ. + c.Step(q.podInitialBackoffDuration) + return medPriorityPodInfo.Pod, updatedPod + }, + }, } - // Updating a pod that is in backoff queue and it is still backing off - // pod will not be moved to active queue, and it will be updated in backoff queue - podInfo := q.newQueuedPodInfo(medPriorityPodInfo.Pod) - if err := q.podBackoffQ.Add(podInfo); err != nil { - t.Errorf("adding pod to backoff queue error: %v", err) - } - q.Update(logger, podInfo.Pod, podInfo.Pod) - rawPodInfo, err := q.podBackoffQ.Pop() - podGotFromBackoffQ := rawPodInfo.(*framework.QueuedPodInfo).Pod - if err != nil || podGotFromBackoffQ != medPriorityPodInfo.Pod { - t.Errorf("Expected: %v after Pop, but got: %v", medPriorityPodInfo.Pod.Name, podGotFromBackoffQ.Name) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger, ctx := ktesting.NewTestContext(t) + objs := []runtime.Object{highPriorityPodInfo.Pod, unschedulablePodInfo.Pod, medPriorityPodInfo.Pod} + ctx, cancel := context.WithCancel(ctx) + defer cancel() + q := NewTestQueueWithObjects(ctx, newDefaultQueueSort(), objs, WithClock(c)) - // To simulate the pod is failed in scheduling in the real world, Pop() the pod from activeQ before testing AddUnschedulableIfNotPresent. - q.activeQ.Add(podInfo) - if p, err := q.Pop(logger); err != nil || p.Pod != medPriorityPodInfo.Pod { - t.Errorf("Expected: %v after Pop, but got: %v", medPriorityPodInfo.Pod.Name, p.Pod.Name) - } - err = q.AddUnschedulableIfNotPresent(logger, q.newQueuedPodInfo(medPriorityPodInfo.Pod, "plugin"), q.SchedulingCycle()) - if err != nil { - t.Fatalf("unexpected error from AddUnschedulableIfNotPresent: %v", err) - } - if len(q.unschedulablePods.podInfoMap) != 1 { - t.Error("Expected unschedulablePods to be 1.") - } - updatedPod := medPriorityPodInfo.Pod.DeepCopy() - updatedPod.Annotations["foo"] = "test" - q.Update(logger, medPriorityPodInfo.Pod, updatedPod) - rawPodInfo, err = q.podBackoffQ.Pop() - podGotFromBackoffQ = rawPodInfo.(*framework.QueuedPodInfo).Pod - if err != nil || podGotFromBackoffQ != updatedPod { - t.Errorf("Expected: %v after Pop, but got: %v", updatedPod.Name, podGotFromBackoffQ.Name) - } + oldPod, newPod := tt.prepareFunc(t, logger, q) - // To simulate the pod is failed in scheduling in the real world, Pop() the pod from activeQ before testing AddUnschedulableIfNotPresent. - err = q.activeQ.Add(podInfo) - if err != nil { - t.Fatalf("unexpected error from activeQ.Add: %v", err) - } - if p, err := q.Pop(logger); err != nil || p.Pod != medPriorityPodInfo.Pod { - t.Errorf("Expected: %v after Pop, but got: %v", medPriorityPodInfo.Pod.Name, p.Pod.Name) - } - // updating a pod which is in unschedulable queue, and it is not backing off, - // we will move it to active queue - err = q.AddUnschedulableIfNotPresent(logger, q.newQueuedPodInfo(medPriorityPodInfo.Pod, "plugin"), q.SchedulingCycle()) - if err != nil { - t.Fatalf("unexpected error from AddUnschedulableIfNotPresent: %v", err) - } - if len(q.unschedulablePods.podInfoMap) != 1 { - t.Error("Expected unschedulablePods to be 1.") - } - updatedPod = medPriorityPodInfo.Pod.DeepCopy() - updatedPod.Annotations["foo"] = "test1" - // Move clock by podInitialBackoffDuration, so that pods in the unschedulablePods would pass the backing off, - // and the pods will be moved into activeQ. - c.Step(q.podInitialBackoffDuration) - q.Update(logger, medPriorityPodInfo.Pod, updatedPod) - if p, err := q.Pop(logger); err != nil || p.Pod != updatedPod { - t.Errorf("Expected: %v after Pop, but got: %v", updatedPod.Name, p.Pod.Name) + if err := q.Update(logger, oldPod, newPod); err != nil { + t.Fatalf("unexpected error from Update: %v", err) + } + + var pInfo *framework.QueuedPodInfo + + // validate expected queue + if obj, exists, _ := q.podBackoffQ.Get(newQueuedPodInfoForLookup(newPod)); exists { + if tt.wantQ != backoffQ { + t.Errorf("expected pod %s not to be queued to backoffQ, but it was", newPod.Name) + } + pInfo = obj.(*framework.QueuedPodInfo) + } + + if obj, exists, _ := q.activeQ.Get(newQueuedPodInfoForLookup(newPod)); exists { + if tt.wantQ != activeQ { + t.Errorf("expected pod %s not to be queued to activeQ, but it was", newPod.Name) + } + pInfo = obj.(*framework.QueuedPodInfo) + } + + if pInfoFromUnsched := q.unschedulablePods.get(newPod); pInfoFromUnsched != nil { + if tt.wantQ != unschedulablePods { + t.Errorf("expected pod %s to not be queued to unschedulablePods, but it was", newPod.Name) + } + pInfo = pInfoFromUnsched + } + + if diff := cmp.Diff(newPod, pInfo.PodInfo.Pod); diff != "" { + t.Errorf("Unexpected updated pod diff (-want, +got): %s", diff) + } + + if tt.wantAddedToNominated && len(q.nominator.nominatedPods) != 1 { + t.Errorf("Expected one item in nomindatePods map: %v", q.nominator) + } + + }) } }