removepodsviolatingtopologyspreadconstraint: implement explicit constraints

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)
+	}
 }

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),
 			},
 		},
 	}

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

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",

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)
 }

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")
+			}
+		})
+	}
+}

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)