add strategy defaults and fix nodeFit for node affinity strategy

This adds defaults for strategies. Now each strategy may have
different default parameters, if some are omitted from the configmap.

Since the NodeFit parameter was a bool instead of a pointer, it
couldn't be detected whether it was being set in the configmap, or
not. Hence it is changed to a pointer. If it is not set in the
configmap, the default is taken.

Except for node affinity, all other  strategies are now having a
default of false for an omitted NodeFit parameter, which matches
exactly how things have worked.

For the node affinity strategy, the legacy mode of operation has
always been what a "true" NodeFit would be. Thus an omitted NodeFit
parameter for that strategy is set to true, which matches how
node affinity has been working, effectively not evicting anything
if the pod doesn't fit into another node.

The change is in how things work when the NodeFit is explicitly set
to "false". The hard-coded check for pod fitting any node is removed,
and the NodeFit feature has control. NodeFit being false, node
affinity strategy will evict the pod regardless of whether there is
another node where it would fit.

Signed-off-by: Ukri Niemimuukko <ukri.niemimuukko@intel.com>

pkg/descheduler/strategies/nodeutilization/highnodeutilization.go
pkg/descheduler/strategies/nodeutilization/lownodeutilization.go

README.md

@@ -735,6 +735,9 @@ The following strategies accept a `nodeFit` boolean parameter which can optimize
 * `RemovePodsHavingTooManyRestarts`
 * `RemoveFailedPods`
 
+If omitted, the `nodeFit` boolean will default to `false` except for `RemovePodsViolatingNodeAffinity`
+for which it defaults to `true` due to backwards compatibility reasons.
+
  If set to `true` the descheduler will consider whether or not the pods that meet eviction criteria will fit on other nodes before evicting them. If a pod cannot be rescheduled to another node, it will not be evicted. Currently the following criteria are considered when setting `nodeFit` to `true`:
 - A `nodeSelector` on the pod
 - Any `tolerations` on the pod and any `taints` on the other nodes

pkg/api/types.go

@@ -87,7 +87,7 @@ type StrategyParameters struct {
 	ThresholdPriority                 *int32
 	ThresholdPriorityClassName        string
 	LabelSelector                     *metav1.LabelSelector
-	NodeFit                           bool
+	NodeFit                           *bool
 	IncludePreferNoSchedule           bool
 	ExcludedTaints                    []string
 }

pkg/api/v1alpha1/defaults.go

@@ -16,7 +16,51 @@ limitations under the License.
 
 package v1alpha1
 
