refactor(utils): refactor ComputePodQOS to make it more readable and …

…maintainable

pkg/utils/pod.go

@@ -14,21 +14,6 @@ import (
 	"k8s.io/klog/v2"
 )
 
-// GetResourceRequest finds and returns the request value for a specific resource.
-func GetResourceRequest(pod *v1.Pod, resource v1.ResourceName) int64 {
-	if resource == v1.ResourcePods {
-		return 1
-	}
-
-	requestQuantity := GetResourceRequestQuantity(pod, resource)
-
-	if resource == v1.ResourceCPU {
-		return requestQuantity.MilliValue()
-	}
-
-	return requestQuantity.Value()
-}
-
 // GetResourceRequestQuantity finds and returns the request quantity for a specific resource.
 func GetResourceRequestQuantity(pod *v1.Pod, resourceName v1.ResourceName) resource.Quantity {
 	requestQuantity := resource.Quantity{}

pkg/utils/qos.go

@@ -15,6 +15,8 @@ func isSupportedQoSComputeResource(name v1.ResourceName) bool {
 	return supportedQoSComputeResources.Has(string(name))
 }
 
+var zeroQuantity = resource.MustParse("0")
+
 // GetPodQOS returns the QoS class of a pod.
 // A pod is besteffort if none of its containers have specified any requests or limits.
 // A pod is guaranteed only when requests and limits are specified for all the containers and they are equal.
@@ -26,64 +28,82 @@ func GetPodQOS(pod *v1.Pod) v1.PodQOSClass {
 
 	requests := v1.ResourceList{}
 	limits := v1.ResourceList{}
-	zeroQuantity := resource.MustParse("0")
 	isGuaranteed := true
+	for _, container := range getAllContainers(pod) {
+		// Use a logical AND operation to accumulate the isGuaranteed status,
+		// ensuring that all containers must meet the Guaranteed condition.
+		isGuaranteed = processContainerResources(container, &requests, &limits) && isGuaranteed
+	}
+	if len(requests) == 0 && len(limits) == 0 {
+		return v1.PodQOSBestEffort
+	}
+	// Check is requests match limits for all resources.
+	if isGuaranteed && areRequestsMatchingLimits(requests, limits) {
+		return v1.PodQOSGuaranteed
+	}
+	return v1.PodQOSBurstable
+}
+
+// processContainerResources processes the resources of a single container and updates the provided requests and limits lists.
+func processContainerResources(container v1.Container, requests, limits *v1.ResourceList) bool {
+	isGuaranteed := true
+	processResourceList(*requests, container.Resources.Requests)
+	qosLimitsFound := getQOSResources(container.Resources.Limits)
+	processResourceList(*limits, container.Resources.Limits)
+	if !qosLimitsFound.HasAll(string(v1.ResourceMemory), string(v1.ResourceCPU)) {
+		isGuaranteed = false
+	}
+	return isGuaranteed
+}
+
+// getQOSResources returns a set of resource names from the provided resource list that:
+// 1. Are supported QoS compute resources
+// 2. Have quantities greater than zero
+func getQOSResources(list v1.ResourceList) sets.Set[string] {
+	qosResources := sets.New[string]()
+	for name, quantity := range list {
+		if !isSupportedQoSComputeResource(name) {
+			continue
+		}
+		if quantity.Cmp(zeroQuantity) == 1 {
+			qosResources.Insert(string(name))
+		}
+	}
+	return qosResources
+}
+
+func getAllContainers(pod *v1.Pod) []v1.Container {
 	allContainers := []v1.Container{}
 	allContainers = append(allContainers, pod.Spec.Containers...)
 	allContainers = append(allContainers, pod.Spec.InitContainers...)
-	for _, container := range allContainers {
-		// process requests
-		for name, quantity := range container.Resources.Requests {
-			if !isSupportedQoSComputeResource(name) {
-				continue
-			}
-			if quantity.Cmp(zeroQuantity) == 1 {
-				delta := quantity.DeepCopy()
-				if _, exists := requests[name]; !exists {
-					requests[name] = delta
-				} else {
-					delta.Add(requests[name])
-					requests[name] = delta
-				}
-			}
-		}
-		// process limits
-		qosLimitsFound := sets.New[string]()
-		for name, quantity := range container.Resources.Limits {
-			if !isSupportedQoSComputeResource(name) {
-				continue
-			}
-			if quantity.Cmp(zeroQuantity) == 1 {
-				qosLimitsFound.Insert(string(name))
-				delta := quantity.DeepCopy()
-				if _, exists := limits[name]; !exists {
-					limits[name] = delta
-				} else {
-					delta.Add(limits[name])
-					limits[name] = delta
-				}
-			}
-		}
+	return allContainers
+}
 
-		if !qosLimitsFound.HasAll(string(v1.ResourceMemory), string(v1.ResourceCPU)) {
-			isGuaranteed = false
+// areRequestsMatchingLimits checks if all resource requests match their respective limits.
+func areRequestsMatchingLimits(requests, limits v1.ResourceList) bool {
+	for name, req := range requests {
+		if lim, exists := limits[name]; !exists || lim.Cmp(req) != 0 {
+			return false
 		}
 	}
-	if len(requests) == 0 && len(limits) == 0 {
-		return v1.PodQOSBestEffort
-	}
-	// Check is requests match limits for all resources.
-	if isGuaranteed {
-		for name, req := range requests {
-			if lim, exists := limits[name]; !exists || lim.Cmp(req) != 0 {
-				isGuaranteed = false
-				break
+	return len(requests) == len(limits)
+}
+
+// processResourceList adds non-zero quantities for supported QoS compute resources
+// quantities from newList to list.
+func processResourceList(list, newList v1.ResourceList) {
+	for name, quantity := range newList {
+		if !isSupportedQoSComputeResource(name) {
+			continue
+		}
+		if quantity.Cmp(zeroQuantity) == 1 {
+			delta := quantity.DeepCopy()
+			if _, exists := list[name]; !exists {
+				list[name] = delta
+			} else {
+				delta.Add(list[name])
+				list[name] = delta
 			}
 		}
 	}
-	if isGuaranteed &&
-		len(requests) == len(limits) {
-		return v1.PodQOSGuaranteed
-	}
-	return v1.PodQOSBurstable
 }

pkg/utils/qos_test.go

@@ -0,0 +1,196 @@
+package utils
+
+import (
+	"testing"
+
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestComputePodQOS(t *testing.T) {
+	testCases := []struct {
+		name                     string
+		pod                      *v1.Pod
+		expected                 v1.PodQOSClass
+		podLevelResourcesEnabled bool
+	}{
+		{
+			name: "Single Guaranteed Container",
+			pod: newPod("guaranteed", []v1.Container{
+				newContainer("guaranteed", getResourceList("100m", "100Mi"), getResourceList("100m", "100Mi")),
+			}),
+			expected: v1.PodQOSGuaranteed,
+		},
+		{
+			name: "Two Guaranteed Containers",
+			pod: newPod("guaranteed-guaranteed", []v1.Container{
+				newContainer("guaranteed", getResourceList("100m", "100Mi"), getResourceList("100m", "100Mi")),
+				newContainer("guaranteed", getResourceList("100m", "100Mi"), getResourceList("100m", "100Mi")),
+			}),
+			expected: v1.PodQOSGuaranteed,
+		},
+		{
+			name: "Two BestEffort Containers",
+			pod: newPod("best-effort-best-effort", []v1.Container{
+				newContainer("best-effort", getResourceList("", ""), getResourceList("", "")),
+				newContainer("best-effort", getResourceList("", ""), getResourceList("", "")),
+			}),
+			expected: v1.PodQOSBestEffort,
+		},
+		{
+			name: "Single BestEffort Container",
+			pod: newPod("best-effort", []v1.Container{
+				newContainer("best-effort", getResourceList("", ""), getResourceList("", "")),
+			}),
+			expected: v1.PodQOSBestEffort,
+		},
+		{
+			name: "BestEffort and Burstable Containers",
+			pod: newPod("best-effort-burstable", []v1.Container{
+				newContainer("best-effort", getResourceList("", ""), getResourceList("", "")),
+				newContainer("burstable", getResourceList("1", ""), getResourceList("2", "")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "BestEffort and Guaranteed Containers",
+			pod: newPod("best-effort-guaranteed", []v1.Container{
+				newContainer("best-effort", getResourceList("", ""), getResourceList("", "")),
+				newContainer("guaranteed", getResourceList("10m", "100Mi"), getResourceList("10m", "100Mi")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "Burstable CPU, Guaranteed Memory",
+			pod: newPod("burstable-cpu-guaranteed-memory", []v1.Container{
+				newContainer("burstable", getResourceList("", "100Mi"), getResourceList("", "100Mi")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "Burstable Without Limits",
+			pod: newPod("burstable-no-limits", []v1.Container{
+				newContainer("burstable", getResourceList("100m", "100Mi"), getResourceList("", "")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "Burstable and Guaranteed Containers",
+			pod: newPod("burstable-guaranteed", []v1.Container{
+				newContainer("burstable", getResourceList("1", "100Mi"), getResourceList("2", "100Mi")),
+				newContainer("guaranteed", getResourceList("100m", "100Mi"), getResourceList("100m", "100Mi")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "Burstable Containers With Unbounded But Requests Match Limits",
+			pod: newPod("burstable-unbounded-but-requests-match-limits", []v1.Container{
+				newContainer("burstable", getResourceList("100m", "100Mi"), getResourceList("200m", "200Mi")),
+				newContainer("burstable-unbounded", getResourceList("100m", "100Mi"), getResourceList("", "")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "Burstable Container 1",
+			pod: newPod("burstable-1", []v1.Container{
+				newContainer("burstable", getResourceList("10m", "100Mi"), getResourceList("100m", "200Mi")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "Burstable Container 2",
+			pod: newPod("burstable-2", []v1.Container{
+				newContainer("burstable", getResourceList("0", "0"), getResourceList("100m", "200Mi")),
+			}),
+			expected: v1.PodQOSBurstable,
+		},
+		{
+			name: "BestEffort Container With HugePages",
+			pod: newPod("best-effort-hugepages", []v1.Container{
+				newContainer("best-effort", addResource("hugepages-2Mi", "1Gi", getResourceList("0", "0")), addResource("hugepages-2Mi", "1Gi", getResourceList("0", "0"))),
+			}),
+			expected: v1.PodQOSBestEffort,
+		},
+		{
+			name: "Init Container with BestEffort Main and Burstable Init",
+			pod: newPodWithInitContainers("init-container",
+				[]v1.Container{
+					newContainer("best-effort", getResourceList("", ""), getResourceList("", "")),
+				},
+				[]v1.Container{
+					newContainer("burstable", getResourceList("10m", "100Mi"), getResourceList("100m", "200Mi")),
+				}),
+			expected: v1.PodQOSBurstable,
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			qos := GetPodQOS(testCase.pod)
+			if qos != testCase.expected {
+				t.Errorf("Expected QoS class %v, got %v", testCase.expected, qos)
+			}
+		})
+	}
+}
+
+func getResourceList(cpu, memory string) v1.ResourceList {
+	res := v1.ResourceList{}
+	if cpu != "" {
+		res[v1.ResourceCPU] = resource.MustParse(cpu)
+	}
+	if memory != "" {
+		res[v1.ResourceMemory] = resource.MustParse(memory)
+	}
+	return res
+}
+
+func addResource(rName, value string, rl v1.ResourceList) v1.ResourceList {
+	rl[v1.ResourceName(rName)] = resource.MustParse(value)
+	return rl
+}
+
+func getResourceRequirements(requests, limits v1.ResourceList) *v1.ResourceRequirements {
+	res := v1.ResourceRequirements{}
+	res.Requests = requests
+	res.Limits = limits
+	return &res
+}
+
+func newContainer(name string, requests v1.ResourceList, limits v1.ResourceList) v1.Container {
+	return v1.Container{
+		Name:      name,
+		Resources: *(getResourceRequirements(requests, limits)),
+	}
+}
+
+func newPod(name string, containers []v1.Container) *v1.Pod {
+	return &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+		},
+		Spec: v1.PodSpec{
+			Containers: containers,
+		},
+	}
+}
+
+func newPodWithResources(name string, containers []v1.Container, podResources *v1.ResourceRequirements) *v1.Pod {
+	pod := newPod(name, containers)
+	if podResources != nil {
+		pod.Spec.Resources = podResources
+	}
+	return pod
+}
+
+func newPodWithInitContainers(name string, containers []v1.Container, initContainers []v1.Container) *v1.Pod {
+	return &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+		},
+		Spec: v1.PodSpec{
+			Containers:     containers,
+			InitContainers: initContainers,
+		},
+	}
+}