From 90f58c11575efecbbe05d71340d43ed3400d6f1a Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Fri, 7 Sep 2018 17:43:42 -0700 Subject: [PATCH] critical pod test should not rely on feature gate set in framework; non-critical pods are always preemptable --- pkg/kubelet/preemption/preemption_test.go | 12 +++++++++++- pkg/kubelet/types/pod_update.go | 13 ++++++------- test/e2e_node/critical_pod_test.go | 7 +++++-- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/pkg/kubelet/preemption/preemption_test.go b/pkg/kubelet/preemption/preemption_test.go index e66837e102..3e85348078 100644 --- a/pkg/kubelet/preemption/preemption_test.go +++ b/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, diff --git a/pkg/kubelet/types/pod_update.go b/pkg/kubelet/types/pod_update.go index 062d10e123..bdc5ee7944 100644 --- a/pkg/kubelet/types/pod_update.go +++ b/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 diff --git a/test/e2e_node/critical_pod_test.go b/test/e2e_node/critical_pod_test.go index 6eecd6ca94..297be27707 100644 --- a/test/e2e_node/critical_pod_test.go +++ b/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 }