-import "k8s.io/apimachinery/pkg/runtime"
+import (
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/pointer"
+)
+
+var (
+	nodeAffinityDefaults = DeschedulerStrategy{
+		Params: &StrategyParameters{
+			NodeFit: pointer.Bool(true),
+		},
+	}
+	genericStrategyDefaults = DeschedulerStrategy{
+		Params: &StrategyParameters{
+			NodeFit: pointer.Bool(false),
+		},
+	}
+
+	strategyDefaults = StrategyList{
+		StrategyName("RemoveDuplicates"):                            genericStrategyDefaults,
+		StrategyName("LowNodeUtilization"):                          genericStrategyDefaults,
+		StrategyName("HighNodeUtilization"):                         genericStrategyDefaults,
+		StrategyName("RemovePodsViolatingInterPodAntiAffinity"):     genericStrategyDefaults,
+		StrategyName("RemovePodsViolatingNodeAffinity"):             nodeAffinityDefaults,
+		StrategyName("RemovePodsViolatingNodeTaints"):               genericStrategyDefaults,
+		StrategyName("RemovePodsHavingTooManyRestarts"):             genericStrategyDefaults,
+		StrategyName("PodLifeTime"):                                 genericStrategyDefaults,
+		StrategyName("RemovePodsViolatingTopologySpreadConstraint"): genericStrategyDefaults,
+		StrategyName("RemoveFailedPods"):                            genericStrategyDefaults,
+	}
+)
+
+func SetDefaults_DeschedulerPolicy(obj *DeschedulerPolicy) {
+	for strategyName, strategy := range obj.Strategies {
+		if defaults, ok := strategyDefaults[strategyName]; ok {
+			if strategy.Params == nil {
+				strategy.Params = defaults.Params
+			} else {
+				if strategy.Params.NodeFit == nil {
+					strategy.Params.NodeFit = defaults.Params.NodeFit
+				}
+				// put other parameter default copying here after adding new ones to strategyDefaults
+			}
+		}
+	}
+}
 
 func addDefaultingFuncs(scheme *runtime.Scheme) error {
 	return RegisterDefaults(scheme)

pkg/api/v1alpha1/types.go

@@ -85,7 +85,7 @@ type StrategyParameters struct {
 	ThresholdPriority                 *int32                             `json:"thresholdPriority"`
 	ThresholdPriorityClassName        string                             `json:"thresholdPriorityClassName"`
 	LabelSelector                     *metav1.LabelSelector              `json:"labelSelector"`
-	NodeFit                           bool                               `json:"nodeFit"`
+	NodeFit                           *bool                              `json:"nodeFit"`
 	IncludePreferNoSchedule           bool                               `json:"includePreferNoSchedule"`
 	ExcludedTaints                    []string                           `json:"excludedTaints,omitempty"`
 }

pkg/descheduler/descheduler.go

@@ -292,8 +292,8 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 				if strategy.Enabled {
 					nodeFit := false
 					if name != "PodLifeTime" {
-						if strategy.Params != nil {
-							nodeFit = strategy.Params.NodeFit
+						if strategy.Params != nil && strategy.Params.NodeFit != nil {
+							nodeFit = *strategy.Params.NodeFit
 						}
 					}
 					thresholdPriority, err := utils.GetPriorityFromStrategyParams(ctx, rs.Client, strategy.Params)

pkg/descheduler/node/node.go

@@ -154,24 +154,6 @@ func PodFitsAnyOtherNode(nodeIndexer podutil.GetPodsAssignedToNodeFunc, pod *v1.
 	return false
 }
 
-// PodFitsAnyNode checks if the given pod will fit any of the given nodes. The predicates used
-// to determine if the pod will fit can be found in the NodeFit function.
-func PodFitsAnyNode(nodeIndexer podutil.GetPodsAssignedToNodeFunc, pod *v1.Pod, nodes []*v1.Node) bool {
-	for _, node := range nodes {
-		errors := NodeFit(nodeIndexer, pod, node)
-		if len(errors) == 0 {
-			klog.V(4).InfoS("Pod fits on node", "pod", klog.KObj(pod), "node", klog.KObj(node))
-			return true
-		} else {
-			klog.V(4).InfoS("Pod does not fit on node", "pod", klog.KObj(pod), "node", klog.KObj(node))
-			for _, err := range errors {
-				klog.V(4).InfoS(err.Error())
-			}
-		}
-	}
-	return false
-}
-
 // PodFitsCurrentNode checks if the given pod will fit onto the given node. The predicates used
 // to determine if the pod will fit can be found in the NodeFit function.
 func PodFitsCurrentNode(nodeIndexer podutil.GetPodsAssignedToNodeFunc, pod *v1.Pod, node *v1.Node) bool {

pkg/descheduler/strategies/duplicates_test.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
@@ -256,35 +257,35 @@ func TestFindDuplicatePods(t *testing.T) {
 			pods:                    []*v1.Pod{p1, p2, p3},
 			nodes:                   []*v1.Node{node1, node3},
 			expectedEvictedPodCount: 0,
-			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: true}},
+			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: pointer.Bool(true)}},
 		},
 		{
 			description:             "Three pods in the `node-fit` Namespace, bound to same ReplicaSet, all with a nodeSelector. Only node available has an incorrect node label, and nodeFit set to true. 0 should be evicted.",
 			pods:                    []*v1.Pod{p15, p16, p17},
 			nodes:                   []*v1.Node{node1, node4},
 			expectedEvictedPodCount: 0,
-			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: true}},
+			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: pointer.Bool(true)}},
 		},
 		{
 			description:             "Three pods in the `node-fit` Namespace, bound to same ReplicaSet. Only node available is not schedulable, and nodeFit set to true. 0 should be evicted.",
 			pods:                    []*v1.Pod{p1, p2, p3},
 			nodes:                   []*v1.Node{node1, node5},
 			expectedEvictedPodCount: 0,
-			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: true}},
+			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: pointer.Bool(true)}},
 		},
 		{
 			description:             "Three pods in the `node-fit` Namespace, bound to same ReplicaSet. Only node available does not have enough CPU, and nodeFit set to true. 0 should be evicted.",
 			pods:                    []*v1.Pod{p1, p2, p3, p19},
 			nodes:                   []*v1.Node{node1, node6},
 			expectedEvictedPodCount: 0,
-			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: true}},
+			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: pointer.Bool(true)}},
 		},
 		{
 			description:             "Three pods in the `node-fit` Namespace, bound to same ReplicaSet. Only node available has enough CPU, and nodeFit set to true. 1 should be evicted.",
 			pods:                    []*v1.Pod{p1, p2, p3, p20},
 			nodes:                   []*v1.Node{node1, node6},
 			expectedEvictedPodCount: 1,
-			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: true}},
+			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: pointer.Bool(true)}},
 		},
 	}
 
