Merge pull request #6577 from BigDarkClown/priority

Consider preemption policy for expandable pods
kubernetes · Feb 28, 2024 · 79269d8 · 79269d8
2 parents dffff4f + 6632bab
commit 79269d8
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 17 deletions.
diff --git a/cluster-autoscaler/core/utils/expendable.go b/cluster-autoscaler/core/utils/expendable.go
@@ -34,7 +34,7 @@ func FilterOutExpendableAndSplit(unschedulableCandidates []*apiv1.Pod, nodes []*
 	}
 
 	for _, pod := range unschedulableCandidates {
-		if pod.Spec.Priority != nil && int(*pod.Spec.Priority) < expendablePodsPriorityCutoff {
+		if IsExpendablePod(pod, expendablePodsPriorityCutoff) {
 			klog.V(4).Infof("Pod %s has priority below %d (%d) and will scheduled when enough resources is free. Ignoring in scale up.", pod.Name, expendablePodsPriorityCutoff, *pod.Spec.Priority)
 		} else if nominatedNodeName := pod.Status.NominatedNodeName; nominatedNodeName != "" {
 			if nodeNames[nominatedNodeName] {
@@ -64,5 +64,7 @@ func FilterOutExpendablePods(pods []*apiv1.Pod, expendablePodsPriorityCutoff int
 
 // IsExpendablePod tests if pod is expendable for give priority cutoff
 func IsExpendablePod(pod *apiv1.Pod, expendablePodsPriorityCutoff int) bool {
-	return pod.Spec.Priority != nil && int(*pod.Spec.Priority) < expendablePodsPriorityCutoff
+	preemptLowerPriority := pod.Spec.PreemptionPolicy == nil || *pod.Spec.PreemptionPolicy == apiv1.PreemptLowerPriority
+	lowPriority := pod.Spec.Priority != nil && int(*pod.Spec.Priority) < expendablePodsPriorityCutoff
+	return preemptLowerPriority && lowPriority
 }
diff --git a/cluster-autoscaler/core/utils/expendable_test.go b/cluster-autoscaler/core/utils/expendable_test.go
@@ -88,20 +88,92 @@ func TestFilterOutExpendablePods(t *testing.T) {
 	assert.Equal(t, podWaitingForPreemption2, res[2])
 }
 
-func TestIsExpendablePod(t *testing.T) {
-	pod1 := BuildTestPod("p1", 1500, 200000)
-	pod2 := BuildTestPod("w1", 1500, 200000)
-	var priority1 int32 = -10
-	pod2.Spec.Priority = &priority1
-	pod2.Status.NominatedNodeName = "node1"
-
-	assert.False(t, IsExpendablePod(pod1, 0))
-	assert.False(t, IsExpendablePod(pod1, -9))
-	assert.False(t, IsExpendablePod(pod1, -10))
-	assert.False(t, IsExpendablePod(pod1, -11))
-	assert.True(t, IsExpendablePod(pod2, 0))
-	assert.True(t, IsExpendablePod(pod2, -9))
-	assert.False(t, IsExpendablePod(pod2, -10))
-	assert.False(t, IsExpendablePod(pod2, -11))
+func TestIsExpandablePod(t *testing.T) {
+	preemptLowerPriorityPolicy := apiv1.PreemptLowerPriority
+	neverPolicy := apiv1.PreemptNever
+
+	testCases := []struct {
+		name   string
+		pod    *apiv1.Pod
+		cutoff int
+		want   bool
+	}{
+		{
+			name:   "pod priority not set, zero cutoff",
+			pod:    BuildTestPod("p", 0, 0),
+			cutoff: 0,
+			want:   false,
+		},
+		{
+			name:   "pod priority not set, negative cutoff",
+			pod:    BuildTestPod("p", 0, 0),
+			cutoff: -1,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, default preemption policy, higher cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, nil),
+			cutoff: 0,
+			want:   true,
+		},
+		{
+			name:   "pod priority set, default preemption policy, equal cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, nil),
+			cutoff: -1,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, default preemption policy, smaller cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, nil),
+			cutoff: -2,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, preempt lower priority preemption policy, higher cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, &preemptLowerPriorityPolicy),
+			cutoff: 0,
+			want:   true,
+		},
+		{
+			name:   "pod priority set, preempt lower priority preemption policy, equal cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, &preemptLowerPriorityPolicy),
+			cutoff: -1,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, preempt lower priority preemption policy, smaller cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, &preemptLowerPriorityPolicy),
+			cutoff: -2,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, never preemption policy, higher cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, &neverPolicy),
+			cutoff: 0,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, never preemption policy, equal cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, &neverPolicy),
+			cutoff: -1,
+			want:   false,
+		},
+		{
+			name:   "pod priority set, never preemption policy, smaller cutoff",
+			pod:    withPodPriority(BuildTestPod("p", 0, 0), -1, &neverPolicy),
+			cutoff: -2,
+			want:   false,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, IsExpendablePod(tc.pod, tc.cutoff))
+		})
+	}
+}
 
+func withPodPriority(pod *apiv1.Pod, priority int32, preemptionPolicy *apiv1.PreemptionPolicy) *apiv1.Pod {
+	pod.Spec.Priority = &priority
+	pod.Spec.PreemptionPolicy = preemptionPolicy
+	return pod
 }