From 95ce2a4ff77a6058efbd40e110005d4756ce61ee Mon Sep 17 00:00:00 2001 From: lixiang Date: Fri, 7 Aug 2020 16:03:50 +0800 Subject: [PATCH] PodEvictor: add a new param thresholdPriority to IsEvictable --- pkg/descheduler/evictions/evictions.go | 11 ++++- pkg/descheduler/evictions/evictions_test.go | 45 +++++++++++++------ pkg/descheduler/strategies/duplicates.go | 5 ++- .../strategies/lownodeutilization.go | 2 +- pkg/descheduler/strategies/node_affinity.go | 3 +- pkg/descheduler/strategies/node_taint.go | 4 +- .../strategies/pod_antiaffinity.go | 4 +- pkg/descheduler/strategies/pod_lifetime.go | 7 ++- pkg/descheduler/strategies/toomanyrestarts.go | 5 ++- pkg/utils/pod.go | 10 +---- test/e2e/e2e_test.go | 11 ++++- 11 files changed, 73 insertions(+), 34 deletions(-) diff --git a/pkg/descheduler/evictions/evictions.go b/pkg/descheduler/evictions/evictions.go index 2c3acf2cb..7a5c71234 100644 --- a/pkg/descheduler/evictions/evictions.go +++ b/pkg/descheduler/evictions/evictions.go @@ -78,12 +78,16 @@ func NewPodEvictor( } // IsEvictable checks if a pod is evictable or not. -func (pe *PodEvictor) IsEvictable(pod *v1.Pod) bool { +func (pe *PodEvictor) IsEvictable(pod *v1.Pod, thresholdPriority int32) bool { checkErrs := []error{} if IsCriticalPod(pod) { checkErrs = append(checkErrs, fmt.Errorf("pod is critical")) } + if !IsPodEvictableBasedOnPriority(pod, thresholdPriority) { + checkErrs = append(checkErrs, fmt.Errorf("pod is not evictable due to its priority")) + } + ownerRefList := podutil.OwnerRef(pod) if IsDaemonsetPod(ownerRefList) { checkErrs = append(checkErrs, fmt.Errorf("pod is a DaemonSet pod")) @@ -216,3 +220,8 @@ func IsPodWithLocalStorage(pod *v1.Pod) bool { return false } + +// IsPodEvictableBasedOnPriority checks if the given pod is evictable based on priority resolved from pod Spec. +func IsPodEvictableBasedOnPriority(pod *v1.Pod, priority int32) bool { + return pod.Spec.Priority == nil || (*pod.Spec.Priority < priority && *pod.Spec.Priority < utils.SystemCriticalPriority) +} diff --git a/pkg/descheduler/evictions/evictions_test.go b/pkg/descheduler/evictions/evictions_test.go index 52c9d3f5f..81f3155f5 100644 --- a/pkg/descheduler/evictions/evictions_test.go +++ b/pkg/descheduler/evictions/evictions_test.go @@ -71,10 +71,13 @@ func TestEvictPod(t *testing.T) { func TestIsEvictable(t *testing.T) { n1 := test.BuildTestNode("node1", 1000, 2000, 13, nil) + lowPriority := int32(800) + highPriority := int32(900) type testCase struct { pod *v1.Pod runBefore func(*v1.Pod) evictLocalStoragePods bool + priorityThreshold *int32 result bool } @@ -179,6 +182,7 @@ func TestIsEvictable(t *testing.T) { }, { pod: test.BuildTestPod("p10", 400, 0, n1.Name, nil), runBefore: func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() pod.Annotations = test.GetMirrorPodAnnotation() }, evictLocalStoragePods: false, @@ -186,6 +190,7 @@ func TestIsEvictable(t *testing.T) { }, { pod: test.BuildTestPod("p11", 400, 0, n1.Name, nil), runBefore: func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() pod.Annotations = test.GetMirrorPodAnnotation() pod.Annotations["descheduler.alpha.kubernetes.io/evict"] = "true" }, @@ -194,6 +199,7 @@ func TestIsEvictable(t *testing.T) { }, { pod: test.BuildTestPod("p12", 400, 0, n1.Name, nil), runBefore: func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() priority := utils.SystemCriticalPriority pod.Spec.Priority = &priority }, @@ -202,6 +208,7 @@ func TestIsEvictable(t *testing.T) { }, { pod: test.BuildTestPod("p13", 400, 0, n1.Name, nil), runBefore: func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() priority := utils.SystemCriticalPriority pod.Spec.Priority = &priority pod.Annotations = map[string]string{ @@ -210,6 +217,25 @@ func TestIsEvictable(t *testing.T) { }, evictLocalStoragePods: false, result: true, + }, { + pod: test.BuildTestPod("p14", 400, 0, n1.Name, nil), + runBefore: func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.Priority = &highPriority + }, + evictLocalStoragePods: false, + priorityThreshold: &lowPriority, + result: false, + }, { + pod: test.BuildTestPod("p15", 400, 0, n1.Name, nil), + runBefore: func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Spec.Priority = &highPriority + }, + evictLocalStoragePods: false, + priorityThreshold: &lowPriority, + result: true, }, } @@ -218,7 +244,11 @@ func TestIsEvictable(t *testing.T) { podEvictor := &PodEvictor{ evictLocalStoragePods: test.evictLocalStoragePods, } - result := podEvictor.IsEvictable(test.pod) + testPriorityThreshold := utils.SystemCriticalPriority + if test.priorityThreshold != nil { + testPriorityThreshold = *test.priorityThreshold + } + result := podEvictor.IsEvictable(test.pod, testPriorityThreshold) if result != test.result { t.Errorf("IsEvictable should return for pod %s %t, but it returns %t", test.pod.Name, test.result, result) } @@ -233,10 +263,6 @@ func TestPodTypes(t *testing.T) { p2 := test.BuildTestPod("p2", 400, 0, n1.Name, nil) p3 := test.BuildTestPod("p3", 400, 0, n1.Name, nil) p4 := test.BuildTestPod("p4", 400, 0, n1.Name, nil) - p5 := test.BuildTestPod("p5", 400, 0, n1.Name, nil) - p6 := test.BuildTestPod("p6", 400, 0, n1.Name, nil) - - p6.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() p1.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList() // The following 4 pods won't get evicted. @@ -257,18 +283,9 @@ func TestPodTypes(t *testing.T) { } // A Mirror Pod. p4.Annotations = test.GetMirrorPodAnnotation() - // A Critical Pod. - p5.Namespace = "kube-system" - priority := utils.SystemCriticalPriority - p5.Spec.Priority = &priority - systemCriticalPriority := utils.SystemCriticalPriority - p5.Spec.Priority = &systemCriticalPriority if !IsMirrorPod(p4) { t.Errorf("Expected p4 to be a mirror pod.") } - if !IsCriticalPod(p5) { - t.Errorf("Expected p5 to be a critical pod.") - } if !IsPodWithLocalStorage(p3) { t.Errorf("Expected p3 to be a pod with local storage.") } diff --git a/pkg/descheduler/strategies/duplicates.go b/pkg/descheduler/strategies/duplicates.go index d14582352..86cdc4568 100644 --- a/pkg/descheduler/strategies/duplicates.go +++ b/pkg/descheduler/strategies/duplicates.go @@ -31,6 +31,7 @@ import ( "sigs.k8s.io/descheduler/pkg/api" "sigs.k8s.io/descheduler/pkg/descheduler/evictions" podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod" + "sigs.k8s.io/descheduler/pkg/utils" ) // RemoveDuplicatePods removes the duplicate pods on node. This strategy evicts all duplicate pods on node. @@ -46,7 +47,9 @@ func RemoveDuplicatePods( ) { for _, node := range nodes { klog.V(1).Infof("Processing node: %#v", node.Name) - pods, err := podutil.ListPodsOnANode(ctx, client, node, podutil.WithFilter(podEvictor.IsEvictable)) + pods, err := podutil.ListPodsOnANode(ctx, client, node, podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + })) if err != nil { klog.Errorf("error listing evictable pods on node %s: %+v", node.Name, err) continue diff --git a/pkg/descheduler/strategies/lownodeutilization.go b/pkg/descheduler/strategies/lownodeutilization.go index 788775946..1042212e5 100644 --- a/pkg/descheduler/strategies/lownodeutilization.go +++ b/pkg/descheduler/strategies/lownodeutilization.go @@ -397,7 +397,7 @@ func classifyPods(pods []*v1.Pod, evictor *evictions.PodEvictor) ([]*v1.Pod, []* var nonRemovablePods, removablePods []*v1.Pod for _, pod := range pods { - if !evictor.IsEvictable(pod) { + if !evictor.IsEvictable(pod, utils.SystemCriticalPriority) { nonRemovablePods = append(nonRemovablePods, pod) } else { removablePods = append(removablePods, pod) diff --git a/pkg/descheduler/strategies/node_affinity.go b/pkg/descheduler/strategies/node_affinity.go index 5454ce947..6da8bc240 100644 --- a/pkg/descheduler/strategies/node_affinity.go +++ b/pkg/descheduler/strategies/node_affinity.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/descheduler/pkg/descheduler/evictions" nodeutil "sigs.k8s.io/descheduler/pkg/descheduler/node" podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod" + "sigs.k8s.io/descheduler/pkg/utils" ) func validatePodsViolatingNodeAffinityParams(params *api.StrategyParameters) error { @@ -61,7 +62,7 @@ func RemovePodsViolatingNodeAffinity(ctx context.Context, client clientset.Inter client, node, podutil.WithFilter(func(pod *v1.Pod) bool { - return podEvictor.IsEvictable(pod) && + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) && !nodeutil.PodFitsCurrentNode(pod, node) && nodeutil.PodFitsAnyNode(pod, nodes) }), diff --git a/pkg/descheduler/strategies/node_taint.go b/pkg/descheduler/strategies/node_taint.go index 529b6b6b3..a1b34c61b 100644 --- a/pkg/descheduler/strategies/node_taint.go +++ b/pkg/descheduler/strategies/node_taint.go @@ -60,7 +60,9 @@ func RemovePodsViolatingNodeTaints(ctx context.Context, client clientset.Interfa ctx, client, node, - podutil.WithFilter(podEvictor.IsEvictable), + podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + }), podutil.WithNamespaces(namespaces.Include), podutil.WithoutNamespaces(namespaces.Exclude), ) diff --git a/pkg/descheduler/strategies/pod_antiaffinity.go b/pkg/descheduler/strategies/pod_antiaffinity.go index 66667e3fc..759345b5b 100644 --- a/pkg/descheduler/strategies/pod_antiaffinity.go +++ b/pkg/descheduler/strategies/pod_antiaffinity.go @@ -57,7 +57,9 @@ func RemovePodsViolatingInterPodAntiAffinity(ctx context.Context, client clients ctx, client, node, - podutil.WithFilter(podEvictor.IsEvictable), + podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + }), podutil.WithNamespaces(namespaces.Include), podutil.WithoutNamespaces(namespaces.Exclude), ) diff --git a/pkg/descheduler/strategies/pod_lifetime.go b/pkg/descheduler/strategies/pod_lifetime.go index a214dc3cf..828ccfa12 100644 --- a/pkg/descheduler/strategies/pod_lifetime.go +++ b/pkg/descheduler/strategies/pod_lifetime.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/descheduler/pkg/api" "sigs.k8s.io/descheduler/pkg/descheduler/evictions" podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod" + "sigs.k8s.io/descheduler/pkg/utils" ) func validatePodLifeTimeParams(params *api.StrategyParameters) error { @@ -69,12 +70,14 @@ func PodLifeTime(ctx context.Context, client clientset.Interface, strategy api.D } } -func listOldPodsOnNode(ctx context.Context, client clientset.Interface, node *v1.Node, params *api.StrategyParameters, evictor *evictions.PodEvictor) []*v1.Pod { +func listOldPodsOnNode(ctx context.Context, client clientset.Interface, node *v1.Node, params *api.StrategyParameters, podEvictor *evictions.PodEvictor) []*v1.Pod { pods, err := podutil.ListPodsOnANode( ctx, client, node, - podutil.WithFilter(evictor.IsEvictable), + podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + }), podutil.WithNamespaces(params.Namespaces.Include), podutil.WithoutNamespaces(params.Namespaces.Exclude), ) diff --git a/pkg/descheduler/strategies/toomanyrestarts.go b/pkg/descheduler/strategies/toomanyrestarts.go index ae745b45f..dac1d704f 100644 --- a/pkg/descheduler/strategies/toomanyrestarts.go +++ b/pkg/descheduler/strategies/toomanyrestarts.go @@ -27,6 +27,7 @@ import ( "sigs.k8s.io/descheduler/pkg/api" "sigs.k8s.io/descheduler/pkg/descheduler/evictions" podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod" + "sigs.k8s.io/descheduler/pkg/utils" ) func validateRemovePodsHavingTooManyRestartsParams(params *api.StrategyParameters) error { @@ -56,7 +57,9 @@ func RemovePodsHavingTooManyRestarts(ctx context.Context, client clientset.Inter ctx, client, node, - podutil.WithFilter(podEvictor.IsEvictable), + podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + }), podutil.WithNamespaces(strategy.Params.Namespaces.Include), podutil.WithoutNamespaces(strategy.Params.Namespaces.Exclude), ) diff --git a/pkg/utils/pod.go b/pkg/utils/pod.go index 7299491c3..0beaa6610 100644 --- a/pkg/utils/pod.go +++ b/pkg/utils/pod.go @@ -104,7 +104,7 @@ func GetPodSource(pod *v1.Pod) (string, error) { return "", fmt.Errorf("cannot get source of pod %q", pod.UID) } -// IsCriticalPod returns true if pod's priority is greater than or equal to SystemCriticalPriority. +// IsCriticalPod returns true if the pod is a static or mirror pod. func IsCriticalPod(pod *v1.Pod) bool { if IsStaticPod(pod) { return true @@ -112,17 +112,9 @@ func IsCriticalPod(pod *v1.Pod) bool { if IsMirrorPod(pod) { return true } - if pod.Spec.Priority != nil && IsCriticalPodBasedOnPriority(*pod.Spec.Priority) { - return true - } return false } -// IsCriticalPodBasedOnPriority checks if the given pod is a critical pod based on priority resolved from pod Spec. -func IsCriticalPodBasedOnPriority(priority int32) bool { - return priority >= SystemCriticalPriority -} - // PodRequestsAndLimits returns a dictionary of all defined resources summed up for all // containers of the pod. If PodOverhead feature is enabled, pod overhead is added to the // total container resource requests and to the total container limits which have a diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 2449298f0..307b1505a 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -20,6 +20,7 @@ import ( "context" "math" "os" + "sigs.k8s.io/descheduler/pkg/utils" "sort" "strings" "testing" @@ -509,7 +510,10 @@ func evictPods(ctx context.Context, t *testing.T, clientSet clientset.Interface, continue } // List all the pods on the current Node - podsOnANode, err := podutil.ListPodsOnANode(ctx, clientSet, node, podutil.WithFilter(podEvictor.IsEvictable)) + podsOnANode, err := podutil.ListPodsOnANode(ctx, clientSet, node, + podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + })) if err != nil { t.Errorf("Error listing pods on a node %v", err) } @@ -521,7 +525,10 @@ func evictPods(ctx context.Context, t *testing.T, clientSet clientset.Interface, } t.Log("Eviction of pods starting") startEndToEndForLowNodeUtilization(ctx, clientSet, nodeInformer, podEvictor) - podsOnleastUtilizedNode, err := podutil.ListPodsOnANode(ctx, clientSet, leastLoadedNode, podutil.WithFilter(podEvictor.IsEvictable)) + podsOnleastUtilizedNode, err := podutil.ListPodsOnANode(ctx, clientSet, leastLoadedNode, + podutil.WithFilter(func(pod *v1.Pod) bool { + return podEvictor.IsEvictable(pod, utils.SystemCriticalPriority) + })) if err != nil { t.Errorf("Error listing pods on a node %v", err) }