@@ -324,8 +325,8 @@ func TestFindDuplicatePods(t *testing.T) {
 			)
 
 			nodeFit := false
-			if testCase.strategy.Params != nil {
-				nodeFit = testCase.strategy.Params.NodeFit
+			if testCase.strategy.Params != nil && testCase.strategy.Params.NodeFit != nil {
+				nodeFit = *testCase.strategy.Params.NodeFit
 			}
 
 			evictorFilter := evictions.NewEvictorFilter(

pkg/descheduler/strategies/failedpods_test.go

@@ -10,6 +10,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
@@ -32,7 +33,7 @@ func TestRemoveFailedPods(t *testing.T) {
 					ExcludeOwnerKinds:       excludeKinds,
 					MinPodLifetimeSeconds:   minAgeSeconds,
 				},
-				NodeFit: nodeFit,
+				NodeFit: &nodeFit,
 			},
 		}
 	}
@@ -46,7 +47,7 @@ func TestRemoveFailedPods(t *testing.T) {
 	}{
 		{
 			description:             "default empty strategy, 0 failures, 0 evictions",
-			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: false}},
+			strategy:                api.DeschedulerStrategy{Params: &api.StrategyParameters{NodeFit: pointer.Bool(false)}},
 			nodes:                   []*v1.Node{test.BuildTestNode("node1", 2000, 3000, 10, nil)},
 			expectedEvictedPodCount: 0,
 			pods:                    []*v1.Pod{}, // no pods come back with field selector phase=Failed
@@ -285,7 +286,7 @@ func TestRemoveFailedPods(t *testing.T) {
 				false,
 				false,
 				false,
-				evictions.WithNodeFit(tc.strategy.Params.NodeFit),
+				evictions.WithNodeFit(*tc.strategy.Params.NodeFit),
 			)
 
 			RemoveFailedPods(ctx, fakeClient, tc.strategy, tc.nodes, podEvictor, evictorFilter, getPodsAssignedToNode)

pkg/descheduler/strategies/node_affinity.go

