From a2dd86ac3bab759320c92efef137828e297df239 Mon Sep 17 00:00:00 2001 From: Lucas Severo Alves Date: Thu, 11 Aug 2022 18:22:45 +0200 Subject: [PATCH] Migrate RemovePodsViolatingInterPodAntiAffinity into a plugin --- pkg/api/types.go | 3 +- pkg/apis/componentconfig/types_pluginargs.go | 10 +++ .../validation/validation_pluginargs.go | 8 ++ .../componentconfig/zz_generated.deepcopy.go | 35 ++++++++ pkg/descheduler/descheduler.go | 3 +- pkg/descheduler/strategy_migration.go | 20 +++++ .../pod_antiaffinity.go | 80 ++++++++++--------- .../pod_antiaffinity_test.go | 38 +++++---- 8 files changed, 142 insertions(+), 55 deletions(-) rename pkg/{descheduler/strategies => framework/plugins/removepodsviolatinginterpodantiaffinity}/pod_antiaffinity.go (58%) rename pkg/{descheduler/strategies => framework/plugins/removepodsviolatinginterpodantiaffinity}/pod_antiaffinity_test.go (90%) diff --git a/pkg/api/types.go b/pkg/api/types.go index e4e707376..4880eaa75 100644 --- a/pkg/api/types.go +++ b/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 diff --git a/pkg/apis/componentconfig/types_pluginargs.go b/pkg/apis/componentconfig/types_pluginargs.go index a48dd84c9..a06ed5d05 100644 --- a/pkg/apis/componentconfig/types_pluginargs.go +++ b/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 +} diff --git a/pkg/apis/componentconfig/validation/validation_pluginargs.go b/pkg/apis/componentconfig/validation/validation_pluginargs.go index d5b6f3a20..1e009bf92 100644 --- a/pkg/apis/componentconfig/validation/validation_pluginargs.go +++ b/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 { diff --git a/pkg/apis/componentconfig/zz_generated.deepcopy.go b/pkg/apis/componentconfig/zz_generated.deepcopy.go index cd7349707..4461234ed 100644 --- a/pkg/apis/componentconfig/zz_generated.deepcopy.go +++ b/pkg/apis/componentconfig/zz_generated.deepcopy.go @@ -219,6 +219,41 @@ func (in *RemovePodsHavingTooManyRestartsArgs) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RemovePodsViolatingInterPodAntiAffinityArgs) DeepCopyInto(out *RemovePodsViolatingInterPodAntiAffinityArgs) { + *out = *in + out.TypeMeta = in.TypeMeta + if in.Namespaces != nil { + in, out := &in.Namespaces, &out.Namespaces + *out = new(api.Namespaces) + (*in).DeepCopyInto(*out) + } + if in.LabelSelector != nil { + in, out := &in.LabelSelector, &out.LabelSelector + *out = new(v1.LabelSelector) + (*in).DeepCopyInto(*out) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RemovePodsViolatingInterPodAntiAffinityArgs. +func (in *RemovePodsViolatingInterPodAntiAffinityArgs) DeepCopy() *RemovePodsViolatingInterPodAntiAffinityArgs { + if in == nil { + return nil + } + out := new(RemovePodsViolatingInterPodAntiAffinityArgs) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *RemovePodsViolatingInterPodAntiAffinityArgs) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RemovePodsViolatingNodeAffinityArgs) DeepCopyInto(out *RemovePodsViolatingNodeAffinityArgs) { *out = *in diff --git a/pkg/descheduler/descheduler.go b/pkg/descheduler/descheduler.go index 2bb8f0a07..af0a7b0fa 100644 --- a/pkg/descheduler/descheduler.go +++ b/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, diff --git a/pkg/descheduler/strategy_migration.go b/pkg/descheduler/strategy_migration.go index dd7d24c10..049e63a7e 100644 --- a/pkg/descheduler/strategy_migration.go +++ b/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 { diff --git a/pkg/descheduler/strategies/pod_antiaffinity.go b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go similarity index 58% rename from pkg/descheduler/strategies/pod_antiaffinity.go rename to pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity.go index c590bfb67..87e00032a 100644 --- a/pkg/descheduler/strategies/pod_antiaffinity.go +++ b/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 - } +const PluginName = "RemovePodsViolatingInterPodAntiAffinity" - // 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") - } - - 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. diff --git a/pkg/descheduler/strategies/pod_antiaffinity_test.go b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity_test.go similarity index 90% rename from pkg/descheduler/strategies/pod_antiaffinity_test.go rename to pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/pod_antiaffinity_test.go index fd1034dc4..f0787f306 100644 --- a/pkg/descheduler/strategies/pod_antiaffinity_test.go +++ b/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)