Merge pull request #903 from knelasevero/migrate-podantiaffinity

Migrate RemovePodsViolatingInterPodAntiAffinity into a plugin

pkg/api/types.go

@@ -74,7 +74,8 @@ type Namespaces struct {
 
 // Besides Namespaces only one of its members may be specified
 // TODO(jchaloup): move Namespaces ThresholdPriority and ThresholdPriorityClassName to individual strategies
-//  once the policy version is bumped to v1alpha2
+//
+//	once the policy version is bumped to v1alpha2
 type StrategyParameters struct {
 	NodeResourceUtilizationThresholds *NodeResourceUtilizationThresholds
 	NodeAffinityType                  []string

pkg/apis/componentconfig/types_pluginargs.go

@@ -101,3 +101,13 @@ type RemovePodsViolatingTopologySpreadConstraintArgs struct {
 	LabelSelector          *metav1.LabelSelector
 	IncludeSoftConstraints bool
 }
+
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// RemovePodsViolatingInterPodAntiAffinity holds arguments used to configure RemovePodsViolatingInterPodAntiAffinity plugin.
+type RemovePodsViolatingInterPodAntiAffinityArgs struct {
+	metav1.TypeMeta
+
+	Namespaces    *api.Namespaces
+	LabelSelector *metav1.LabelSelector
+}

pkg/apis/componentconfig/validation/validation_pluginargs.go

@@ -94,6 +94,14 @@ func ValidateRemovePodsViolatingTopologySpreadConstraintArgs(args *componentconf
 	)
 }
 
+// ValidateRemovePodsViolatingInterPodAntiAffinityArgs validates ValidateRemovePodsViolatingInterPodAntiAffinity arguments
+func ValidateRemovePodsViolatingInterPodAntiAffinityArgs(args *componentconfig.RemovePodsViolatingInterPodAntiAffinityArgs) error {
+	return errorsAggregate(
+		validateNamespaceArgs(args.Namespaces),
+		validateLabelSelectorArgs(args.LabelSelector),
+	)
+}
+
 // errorsAggregate converts all arg validation errors to a single error interface.
 // if no errors, it will return nil.
 func errorsAggregate(errors ...error) error {

pkg/descheduler/descheduler.go

@@ -43,7 +43,6 @@ import (
 	eutils "sigs.k8s.io/descheduler/pkg/descheduler/evictions/utils"
 	nodeutil "sigs.k8s.io/descheduler/pkg/descheduler/node"
 	podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod"
-	"sigs.k8s.io/descheduler/pkg/descheduler/strategies"
 	"sigs.k8s.io/descheduler/pkg/descheduler/strategies/nodeutilization"
 	"sigs.k8s.io/descheduler/pkg/framework"
 	"sigs.k8s.io/descheduler/pkg/utils"
@@ -247,7 +246,7 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 		"RemoveDuplicates":                            nil,
 		"LowNodeUtilization":                          nodeutilization.LowNodeUtilization,
 		"HighNodeUtilization":                         nodeutilization.HighNodeUtilization,
-		"RemovePodsViolatingInterPodAntiAffinity":     strategies.RemovePodsViolatingInterPodAntiAffinity,
+		"RemovePodsViolatingInterPodAntiAffinity":     nil,
 		"RemovePodsViolatingNodeAffinity":             nil,
 		"RemovePodsViolatingNodeTaints":               nil,
 		"RemovePodsHavingTooManyRestarts":             nil,

pkg/descheduler/strategy_migration.go

@@ -29,6 +29,7 @@ import (
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/removeduplicates"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/removefailedpods"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/removepodshavingtoomanyrestarts"
+	"sigs.k8s.io/descheduler/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/removepodsviolatingnodeaffinity"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/removepodsviolatingnodetaints"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint"
@@ -107,6 +108,25 @@ var pluginsMap = map[string]func(ctx context.Context, nodes []*v1.Node, params *
 			klog.V(1).ErrorS(err, "plugin finished with error", "pluginName", removepodsviolatingnodeaffinity.PluginName)
 		}
 	},
+	"RemovePodsViolatingInterPodAntiAffinity": func(ctx context.Context, nodes []*v1.Node, params *api.StrategyParameters, handle *handleImpl) {
+		args := &componentconfig.RemovePodsViolatingInterPodAntiAffinityArgs{
+			Namespaces:    params.Namespaces,
+			LabelSelector: params.LabelSelector,
+		}
+		if err := validation.ValidateRemovePodsViolatingInterPodAntiAffinityArgs(args); err != nil {
+			klog.V(1).ErrorS(err, "unable to validate plugin arguments", "pluginName", removepodsviolatinginterpodantiaffinity.PluginName)
+			return
+		}
+		pg, err := removepodsviolatinginterpodantiaffinity.New(args, handle)
+		if err != nil {
+			klog.V(1).ErrorS(err, "unable to initialize a plugin", "pluginName", removepodsviolatinginterpodantiaffinity.PluginName)
+			return
+		}
+		status := pg.(framework.DeschedulePlugin).Deschedule(ctx, nodes)
+		if status != nil && status.Err != nil {
+			klog.V(1).ErrorS(err, "plugin finished with error", "pluginName", removepodsviolatinginterpodantiaffinity.PluginName)
+		}
+	},
 	"RemovePodsHavingTooManyRestarts": func(ctx context.Context, nodes []*v1.Node, params *api.StrategyParameters, handle *handleImpl) {
 		tooManyRestartsParams := params.PodsHavingTooManyRestarts
 		if tooManyRestartsParams == nil {

pkg/descheduler/strategies/pod_antiaffinity.go → pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go

@@ -14,80 +14,87 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package strategies
+package removepodsviolatinginterpodantiaffinity
 
 import (
 	"context"
 	"fmt"
 
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"sigs.k8s.io/descheduler/pkg/api"
+	"sigs.k8s.io/descheduler/pkg/apis/componentconfig"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
 	podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod"
+	"sigs.k8s.io/descheduler/pkg/framework"
 	"sigs.k8s.io/descheduler/pkg/utils"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 )
 
-func validateRemovePodsViolatingInterPodAntiAffinityParams(params *api.StrategyParameters) error {
-	if params == nil {
-		return 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 fmt.Errorf("only one of Include/Exclude namespaces can be set")
-	}
-	if params.ThresholdPriority != nil && params.ThresholdPriorityClassName != "" {
-		return fmt.Errorf("only one of thresholdPriority and thresholdPriorityClassName can be set")
-	}
+const PluginName = "RemovePodsViolatingInterPodAntiAffinity"
 
-	return nil
+// RemovePodsViolatingInterPodAntiAffinity evicts pods on the node which violate inter pod anti affinity
+type RemovePodsViolatingInterPodAntiAffinity struct {
+	handle    framework.Handle
+	args      *componentconfig.RemovePodsViolatingInterPodAntiAffinityArgs
+	podFilter podutil.FilterFunc
 }
 
-// RemovePodsViolatingInterPodAntiAffinity evicts pods on the node which are having a pod affinity rules.
-func RemovePodsViolatingInterPodAntiAffinity(ctx context.Context, client clientset.Interface, strategy api.DeschedulerStrategy, nodes []*v1.Node, podEvictor *evictions.PodEvictor, evictorFilter *evictions.EvictorFilter, getPodsAssignedToNode podutil.GetPodsAssignedToNodeFunc) {
-	if err := validateRemovePodsViolatingInterPodAntiAffinityParams(strategy.Params); err != nil {
-		klog.ErrorS(err, "Invalid RemovePodsViolatingInterPodAntiAffinity parameters")
-		return
+var _ framework.Plugin = &RemovePodsViolatingInterPodAntiAffinity{}
+var _ framework.DeschedulePlugin = &RemovePodsViolatingInterPodAntiAffinity{}
+
+// New builds plugin from its arguments while passing a handle
+func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error) {
+	interPodAntiAffinityArgs, ok := args.(*componentconfig.RemovePodsViolatingInterPodAntiAffinityArgs)
+	if !ok {
+		return nil, fmt.Errorf("want args to be of type RemovePodsViolatingInterPodAntiAffinityArgs, got %T", args)
 	}
 
 	var includedNamespaces, excludedNamespaces sets.String
-	var labelSelector *metav1.LabelSelector
-	if strategy.Params != nil {
-		if strategy.Params.Namespaces != nil {
-			includedNamespaces = sets.NewString(strategy.Params.Namespaces.Include...)
-			excludedNamespaces = sets.NewString(strategy.Params.Namespaces.Exclude...)
-		}
-		labelSelector = strategy.Params.LabelSelector
+	if interPodAntiAffinityArgs.Namespaces != nil {
+		includedNamespaces = sets.NewString(interPodAntiAffinityArgs.Namespaces.Include...)
+		excludedNamespaces = sets.NewString(interPodAntiAffinityArgs.Namespaces.Exclude...)
 	}
 
 	podFilter, err := podutil.NewOptions().
 		WithNamespaces(includedNamespaces).
 		WithoutNamespaces(excludedNamespaces).
-		WithLabelSelector(labelSelector).
+		WithLabelSelector(interPodAntiAffinityArgs.LabelSelector).
 		BuildFilterFunc()
 	if err != nil {
-		klog.ErrorS(err, "Error initializing pod filter function")
-		return
+		return nil, fmt.Errorf("error initializing pod filter function: %v", err)
 	}
 
+	return &RemovePodsViolatingInterPodAntiAffinity{
+		handle:    handle,
+		podFilter: podFilter,
+		args:      interPodAntiAffinityArgs,
+	}, nil
+}
+
+// Name retrieves the plugin name
+func (d *RemovePodsViolatingInterPodAntiAffinity) Name() string {
+	return PluginName
+}
+
+func (d *RemovePodsViolatingInterPodAntiAffinity) Deschedule(ctx context.Context, nodes []*v1.Node) *framework.Status {
 loop:
 	for _, node := range nodes {
 		klog.V(1).InfoS("Processing node", "node", klog.KObj(node))
-		pods, err := podutil.ListPodsOnANode(node.Name, getPodsAssignedToNode, podFilter)
+		pods, err := podutil.ListPodsOnANode(node.Name, d.handle.GetPodsAssignedToNodeFunc(), d.podFilter)
 		if err != nil {
-			return
+			return &framework.Status{
+				Err: fmt.Errorf("error listing pods: %v", err),
+			}
 		}
 		// sort the evictable Pods based on priority, if there are multiple pods with same priority, they are sorted based on QoS tiers.
 		podutil.SortPodsBasedOnPriorityLowToHigh(pods)
 		totalPods := len(pods)
 		for i := 0; i < totalPods; i++ {
-			if checkPodsWithAntiAffinityExist(pods[i], pods) && evictorFilter.Filter(pods[i]) {
-				if podEvictor.EvictPod(ctx, pods[i], evictions.EvictOptions{}) {
+			if checkPodsWithAntiAffinityExist(pods[i], pods) && d.handle.Evictor().Filter(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.
 					// Update pods.
@@ -96,11 +103,12 @@ loop:
 					totalPods--
 				}
 			}
-			if podEvictor.NodeLimitExceeded(node) {
+			if d.handle.Evictor().NodeLimitExceeded(node) {
 				continue loop
 			}
 		}
 	}
+	return nil
 }
 
 // checkPodsWithAntiAffinityExist checks if there are other pods on the node that the current pod cannot tolerate.

pkg/descheduler/strategies/pod_antiaffinity_test.go → pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package strategies
+package removepodsviolatinginterpodantiaffinity
 
 import (
 	"context"
@@ -27,10 +27,12 @@ import (
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/tools/events"
+	"sigs.k8s.io/descheduler/pkg/apis/componentconfig"
+	"sigs.k8s.io/descheduler/pkg/framework"
 
-	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
 	podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod"
+	frameworkfake "sigs.k8s.io/descheduler/pkg/framework/fake"
 	"sigs.k8s.io/descheduler/pkg/utils"
 	"sigs.k8s.io/descheduler/test"
 )
@@ -224,23 +226,27 @@ func TestPodAntiAffinity(t *testing.T) {
 				false,
 				eventRecorder,
 			)
-			strategy := api.DeschedulerStrategy{
-				Params: &api.StrategyParameters{
-					NodeFit: test.nodeFit,
-				},
+			handle := &frameworkfake.HandleImpl{
+				ClientsetImpl:                 fakeClient,
+				GetPodsAssignedToNodeFuncImpl: getPodsAssignedToNode,
+				PodEvictorImpl:                podEvictor,
+				SharedInformerFactoryImpl:     sharedInformerFactory,
+				EvictorFilterImpl: evictions.NewEvictorFilter(
+					test.nodes,
+					getPodsAssignedToNode,
+					false,
+					false,
+					false,
+					false,
+					evictions.WithNodeFit(test.nodeFit),
+				),
 			}
-
-			evictorFilter := evictions.NewEvictorFilter(
-				test.nodes,
-				getPodsAssignedToNode,
-				false,
-				false,
-				false,
-				false,
-				evictions.WithNodeFit(test.nodeFit),
+			plugin, err := New(
+				&componentconfig.RemovePodsViolatingInterPodAntiAffinityArgs{},
+				handle,
 			)
 
-			RemovePodsViolatingInterPodAntiAffinity(ctx, fakeClient, strategy, test.nodes, podEvictor, evictorFilter, getPodsAssignedToNode)
+			plugin.(framework.DeschedulePlugin).Deschedule(ctx, test.nodes)
 			podsEvicted := podEvictor.TotalEvicted()
 			if podsEvicted != test.expectedEvictedPodCount {
 				t.Errorf("Unexpected no of pods evicted: pods evicted: %d, expected: %d", podsEvicted, test.expectedEvictedPodCount)