Merge pull request #855 from googs1025/qos_sort

fix(QOSSort): refine queue sorting logic by adding inqueue timestamp

Author: k8s-ci-robot

pkg/qos/queue_sort.go

@@ -41,22 +41,43 @@ func (pl *Sort) Name() string {
 
 // Less is the function used by the activeQ heap algorithm to sort pods.
 // It sorts pods based on their priorities. When the priorities are equal, it uses
-// the Pod QoS classes to break the tie.
+// the Pod QoS classes to break the tie. If both the priority and QoS class are equal,
+// it uses PodQueueInfo.timestamp to determine the order.
 func (*Sort) Less(pInfo1, pInfo2 *framework.QueuedPodInfo) bool {
 	p1 := corev1helpers.PodPriority(pInfo1.Pod)
 	p2 := corev1helpers.PodPriority(pInfo2.Pod)
-	return (p1 > p2) || (p1 == p2 && compQOS(pInfo1.Pod, pInfo2.Pod))
+
+	if p1 != p2 {
+		return p1 > p2
+	}
+	qosResult := compQOS(pInfo1.Pod, pInfo2.Pod)
+	if qosResult != 0 {
+		return qosResult > 0
+	}
+	return pInfo1.Timestamp.Before(pInfo2.Timestamp)
 }
 
-func compQOS(p1, p2 *v1.Pod) bool {
+// compQOS compares the QoS classes of two Pods and returns:
+//
+//	 1 if p1 has a higher precedence QoS class than p2,
+//	-1 if p2 has a higher precedence QoS class than p1,
+//	 0 if both have the same QoS class.
+func compQOS(p1, p2 *v1.Pod) int {
 	p1QOS, p2QOS := v1qos.GetPodQOS(p1), v1qos.GetPodQOS(p2)
-	if p1QOS == v1.PodQOSGuaranteed {
-		return true
+
+	// Define the precedence order of QoS classes using a map
+	qosOrder := map[v1.PodQOSClass]int{
+		v1.PodQOSBestEffort: 1,
+		v1.PodQOSBurstable:  2,
+		v1.PodQOSGuaranteed: 3,
 	}
-	if p1QOS == v1.PodQOSBurstable {
-		return p2QOS != v1.PodQOSGuaranteed
+
+	if qosOrder[p1QOS] > qosOrder[p2QOS] {
+		return 1
+	} else if qosOrder[p1QOS] < qosOrder[p2QOS] {
+		return -1
 	}
-	return p2QOS == v1.PodQOSBestEffort
+	return 0
 }
 
 // New initializes a new plugin and returns it.

pkg/qos/queue_sort_test.go

@@ -18,6 +18,7 @@ package qos
 
 import (
 	"testing"
+	"time"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -31,6 +32,8 @@ func createPodInfo(pod *v1.Pod) *framework.PodInfo {
 }
 
 func TestSortLess(t *testing.T) {
+	earlierTime := time.Now()
+	laterTime := earlierTime.Add(time.Second)
 	tests := []struct {
 		name   string
 		pInfo1 *framework.QueuedPodInfo
@@ -58,14 +61,16 @@ func TestSortLess(t *testing.T) {
 			want: false,
 		},
 		{
-			name: "p1 and p2 are both BestEfforts",
+			name: "p1 and p2 are both BestEfforts, but p2 is added to schedulingQ earlier than p1",
 			pInfo1: &framework.QueuedPodInfo{
-				PodInfo: createPodInfo(makePod("p1", 0, nil, nil)),
+				PodInfo:   createPodInfo(makePod("p1", 0, nil, nil)),
+				Timestamp: laterTime,
 			},
 			pInfo2: &framework.QueuedPodInfo{
-				PodInfo: createPodInfo(makePod("p2", 0, nil, nil)),
+				PodInfo:   createPodInfo(makePod("p2", 0, nil, nil)),
+				Timestamp: earlierTime,
 			},
-			want: true,
+			want: false,
 		},
 		{
 			name: "p1 is BestEfforts, p2 is Guaranteed",
@@ -88,14 +93,16 @@ func TestSortLess(t *testing.T) {
 			want: false,
 		},
 		{
-			name: "both p1 and p2 are Burstable",
+			name: "both p1 and p2 are Burstable, but p2 is added to schedulingQ earlier than p1",
 			pInfo1: &framework.QueuedPodInfo{
-				PodInfo: createPodInfo(makePod("p1", 0, getResList("100m", "100Mi"), getResList("200m", "200Mi"))),
+				PodInfo:   createPodInfo(makePod("p1", 0, getResList("100m", "100Mi"), getResList("200m", "200Mi"))),
+				Timestamp: laterTime,
 			},
 			pInfo2: &framework.QueuedPodInfo{
-				PodInfo: createPodInfo(makePod("p2", 0, getResList("100m", "100Mi"), getResList("200m", "200Mi"))),
+				PodInfo:   createPodInfo(makePod("p2", 0, getResList("100m", "100Mi"), getResList("200m", "200Mi"))),
+				Timestamp: earlierTime,
 			},
-			want: true,
+			want: false,
 		},
 		{
 			name: "p1 is Guaranteed, p2 is Burstable",
@@ -108,12 +115,26 @@ func TestSortLess(t *testing.T) {
 			want: true,
 		},
 		{
-			name: "both p1 and p2 are Guaranteed",
+			name: "both p1 and p2 are Guaranteed, but p1 is added to schedulingQ earlier than p2",
 			pInfo1: &framework.QueuedPodInfo{
-				PodInfo: createPodInfo(makePod("p1", 0, getResList("100m", "100Mi"), getResList("100m", "100Mi"))),
+				PodInfo:   createPodInfo(makePod("p1", 0, getResList("100m", "100Mi"), getResList("100m", "100Mi"))),
+				Timestamp: earlierTime,
 			},
 			pInfo2: &framework.QueuedPodInfo{
-				PodInfo: createPodInfo(makePod("p2", 0, getResList("100m", "100Mi"), getResList("100m", "100Mi"))),
+				PodInfo:   createPodInfo(makePod("p2", 0, getResList("100m", "100Mi"), getResList("100m", "100Mi"))),
+				Timestamp: laterTime,
+			},
+			want: true,
+		},
+		{
+			name: "both p1 and p2 are Guaranteed, but p1 is added to schedulingQ earlier than p2",
+			pInfo1: &framework.QueuedPodInfo{
+				PodInfo:   createPodInfo(makePod("p1", 0, getResList("100m", "100Mi"), getResList("100m", "100Mi"))),
+				Timestamp: earlierTime,
+			},
+			pInfo2: &framework.QueuedPodInfo{
+				PodInfo:   createPodInfo(makePod("p2", 0, getResList("100m", "100Mi"), getResList("100m", "100Mi"))),
+				Timestamp: laterTime,
 			},
 			want: true,
 		},

test/integration/qos_test.go

@@ -19,6 +19,9 @@ package integration
 import (
 	"context"
 	"fmt"
+	"reflect"
+	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -114,19 +117,33 @@ func TestQOSPlugin(t *testing.T) {
 	// Create 3 Pods with the order: BestEfforts, Burstable, Guaranteed.
 	// We will expect them to be scheduled in a reversed order.
 	t.Logf("Start to create 3 Pods.")
-	for i := range pods {
-		t.Logf("Creating Pod %q", pods[i].Name)
-		_, err = cs.CoreV1().Pods(ns).Create(testCtx.Ctx, pods[i], metav1.CreateOptions{})
-		if err != nil {
-			t.Fatalf("Failed to create Pod %q: %v", pods[i].Name, err)
-		}
+	// Concurrently create all Pods.
+	var wg sync.WaitGroup
+	for _, pod := range pods {
+		wg.Add(1)
+		go func(p *v1.Pod) {
+			defer wg.Done()
+			_, err = cs.CoreV1().Pods(ns).Create(testCtx.Ctx, p, metav1.CreateOptions{})
+			if err != nil {
+				t.Errorf("Failed to create Pod %q: %v", p.Name, err)
+			} else {
+				t.Logf("Created Pod %q", p.Name)
+			}
+		}(pod)
 	}
+	wg.Wait()
 	defer cleanupPods(t, testCtx, pods)
 
 	// Wait for all Pods are in the scheduling queue.
 	err = wait.PollUntilContextTimeout(testCtx.Ctx, time.Millisecond*200, wait.ForeverTestTimeout, false, func(ctx context.Context) (bool, error) {
 		pendingPods, _ := testCtx.Scheduler.SchedulingQueue.PendingPods()
 		if len(pendingPods) == len(pods) {
+			// Collect Pod names into a slice.
+			podNames := make([]string, len(pendingPods))
+			for i, podInfo := range pendingPods {
+				podNames[i] = podInfo.Name
+			}
+			t.Logf("All Pods are in the pending queue: %v", strings.Join(podNames, ", "))
 			return true, nil
 		}
 		return false, nil
@@ -137,12 +154,17 @@ func TestQOSPlugin(t *testing.T) {
 
 	// Expect Pods are popped in the QoS class order.
 	logger := klog.FromContext(testCtx.Ctx)
-	for i := len(podNames) - 1; i >= 0; i-- {
+	expectedOrder := []string{"guaranteed", "burstable", "bestefforts"}
+	actualOrder := make([]string, len(expectedOrder))
+	for i := 0; i < len(expectedOrder); i++ {
 		podInfo, _ := testCtx.Scheduler.NextPod(logger)
-		if podInfo.Pod.Name != podNames[i] {
-			t.Errorf("Expect Pod %q, but got %q", podNames[i], podInfo.Pod.Name)
-		} else {
-			t.Logf("Pod %q is popped out as expected.", podInfo.Pod.Name)
-		}
+		actualOrder[i] = podInfo.Pod.Name
+		t.Logf("Popped Pod %q", podInfo.Pod.Name)
+	}
+
+	if !reflect.DeepEqual(actualOrder, expectedOrder) {
+		t.Errorf("Expected Pod order %v, but got %v", expectedOrder, actualOrder)
+	} else {
+		t.Logf("Pods were popped out in the expected order.")
 	}
 }