From edec8fe01002a872a496b3bf4c10be6be624ab70 Mon Sep 17 00:00:00 2001 From: Lucas Severo Alves Date: Mon, 19 Sep 2022 18:18:46 +0200 Subject: [PATCH] add PreEvictionFilter extension to DefaultEvictor Plugin --- pkg/descheduler/descheduler.go | 5 + pkg/framework/fake/fake.go | 3 + .../plugins/defaultevictor/defaultevictor.go | 33 +- .../defaultevictor/defaultevictor_test.go | 528 +++++++++++------- .../nodeutilization/nodeutilization.go | 57 +- .../plugins/podlifetime/pod_lifetime.go | 3 +- .../removeduplicates/removeduplicates.go | 3 +- .../plugins/removefailedpods/failedpods.go | 3 +- .../toomanyrestarts.go | 3 +- .../pod_antiaffinity.go | 2 +- .../node_affinity.go | 3 +- .../node_taint.go | 3 +- .../topologyspreadconstraint.go | 5 +- pkg/framework/types.go | 3 + 14 files changed, 407 insertions(+), 247 deletions(-) diff --git a/pkg/descheduler/descheduler.go b/pkg/descheduler/descheduler.go index 35df742e53..b2a887c37b 100644 --- a/pkg/descheduler/descheduler.go +++ b/pkg/descheduler/descheduler.go @@ -181,6 +181,11 @@ func (ei *evictorImpl) Filter(pod *v1.Pod) bool { return ei.evictorFilter.Filter(pod) } +// PreEvictionFilter checks if pod can be evicted right before eviction +func (ei *evictorImpl) PreEvictionFilter(pod *v1.Pod) bool { + return ei.evictorFilter.PreEvictionFilter(pod) +} + // Evict evicts a pod (no pre-check performed) func (ei *evictorImpl) Evict(ctx context.Context, pod *v1.Pod, opts evictions.EvictOptions) bool { return ei.podEvictor.EvictPod(ctx, pod, opts) diff --git a/pkg/framework/fake/fake.go b/pkg/framework/fake/fake.go index 98e71e4d5a..c85f8b4956 100644 --- a/pkg/framework/fake/fake.go +++ b/pkg/framework/fake/fake.go @@ -37,6 +37,9 @@ func (hi *HandleImpl) Evictor() framework.Evictor { func (hi *HandleImpl) Filter(pod *v1.Pod) bool { return hi.EvictorFilterImpl.Filter(pod) } +func (hi *HandleImpl) PreEvictionFilter(pod *v1.Pod) bool { + return hi.EvictorFilterImpl.PreEvictionFilter(pod) +} func (hi *HandleImpl) Evict(ctx context.Context, pod *v1.Pod, opts evictions.EvictOptions) bool { return hi.PodEvictorImpl.EvictPod(ctx, pod, opts) } diff --git a/pkg/framework/plugins/defaultevictor/defaultevictor.go b/pkg/framework/plugins/defaultevictor/defaultevictor.go index 04d51ba771..8717fcdd48 100644 --- a/pkg/framework/plugins/defaultevictor/defaultevictor.go +++ b/pkg/framework/plugins/defaultevictor/defaultevictor.go @@ -44,7 +44,9 @@ type constraint func(pod *v1.Pod) error // This plugin is only meant to customize other actions (extension points) of the evictor, // like filtering, sorting, and other ones that might be relevant in the future type DefaultEvictor struct { + args runtime.Object constraints []constraint + handle framework.Handle } // IsPodEvictableBasedOnPriority checks if the given pod is evictable based on priority resolved from pod Spec. @@ -66,6 +68,8 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) } ev := &DefaultEvictor{} + ev.handle = handle + ev.args = defaultEvictorArgs if defaultEvictorArgs.EvictFailedBarePods { klog.V(1).InfoS("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.") @@ -125,18 +129,6 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) return nil }) } - if defaultEvictorArgs.NodeFit { - ev.constraints = append(ev.constraints, func(pod *v1.Pod) error { - nodes, err := nodeutil.ReadyNodes(context.TODO(), handle.ClientSet(), handle.SharedInformerFactory().Core().V1().Nodes(), defaultEvictorArgs.NodeSelector) - if err != nil { - return fmt.Errorf("could not list nodes when processing NodeFit") - } - if !nodeutil.PodFitsAnyOtherNode(handle.GetPodsAssignedToNodeFunc(), pod, nodes) { - return fmt.Errorf("pod does not fit on any other node because of nodeSelector(s), Taint(s), or nodes marked as unschedulable") - } - return nil - }) - } if defaultEvictorArgs.LabelSelector != nil && !defaultEvictorArgs.LabelSelector.Empty() { ev.constraints = append(ev.constraints, func(pod *v1.Pod) error { if !defaultEvictorArgs.LabelSelector.Matches(labels.Set(pod.Labels)) { @@ -154,6 +146,23 @@ func (d *DefaultEvictor) Name() string { return PluginName } +func (d *DefaultEvictor) PreEvictionFilter(pod *v1.Pod) bool { + defaultEvictorArgs := d.args.(*DefaultEvictorArgs) + if defaultEvictorArgs.NodeFit { + nodes, err := nodeutil.ReadyNodes(context.TODO(), d.handle.ClientSet(), d.handle.SharedInformerFactory().Core().V1().Nodes(), defaultEvictorArgs.NodeSelector) + if err != nil { + klog.V(1).ErrorS(fmt.Errorf("Pod fails the following checks"), "pod", klog.KObj(pod)) + return false + } + if !nodeutil.PodFitsAnyOtherNode(d.handle.GetPodsAssignedToNodeFunc(), pod, nodes) { + klog.V(1).ErrorS(fmt.Errorf("pod does not fit on any other node because of nodeSelector(s), Taint(s), or nodes marked as unschedulable"), "pod", klog.KObj(pod)) + return false + } + return true + } + return true +} + func (d *DefaultEvictor) Filter(pod *v1.Pod) bool { checkErrs := []error{} diff --git a/pkg/framework/plugins/defaultevictor/defaultevictor_test.go b/pkg/framework/plugins/defaultevictor/defaultevictor_test.go index 00c0a550e4..cf36b23518 100644 --- a/pkg/framework/plugins/defaultevictor/defaultevictor_test.go +++ b/pkg/framework/plugins/defaultevictor/defaultevictor_test.go @@ -30,6 +30,332 @@ import ( "sigs.k8s.io/descheduler/test" ) +func TestDefaultEvictorPreEvictionFilter(t *testing.T) { + n1 := test.BuildTestNode("node1", 1000, 2000, 13, nil) + + nodeTaintKey := "hardware" + nodeTaintValue := "gpu" + + nodeLabelKey := "datacenter" + nodeLabelValue := "east" + type testCase struct { + description string + pods []*v1.Pod + nodes []*v1.Node + evictFailedBarePods bool + evictLocalStoragePods bool + evictSystemCriticalPods bool + priorityThreshold *int32 + nodeFit bool + result bool + } + + testCases := []testCase{ + { + description: "Pod with no tolerations running on normal node, all other nodes tainted", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { + node.Spec.Taints = []v1.Taint{ + { + Key: nodeTaintKey, + Value: nodeTaintValue, + Effect: v1.TaintEffectNoSchedule, + }, + } + }), + test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { + node.Spec.Taints = []v1.Taint{ + { + Key: nodeTaintKey, + Value: nodeTaintValue, + Effect: v1.TaintEffectNoSchedule, + }, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: false, + }, { + description: "Pod with correct tolerations running on normal node, all other nodes tainted", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.Tolerations = []v1.Toleration{ + { + Key: nodeTaintKey, + Value: nodeTaintValue, + Effect: v1.TaintEffectNoSchedule, + }, + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { + node.Spec.Taints = []v1.Taint{ + { + Key: nodeTaintKey, + Value: nodeTaintValue, + Effect: v1.TaintEffectNoSchedule, + }, + } + }), + test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { + node.Spec.Taints = []v1.Taint{ + { + Key: nodeTaintKey, + Value: nodeTaintValue, + Effect: v1.TaintEffectNoSchedule, + }, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: true, + }, { + description: "Pod with incorrect node selector", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.NodeSelector = map[string]string{ + nodeLabelKey: "fail", + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: false, + }, { + description: "Pod with correct node selector", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.NodeSelector = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: true, + }, { + description: "Pod with correct node selector, but only available node doesn't have enough CPU", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 12, 8, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.NodeSelector = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2-TEST", 10, 16, 10, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestNode("node3-TEST", 10, 16, 10, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: false, + }, { + description: "Pod with correct node selector, and one node has enough memory", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 12, 8, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.NodeSelector = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestPod("node2-pod-10GB-mem", 20, 10, "node2", func(pod *v1.Pod) { + pod.ObjectMeta.Labels = map[string]string{ + "test": "true", + } + }), + test.BuildTestPod("node3-pod-10GB-mem", 20, 10, "node3", func(pod *v1.Pod) { + pod.ObjectMeta.Labels = map[string]string{ + "test": "true", + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 100, 16, 10, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestNode("node3", 100, 20, 10, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: true, + }, { + description: "Pod with correct node selector, but both nodes don't have enough memory", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 12, 8, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.NodeSelector = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestPod("node2-pod-10GB-mem", 10, 10, "node2", func(pod *v1.Pod) { + pod.ObjectMeta.Labels = map[string]string{ + "test": "true", + } + }), + test.BuildTestPod("node3-pod-10GB-mem", 10, 10, "node3", func(pod *v1.Pod) { + pod.ObjectMeta.Labels = map[string]string{ + "test": "true", + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 100, 16, 10, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestNode("node3", 100, 16, 10, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: true, + result: false, + }, { + description: "Pod with incorrect node selector, but nodefit false, should still be evicted", + pods: []*v1.Pod{ + test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { + pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() + pod.Spec.NodeSelector = map[string]string{ + nodeLabelKey: "fail", + } + }), + }, + nodes: []*v1.Node{ + test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { + node.ObjectMeta.Labels = map[string]string{ + nodeLabelKey: nodeLabelValue, + } + }), + }, + evictLocalStoragePods: false, + evictSystemCriticalPods: false, + nodeFit: false, + result: true, + }, + } + + for _, test := range testCases { + + t.Run(test.description, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var objs []runtime.Object + for _, node := range test.nodes { + objs = append(objs, node) + } + for _, pod := range test.pods { + objs = append(objs, pod) + } + + fakeClient := fake.NewSimpleClientset(objs...) + + sharedInformerFactory := informers.NewSharedInformerFactory(fakeClient, 0) + podInformer := sharedInformerFactory.Core().V1().Pods() + + getPodsAssignedToNode, err := podutil.BuildGetPodsAssignedToNodeFunc(podInformer) + if err != nil { + t.Errorf("Build get pods assigned to node function error: %v", err) + } + + sharedInformerFactory.Start(ctx.Done()) + sharedInformerFactory.WaitForCacheSync(ctx.Done()) + + defaultEvictorArgs := &DefaultEvictorArgs{ + EvictLocalStoragePods: test.evictLocalStoragePods, + EvictSystemCriticalPods: test.evictSystemCriticalPods, + IgnorePvcPods: false, + EvictFailedBarePods: test.evictFailedBarePods, + PriorityThreshold: &api.PriorityThreshold{ + Value: test.priorityThreshold, + }, + NodeFit: test.nodeFit, + } + + evictorPlugin, err := New( + defaultEvictorArgs, + &frameworkfake.HandleImpl{ + ClientsetImpl: fakeClient, + GetPodsAssignedToNodeFuncImpl: getPodsAssignedToNode, + SharedInformerFactoryImpl: sharedInformerFactory, + }) + if err != nil { + t.Fatalf("Unable to initialize the plugin: %v", err) + } + + result := evictorPlugin.(framework.EvictorPlugin).PreEvictionFilter(test.pods[0]) + if (result) != test.result { + t.Errorf("Filter should return for pod %s %t, but it returns %t", test.pods[0].Name, test.result, result) + } + }) + } +} + func TestDefaultEvictorFilter(t *testing.T) { n1 := test.BuildTestNode("node1", 1000, 2000, 13, nil) lowPriority := int32(800) @@ -38,8 +364,6 @@ func TestDefaultEvictorFilter(t *testing.T) { nodeTaintKey := "hardware" nodeTaintValue := "gpu" - nodeLabelKey := "datacenter" - nodeLabelValue := "east" type testCase struct { description string pods []*v1.Pod @@ -348,48 +672,10 @@ func TestDefaultEvictorFilter(t *testing.T) { priorityThreshold: &lowPriority, result: true, }, { - description: "Pod with no tolerations running on normal node, all other nodes tainted", - pods: []*v1.Pod{ - test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - }), - }, - nodes: []*v1.Node{ - test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { - node.Spec.Taints = []v1.Taint{ - { - Key: nodeTaintKey, - Value: nodeTaintValue, - Effect: v1.TaintEffectNoSchedule, - }, - } - }), - test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { - node.Spec.Taints = []v1.Taint{ - { - Key: nodeTaintKey, - Value: nodeTaintValue, - Effect: v1.TaintEffectNoSchedule, - }, - } - }), - }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, - }, { - description: "Pod with correct tolerations running on normal node, all other nodes tainted", + description: "Pod with no tolerations running on normal node, all other nodes tainted, no PreEvictionFilter, should ignore nodeFit", pods: []*v1.Pod{ test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Spec.Tolerations = []v1.Toleration{ - { - Key: nodeTaintKey, - Value: nodeTaintValue, - Effect: v1.TaintEffectNoSchedule, - }, - } }), }, nodes: []*v1.Node{ @@ -416,156 +702,6 @@ func TestDefaultEvictorFilter(t *testing.T) { evictSystemCriticalPods: false, nodeFit: true, result: true, - }, { - description: "Pod with incorrect node selector", - pods: []*v1.Pod{ - test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Spec.NodeSelector = map[string]string{ - nodeLabelKey: "fail", - } - }), - }, - nodes: []*v1.Node{ - test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, - }, { - description: "Pod with correct node selector", - pods: []*v1.Pod{ - test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Spec.NodeSelector = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - nodes: []*v1.Node{ - test.BuildTestNode("node2", 1000, 2000, 13, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestNode("node3", 1000, 2000, 13, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: true, - }, { - description: "Pod with correct node selector, but only available node doesn't have enough CPU", - pods: []*v1.Pod{ - test.BuildTestPod("p1", 12, 8, n1.Name, func(pod *v1.Pod) { - pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Spec.NodeSelector = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - nodes: []*v1.Node{ - test.BuildTestNode("node2-TEST", 10, 16, 10, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestNode("node3-TEST", 10, 16, 10, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, - }, { - description: "Pod with correct node selector, and one node has enough memory", - pods: []*v1.Pod{ - test.BuildTestPod("p1", 12, 8, n1.Name, func(pod *v1.Pod) { - pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Spec.NodeSelector = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestPod("node2-pod-10GB-mem", 20, 10, "node2", func(pod *v1.Pod) { - pod.ObjectMeta.Labels = map[string]string{ - "test": "true", - } - }), - test.BuildTestPod("node3-pod-10GB-mem", 20, 10, "node3", func(pod *v1.Pod) { - pod.ObjectMeta.Labels = map[string]string{ - "test": "true", - } - }), - }, - nodes: []*v1.Node{ - test.BuildTestNode("node2", 100, 16, 10, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestNode("node3", 100, 20, 10, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: true, - }, { - description: "Pod with correct node selector, but both nodes don't have enough memory", - pods: []*v1.Pod{ - test.BuildTestPod("p1", 12, 8, n1.Name, func(pod *v1.Pod) { - pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Spec.NodeSelector = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestPod("node2-pod-10GB-mem", 10, 10, "node2", func(pod *v1.Pod) { - pod.ObjectMeta.Labels = map[string]string{ - "test": "true", - } - }), - test.BuildTestPod("node3-pod-10GB-mem", 10, 10, "node3", func(pod *v1.Pod) { - pod.ObjectMeta.Labels = map[string]string{ - "test": "true", - } - }), - }, - nodes: []*v1.Node{ - test.BuildTestNode("node2", 100, 16, 10, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - test.BuildTestNode("node3", 100, 16, 10, func(node *v1.Node) { - node.ObjectMeta.Labels = map[string]string{ - nodeLabelKey: nodeLabelValue, - } - }), - }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, }, } @@ -596,14 +732,6 @@ func TestDefaultEvictorFilter(t *testing.T) { sharedInformerFactory.Start(ctx.Done()) sharedInformerFactory.WaitForCacheSync(ctx.Done()) - // var opts []func(opts *FilterOptions) - // if test.priorityThreshold != nil { - // opts = append(opts, WithPriorityThreshold(*test.priorityThreshold)) - // } - // if test.nodeFit { - // opts = append(opts, WithNodeFit(true)) - // } - defaultEvictorArgs := &DefaultEvictorArgs{ EvictLocalStoragePods: test.evictLocalStoragePods, EvictSystemCriticalPods: test.evictSystemCriticalPods, @@ -627,7 +755,7 @@ func TestDefaultEvictorFilter(t *testing.T) { } result := evictorPlugin.(framework.EvictorPlugin).Filter(test.pods[0]) - if result != test.result { + if (result) != test.result { t.Errorf("Filter should return for pod %s %t, but it returns %t", test.pods[0].Name, test.result, result) } diff --git a/pkg/framework/plugins/nodeutilization/nodeutilization.go b/pkg/framework/plugins/nodeutilization/nodeutilization.go index 8bd1c804ac..2b7ea4a2fa 100644 --- a/pkg/framework/plugins/nodeutilization/nodeutilization.go +++ b/pkg/framework/plugins/nodeutilization/nodeutilization.go @@ -18,9 +18,10 @@ package nodeutilization import ( "context" - "sigs.k8s.io/descheduler/pkg/api" "sort" + "sigs.k8s.io/descheduler/pkg/api" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/klog/v2" @@ -286,36 +287,38 @@ func evictPods( continue } - if podEvictor.Evict(ctx, pod, evictions.EvictOptions{}) { - klog.V(3).InfoS("Evicted pods", "pod", klog.KObj(pod)) - - for name := range totalAvailableUsage { - if name == v1.ResourcePods { - nodeInfo.usage[name].Sub(*resource.NewQuantity(1, resource.DecimalSI)) - totalAvailableUsage[name].Sub(*resource.NewQuantity(1, resource.DecimalSI)) - } else { - quantity := utils.GetResourceRequestQuantity(pod, name) - nodeInfo.usage[name].Sub(quantity) - totalAvailableUsage[name].Sub(quantity) + if podEvictor.PreEvictionFilter(pod) { + if podEvictor.Evict(ctx, pod, evictions.EvictOptions{}) { + klog.V(3).InfoS("Evicted pods", "pod", klog.KObj(pod)) + + for name := range totalAvailableUsage { + if name == v1.ResourcePods { + nodeInfo.usage[name].Sub(*resource.NewQuantity(1, resource.DecimalSI)) + totalAvailableUsage[name].Sub(*resource.NewQuantity(1, resource.DecimalSI)) + } else { + quantity := utils.GetResourceRequestQuantity(pod, name) + nodeInfo.usage[name].Sub(quantity) + totalAvailableUsage[name].Sub(quantity) + } } - } - keysAndValues := []interface{}{ - "node", nodeInfo.node.Name, - "CPU", nodeInfo.usage[v1.ResourceCPU].MilliValue(), - "Mem", nodeInfo.usage[v1.ResourceMemory].Value(), - "Pods", nodeInfo.usage[v1.ResourcePods].Value(), - } - for name := range totalAvailableUsage { - if !nodeutil.IsBasicResource(name) { - keysAndValues = append(keysAndValues, string(name), totalAvailableUsage[name].Value()) + keysAndValues := []interface{}{ + "node", nodeInfo.node.Name, + "CPU", nodeInfo.usage[v1.ResourceCPU].MilliValue(), + "Mem", nodeInfo.usage[v1.ResourceMemory].Value(), + "Pods", nodeInfo.usage[v1.ResourcePods].Value(), + } + for name := range totalAvailableUsage { + if !nodeutil.IsBasicResource(name) { + keysAndValues = append(keysAndValues, string(name), totalAvailableUsage[name].Value()) + } } - } - klog.V(3).InfoS("Updated node usage", keysAndValues...) - // check if pods can be still evicted - if !continueEviction(nodeInfo, totalAvailableUsage) { - break + klog.V(3).InfoS("Updated node usage", keysAndValues...) + // check if pods can be still evicted + if !continueEviction(nodeInfo, totalAvailableUsage) { + break + } } } if podEvictor.NodeLimitExceeded(nodeInfo.node) { diff --git a/pkg/framework/plugins/podlifetime/pod_lifetime.go b/pkg/framework/plugins/podlifetime/pod_lifetime.go index 7bb468c4c1..894b09da8a 100644 --- a/pkg/framework/plugins/podlifetime/pod_lifetime.go +++ b/pkg/framework/plugins/podlifetime/pod_lifetime.go @@ -56,8 +56,9 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) excludedNamespaces = sets.NewString(podLifeTimeArgs.Namespaces.Exclude...) } + // We can combine Filter and PreEvictionFilter since for this strategy it does not matter where we run PreEvictionFilter podFilter, err := podutil.NewOptions(). - WithFilter(handle.Evictor().Filter). + WithFilter(podutil.WrapFilterFuncs(handle.Evictor().Filter, handle.Evictor().PreEvictionFilter)). WithNamespaces(includedNamespaces). WithoutNamespaces(excludedNamespaces). WithLabelSelector(podLifeTimeArgs.LabelSelector). diff --git a/pkg/framework/plugins/removeduplicates/removeduplicates.go b/pkg/framework/plugins/removeduplicates/removeduplicates.go index 40973f1d37..b808bebafc 100644 --- a/pkg/framework/plugins/removeduplicates/removeduplicates.go +++ b/pkg/framework/plugins/removeduplicates/removeduplicates.go @@ -71,8 +71,9 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) excludedNamespaces = sets.NewString(removeDuplicatesArgs.Namespaces.Exclude...) } + // We can combine Filter and PreEvictionFilter since for this strategy it does not matter where we run PreEvictionFilter podFilter, err := podutil.NewOptions(). - WithFilter(handle.Evictor().Filter). + WithFilter(podutil.WrapFilterFuncs(handle.Evictor().Filter, handle.Evictor().PreEvictionFilter)). WithNamespaces(includedNamespaces). WithoutNamespaces(excludedNamespaces). BuildFilterFunc() diff --git a/pkg/framework/plugins/removefailedpods/failedpods.go b/pkg/framework/plugins/removefailedpods/failedpods.go index 99ca628cfd..87435d4554 100644 --- a/pkg/framework/plugins/removefailedpods/failedpods.go +++ b/pkg/framework/plugins/removefailedpods/failedpods.go @@ -57,8 +57,9 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) excludedNamespaces = sets.NewString(failedPodsArgs.Namespaces.Exclude...) } + // We can combine Filter and PreEvictionFilter since for this strategy it does not matter where we run PreEvictionFilter podFilter, err := podutil.NewOptions(). - WithFilter(handle.Evictor().Filter). + WithFilter(podutil.WrapFilterFuncs(handle.Evictor().Filter, handle.Evictor().PreEvictionFilter)). WithNamespaces(includedNamespaces). WithoutNamespaces(excludedNamespaces). WithLabelSelector(failedPodsArgs.LabelSelector). diff --git a/pkg/framework/plugins/removepodshavingtoomanyrestarts/toomanyrestarts.go b/pkg/framework/plugins/removepodshavingtoomanyrestarts/toomanyrestarts.go index c879f6c3ea..7814736cff 100644 --- a/pkg/framework/plugins/removepodshavingtoomanyrestarts/toomanyrestarts.go +++ b/pkg/framework/plugins/removepodshavingtoomanyrestarts/toomanyrestarts.go @@ -57,8 +57,9 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) excludedNamespaces = sets.NewString(tooManyRestartsArgs.Namespaces.Exclude...) } + // We can combine Filter and PreEvictionFilter since for this strategy it does not matter where we run PreEvictionFilter podFilter, err := podutil.NewOptions(). - WithFilter(handle.Evictor().Filter). + WithFilter(podutil.WrapFilterFuncs(handle.Evictor().Filter, handle.Evictor().PreEvictionFilter)). WithNamespaces(includedNamespaces). WithoutNamespaces(excludedNamespaces). WithLabelSelector(tooManyRestartsArgs.LabelSelector). diff --git a/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go index 454fa12ff7..25916a3b6f 100644 --- a/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go +++ b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go @@ -92,7 +92,7 @@ loop: podutil.SortPodsBasedOnPriorityLowToHigh(pods) totalPods := len(pods) for i := 0; i < totalPods; i++ { - if checkPodsWithAntiAffinityExist(pods[i], pods) && d.handle.Evictor().Filter(pods[i]) { + if checkPodsWithAntiAffinityExist(pods[i], pods) && d.handle.Evictor().Filter(pods[i]) && d.handle.Evictor().PreEvictionFilter(pods[i]) { if d.handle.Evictor().Evict(ctx, pods[i], evictions.EvictOptions{}) { // Since the current pod is evicted all other pods which have anti-affinity with this // pod need not be evicted. diff --git a/pkg/framework/plugins/removepodsviolatingnodeaffinity/node_affinity.go b/pkg/framework/plugins/removepodsviolatingnodeaffinity/node_affinity.go index 1143de354e..23fc418b72 100644 --- a/pkg/framework/plugins/removepodsviolatingnodeaffinity/node_affinity.go +++ b/pkg/framework/plugins/removepodsviolatingnodeaffinity/node_affinity.go @@ -54,8 +54,9 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) excludedNamespaces = sets.NewString(nodeAffinityArgs.Namespaces.Exclude...) } + // We can combine Filter and PreEvictionFilter since for this strategy it does not matter where we run PreEvictionFilter podFilter, err := podutil.NewOptions(). - WithFilter(handle.Evictor().Filter). + WithFilter(podutil.WrapFilterFuncs(handle.Evictor().Filter, handle.Evictor().PreEvictionFilter)). WithNamespaces(includedNamespaces). WithoutNamespaces(excludedNamespaces). WithLabelSelector(nodeAffinityArgs.LabelSelector). diff --git a/pkg/framework/plugins/removepodsviolatingnodetaints/node_taint.go b/pkg/framework/plugins/removepodsviolatingnodetaints/node_taint.go index e510ca3e20..da7fbd2c11 100644 --- a/pkg/framework/plugins/removepodsviolatingnodetaints/node_taint.go +++ b/pkg/framework/plugins/removepodsviolatingnodetaints/node_taint.go @@ -57,8 +57,9 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) excludedNamespaces = sets.NewString(nodeTaintsArgs.Namespaces.Exclude...) } + // We can combine Filter and PreEvictionFilter since for this strategy it does not matter where we run PreEvictionFilter podFilter, err := podutil.NewOptions(). - WithFilter(handle.Evictor().Filter). + WithFilter(podutil.WrapFilterFuncs(handle.Evictor().Filter, handle.Evictor().PreEvictionFilter)). WithNamespaces(includedNamespaces). WithoutNamespaces(excludedNamespaces). WithLabelSelector(nodeTaintsArgs.LabelSelector). diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index 2c14ee4eee..034e4d09bc 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go @@ -215,7 +215,10 @@ func (d *RemovePodsViolatingTopologySpreadConstraint) Balance(ctx context.Contex if !d.podFilter(pod) { continue } - d.handle.Evictor().Evict(ctx, pod, evictions.EvictOptions{}) + + if d.handle.Evictor().PreEvictionFilter(pod) { + d.handle.Evictor().Evict(ctx, pod, evictions.EvictOptions{}) + } if d.handle.Evictor().NodeLimitExceeded(nodeMap[pod.Spec.NodeName]) { nodeLimitExceeded[pod.Spec.NodeName] = true } diff --git a/pkg/framework/types.go b/pkg/framework/types.go index b2ffdfb36b..b6f617dd14 100644 --- a/pkg/framework/types.go +++ b/pkg/framework/types.go @@ -43,6 +43,8 @@ type Handle interface { type Evictor interface { // Filter checks if a pod can be evicted Filter(*v1.Pod) bool + // PreEvictionFilter checks if pod can be evicted right before eviction + PreEvictionFilter(*v1.Pod) bool // Evict evicts a pod (no pre-check performed) Evict(context.Context, *v1.Pod, evictions.EvictOptions) bool // NodeLimitExceeded checks if the number of evictions for a node was exceeded @@ -78,4 +80,5 @@ type BalancePlugin interface { type EvictorPlugin interface { Plugin Filter(pod *v1.Pod) bool + PreEvictionFilter(pod *v1.Pod) bool }