Merge pull request #118842 from helayoty/sched-update-unit-tests

test: Use table-driven test for TestPerPodSchedulingMetrics

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -418,7 +418,7 @@ func (p *PriorityQueue) runPreEnqueuePlugins(ctx context.Context, pInfo *framewo
 	logger := klog.FromContext(ctx)
 	var s *framework.Status
 	pod := pInfo.Pod
-	startTime := time.Now()
+	startTime := p.clock.Now()
 	defer func() {
 		metrics.FrameworkExtensionPointDuration.WithLabelValues(preEnqueue, s.Code().String(), pod.Spec.SchedulerName).Observe(metrics.SinceInSeconds(startTime))
 	}()

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -1944,97 +1944,113 @@ scheduler_plugin_execution_duration_seconds_count{extension_point="PreEnqueue",p
 // TestPerPodSchedulingMetrics makes sure pod schedule attempts is updated correctly while
 // initialAttemptTimestamp stays the same during multiple add/pop operations.
 func TestPerPodSchedulingMetrics(t *testing.T) {
-	pod := st.MakePod().Name("test-pod").Namespace("test-ns").UID("test-uid").Obj()
 	timestamp := time.Now()
 
-	// Case 1: A pod is created and scheduled after 1 attempt. The queue operations are
-	// Add -> Pop.
-	c := testingclock.NewFakeClock(timestamp)
 	logger, ctx := ktesting.NewTestContext(t)
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
-	queue := NewTestQueue(ctx, newDefaultQueueSort(), WithClock(c))
-	queue.Add(logger, pod)
-	pInfo, err := queue.Pop()
-	if err != nil {
-		t.Fatalf("Failed to pop a pod %v", err)
-	}
-	checkPerPodSchedulingMetrics("Attempt once", t, pInfo, 1, timestamp)
 
-	// Case 2: A pod is created and scheduled after 2 attempts. The queue operations are
+	tests := []struct {
-	// Add -> Pop -> AddUnschedulableIfNotPresent -> flushUnschedulablePodsLeftover -> Pop.
+		name                            string
-	c = testingclock.NewFakeClock(timestamp)
+		perPodSchedulingMetricsScenario func(*testingclock.FakeClock, *PriorityQueue, *v1.Pod)
-	queue = NewTestQueue(ctx, newDefaultQueueSort(), WithClock(c))
+		wantAttempts                    int
-	queue.Add(logger, pod)
+		wantInitialAttemptTs            time.Time
-	pInfo, err = queue.Pop()
+	}{
-	if err != nil {
+		{
-		t.Fatalf("Failed to pop a pod %v", err)
+			// The queue operations are Add -> Pop.
-	}
+			name: "pod is created and scheduled after 1 attempt",
-	queue.AddUnschedulableIfNotPresent(logger, pInfo, 1)
+			perPodSchedulingMetricsScenario: func(c *testingclock.FakeClock, queue *PriorityQueue, pod *v1.Pod) {
-	// Override clock to exceed the DefaultPodMaxInUnschedulablePodsDuration so that unschedulable pods
+				queue.Add(logger, pod)
-	// will be moved to activeQ
+			},
-	c.SetTime(timestamp.Add(DefaultPodMaxInUnschedulablePodsDuration + 1))
+			wantAttempts:         1,
-	queue.flushUnschedulablePodsLeftover(logger)
+			wantInitialAttemptTs: timestamp,
-	pInfo, err = queue.Pop()
+		},
-	if err != nil {
+		{
-		t.Fatalf("Failed to pop a pod %v", err)
+			// The queue operations are Add -> Pop -> AddUnschedulableIfNotPresent -> flushUnschedulablePodsLeftover -> Pop.
-	}
+			name: "pod is created and scheduled after 2 attempts",
-	checkPerPodSchedulingMetrics("Attempt twice", t, pInfo, 2, timestamp)
+			perPodSchedulingMetricsScenario: func(c *testingclock.FakeClock, queue *PriorityQueue, pod *v1.Pod) {
+				queue.Add(logger, pod)
+				pInfo, err := queue.Pop()
+				if err != nil {
+					t.Fatalf("Failed to pop a pod %v", err)
+				}
 
-	// Case 3: Similar to case 2, but before the second pop, call update, the queue operations are
+				queue.AddUnschedulableIfNotPresent(logger, pInfo, 1)
-	// Add -> Pop -> AddUnschedulableIfNotPresent -> flushUnschedulablePodsLeftover -> Update -> Pop.
+				// Override clock to exceed the DefaultPodMaxInUnschedulablePodsDuration so that unschedulable pods
-	c = testingclock.NewFakeClock(timestamp)
+				// will be moved to activeQ
-	queue = NewTestQueue(ctx, newDefaultQueueSort(), WithClock(c))
+				c.SetTime(timestamp.Add(DefaultPodMaxInUnschedulablePodsDuration + 1))
-	queue.Add(logger, pod)
+				queue.flushUnschedulablePodsLeftover(logger)
-	pInfo, err = queue.Pop()
+			},
-	if err != nil {
+			wantAttempts:         2,
-		t.Fatalf("Failed to pop a pod %v", err)
+			wantInitialAttemptTs: timestamp,
-	}
+		},
-	queue.AddUnschedulableIfNotPresent(logger, pInfo, 1)
+		{
-	// Override clock to exceed the DefaultPodMaxInUnschedulablePodsDuration so that unschedulable pods
+			// The queue operations are Add -> Pop -> AddUnschedulableIfNotPresent -> flushUnschedulablePodsLeftover -> Update -> Pop.
-	// will be moved to activeQ
+			name: "pod is created and scheduled after 2 attempts but before the second pop, call update",
-	c.SetTime(timestamp.Add(DefaultPodMaxInUnschedulablePodsDuration + 1))
+			perPodSchedulingMetricsScenario: func(c *testingclock.FakeClock, queue *PriorityQueue, pod *v1.Pod) {
-	queue.flushUnschedulablePodsLeftover(logger)
+				queue.Add(logger, pod)
-	newPod := pod.DeepCopy()
+				pInfo, err := queue.Pop()
-	newPod.Generation = 1
+				if err != nil {
-	queue.Update(logger, pod, newPod)
+					t.Fatalf("Failed to pop a pod %v", err)
-	pInfo, err = queue.Pop()
+				}
-	if err != nil {
-		t.Fatalf("Failed to pop a pod %v", err)
-	}
-	checkPerPodSchedulingMetrics("Attempt twice with update", t, pInfo, 2, timestamp)
 
-	// Case 4: A gated pod is created and scheduled after lifting gate. The queue operations are
+				queue.AddUnschedulableIfNotPresent(logger, pInfo, 1)
-	//  Add gated pod -> check unschedulablePods -> lift gate & update pod -> Pop.
+				// Override clock to exceed the DefaultPodMaxInUnschedulablePodsDuration so that unschedulable pods
-	c = testingclock.NewFakeClock(timestamp)
+				// will be moved to activeQ
-	// Create a queue with PreEnqueuePlugin
+				updatedTimestamp := timestamp
-	m := map[string][]framework.PreEnqueuePlugin{"": {&preEnqueuePlugin{allowlists: []string{"foo"}}}}
+				c.SetTime(updatedTimestamp.Add(DefaultPodMaxInUnschedulablePodsDuration + 1))
-	queue = NewTestQueue(ctx, newDefaultQueueSort(), WithClock(c), WithPreEnqueuePluginMap(m), WithPluginMetricsSamplePercent(0))
+				queue.flushUnschedulablePodsLeftover(logger)
+				newPod := pod.DeepCopy()
+				newPod.Generation = 1
+				queue.Update(logger, pod, newPod)
+			},
+			wantAttempts:         2,
+			wantInitialAttemptTs: timestamp,
+		},
+		{
+			// The queue operations are Add gated pod -> check unschedulablePods -> lift gate & update pod -> Pop.
+			name: "A gated pod is created and scheduled after lifting gate",
+			perPodSchedulingMetricsScenario: func(c *testingclock.FakeClock, queue *PriorityQueue, pod *v1.Pod) {
+				// Create a queue with PreEnqueuePlugin
+				queue.preEnqueuePluginMap = map[string][]framework.PreEnqueuePlugin{"": {&preEnqueuePlugin{allowlists: []string{"foo"}}}}
+				queue.pluginMetricsSamplePercent = 0
+				queue.Add(logger, pod)
+				// Check pod is added to the unschedulablePods queue.
+				if getUnschedulablePod(queue, pod) != pod {
+					t.Errorf("Pod %v was not found in the unschedulablePods.", pod.Name)
+				}
+				// Override clock to get different InitialAttemptTimestamp
+				c.Step(1 * time.Minute)
 
-	// Create a pod without PreEnqueuePlugin label.
+				// Update pod with the required label to get it out of unschedulablePods queue.
-	gatedPod := st.MakePod().Name("gated-test-pod").Namespace("test-ns").UID("test-uid").Obj()
+				updateGatedPod := pod.DeepCopy()
-	err = queue.Add(logger, gatedPod)
+				updateGatedPod.Labels = map[string]string{"foo": ""}
-	if err != nil {
+				queue.Update(logger, pod, updateGatedPod)
-		t.Fatalf("Failed to add a pod %v", err)
+			},
+			wantAttempts:         1,
+			wantInitialAttemptTs: timestamp.Add(1 * time.Minute),
+		},
 	}
-	// Check pod is added to the unschedulablePods queue.
+	for _, test := range tests {
-	if getUnschedulablePod(queue, gatedPod) != gatedPod {
+		t.Run(test.name, func(t *testing.T) {
-		t.Errorf("Pod %v was not found in the unschedulablePods.", gatedPod.Name)
+
+			c := testingclock.NewFakeClock(timestamp)
+			pod := st.MakePod().Name("test-pod").Namespace("test-ns").UID("test-uid").Obj()
+			queue := NewTestQueue(ctx, newDefaultQueueSort(), WithClock(c))
+
+			test.perPodSchedulingMetricsScenario(c, queue, pod)
+			podInfo, err := queue.Pop()
+			if err != nil {
+				t.Fatal(err)
+			}
+			if podInfo.Attempts != test.wantAttempts {
+				t.Errorf("Pod schedule attempt unexpected, got %v, want %v", podInfo.Attempts, test.wantAttempts)
+			}
+			if *podInfo.InitialAttemptTimestamp != test.wantInitialAttemptTs {
+				t.Errorf("Pod initial schedule attempt timestamp unexpected, got %v, want %v", *podInfo.InitialAttemptTimestamp, test.wantInitialAttemptTs)
+			}
+		})
 	}
-	// Override clock to get different InitialAttemptTimestamp
-	c.Step(1 * time.Minute)
-
-	// Update pod with the required label to get it out of unschedulablePods queue.
-	updateGatedPod := gatedPod.DeepCopy()
-	updateGatedPod.Labels = map[string]string{"foo": ""}
-	queue.Update(logger, gatedPod, updateGatedPod)
-
-	pInfo, err = queue.Pop()
-	if err != nil {
-		t.Fatalf("Failed to pop a pod %v", err)
-	}
-
-	checkPerPodSchedulingMetrics("Attempt once/gated", t, pInfo, 1, timestamp.Add(1*time.Minute))
 }
 
 func TestIncomingPodsMetrics(t *testing.T) {
@@ -2127,15 +2143,6 @@ func TestIncomingPodsMetrics(t *testing.T) {
 	}
 }
 
-func checkPerPodSchedulingMetrics(name string, t *testing.T, pInfo *framework.QueuedPodInfo, wantAttempts int, wantInitialAttemptTs time.Time) {
-	if pInfo.Attempts != wantAttempts {
-		t.Errorf("[%s] Pod schedule attempt unexpected, got %v, want %v", name, pInfo.Attempts, wantAttempts)
-	}
-	if *pInfo.InitialAttemptTimestamp != wantInitialAttemptTs {
-		t.Errorf("[%s] Pod initial schedule attempt timestamp unexpected, got %v, want %v", name, *pInfo.InitialAttemptTimestamp, wantInitialAttemptTs)
-	}
-}
-
 func TestBackOffFlow(t *testing.T) {
 	cl := testingclock.NewFakeClock(time.Now())
 	logger, ctx := ktesting.NewTestContext(t)