From 5f0edb5f931ee43f2bc68edbdf30178ad29e5da4 Mon Sep 17 00:00:00 2001 From: Amir Alavi Date: Sat, 3 Jun 2023 17:49:20 -0400 Subject: [PATCH 1/2] removepodsviolatingtopologyspreadconstraint: topologyBalanceNodeFit to control whether to perform nodefit when balacning domains --- README.md | 8 ++++- pkg/api/v1alpha1/strategymigration.go | 2 ++ pkg/api/v1alpha1/strategymigration_test.go | 1 + pkg/descheduler/policyconfig_test.go | 5 ++- .../defaults.go | 4 +++ .../defaults_test.go | 31 +++++++++++++++++-- .../topologyspreadconstraint.go | 12 ++++--- .../types.go | 8 +++-- .../zz_generated.deepcopy.go | 5 +++ 9 files changed, 64 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index b1febbc69..aef447b3b 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,7 @@ See the [user guide](docs/user-guide.md) in the `/docs` directory. ## Policy, Default Evictor and Strategy plugins -**⚠️ v1alpha1 configuration is still suported, but deprecated (and soon will be removed). Please consider migrating to v1alpha2 (described bellow). For previous v1alpha1 documentation go to [docs/deprecated/v1alpha1.md](docs/deprecated/v1alpha1.md) ⚠️** +**⚠️ v1alpha1 configuration is still supported, but deprecated (and soon will be removed). Please consider migrating to v1alpha2 (described bellow). For previous v1alpha1 documentation go to [docs/deprecated/v1alpha1.md](docs/deprecated/v1alpha1.md) ⚠️** The Descheduler Policy is configurable and includes default strategy plugins that can be enabled or disabled. It includes a common eviction configuration at the top level, as well as configuration from the Evictor plugin (Default Evictor, if not specified otherwise). Top-level configuration and Evictor plugin configuration are applied to all evictions. @@ -525,6 +525,11 @@ 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. +The `topologyBalanceNodeFit` arg is used when balancing topology domains while the Default Evictor's `nodeFit` is used in pre-eviction to determine if a pod can be evicted. +```yaml +topologyBalanceNodeFit: false +``` + 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:** @@ -534,6 +539,7 @@ Strategy parameter `labelSelector` is not utilized when balancing topology domai |`includeSoftConstraints`|bool| |`namespaces`|(see [namespace filtering](#namespace-filtering))| |`labelSelector`|(see [label filtering](#label-filtering))| +|`topologyBalanceNodeFit`|bool|default `true`. [node fit filtering](#node-fit-filtering) when balancing topology domains| **Example:** diff --git a/pkg/api/v1alpha1/strategymigration.go b/pkg/api/v1alpha1/strategymigration.go index 302b768ce..1c7153aea 100644 --- a/pkg/api/v1alpha1/strategymigration.go +++ b/pkg/api/v1alpha1/strategymigration.go @@ -20,6 +20,7 @@ import ( "fmt" "k8s.io/klog/v2" + utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/api" "sigs.k8s.io/descheduler/pkg/framework/plugins/nodeutilization" "sigs.k8s.io/descheduler/pkg/framework/plugins/podlifetime" @@ -174,6 +175,7 @@ var StrategyParamsToPluginArgs = map[string]func(params *StrategyParameters) (*a Namespaces: v1alpha1NamespacesToInternal(params.Namespaces), LabelSelector: params.LabelSelector, IncludeSoftConstraints: params.IncludeSoftConstraints, + TopologyBalanceNodeFit: utilpointer.Bool(true), } if err := removepodsviolatingtopologyspreadconstraint.ValidateRemovePodsViolatingTopologySpreadConstraintArgs(args); err != nil { klog.ErrorS(err, "unable to validate plugin arguments", "pluginName", removepodsviolatingtopologyspreadconstraint.PluginName) diff --git a/pkg/api/v1alpha1/strategymigration_test.go b/pkg/api/v1alpha1/strategymigration_test.go index 983168014..bf083607d 100644 --- a/pkg/api/v1alpha1/strategymigration_test.go +++ b/pkg/api/v1alpha1/strategymigration_test.go @@ -568,6 +568,7 @@ func TestStrategyParamsToPluginArgsRemovePodsViolatingTopologySpreadConstraint(t Name: removepodsviolatingtopologyspreadconstraint.PluginName, Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ IncludeSoftConstraints: true, + TopologyBalanceNodeFit: utilpointer.Bool(true), Namespaces: &api.Namespaces{ Exclude: []string{"test1"}, }, diff --git a/pkg/descheduler/policyconfig_test.go b/pkg/descheduler/policyconfig_test.go index 629bb3f40..7b8e28809 100644 --- a/pkg/descheduler/policyconfig_test.go +++ b/pkg/descheduler/policyconfig_test.go @@ -399,7 +399,9 @@ func TestV1alpha1ToV1alpha2(t *testing.T) { defaultEvictorPluginConfig, { Name: removepodsviolatingtopologyspreadconstraint.PluginName, - Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{}, + Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ + TopologyBalanceNodeFit: utilpointer.Bool(true), + }, }, }, Plugins: api.Plugins{ @@ -715,6 +717,7 @@ func TestV1alpha1ToV1alpha2(t *testing.T) { Name: removepodsviolatingtopologyspreadconstraint.PluginName, Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ IncludeSoftConstraints: true, + TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, }, diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go index ff468c215..66d3d0e45 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go @@ -15,6 +15,7 @@ package removepodsviolatingtopologyspreadconstraint import ( "k8s.io/apimachinery/pkg/runtime" + utilpointer "k8s.io/utils/pointer" ) func addDefaultingFuncs(scheme *runtime.Scheme) error { @@ -34,4 +35,7 @@ func SetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(obj runtime.Obj if !args.IncludeSoftConstraints { args.IncludeSoftConstraints = false } + if args.TopologyBalanceNodeFit == nil { + args.TopologyBalanceNodeFit = utilpointer.Bool(true) + } } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go index 605bd5a11..c89f783ec 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go @@ -20,9 +20,20 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/api" ) +var scheme *runtime.Scheme + +func init() { + scheme = runtime.NewScheme() + scheme.AddTypeDefaultingFunc(&RemovePodsViolatingTopologySpreadConstraintArgs{}, func(obj interface{}) { + SetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(obj.(*RemovePodsViolatingTopologySpreadConstraintArgs)) + }) + utilruntime.Must(AddToScheme(scheme)) +} + func TestSetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { tests := []struct { name string @@ -36,6 +47,7 @@ func TestSetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(t *testing. Namespaces: nil, LabelSelector: nil, IncludeSoftConstraints: false, + TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, { @@ -49,12 +61,27 @@ func TestSetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(t *testing. Namespaces: &api.Namespaces{}, LabelSelector: &metav1.LabelSelector{}, IncludeSoftConstraints: true, + TopologyBalanceNodeFit: utilpointer.Bool(true), + }, + }, + { + name: "RemovePodsViolatingTopologySpreadConstraintArgs without TopologyBalanceNodeFit", + in: &RemovePodsViolatingTopologySpreadConstraintArgs{}, + want: &RemovePodsViolatingTopologySpreadConstraintArgs{ + TopologyBalanceNodeFit: utilpointer.Bool(true), + }, + }, + { + name: "RemovePodsViolatingTopologySpreadConstraintArgs with TopologyBalanceNodeFit=false", + in: &RemovePodsViolatingTopologySpreadConstraintArgs{ + TopologyBalanceNodeFit: utilpointer.Bool(false), + }, + want: &RemovePodsViolatingTopologySpreadConstraintArgs{ + TopologyBalanceNodeFit: utilpointer.Bool(false), }, }, } for _, tc := range tests { - scheme := runtime.NewScheme() - utilruntime.Must(AddToScheme(scheme)) t.Run(tc.name, func(t *testing.T) { scheme.Default(tc.in) if diff := cmp.Diff(tc.in, tc.want); diff != "" { diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index eeb049b50..2d5fbbf65 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" + utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/descheduler/evictions" "sigs.k8s.io/descheduler/pkg/descheduler/node" @@ -197,7 +198,7 @@ func (d *RemovePodsViolatingTopologySpreadConstraint) Balance(ctx context.Contex klog.V(2).InfoS("Skipping topology constraint because it is already balanced", "constraint", constraint) continue } - balanceDomains(d.handle.GetPodsAssignedToNodeFunc(), podsForEviction, constraint, constraintTopologies, sumPods, d.handle.Evictor().Filter, nodes) + d.balanceDomains(podsForEviction, constraint, constraintTopologies, sumPods, nodes) } } @@ -270,17 +271,18 @@ func topologyIsBalanced(topology map[topologyPair][]*v1.Pod, constraint v1.Topol // Following this, the above topology domains end up "sorted" as: // [5, 5, 5, 5, 5, 5] // (assuming even distribution by the scheduler of the evicted pods) -func balanceDomains( - getPodsAssignedToNode podutil.GetPodsAssignedToNodeFunc, +func (d *RemovePodsViolatingTopologySpreadConstraint) balanceDomains( podsForEviction map[*v1.Pod]struct{}, constraint v1.TopologySpreadConstraint, constraintTopologies map[topologyPair][]*v1.Pod, sumPods float64, - isEvictable func(pod *v1.Pod) bool, nodes []*v1.Node, ) { idealAvg := sumPods / float64(len(constraintTopologies)) + isEvictable := d.handle.Evictor().Filter sortedDomains := sortDomains(constraintTopologies, isEvictable) + getPodsAssignedToNode := d.handle.GetPodsAssignedToNodeFunc() + topologyBalanceNodeFit := utilpointer.BoolDeref(d.args.TopologyBalanceNodeFit, true) nodesBelowIdealAvg := filterNodesBelowIdealAvg(nodes, sortedDomains, constraint.TopologyKey, idealAvg) @@ -335,7 +337,7 @@ func balanceDomains( // This is because the chosen pods aren't sorted, but immovable pods still count as "evicted" toward the PTS algorithm. // So, a better selection heuristic could improve performance. - if !node.PodFitsAnyOtherNode(getPodsAssignedToNode, aboveToEvict[k], nodesBelowIdealAvg) { + if topologyBalanceNodeFit && !node.PodFitsAnyOtherNode(getPodsAssignedToNode, aboveToEvict[k], nodesBelowIdealAvg) { klog.V(2).InfoS("ignoring pod for eviction as it does not fit on any other node", "pod", klog.KObj(aboveToEvict[k])) continue } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/types.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/types.go index a661608b9..14a1da276 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/types.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/types.go @@ -17,6 +17,7 @@ limitations under the License. package removepodsviolatingtopologyspreadconstraint import ( + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/descheduler/pkg/api" ) @@ -28,7 +29,8 @@ import ( type RemovePodsViolatingTopologySpreadConstraintArgs struct { metav1.TypeMeta `json:",inline"` - Namespaces *api.Namespaces `json:"namespaces"` - LabelSelector *metav1.LabelSelector `json:"labelSelector"` - IncludeSoftConstraints bool `json:"includeSoftConstraints"` + Namespaces *api.Namespaces `json:"namespaces"` + LabelSelector *metav1.LabelSelector `json:"labelSelector"` + Constraints []v1.UnsatisfiableConstraintAction `json:"constraints"` + TopologyBalanceNodeFit *bool `json:"topologyBalanceNodeFit"` } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go index dd15101a1..2a92c4746 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go @@ -41,6 +41,11 @@ func (in *RemovePodsViolatingTopologySpreadConstraintArgs) DeepCopyInto(out *Rem *out = new(v1.LabelSelector) (*in).DeepCopyInto(*out) } + if in.TopologyBalanceNodeFit != nil { + in, out := &in.TopologyBalanceNodeFit, &out.TopologyBalanceNodeFit + *out = new(bool) + **out = **in + } return } From 7f2f6f2b16a9dde280bb7bc9d5c3327770dab590 Mon Sep 17 00:00:00 2001 From: Amir Alavi Date: Sat, 3 Jun 2023 18:58:18 -0400 Subject: [PATCH 2/2] removepodsviolatingtopologyspreadconstraint: implement explicit constraints --- README.md | 13 ++- examples/policy.yaml | 4 +- examples/topology-spread-constraint.yaml | 4 +- pkg/api/v1alpha1/strategymigration.go | 7 +- pkg/api/v1alpha1/strategymigration_test.go | 17 +++- pkg/descheduler/policyconfig_test.go | 4 +- .../defaults.go | 7 +- .../defaults_test.go | 23 +++-- .../topologyspreadconstraint.go | 6 +- .../topologyspreadconstraint_test.go | 83 +++++++++++++++++- .../validation.go | 20 ++++- .../validation_test.go | 85 +++++++++++++++++++ .../zz_generated.deepcopy.go | 6 ++ test/e2e/e2e_topologyspreadconstraint_test.go | 2 +- 14 files changed, 254 insertions(+), 27 deletions(-) create mode 100644 pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go diff --git a/README.md b/README.md index aef447b3b..c755604b3 100644 --- a/README.md +++ b/README.md @@ -522,8 +522,12 @@ This strategy makes sure that pods violating [topology spread constraints](https are evicted from nodes. Specifically, it tries to evict the minimum number of pods required to balance topology domains to within each constraint's `maxSkew`. 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. +By default, this strategy only includes hard constraints, you can explicitly set `constraints` as shown below to include both: +```yaml +constraints: +- DoNotSchedule +- ScheduleAnyway +``` The `topologyBalanceNodeFit` arg is used when balancing topology domains while the Default Evictor's `nodeFit` is used in pre-eviction to determine if a pod can be evicted. ```yaml @@ -536,9 +540,9 @@ Strategy parameter `labelSelector` is not utilized when balancing topology domai |Name|Type| |---|---| -|`includeSoftConstraints`|bool| |`namespaces`|(see [namespace filtering](#namespace-filtering))| |`labelSelector`|(see [label filtering](#label-filtering))| +|`constraints`|(see [whenUnsatisfiable](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#topologyspreadconstraint-v1-core))|| |`topologyBalanceNodeFit`|bool|default `true`. [node fit filtering](#node-fit-filtering) when balancing topology domains| **Example:** @@ -551,7 +555,8 @@ profiles: pluginConfig: - name: "RemovePodsViolatingTopologySpreadConstraint" args: - includeSoftConstraints: false + constraints: + - DoNotSchedule plugins: balance: enabled: diff --git a/examples/policy.yaml b/examples/policy.yaml index aa34f3fd3..a161ba440 100644 --- a/examples/policy.yaml +++ b/examples/policy.yaml @@ -21,7 +21,9 @@ profiles: includingInitContainers: true - name: "RemovePodsViolatingTopologySpreadConstraint" args: - includeSoftConstraints: true + constraints: + - DoNotSchedule + - ScheduleAnyway plugins: deschedule: enabled: diff --git a/examples/topology-spread-constraint.yaml b/examples/topology-spread-constraint.yaml index 847f0636c..1d3ddf47b 100644 --- a/examples/topology-spread-constraint.yaml +++ b/examples/topology-spread-constraint.yaml @@ -5,7 +5,9 @@ profiles: pluginConfig: - name: "RemovePodsViolatingTopologySpreadConstraint" args: - includeSoftConstraints: true # Include 'ScheduleAnyways' constraints + constraints: + - DoNotSchedule + - ScheduleAnyway plugins: balance: enabled: diff --git a/pkg/api/v1alpha1/strategymigration.go b/pkg/api/v1alpha1/strategymigration.go index 1c7153aea..5dbe0a324 100644 --- a/pkg/api/v1alpha1/strategymigration.go +++ b/pkg/api/v1alpha1/strategymigration.go @@ -19,6 +19,7 @@ package v1alpha1 import ( "fmt" + v1 "k8s.io/api/core/v1" "k8s.io/klog/v2" utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/api" @@ -171,10 +172,14 @@ var StrategyParamsToPluginArgs = map[string]func(params *StrategyParameters) (*a }, nil }, "RemovePodsViolatingTopologySpreadConstraint": func(params *StrategyParameters) (*api.PluginConfig, error) { + constraints := []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule} + if params.IncludeSoftConstraints { + constraints = append(constraints, v1.ScheduleAnyway) + } args := &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ Namespaces: v1alpha1NamespacesToInternal(params.Namespaces), LabelSelector: params.LabelSelector, - IncludeSoftConstraints: params.IncludeSoftConstraints, + Constraints: constraints, TopologyBalanceNodeFit: utilpointer.Bool(true), } if err := removepodsviolatingtopologyspreadconstraint.ValidateRemovePodsViolatingTopologySpreadConstraintArgs(args); err != nil { diff --git a/pkg/api/v1alpha1/strategymigration_test.go b/pkg/api/v1alpha1/strategymigration_test.go index bf083607d..78639d322 100644 --- a/pkg/api/v1alpha1/strategymigration_test.go +++ b/pkg/api/v1alpha1/strategymigration_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + v1 "k8s.io/api/core/v1" utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/api" "sigs.k8s.io/descheduler/pkg/framework/plugins/nodeutilization" @@ -567,7 +568,7 @@ func TestStrategyParamsToPluginArgsRemovePodsViolatingTopologySpreadConstraint(t result: &api.PluginConfig{ Name: removepodsviolatingtopologyspreadconstraint.PluginName, Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ - IncludeSoftConstraints: true, + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, TopologyBalanceNodeFit: utilpointer.Bool(true), Namespaces: &api.Namespaces{ Exclude: []string{"test1"}, @@ -575,6 +576,20 @@ func TestStrategyParamsToPluginArgsRemovePodsViolatingTopologySpreadConstraint(t }, }, }, + { + description: "params without soft constraints", + params: &StrategyParameters{ + IncludeSoftConstraints: false, + }, + err: nil, + result: &api.PluginConfig{ + Name: removepodsviolatingtopologyspreadconstraint.PluginName, + Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule}, + TopologyBalanceNodeFit: utilpointer.Bool(true), + }, + }, + }, { description: "invalid params namespaces", params: &StrategyParameters{ diff --git a/pkg/descheduler/policyconfig_test.go b/pkg/descheduler/policyconfig_test.go index 7b8e28809..5d1ec3947 100644 --- a/pkg/descheduler/policyconfig_test.go +++ b/pkg/descheduler/policyconfig_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + v1 "k8s.io/api/core/v1" fakeclientset "k8s.io/client-go/kubernetes/fake" utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/api" @@ -400,6 +401,7 @@ func TestV1alpha1ToV1alpha2(t *testing.T) { { Name: removepodsviolatingtopologyspreadconstraint.PluginName, Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule}, TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, @@ -716,7 +718,7 @@ func TestV1alpha1ToV1alpha2(t *testing.T) { { Name: removepodsviolatingtopologyspreadconstraint.PluginName, Args: &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ - IncludeSoftConstraints: true, + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go index 66d3d0e45..205f39f11 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults.go @@ -14,6 +14,7 @@ limitations under the License. package removepodsviolatingtopologyspreadconstraint import ( + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" utilpointer "k8s.io/utils/pointer" ) @@ -32,10 +33,10 @@ func SetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(obj runtime.Obj if args.LabelSelector == nil { args.LabelSelector = nil } - if !args.IncludeSoftConstraints { - args.IncludeSoftConstraints = false - } if args.TopologyBalanceNodeFit == nil { args.TopologyBalanceNodeFit = utilpointer.Bool(true) } + if len(args.Constraints) == 0 { + args.Constraints = append(args.Constraints, v1.DoNotSchedule) + } } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go index c89f783ec..926e08562 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/defaults_test.go @@ -17,6 +17,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -46,21 +47,21 @@ func TestSetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(t *testing. want: &RemovePodsViolatingTopologySpreadConstraintArgs{ Namespaces: nil, LabelSelector: nil, - IncludeSoftConstraints: false, + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule}, TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, { name: "RemovePodsViolatingTopologySpreadConstraintArgs with value", in: &RemovePodsViolatingTopologySpreadConstraintArgs{ - Namespaces: &api.Namespaces{}, - LabelSelector: &metav1.LabelSelector{}, - IncludeSoftConstraints: true, + Namespaces: &api.Namespaces{}, + LabelSelector: &metav1.LabelSelector{}, + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, }, want: &RemovePodsViolatingTopologySpreadConstraintArgs{ Namespaces: &api.Namespaces{}, LabelSelector: &metav1.LabelSelector{}, - IncludeSoftConstraints: true, + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, @@ -68,6 +69,7 @@ func TestSetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(t *testing. name: "RemovePodsViolatingTopologySpreadConstraintArgs without TopologyBalanceNodeFit", in: &RemovePodsViolatingTopologySpreadConstraintArgs{}, want: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule}, TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, @@ -78,6 +80,17 @@ func TestSetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(t *testing. }, want: &RemovePodsViolatingTopologySpreadConstraintArgs{ TopologyBalanceNodeFit: utilpointer.Bool(false), + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule}, + }, + }, + { + name: "RemovePodsViolatingTopologySpreadConstraintArgs with nil constraints", + in: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: nil, + }, + want: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule}, + TopologyBalanceNodeFit: utilpointer.Bool(true), }, }, } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index 2d5fbbf65..ab37c101b 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go @@ -118,6 +118,8 @@ func (d *RemovePodsViolatingTopologySpreadConstraint) Balance(ctx context.Contex } } + allowedConstraints := sets.New[v1.UnsatisfiableConstraintAction](d.args.Constraints...) + namespacedPods := podutil.GroupByNamespace(pods) // 1. for each namespace... @@ -131,8 +133,8 @@ func (d *RemovePodsViolatingTopologySpreadConstraint) Balance(ctx context.Contex namespaceTopologySpreadConstraints := []v1.TopologySpreadConstraint{} for _, pod := range namespacedPods[namespace] { for _, constraint := range pod.Spec.TopologySpreadConstraints { - // Ignore soft topology constraints if they are not included - if constraint.WhenUnsatisfiable == v1.ScheduleAnyway && (d.args == nil || !d.args.IncludeSoftConstraints) { + // Ignore topology constraints if they are not included + if !allowedConstraints.Has(constraint.WhenUnsatisfiable) { continue } // Need to check v1.TopologySpreadConstraint deepEquality because diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go index bd94677b4..e70fa2967 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go @@ -15,6 +15,7 @@ import ( "k8s.io/client-go/kubernetes/fake" core "k8s.io/client-go/testing" "k8s.io/client-go/tools/events" + utilpointer "k8s.io/utils/pointer" "sigs.k8s.io/descheduler/pkg/api" "sigs.k8s.io/descheduler/pkg/descheduler/evictions" @@ -99,6 +100,43 @@ func TestTopologySpreadConstraint(t *testing.T) { namespaces: []string{"ns1"}, args: RemovePodsViolatingTopologySpreadConstraintArgs{}, }, + { + name: "2 domains, sizes [3,1], maxSkew=1, move 1 pod to achieve [2,2] (both constraints)", + 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: []v1.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "zone", + WhenUnsatisfiable: v1.ScheduleAnyway, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + }, + }, + }, + { + count: 2, + node: "n1", + labels: map[string]string{"foo": "bar"}, + }, + { + count: 1, + node: "n2", + labels: map[string]string{"foo": "bar"}, + }, + }), + expectedEvictedCount: 1, + namespaces: []string{"ns1"}, + args: RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, + }, + }, { name: "2 domains, sizes [3,1], maxSkew=1, move 1 pod to achieve [2,2] (soft constraints)", nodes: []*v1.Node{ @@ -132,7 +170,9 @@ func TestTopologySpreadConstraint(t *testing.T) { }), expectedEvictedCount: 1, namespaces: []string{"ns1"}, - args: RemovePodsViolatingTopologySpreadConstraintArgs{IncludeSoftConstraints: true}, + args: RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, + }, }, { name: "2 domains, sizes [3,1], maxSkew=1, no pods eligible, move 0 pods", @@ -588,7 +628,9 @@ func TestTopologySpreadConstraint(t *testing.T) { }), expectedEvictedCount: 1, namespaces: []string{"ns1"}, - args: RemovePodsViolatingTopologySpreadConstraintArgs{IncludeSoftConstraints: true}, + args: RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, + }, }, { name: "3 domains size [8 7 0], maxSkew=1, should move 5 to get [5 5 5]", @@ -924,6 +966,38 @@ func TestTopologySpreadConstraint(t *testing.T) { args: RemovePodsViolatingTopologySpreadConstraintArgs{}, nodeFit: true, }, + { + name: "3 domains, sizes [2,3,4], maxSkew=1, args.NodeFit is false, and not enough cpu on zoneA; 1 should be moved to force scale-up", + nodes: []*v1.Node{ + test.BuildTestNode("n1", 250, 2000, 9, func(n *v1.Node) { n.Labels["zone"] = "zoneA" }), + test.BuildTestNode("n2", 1000, 2000, 9, func(n *v1.Node) { n.Labels["zone"] = "zoneB" }), + test.BuildTestNode("n3", 1000, 2000, 9, func(n *v1.Node) { n.Labels["zone"] = "zoneC" }), + }, + pods: createTestPods([]testPodList{ + { + count: 2, + node: "n1", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + }, + { + count: 3, + node: "n2", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + }, + { + count: 4, + node: "n3", + labels: map[string]string{"foo": "bar"}, + constraints: getDefaultTopologyConstraints(1), + }, + }), + expectedEvictedCount: 1, + namespaces: []string{"ns1"}, + args: RemovePodsViolatingTopologySpreadConstraintArgs{TopologyBalanceNodeFit: utilpointer.Bool(false)}, + nodeFit: true, + }, { name: "3 domains, sizes [[1,0], [1,1], [2,1]], maxSkew=1, NodeFit is enabled, and not enough cpu on ZoneA; nothing should be moved", nodes: []*v1.Node{ @@ -1211,6 +1285,8 @@ func TestTopologySpreadConstraint(t *testing.T) { SharedInformerFactoryImpl: sharedInformerFactory, } + SetDefaults_RemovePodsViolatingTopologySpreadConstraintArgs(&tc.args) + plugin, err := New( &tc.args, handle, @@ -1308,8 +1384,7 @@ func TestCheckIdenticalConstraints(t *testing.T) { WhenUnsatisfiable: v1.DoNotSchedule, LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, } - namespaceTopologySpreadConstraint := []v1.TopologySpreadConstraint{} - namespaceTopologySpreadConstraint = []v1.TopologySpreadConstraint{ + namespaceTopologySpreadConstraint := []v1.TopologySpreadConstraint{ { MaxSkew: 2, TopologyKey: "zone", diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation.go index 60ebf68ef..7cc4302a3 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation.go @@ -19,23 +19,37 @@ package removepodsviolatingtopologyspreadconstraint import ( "fmt" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" ) // ValidateRemovePodsViolatingTopologySpreadConstraintArgs validates RemovePodsViolatingTopologySpreadConstraint arguments func ValidateRemovePodsViolatingTopologySpreadConstraintArgs(obj runtime.Object) error { + var errs []error + args := obj.(*RemovePodsViolatingTopologySpreadConstraintArgs) // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + errs = append(errs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + errs = append(errs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } - return nil + if len(args.Constraints) > 0 { + supportedConstraints := sets.New(v1.DoNotSchedule, v1.ScheduleAnyway) + for _, constraint := range args.Constraints { + if !supportedConstraints.Has(constraint) { + errs = append(errs, fmt.Errorf("constraint %s is not one of %v", constraint, supportedConstraints)) + } + } + } + + return errors.NewAggregate(errs) } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go new file mode 100644 index 000000000..f7f7f1164 --- /dev/null +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go @@ -0,0 +1,85 @@ +package removepodsviolatingtopologyspreadconstraint + +import ( + "testing" + + v1 "k8s.io/api/core/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/descheduler/pkg/api" +) + +func TestValidateRemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { + testCases := []struct { + description string + args *RemovePodsViolatingTopologySpreadConstraintArgs + expectError bool + }{ + { + description: "valid namespace args, no errors", + args: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Namespaces: &api.Namespaces{ + Include: []string{"default"}, + }, + }, + expectError: false, + }, + { + description: "invalid namespaces args, expects error", + args: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Namespaces: &api.Namespaces{ + Include: []string{"default"}, + Exclude: []string{"kube-system"}, + }, + }, + expectError: true, + }, + { + description: "valid label selector args, no errors", + args: &RemovePodsViolatingTopologySpreadConstraintArgs{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role.kubernetes.io/node": ""}, + }, + }, + expectError: false, + }, + { + description: "invalid label selector args, expects errors", + args: &RemovePodsViolatingTopologySpreadConstraintArgs{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Operator: metav1.LabelSelectorOpIn, + }, + }, + }, + }, + expectError: true, + }, + { + description: "valid constraints args, no errors", + args: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, + }, + expectError: false, + }, + { + description: "invalid constraints args, expects errors", + args: &RemovePodsViolatingTopologySpreadConstraintArgs{ + Constraints: []v1.UnsatisfiableConstraintAction{"foo"}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + err := ValidateRemovePodsViolatingTopologySpreadConstraintArgs(tc.args) + + hasError := err != nil + if tc.expectError != hasError { + t.Error("unexpected arg validation behavior") + } + }) + } +} diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go index 2a92c4746..a1af667b6 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/zz_generated.deepcopy.go @@ -22,6 +22,7 @@ limitations under the License. package removepodsviolatingtopologyspreadconstraint import ( + corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" api "sigs.k8s.io/descheduler/pkg/api" @@ -41,6 +42,11 @@ func (in *RemovePodsViolatingTopologySpreadConstraintArgs) DeepCopyInto(out *Rem *out = new(v1.LabelSelector) (*in).DeepCopyInto(*out) } + if in.Constraints != nil { + in, out := &in.Constraints, &out.Constraints + *out = make([]corev1.UnsatisfiableConstraintAction, len(*in)) + copy(*out, *in) + } if in.TopologyBalanceNodeFit != nil { in, out := &in.TopologyBalanceNodeFit, &out.TopologyBalanceNodeFit *out = new(bool) diff --git a/test/e2e/e2e_topologyspreadconstraint_test.go b/test/e2e/e2e_topologyspreadconstraint_test.go index e19a07b46..8117c06a2 100644 --- a/test/e2e/e2e_topologyspreadconstraint_test.go +++ b/test/e2e/e2e_topologyspreadconstraint_test.go @@ -103,7 +103,7 @@ func TestTopologySpreadConstraint(t *testing.T) { } plugin, err := removepodsviolatingtopologyspreadconstraint.New(&removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{ - IncludeSoftConstraints: tc.constraint != v1.DoNotSchedule, + Constraints: []v1.UnsatisfiableConstraintAction{tc.constraint}, }, &frameworkfake.HandleImpl{ ClientsetImpl: clientSet,