@@ -82,8 +82,7 @@ func RemovePodsViolatingNodeAffinity(ctx context.Context, client clientset.Inter
 					getPodsAssignedToNode,
 					podutil.WrapFilterFuncs(podFilter, func(pod *v1.Pod) bool {
 						return evictorFilter.Filter(pod) &&
-							!nodeutil.PodFitsCurrentNode(getPodsAssignedToNode, pod, node) &&
-							nodeutil.PodFitsAnyNode(getPodsAssignedToNode, pod, nodes)
+							!nodeutil.PodFitsCurrentNode(getPodsAssignedToNode, pod, node)
 					}),
 				)
 				if err != nil {

pkg/descheduler/strategies/node_affinity_test.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
@@ -49,7 +50,7 @@ func TestRemovePodsViolatingNodeAffinity(t *testing.T) {
 			NodeAffinityType: []string{
 				"requiredDuringSchedulingIgnoredDuringExecution",
 			},
-			NodeFit: true,
+			NodeFit: pointer.Bool(true),
 		},
 	}
 
@@ -226,8 +227,8 @@ func TestRemovePodsViolatingNodeAffinity(t *testing.T) {
 			)
 
 			nodeFit := false
-			if tc.strategy.Params != nil {
-				nodeFit = tc.strategy.Params.NodeFit
+			if tc.strategy.Params != nil && tc.strategy.Params.NodeFit != nil {
+				nodeFit = *tc.strategy.Params.NodeFit
 			}
 
 			evictorFilter := evictions.NewEvictorFilter(

pkg/descheduler/strategies/node_taint_test.go

@@ -343,7 +343,7 @@ func TestDeletePodsViolatingNodeTaints(t *testing.T) {
 
 			strategy := api.DeschedulerStrategy{
 				Params: &api.StrategyParameters{
-					NodeFit:                 tc.nodeFit,
+					NodeFit:                 &tc.nodeFit,
 					IncludePreferNoSchedule: tc.includePreferNoSchedule,
 					ExcludedTaints:          tc.excludedTaints,
 				},

pkg/descheduler/strategies/nodeutilization/highnodeutilization_test.go

@@ -28,7 +28,7 @@ import (
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
-
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
 	podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod"
@@ -516,7 +516,7 @@ func TestHighNodeUtilization(t *testing.T) {
 					NodeResourceUtilizationThresholds: &api.NodeResourceUtilizationThresholds{
 						Thresholds: testCase.thresholds,
 					},
-					NodeFit: true,
+					NodeFit: pointer.Bool(true),
 				},
 			}
 
@@ -527,7 +527,7 @@ func TestHighNodeUtilization(t *testing.T) {
 				false,
 				false,
 				false,
-				evictions.WithNodeFit(strategy.Params.NodeFit),
+				evictions.WithNodeFit(*strategy.Params.NodeFit),
 			)
 
 			HighNodeUtilization(ctx, fakeClient, strategy, testCase.nodes, podEvictor, evictorFilter, getPodsAssignedToNode)

pkg/descheduler/strategies/nodeutilization/lownodeutilization_test.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
@@ -783,7 +784,7 @@ func TestLowNodeUtilization(t *testing.T) {
 						TargetThresholds:       test.targetThresholds,
 						UseDeviationThresholds: test.useDeviationThresholds,
 					},
-					NodeFit: true,
+					NodeFit: pointer.Bool(true),
 				},
 			}
 
@@ -794,7 +795,7 @@ func TestLowNodeUtilization(t *testing.T) {
 				false,
 				false,
 				false,
-				evictions.WithNodeFit(strategy.Params.NodeFit),
+				evictions.WithNodeFit(*strategy.Params.NodeFit),
 			)
 
 			LowNodeUtilization(ctx, fakeClient, strategy, test.nodes, podEvictor, evictorFilter, getPodsAssignedToNode)
@@ -977,7 +978,7 @@ func TestLowNodeUtilizationWithTaints(t *testing.T) {
 					v1.ResourcePods: 70,
 				},
 			},
-			NodeFit: true,
+			NodeFit: pointer.Bool(true),
 		},
 	}
 

pkg/descheduler/strategies/pod_antiaffinity_test.go

@@ -222,7 +222,7 @@ func TestPodAntiAffinity(t *testing.T) {
 			)
 			strategy := api.DeschedulerStrategy{
 				Params: &api.StrategyParameters{
-					NodeFit: test.nodeFit,
+					NodeFit: &test.nodeFit,
 				},
 			}