From 012ca2398fe6d0526641db2735005138bd5d6f0a Mon Sep 17 00:00:00 2001 From: Amir Alavi Date: Mon, 24 May 2021 19:51:36 -0400 Subject: [PATCH] Filter pods by labelSelector during eviction for TopologySpreadConstraint strategy --- README.md | 4 + pkg/descheduler/evictions/evictions.go | 24 +- .../strategies/topologyspreadconstraint.go | 52 ++- .../topologyspreadconstraint_test.go | 295 +++++++++--------- 4 files changed, 213 insertions(+), 162 deletions(-) diff --git a/README.md b/README.md index 87b88475f..55767cfdc 100644 --- a/README.md +++ b/README.md @@ -346,6 +346,8 @@ This strategy requires k8s version 1.18 at a minimum. By default, this strategy only deals with hard constraints, setting parameter `includeSoftConstraints` to `true` will include soft constraints. +Strategy parameter `labelSelector` is not utilized when balancing topology domains and is only applied during eviction to determine if the pod can be evicted. + **Parameters:** |Name|Type| @@ -354,6 +356,7 @@ include soft constraints. |`thresholdPriority`|int (see [priority filtering](#priority-filtering))| |`thresholdPriorityClassName`|string (see [priority filtering](#priority-filtering))| |`namespaces`|(see [namespace filtering](#namespace-filtering))| +|`labelSelector`|(see [label filtering](#label-filtering))| |`nodeFit`|bool (see [node fit filtering](#node-fit-filtering))| **Example:** @@ -537,6 +540,7 @@ to filter pods by their labels: * `RemovePodsViolatingNodeTaints` * `RemovePodsViolatingNodeAffinity` * `RemovePodsViolatingInterPodAntiAffinity` +* `RemovePodsViolatingTopologySpreadConstraint` This allows running strategies among pods the descheduler is interested in. diff --git a/pkg/descheduler/evictions/evictions.go b/pkg/descheduler/evictions/evictions.go index 127041f70..b156618f9 100644 --- a/pkg/descheduler/evictions/evictions.go +++ b/pkg/descheduler/evictions/evictions.go @@ -25,6 +25,7 @@ import ( policy "k8s.io/api/policy/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/errors" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" @@ -166,8 +167,9 @@ func evictPod(ctx context.Context, client clientset.Interface, pod *v1.Pod, poli } type Options struct { - priority *int32 - nodeFit bool + priority *int32 + nodeFit bool + labelSelector labels.Selector } // WithPriorityThreshold sets a threshold for pod's priority class. @@ -180,7 +182,7 @@ func WithPriorityThreshold(priority int32) func(opts *Options) { } // WithNodeFit sets whether or not to consider taints, node selectors, -// and pod affinity when evicting. A pod who's tolerations, node selectors, +// and pod affinity when evicting. A pod whose tolerations, node selectors, // and affinity match a node other than the one it is currently running on // is evictable. func WithNodeFit(nodeFit bool) func(opts *Options) { @@ -189,6 +191,14 @@ func WithNodeFit(nodeFit bool) func(opts *Options) { } } +// WithLabelSelector sets whether or not to apply label filtering when evicting. +// Any pod matching the label selector is considered evictable. +func WithLabelSelector(labelSelector labels.Selector) func(opts *Options) { + return func(opts *Options) { + opts.labelSelector = labelSelector + } +} + type constraint func(pod *v1.Pod) error type evictable struct { @@ -247,6 +257,14 @@ func (pe *PodEvictor) Evictable(opts ...func(opts *Options)) *evictable { return nil }) } + if options.labelSelector != nil && !options.labelSelector.Empty() { + ev.constraints = append(ev.constraints, func(pod *v1.Pod) error { + if !options.labelSelector.Matches(labels.Set(pod.Labels)) { + return fmt.Errorf("pod labels do not match the labelSelector filter in the policy parameter") + } + return nil + }) + } return ev } diff --git a/pkg/descheduler/strategies/topologyspreadconstraint.go b/pkg/descheduler/strategies/topologyspreadconstraint.go index c2bfd1cf0..b5d41556c 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint.go @@ -47,28 +47,51 @@ type topology struct { pods []*v1.Pod } -func validateAndParseTopologySpreadParams(ctx context.Context, client clientset.Interface, params *api.StrategyParameters) (int32, sets.String, sets.String, error) { +// topologySpreadStrategyParams contains validated strategy parameters +type topologySpreadStrategyParams struct { + thresholdPriority int32 + includedNamespaces sets.String + excludedNamespaces sets.String + labelSelector labels.Selector + nodeFit bool +} + +// validateAndParseTopologySpreadParams will validate parameters to ensure that they do not contain invalid values. +func validateAndParseTopologySpreadParams(ctx context.Context, client clientset.Interface, params *api.StrategyParameters) (*topologySpreadStrategyParams, error) { var includedNamespaces, excludedNamespaces sets.String if params == nil { - return 0, includedNamespaces, excludedNamespaces, nil + return &topologySpreadStrategyParams{includedNamespaces: includedNamespaces, excludedNamespaces: excludedNamespaces}, nil } // At most one of include/exclude can be set if params.Namespaces != nil && len(params.Namespaces.Include) > 0 && len(params.Namespaces.Exclude) > 0 { - return 0, includedNamespaces, excludedNamespaces, fmt.Errorf("only one of Include/Exclude namespaces can be set") + return nil, fmt.Errorf("only one of Include/Exclude namespaces can be set") } if params.ThresholdPriority != nil && params.ThresholdPriorityClassName != "" { - return 0, includedNamespaces, excludedNamespaces, fmt.Errorf("only one of thresholdPriority and thresholdPriorityClassName can be set") + return nil, fmt.Errorf("only one of thresholdPriority and thresholdPriorityClassName can be set") } thresholdPriority, err := utils.GetPriorityFromStrategyParams(ctx, client, params) if err != nil { - return 0, includedNamespaces, excludedNamespaces, fmt.Errorf("failed to get threshold priority from strategy's params: %+v", err) + return nil, fmt.Errorf("failed to get threshold priority from strategy's params: %+v", err) } if params.Namespaces != nil { includedNamespaces = sets.NewString(params.Namespaces.Include...) excludedNamespaces = sets.NewString(params.Namespaces.Exclude...) } + var selector labels.Selector + if params.LabelSelector != nil { + selector, err = metav1.LabelSelectorAsSelector(params.LabelSelector) + if err != nil { + return nil, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + } + } - return thresholdPriority, includedNamespaces, excludedNamespaces, nil + return &topologySpreadStrategyParams{ + thresholdPriority: thresholdPriority, + includedNamespaces: includedNamespaces, + excludedNamespaces: excludedNamespaces, + labelSelector: selector, + nodeFit: params.NodeFit, + }, nil } func RemovePodsViolatingTopologySpreadConstraint( @@ -78,18 +101,17 @@ func RemovePodsViolatingTopologySpreadConstraint( nodes []*v1.Node, podEvictor *evictions.PodEvictor, ) { - thresholdPriority, includedNamespaces, excludedNamespaces, err := validateAndParseTopologySpreadParams(ctx, client, strategy.Params) + strategyParams, err := validateAndParseTopologySpreadParams(ctx, client, strategy.Params) if err != nil { klog.ErrorS(err, "Invalid RemovePodsViolatingTopologySpreadConstraint parameters") return } - nodeFit := false - if strategy.Params != nil { - nodeFit = strategy.Params.NodeFit - } - - evictable := podEvictor.Evictable(evictions.WithPriorityThreshold(thresholdPriority), evictions.WithNodeFit(nodeFit)) + evictable := podEvictor.Evictable( + evictions.WithPriorityThreshold(strategyParams.thresholdPriority), + evictions.WithNodeFit(strategyParams.nodeFit), + evictions.WithLabelSelector(strategyParams.labelSelector), + ) nodeMap := make(map[string]*v1.Node, len(nodes)) for _, node := range nodes { @@ -118,8 +140,8 @@ func RemovePodsViolatingTopologySpreadConstraint( podsForEviction := make(map[*v1.Pod]struct{}) // 1. for each namespace... for _, namespace := range namespaces.Items { - if (len(includedNamespaces) > 0 && !includedNamespaces.Has(namespace.Name)) || - (len(excludedNamespaces) > 0 && excludedNamespaces.Has(namespace.Name)) { + if (len(strategyParams.includedNamespaces) > 0 && !strategyParams.includedNamespaces.Has(namespace.Name)) || + (len(strategyParams.excludedNamespaces) > 0 && strategyParams.excludedNamespaces.Has(namespace.Name)) { continue } namespacePods, err := client.CoreV1().Pods(namespace.Name).List(ctx, metav1.ListOptions{}) diff --git a/pkg/descheduler/strategies/topologyspreadconstraint_test.go b/pkg/descheduler/strategies/topologyspreadconstraint_test.go index 2d605fa04..d1b608be4 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint_test.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint_test.go @@ -73,17 +73,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 2, @@ -147,18 +140,11 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, - noOwners: true, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + noOwners: true, }, { count: 2, @@ -189,17 +175,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 2, @@ -224,17 +203,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 4, @@ -263,17 +235,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 3, @@ -297,17 +262,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), nodeSelector: map[string]string{"zone": "zoneA"}, }, { @@ -354,17 +312,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), nodeSelector: map[string]string{"region": "boston"}, }, { @@ -403,17 +354,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n2", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n2", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 100, @@ -435,17 +379,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n2", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n2", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 3, @@ -474,17 +411,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 2, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(2), }, { count: 1, @@ -675,17 +605,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), tolerations: []v1.Toleration{ { Key: "taint-test", @@ -723,17 +646,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 1, @@ -763,17 +679,10 @@ func TestTopologySpreadConstraint(t *testing.T) { }, pods: createTestPods([]testPodList{ { - count: 1, - node: "n1", - labels: map[string]string{"foo": "bar"}, - constraints: []v1.TopologySpreadConstraint{ - { - MaxSkew: 1, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - }, - }, + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), }, { count: 1, @@ -786,6 +695,87 @@ func TestTopologySpreadConstraint(t *testing.T) { strategy: api.DeschedulerStrategy{}, namespaces: []string{"ns1"}, }, + { + name: "2 domains, sizes [2,0], maxSkew=1, move 0 pod for node with unmatched label filtering", + nodes: []*v1.Node{ + test.BuildTestNode("n1", 2000, 3000, 10, func(n *v1.Node) { n.Labels["zone"] = "zoneA" }), + test.BuildTestNode("n2", 2000, 3000, 10, func(n *v1.Node) { n.Labels["zone"] = "zoneB" }), + }, + pods: createTestPods([]testPodList{ + { + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + }, + { + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + }, + }), + expectedEvictedCount: 0, + strategy: api.DeschedulerStrategy{ + Params: &api.StrategyParameters{ + LabelSelector: getLabelSelector("foo", []string{"baz"}, metav1.LabelSelectorOpIn), + }, + }, + namespaces: []string{"ns1"}, + }, + { + name: "2 domains, sizes [2,0], maxSkew=1, move 1 pod for node with matched label filtering", + nodes: []*v1.Node{ + test.BuildTestNode("n1", 2000, 3000, 10, func(n *v1.Node) { n.Labels["zone"] = "zoneA" }), + test.BuildTestNode("n2", 2000, 3000, 10, func(n *v1.Node) { n.Labels["zone"] = "zoneB" }), + }, + pods: createTestPods([]testPodList{ + { + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + }, + { + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + }, + }), + expectedEvictedCount: 1, + strategy: api.DeschedulerStrategy{ + Params: &api.StrategyParameters{ + LabelSelector: getLabelSelector("foo", []string{"bar"}, metav1.LabelSelectorOpIn), + }, + }, + namespaces: []string{"ns1"}, + }, + { + name: "2 domains, sizes [2,0], maxSkew=1, move 1 pod for node with matched label filtering (NotIn op)", + nodes: []*v1.Node{ + test.BuildTestNode("n1", 2000, 3000, 10, func(n *v1.Node) { n.Labels["zone"] = "zoneA" }), + test.BuildTestNode("n2", 2000, 3000, 10, func(n *v1.Node) { n.Labels["zone"] = "zoneB" }), + }, + pods: createTestPods([]testPodList{ + { + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + }, + { + count: 1, + node: "n1", + labels: map[string]string{"foo": "bar"}, + }, + }), + expectedEvictedCount: 1, + strategy: api.DeschedulerStrategy{ + Params: &api.StrategyParameters{ + LabelSelector: getLabelSelector("foo", []string{"baz"}, metav1.LabelSelectorOpNotIn), + }, + }, + namespaces: []string{"ns1"}, + }, } for _, tc := range testCases { @@ -856,3 +846,20 @@ func createTestPods(testPods []testPodList) []*v1.Pod { } return pods } + +func getLabelSelector(key string, values []string, operator metav1.LabelSelectorOperator) *metav1.LabelSelector { + return &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: key, Operator: operator, Values: values}}, + } +} + +func getDefaultTopologyConstraints(maxSkew int32) []v1.TopologySpreadConstraint { + return []v1.TopologySpreadConstraint{ + { + MaxSkew: maxSkew, + TopologyKey: "zone", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + }, + } +}