Merge pull request #67781 from dashpole/fix_priority_tests

pkg/kubelet/preemption/preemption_test.go

@@ -130,7 +130,7 @@ func TestEvictPodsToFreeRequests(t *testing.T) {
 	}
 	for _, r := range runs {
 		podProvider.setPods(r.inputPods)
-		outErr := criticalPodAdmissionHandler.evictPodsToFreeRequests(nil, r.insufficientResources)
+		outErr := criticalPodAdmissionHandler.evictPodsToFreeRequests(allPods[critical], r.insufficientResources)
 		outputPods := podKiller.getKilledPods()
 		if !r.expectErr && outErr != nil {
 			t.Errorf("evictPodsToFreeRequests returned an unexpected error during the %s test.  Err: %v", r.testName, outErr)
@@ -171,6 +171,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 	runs := []testRun{
 		{
 			testName:              "no requirements",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{},
 			insufficientResources: getAdmissionRequirementList(0, 0, 0),
 			expectErr:             false,
@@ -178,6 +179,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "no pods",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{},
 			insufficientResources: getAdmissionRequirementList(0, 0, 1),
 			expectErr:             true,
@@ -185,6 +187,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "equal pods and resources requirements",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[burstable]},
 			insufficientResources: getAdmissionRequirementList(100, 100, 1),
 			expectErr:             false,
@@ -192,6 +195,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "higher requirements than pod requests",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[burstable]},
 			insufficientResources: getAdmissionRequirementList(200, 200, 2),
 			expectErr:             true,
@@ -199,6 +203,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "choose between bestEffort and burstable",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[burstable], allPods[bestEffort]},
 			insufficientResources: getAdmissionRequirementList(0, 0, 1),
 			expectErr:             false,
@@ -206,6 +211,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "choose between burstable and guaranteed",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[burstable], allPods[guaranteed]},
 			insufficientResources: getAdmissionRequirementList(0, 0, 1),
 			expectErr:             false,
@@ -213,6 +219,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "choose lower request burstable if it meets requirements",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[bestEffort], allPods[highRequestBurstable], allPods[burstable]},
 			insufficientResources: getAdmissionRequirementList(100, 100, 0),
 			expectErr:             false,
@@ -220,6 +227,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "choose higher request burstable if lower does not meet requirements",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[bestEffort], allPods[burstable], allPods[highRequestBurstable]},
 			insufficientResources: getAdmissionRequirementList(150, 150, 0),
 			expectErr:             false,
@@ -227,6 +235,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "multiple pods required",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[bestEffort], allPods[burstable], allPods[highRequestBurstable], allPods[guaranteed], allPods[highRequestGuaranteed]},
 			insufficientResources: getAdmissionRequirementList(350, 350, 0),
 			expectErr:             false,
@@ -234,6 +243,7 @@ func TestGetPodsToPreempt(t *testing.T) {
 		},
 		{
 			testName:              "evict guaranteed when we have to, and dont evict the extra burstable",
+			preemptor:             allPods[critical],
 			inputPods:             []*v1.Pod{allPods[bestEffort], allPods[burstable], allPods[highRequestBurstable], allPods[guaranteed], allPods[highRequestGuaranteed]},
 			insufficientResources: getAdmissionRequirementList(0, 550, 0),
 			expectErr:             false,

pkg/kubelet/types/pod_update.go

@@ -159,13 +159,12 @@ func IsCriticalPod(pod *v1.Pod) bool {
 	return false
 }
 
-// Preemptable returns true if preemptor pod can preempt preemptee pod:
-//   - If preemptor's is greater than preemptee's priority, it's preemptable (return true)
-//   - If preemptor (or its priority) is nil and preemptee bears the critical pod annotation key,
-//     preemptee can not be preempted (return false)
-//   - If preemptor (or its priority) is nil and preemptee's priority is greater than or equal to
-//     SystemCriticalPriority, preemptee can not be preempted (return false)
+// Preemptable returns true if preemptor pod can preempt preemptee pod
+// if preemptee is not critical or if preemptor's priority is greater than preemptee's priority
 func Preemptable(preemptor, preemptee *v1.Pod) bool {
+	if IsCriticalPod(preemptor) && !IsCriticalPod(preemptee) {
+		return true
+	}
 	if utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
 		if (preemptor != nil && preemptor.Spec.Priority != nil) &&
 			(preemptee != nil && preemptee.Spec.Priority != nil) {
@@ -173,7 +172,7 @@ func Preemptable(preemptor, preemptee *v1.Pod) bool {
 		}
 	}
 
-	return !IsCriticalPod(preemptee)
+	return false
 }
 
 // IsCritical returns true if parameters bear the critical pod annotation

test/e2e_node/critical_pod_test.go

@@ -45,6 +45,9 @@ var _ = framework.KubeDescribe("CriticalPod [Serial] [Disruptive] [NodeFeature:C
 
 	Context("when we need to admit a critical pod", func() {
 		tempSetCurrentKubeletConfig(f, func(initialConfig *kubeletconfig.KubeletConfiguration) {
+			if initialConfig.FeatureGates == nil {
+				initialConfig.FeatureGates = make(map[string]bool)
+			}
 			initialConfig.FeatureGates[string(features.ExperimentalCriticalPodAnnotation)] = true
 		})
 
@@ -142,9 +145,9 @@ func getTestPod(critical bool, name string, resources v1.ResourceRequirements) *
 		pod.ObjectMeta.Annotations = map[string]string{
 			kubelettypes.CriticalPodAnnotationKey: "",
 		}
-		Expect(kubelettypes.IsCriticalPod(pod)).To(BeTrue(), "pod should be a critical pod")
+		Expect(kubelettypes.IsCritical(pod.Namespace, pod.Annotations)).To(BeTrue(), "pod should be a critical pod")
 	} else {
-		Expect(kubelettypes.IsCriticalPod(pod)).To(BeFalse(), "pod should not be a critical pod")
+		Expect(kubelettypes.IsCritical(pod.Namespace, pod.Annotations)).To(BeFalse(), "pod should not be a critical pod")
 	}
 	return pod
 }