From 3510aba01dc75c3424a2bae81a51ed5ea54932ff Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Thu, 30 Mar 2023 10:56:19 +0200 Subject: [PATCH] Detect individual extension points from plugin types - Populate extension points automatically from plugin types - Make a list of enabled extension points based on a profile configuration - Populate filter and pre-eviction filter handles from their corresponding extension points --- pkg/framework/fake/plugin/fake.go | 274 +++++++++++- pkg/framework/fake/plugin/fixture.go | 47 +- .../fake/plugin/zz_generated.deepcopy.go | 75 ++++ .../pluginregistry/pluginregistry.go | 2 +- pkg/framework/profile/profile.go | 156 ++++--- pkg/framework/profile/profile_test.go | 419 +++++++++++++++++- 6 files changed, 901 insertions(+), 72 deletions(-) diff --git a/pkg/framework/fake/plugin/fake.go b/pkg/framework/fake/plugin/fake.go index 3ba8396eb..eb37dd5a4 100644 --- a/pkg/framework/fake/plugin/fake.go +++ b/pkg/framework/fake/plugin/fake.go @@ -42,6 +42,9 @@ var ( _ frameworktypes.EvictorPlugin = &FakePlugin{} _ frameworktypes.DeschedulePlugin = &FakePlugin{} _ frameworktypes.BalancePlugin = &FakePlugin{} + _ frameworktypes.EvictorPlugin = &FakeFilterPlugin{} + _ frameworktypes.DeschedulePlugin = &FakeDeschedulePlugin{} + _ frameworktypes.BalancePlugin = &FakeBalancePlugin{} ) // FakePlugin is a configurable plugin used for testing @@ -84,6 +87,10 @@ func New(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plug return ev, nil } +func (c *FakePlugin) AddReactor(extensionPoint string, reaction ReactionFunc) { + c.ReactionChain = append(c.ReactionChain, &SimpleReactor{ExtensionPoint: extensionPoint, Reaction: reaction}) +} + // Name retrieves the plugin name func (d *FakePlugin) Name() string { return d.PluginName @@ -103,7 +110,7 @@ func (d *FakePlugin) handleAction(action Action) *frameworktypes.Status { if !reactor.Handles(actionCopy) { continue } - handled, err := reactor.React(actionCopy) + handled, _, err := reactor.React(actionCopy) if !handled { continue } @@ -136,3 +143,268 @@ func (d *FakePlugin) Balance(ctx context.Context, nodes []*v1.Node) *frameworkty nodes: nodes, }) } + +// +k8s:deepcopy-gen=true +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object + +// FakeDeschedulePluginArgs holds arguments used to configure FakeDeschedulePlugin plugin. +type FakeDeschedulePluginArgs struct { + metav1.TypeMeta `json:",inline"` +} + +// FakeDeschedulePlugin is a configurable plugin used for testing +type FakeDeschedulePlugin struct { + PluginName string + + // ReactionChain is the list of reactors that will be attempted for every + // request in the order they are tried. + ReactionChain []Reactor + + args runtime.Object + handle frameworktypes.Handle +} + +func NewFakeDeschedulePluginFncFromFake(fp *FakeDeschedulePlugin) pluginregistry.PluginBuilder { + return func(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plugin, error) { + fakePluginArgs, ok := args.(*FakeDeschedulePluginArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type FakeDeschedulePluginArgs, got %T", args) + } + + fp.handle = handle + fp.args = fakePluginArgs + + return fp, nil + } +} + +// New builds plugin from its arguments while passing a handle +func NewFakeDeschedule(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plugin, error) { + fakePluginArgs, ok := args.(*FakeDeschedulePluginArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type FakePluginArgs, got %T", args) + } + + ev := &FakeDeschedulePlugin{} + ev.handle = handle + ev.args = fakePluginArgs + + return ev, nil +} + +func (c *FakeDeschedulePlugin) AddReactor(extensionPoint string, reaction ReactionFunc) { + c.ReactionChain = append(c.ReactionChain, &SimpleReactor{ExtensionPoint: extensionPoint, Reaction: reaction}) +} + +// Name retrieves the plugin name +func (d *FakeDeschedulePlugin) Name() string { + return d.PluginName +} + +func (d *FakeDeschedulePlugin) Deschedule(ctx context.Context, nodes []*v1.Node) *frameworktypes.Status { + return d.handleAction(&DescheduleActionImpl{ + ActionImpl: ActionImpl{ + handle: d.handle, + extensionPoint: string(frameworktypes.DescheduleExtensionPoint), + }, + nodes: nodes, + }) +} + +func (d *FakeDeschedulePlugin) handleAction(action Action) *frameworktypes.Status { + actionCopy := action.DeepCopy() + for _, reactor := range d.ReactionChain { + if !reactor.Handles(actionCopy) { + continue + } + handled, _, err := reactor.React(actionCopy) + if !handled { + continue + } + + return &frameworktypes.Status{ + Err: err, + } + } + return &frameworktypes.Status{ + Err: fmt.Errorf("unhandled %q action", action.GetExtensionPoint()), + } +} + +// +k8s:deepcopy-gen=true +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object + +// FakeBalancePluginArgs holds arguments used to configure FakeBalancePlugin plugin. +type FakeBalancePluginArgs struct { + metav1.TypeMeta `json:",inline"` +} + +// FakeBalancePlugin is a configurable plugin used for testing +type FakeBalancePlugin struct { + PluginName string + + // ReactionChain is the list of reactors that will be attempted for every + // request in the order they are tried. + ReactionChain []Reactor + + args runtime.Object + handle frameworktypes.Handle +} + +func NewFakeBalancePluginFncFromFake(fp *FakeBalancePlugin) pluginregistry.PluginBuilder { + return func(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plugin, error) { + fakePluginArgs, ok := args.(*FakeBalancePluginArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type FakeBalancePluginArgs, got %T", args) + } + + fp.handle = handle + fp.args = fakePluginArgs + + return fp, nil + } +} + +// New builds plugin from its arguments while passing a handle +func NewFakeBalance(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plugin, error) { + fakePluginArgs, ok := args.(*FakeBalancePluginArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type FakePluginArgs, got %T", args) + } + + ev := &FakeBalancePlugin{} + ev.handle = handle + ev.args = fakePluginArgs + + return ev, nil +} + +func (c *FakeBalancePlugin) AddReactor(extensionPoint string, reaction ReactionFunc) { + c.ReactionChain = append(c.ReactionChain, &SimpleReactor{ExtensionPoint: extensionPoint, Reaction: reaction}) +} + +// Name retrieves the plugin name +func (d *FakeBalancePlugin) Name() string { + return d.PluginName +} + +func (d *FakeBalancePlugin) Balance(ctx context.Context, nodes []*v1.Node) *frameworktypes.Status { + return d.handleAction(&BalanceActionImpl{ + ActionImpl: ActionImpl{ + handle: d.handle, + extensionPoint: string(frameworktypes.BalanceExtensionPoint), + }, + nodes: nodes, + }) +} + +func (d *FakeBalancePlugin) handleAction(action Action) *frameworktypes.Status { + actionCopy := action.DeepCopy() + for _, reactor := range d.ReactionChain { + if !reactor.Handles(actionCopy) { + continue + } + handled, _, err := reactor.React(actionCopy) + if !handled { + continue + } + + return &frameworktypes.Status{ + Err: err, + } + } + return &frameworktypes.Status{ + Err: fmt.Errorf("unhandled %q action", action.GetExtensionPoint()), + } +} + +// +k8s:deepcopy-gen=true +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object + +// FakeFilterPluginArgs holds arguments used to configure FakeFilterPlugin plugin. +type FakeFilterPluginArgs struct { + metav1.TypeMeta `json:",inline"` +} + +// FakeFilterPlugin is a configurable plugin used for testing +type FakeFilterPlugin struct { + PluginName string + + // ReactionChain is the list of reactors that will be attempted for every + // request in the order they are tried. + ReactionChain []Reactor + + args runtime.Object + handle frameworktypes.Handle +} + +func NewFakeFilterPluginFncFromFake(fp *FakeFilterPlugin) pluginregistry.PluginBuilder { + return func(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plugin, error) { + fakePluginArgs, ok := args.(*FakeFilterPluginArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type FakeFilterPluginArgs, got %T", args) + } + + fp.handle = handle + fp.args = fakePluginArgs + + return fp, nil + } +} + +// New builds plugin from its arguments while passing a handle +func NewFakeFilter(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plugin, error) { + fakePluginArgs, ok := args.(*FakeFilterPluginArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type FakePluginArgs, got %T", args) + } + + ev := &FakeFilterPlugin{} + ev.handle = handle + ev.args = fakePluginArgs + + return ev, nil +} + +func (c *FakeFilterPlugin) AddReactor(extensionPoint string, reaction ReactionFunc) { + c.ReactionChain = append(c.ReactionChain, &SimpleReactor{ExtensionPoint: extensionPoint, Reaction: reaction}) +} + +// Name retrieves the plugin name +func (d *FakeFilterPlugin) Name() string { + return d.PluginName +} + +func (d *FakeFilterPlugin) Filter(pod *v1.Pod) bool { + return d.handleBoolAction(&FilterActionImpl{ + ActionImpl: ActionImpl{ + handle: d.handle, + extensionPoint: string(frameworktypes.FilterExtensionPoint), + }, + }) +} + +func (d *FakeFilterPlugin) PreEvictionFilter(pod *v1.Pod) bool { + return d.handleBoolAction(&PreEvictionFilterActionImpl{ + ActionImpl: ActionImpl{ + handle: d.handle, + extensionPoint: string(frameworktypes.PreEvictionFilterExtensionPoint), + }, + }) +} + +func (d *FakeFilterPlugin) handleBoolAction(action Action) bool { + actionCopy := action.DeepCopy() + for _, reactor := range d.ReactionChain { + if !reactor.Handles(actionCopy) { + continue + } + handled, filter, _ := reactor.React(actionCopy) + if !handled { + continue + } + + return filter + } + panic(fmt.Errorf("unhandled %q action", action.GetExtensionPoint())) +} diff --git a/pkg/framework/fake/plugin/fixture.go b/pkg/framework/fake/plugin/fixture.go index 574d796b3..f4bd78a9b 100644 --- a/pkg/framework/fake/plugin/fixture.go +++ b/pkg/framework/fake/plugin/fixture.go @@ -24,6 +24,8 @@ type Action interface { DeepCopy() Action } +type ReactionFunc func(action Action) (handled, filter bool, err error) + // Reactor is an interface to allow the composition of reaction functions. type Reactor interface { // Handles indicates whether or not this Reactor deals with a given @@ -31,7 +33,8 @@ type Reactor interface { Handles(action Action) bool // React handles the action. It may choose to // delegate by indicated handled=false. - React(action Action) (handled bool, err error) + // filter is used to store results of filter based actions + React(action Action) (handled, filter bool, err error) } // SimpleReactor is a Reactor. Each reaction function is attached to a given extensionPoint. "*" in either field matches everything for that value. @@ -44,12 +47,10 @@ func (r *SimpleReactor) Handles(action Action) bool { return r.ExtensionPoint == "*" || r.ExtensionPoint == action.GetExtensionPoint() } -func (r *SimpleReactor) React(action Action) (bool, error) { +func (r *SimpleReactor) React(action Action) (bool, bool, error) { return r.Reaction(action) } -type ReactionFunc func(action Action) (handled bool, err error) - type DescheduleAction interface { Action CanDeschedule() bool @@ -62,6 +63,16 @@ type BalanceAction interface { Nodes() []*v1.Node } +type FilterAction interface { + Action + CanFilter() bool +} + +type PreEvictionFilterAction interface { + Action + CanPreEvictionFilter() bool +} + type ActionImpl struct { handle frameworktypes.Handle extensionPoint string @@ -130,6 +141,30 @@ func (a BalanceActionImpl) DeepCopy() Action { } } -func (c *FakePlugin) AddReactor(extensionPoint string, reaction ReactionFunc) { - c.ReactionChain = append(c.ReactionChain, &SimpleReactor{ExtensionPoint: extensionPoint, Reaction: reaction}) +type FilterActionImpl struct { + ActionImpl +} + +func (d FilterActionImpl) CanFilter() bool { + return true +} + +func (a FilterActionImpl) DeepCopy() Action { + return FilterActionImpl{ + ActionImpl: a.ActionImpl.DeepCopy().(ActionImpl), + } +} + +type PreEvictionFilterActionImpl struct { + ActionImpl +} + +func (d PreEvictionFilterActionImpl) CanPreEvictionFilter() bool { + return true +} + +func (a PreEvictionFilterActionImpl) DeepCopy() Action { + return PreEvictionFilterActionImpl{ + ActionImpl: a.ActionImpl.DeepCopy().(ActionImpl), + } } diff --git a/pkg/framework/fake/plugin/zz_generated.deepcopy.go b/pkg/framework/fake/plugin/zz_generated.deepcopy.go index d2e7d2d08..8bbea0210 100644 --- a/pkg/framework/fake/plugin/zz_generated.deepcopy.go +++ b/pkg/framework/fake/plugin/zz_generated.deepcopy.go @@ -25,6 +25,81 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FakeBalancePluginArgs) DeepCopyInto(out *FakeBalancePluginArgs) { + *out = *in + out.TypeMeta = in.TypeMeta + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FakeBalancePluginArgs. +func (in *FakeBalancePluginArgs) DeepCopy() *FakeBalancePluginArgs { + if in == nil { + return nil + } + out := new(FakeBalancePluginArgs) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *FakeBalancePluginArgs) 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 *FakeDeschedulePluginArgs) DeepCopyInto(out *FakeDeschedulePluginArgs) { + *out = *in + out.TypeMeta = in.TypeMeta + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FakeDeschedulePluginArgs. +func (in *FakeDeschedulePluginArgs) DeepCopy() *FakeDeschedulePluginArgs { + if in == nil { + return nil + } + out := new(FakeDeschedulePluginArgs) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *FakeDeschedulePluginArgs) 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 *FakeFilterPluginArgs) DeepCopyInto(out *FakeFilterPluginArgs) { + *out = *in + out.TypeMeta = in.TypeMeta + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FakeFilterPluginArgs. +func (in *FakeFilterPluginArgs) DeepCopy() *FakeFilterPluginArgs { + if in == nil { + return nil + } + out := new(FakeFilterPluginArgs) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *FakeFilterPluginArgs) 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 *FakePluginArgs) DeepCopyInto(out *FakePluginArgs) { *out = *in diff --git a/pkg/framework/pluginregistry/pluginregistry.go b/pkg/framework/pluginregistry/pluginregistry.go index 76e454a94..a200181db 100644 --- a/pkg/framework/pluginregistry/pluginregistry.go +++ b/pkg/framework/pluginregistry/pluginregistry.go @@ -42,7 +42,7 @@ type ( PluginArgDefaulter = func(args runtime.Object) ) -type Registry = map[string]PluginUtilities +type Registry map[string]PluginUtilities func NewRegistry() Registry { return Registry{} diff --git a/pkg/framework/profile/profile.go b/pkg/framework/profile/profile.go index b2d2e3325..d242c5cbe 100644 --- a/pkg/framework/profile/profile.go +++ b/pkg/framework/profile/profile.go @@ -23,11 +23,11 @@ import ( "sigs.k8s.io/descheduler/pkg/descheduler/evictions" podutil "sigs.k8s.io/descheduler/pkg/descheduler/pod" "sigs.k8s.io/descheduler/pkg/framework/pluginregistry" - "sigs.k8s.io/descheduler/pkg/framework/plugins/defaultevictor" frameworktypes "sigs.k8s.io/descheduler/pkg/framework/types" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" @@ -37,20 +37,21 @@ import ( // evictorImpl implements the Evictor interface so plugins // can evict a pod without importing a specific pod evictor type evictorImpl struct { - podEvictor *evictions.PodEvictor - evictorFilter frameworktypes.EvictorPlugin + podEvictor *evictions.PodEvictor + filter podutil.FilterFunc + preEvictionFilter podutil.FilterFunc } var _ frameworktypes.Evictor = &evictorImpl{} // Filter checks if a pod can be evicted func (ei *evictorImpl) Filter(pod *v1.Pod) bool { - return ei.evictorFilter.Filter(pod) + return ei.filter(pod) } // PreEvictionFilter checks if pod can be evicted right before eviction func (ei *evictorImpl) PreEvictionFilter(pod *v1.Pod) bool { - return ei.evictorFilter.PreEvictionFilter(pod) + return ei.preEvictionFilter(pod) } // Evict evicts a pod (no pre-check performed) @@ -92,12 +93,30 @@ func (hi *handleImpl) Evictor() frameworktypes.Evictor { return hi.evictor } +type filterPlugin interface { + frameworktypes.Plugin + Filter(pod *v1.Pod) bool +} + +type preEvictionFilterPlugin interface { + frameworktypes.Plugin + PreEvictionFilter(pod *v1.Pod) bool +} + type profileImpl struct { profileName string podEvictor *evictions.PodEvictor - deschedulePlugins []frameworktypes.DeschedulePlugin - balancePlugins []frameworktypes.BalancePlugin + deschedulePlugins []frameworktypes.DeschedulePlugin + balancePlugins []frameworktypes.BalancePlugin + filterPlugins []filterPlugin + preEvictionFilterPlugins []preEvictionFilterPlugin + + // Each extension point with a list of plugins implementing the extension point. + deschedule sets.String + balance sets.String + filter sets.String + preEvictionFilter sets.String } // Option for the handleImpl. @@ -164,6 +183,26 @@ func buildPlugin(config api.DeschedulerProfile, pluginName string, handle *handl return pg, nil } +func (p *profileImpl) registryToExtensionPoints(registry pluginregistry.Registry) { + p.deschedule = sets.NewString() + p.balance = sets.NewString() + p.filter = sets.NewString() + p.preEvictionFilter = sets.NewString() + + for plugin, pluginUtilities := range registry { + if _, ok := pluginUtilities.PluginType.(frameworktypes.DeschedulePlugin); ok { + p.deschedule.Insert(plugin) + } + if _, ok := pluginUtilities.PluginType.(frameworktypes.BalancePlugin); ok { + p.balance.Insert(plugin) + } + if _, ok := pluginUtilities.PluginType.(frameworktypes.EvictorPlugin); ok { + p.filter.Insert(plugin) + p.preEvictionFilter.Insert(plugin) + } + } +} + func NewProfile(config api.DeschedulerProfile, reg pluginregistry.Registry, opts ...Option) (*profileImpl, error) { hOpts := &handleImplOpts{} for _, optFnc := range opts { @@ -182,16 +221,27 @@ func NewProfile(config api.DeschedulerProfile, reg pluginregistry.Registry, opts return nil, fmt.Errorf("podEvictor missing") } - evictorPlugin, err := buildPlugin(config, defaultevictor.PluginName, &handleImpl{ - clientSet: hOpts.clientSet, - getPodsAssignedToNodeFunc: hOpts.getPodsAssignedToNodeFunc, - sharedInformerFactory: hOpts.sharedInformerFactory, - }, reg) - if err != nil { - return nil, fmt.Errorf("unable to build %v plugin: %v", defaultevictor.PluginName, err) + pi := &profileImpl{ + profileName: config.Name, + podEvictor: hOpts.podEvictor, + deschedulePlugins: []frameworktypes.DeschedulePlugin{}, + balancePlugins: []frameworktypes.BalancePlugin{}, + filterPlugins: []filterPlugin{}, + preEvictionFilterPlugins: []preEvictionFilterPlugin{}, } - if evictorPlugin == nil { - return nil, fmt.Errorf("empty plugin build for %v plugin: %v", defaultevictor.PluginName, err) + pi.registryToExtensionPoints(reg) + + if !pi.deschedule.HasAll(config.Plugins.Deschedule.Enabled...) { + return nil, fmt.Errorf("profile %q configures deschedule extension point of non-existing plugins: %v", config.Name, sets.NewString(config.Plugins.Deschedule.Enabled...).Difference(pi.deschedule)) + } + if !pi.balance.HasAll(config.Plugins.Balance.Enabled...) { + return nil, fmt.Errorf("profile %q configures balance extension point of non-existing plugins: %v", config.Name, sets.NewString(config.Plugins.Balance.Enabled...).Difference(pi.balance)) + } + if !pi.filter.HasAll(config.Plugins.Filter.Enabled...) { + return nil, fmt.Errorf("profile %q configures filter extension point of non-existing plugins: %v", config.Name, sets.NewString(config.Plugins.Filter.Enabled...).Difference(pi.filter)) + } + if !pi.preEvictionFilter.HasAll(config.Plugins.PreEvictionFilter.Enabled...) { + return nil, fmt.Errorf("profile %q configures preEvictionFilter extension point of non-existing plugins: %v", config.Name, sets.NewString(config.Plugins.PreEvictionFilter.Enabled...).Difference(pi.preEvictionFilter)) } handle := &handleImpl{ @@ -199,56 +249,52 @@ func NewProfile(config api.DeschedulerProfile, reg pluginregistry.Registry, opts getPodsAssignedToNodeFunc: hOpts.getPodsAssignedToNodeFunc, sharedInformerFactory: hOpts.sharedInformerFactory, evictor: &evictorImpl{ - podEvictor: hOpts.podEvictor, - evictorFilter: evictorPlugin.(frameworktypes.EvictorPlugin), + podEvictor: hOpts.podEvictor, }, } - deschedulePlugins := []frameworktypes.DeschedulePlugin{} - balancePlugins := []frameworktypes.BalancePlugin{} + pluginNames := append(config.Plugins.Deschedule.Enabled, config.Plugins.Balance.Enabled...) + pluginNames = append(pluginNames, config.Plugins.Filter.Enabled...) + pluginNames = append(pluginNames, config.Plugins.PreEvictionFilter.Enabled...) - descheduleEnabled := make(map[string]struct{}) - balanceEnabled := make(map[string]struct{}) - for _, name := range config.Plugins.Deschedule.Enabled { - descheduleEnabled[name] = struct{}{} - } - for _, name := range config.Plugins.Balance.Enabled { - balanceEnabled[name] = struct{}{} - } - - // Assuming only a list of enabled extension points. - // Later, when a default list of plugins and their extension points is established, - // compute the list of enabled extension points as (DefaultEnabled + Enabled - Disabled) - for _, plugin := range append(config.Plugins.Deschedule.Enabled, config.Plugins.Balance.Enabled...) { + plugins := make(map[string]frameworktypes.Plugin) + for _, plugin := range sets.NewString(pluginNames...).List() { pg, err := buildPlugin(config, plugin, handle, reg) if err != nil { return nil, fmt.Errorf("unable to build %v plugin: %v", plugin, err) } - if pg != nil { - // pg can be of any of each type, or both - - if _, exists := descheduleEnabled[plugin]; exists { - _, ok := pg.(frameworktypes.DeschedulePlugin) - if ok { - deschedulePlugins = append(deschedulePlugins, pg.(frameworktypes.DeschedulePlugin)) - } - } - - if _, exists := balanceEnabled[plugin]; exists { - _, ok := pg.(frameworktypes.BalancePlugin) - if ok { - balancePlugins = append(balancePlugins, pg.(frameworktypes.BalancePlugin)) - } - } + if pg == nil { + return nil, fmt.Errorf("got empty %v plugin build", plugin) } + plugins[plugin] = pg } - return &profileImpl{ - profileName: config.Name, - podEvictor: hOpts.podEvictor, - deschedulePlugins: deschedulePlugins, - balancePlugins: balancePlugins, - }, nil + // Later, when a default list of plugins and their extension points is established, + // compute the list of enabled extension points as (DefaultEnabled + Enabled - Disabled) + for _, pluginName := range config.Plugins.Deschedule.Enabled { + pi.deschedulePlugins = append(pi.deschedulePlugins, plugins[pluginName].(frameworktypes.DeschedulePlugin)) + } + + for _, pluginName := range config.Plugins.Balance.Enabled { + pi.balancePlugins = append(pi.balancePlugins, plugins[pluginName].(frameworktypes.BalancePlugin)) + } + + filters := []podutil.FilterFunc{} + for _, pluginName := range config.Plugins.Filter.Enabled { + pi.filterPlugins = append(pi.filterPlugins, plugins[pluginName].(filterPlugin)) + filters = append(filters, plugins[pluginName].(filterPlugin).Filter) + } + + preEvictionFilters := []podutil.FilterFunc{} + for _, pluginName := range config.Plugins.PreEvictionFilter.Enabled { + pi.preEvictionFilterPlugins = append(pi.preEvictionFilterPlugins, plugins[pluginName].(preEvictionFilterPlugin)) + preEvictionFilters = append(preEvictionFilters, plugins[pluginName].(preEvictionFilterPlugin).PreEvictionFilter) + } + + handle.evictor.filter = podutil.WrapFilterFuncs(filters...) + handle.evictor.preEvictionFilter = podutil.WrapFilterFuncs(preEvictionFilters...) + + return pi, nil } func (d profileImpl) RunDeschedulePlugins(ctx context.Context, nodes []*v1.Node) *frameworktypes.Status { diff --git a/pkg/framework/profile/profile_test.go b/pkg/framework/profile/profile_test.go index 4d48ad8d9..2418ba165 100644 --- a/pkg/framework/profile/profile_test.go +++ b/pkg/framework/profile/profile_test.go @@ -3,12 +3,16 @@ package profile import ( "context" "fmt" + "sort" "testing" + "github.com/google/go-cmp/cmp" + v1 "k8s.io/api/core/v1" policy "k8s.io/api/policy/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/informers" fakeclientset "k8s.io/client-go/kubernetes/fake" core "k8s.io/client-go/testing" @@ -24,7 +28,7 @@ import ( testutils "sigs.k8s.io/descheduler/test" ) -func TestProfileTopExtensionPoints(t *testing.T) { +func TestProfileDescheduleBalanceExtensionPointsEviction(t *testing.T) { tests := []struct { name string config api.DeschedulerProfile @@ -167,25 +171,25 @@ func TestProfileTopExtensionPoints(t *testing.T) { fakePlugin := fakeplugin.FakePlugin{} if test.extensionPoint == frameworktypes.DescheduleExtensionPoint { - fakePlugin.AddReactor(string(frameworktypes.DescheduleExtensionPoint), func(action fakeplugin.Action) (handled bool, err error) { + fakePlugin.AddReactor(string(frameworktypes.DescheduleExtensionPoint), func(action fakeplugin.Action) (handled, filter bool, err error) { if dAction, ok := action.(fakeplugin.DescheduleAction); ok { if dAction.Handle().Evictor().Evict(ctx, p1, evictions.EvictOptions{}) { - return true, nil + return true, false, nil } - return true, fmt.Errorf("pod not evicted") + return true, false, fmt.Errorf("pod not evicted") } - return false, nil + return false, false, nil }) } if test.extensionPoint == frameworktypes.BalanceExtensionPoint { - fakePlugin.AddReactor(string(frameworktypes.BalanceExtensionPoint), func(action fakeplugin.Action) (handled bool, err error) { + fakePlugin.AddReactor(string(frameworktypes.BalanceExtensionPoint), func(action fakeplugin.Action) (handled, filter bool, err error) { if dAction, ok := action.(fakeplugin.BalanceAction); ok { if dAction.Handle().Evictor().Evict(ctx, p1, evictions.EvictOptions{}) { - return true, nil + return true, false, nil } - return true, fmt.Errorf("pod not evicted") + return true, false, fmt.Errorf("pod not evicted") } - return false, nil + return false, false, nil }) } @@ -283,3 +287,400 @@ func podEvictionReactionFuc(evictedPods *[]string) func(action core.Action) (boo return false, nil, nil // fallback to the default reactor } } + +func TestProfileExtensionPoints(t *testing.T) { + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + n1 := testutils.BuildTestNode("n1", 2000, 3000, 10, nil) + n2 := testutils.BuildTestNode("n2", 2000, 3000, 10, nil) + nodes := []*v1.Node{n1, n2} + + p1 := testutils.BuildTestPod(fmt.Sprintf("pod_1_%s", n1.Name), 200, 0, n1.Name, nil) + p1.ObjectMeta.OwnerReferences = []metav1.OwnerReference{{}} + + pluginregistry.PluginRegistry = pluginregistry.NewRegistry() + + for i := 0; i < 3; i++ { + fakePluginName := fmt.Sprintf("FakePlugin_%v", i) + deschedulePluginName := fmt.Sprintf("DeschedulePlugin_%v", i) + balancePluginName := fmt.Sprintf("BalancePlugin_%v", i) + filterPluginName := fmt.Sprintf("FilterPlugin_%v", i) + + fakePlugin := &fakeplugin.FakePlugin{PluginName: fakePluginName} + fakeDeschedulePlugin := &fakeplugin.FakeDeschedulePlugin{PluginName: deschedulePluginName} + fakeBalancePlugin := &fakeplugin.FakeBalancePlugin{PluginName: balancePluginName} + fakeFilterPlugin := &fakeplugin.FakeFilterPlugin{PluginName: filterPluginName} + + pluginregistry.Register( + fakePluginName, + fakeplugin.NewPluginFncFromFake(fakePlugin), + &fakeplugin.FakePlugin{}, + &fakeplugin.FakePluginArgs{}, + fakeplugin.ValidateFakePluginArgs, + fakeplugin.SetDefaults_FakePluginArgs, + pluginregistry.PluginRegistry, + ) + + pluginregistry.Register( + deschedulePluginName, + fakeplugin.NewFakeDeschedulePluginFncFromFake(fakeDeschedulePlugin), + &fakeplugin.FakeDeschedulePlugin{}, + &fakeplugin.FakeDeschedulePluginArgs{}, + fakeplugin.ValidateFakePluginArgs, + fakeplugin.SetDefaults_FakePluginArgs, + pluginregistry.PluginRegistry, + ) + + pluginregistry.Register( + balancePluginName, + fakeplugin.NewFakeBalancePluginFncFromFake(fakeBalancePlugin), + &fakeplugin.FakeBalancePlugin{}, + &fakeplugin.FakeBalancePluginArgs{}, + fakeplugin.ValidateFakePluginArgs, + fakeplugin.SetDefaults_FakePluginArgs, + pluginregistry.PluginRegistry, + ) + + pluginregistry.Register( + filterPluginName, + fakeplugin.NewFakeFilterPluginFncFromFake(fakeFilterPlugin), + &fakeplugin.FakeFilterPlugin{}, + &fakeplugin.FakeFilterPluginArgs{}, + fakeplugin.ValidateFakePluginArgs, + fakeplugin.SetDefaults_FakePluginArgs, + pluginregistry.PluginRegistry, + ) + } + + pluginregistry.Register( + defaultevictor.PluginName, + defaultevictor.New, + &defaultevictor.DefaultEvictor{}, + &defaultevictor.DefaultEvictorArgs{}, + defaultevictor.ValidateDefaultEvictorArgs, + defaultevictor.SetDefaults_DefaultEvictorArgs, + pluginregistry.PluginRegistry, + ) + + client := fakeclientset.NewSimpleClientset(n1, n2, p1) + var evictedPods []string + client.PrependReactor("create", "pods", podEvictionReactionFuc(&evictedPods)) + + sharedInformerFactory := informers.NewSharedInformerFactory(client, 0) + podInformer := sharedInformerFactory.Core().V1().Pods().Informer() + getPodsAssignedToNode, err := podutil.BuildGetPodsAssignedToNodeFunc(podInformer) + if err != nil { + t.Fatalf("build get pods assigned to node function error: %v", err) + } + + sharedInformerFactory.Start(ctx.Done()) + sharedInformerFactory.WaitForCacheSync(ctx.Done()) + + eventClient := fakeclientset.NewSimpleClientset(n1, n2, p1) + eventBroadcaster, eventRecorder := utils.GetRecorderAndBroadcaster(ctx, eventClient) + defer eventBroadcaster.Shutdown() + + podEvictor := evictions.NewPodEvictor(client, "policy/v1", false, nil, nil, nodes, true, eventRecorder) + + prfl, err := NewProfile( + api.DeschedulerProfile{ + Name: "strategy-test-profile", + PluginConfigs: []api.PluginConfig{ + { + Name: defaultevictor.PluginName, + Args: &defaultevictor.DefaultEvictorArgs{ + PriorityThreshold: &api.PriorityThreshold{ + Value: nil, + }, + }, + }, + { + Name: "FakePlugin_0", + Args: &fakeplugin.FakePluginArgs{}, + }, + { + Name: "FilterPlugin_0", + Args: &fakeplugin.FakeFilterPluginArgs{}, + }, + { + Name: "FilterPlugin_1", + Args: &fakeplugin.FakeFilterPluginArgs{}, + }, + }, + Plugins: api.Plugins{ + Deschedule: api.PluginSet{ + Enabled: []string{"FakePlugin_0"}, + }, + Filter: api.PluginSet{ + Enabled: []string{"FilterPlugin_1", "FilterPlugin_0"}, + }, + Evict: api.PluginSet{ + Enabled: []string{defaultevictor.PluginName}, + }, + }, + }, + pluginregistry.PluginRegistry, + WithClientSet(client), + WithSharedInformerFactory(sharedInformerFactory), + WithPodEvictor(podEvictor), + WithGetPodsAssignedToNodeFnc(getPodsAssignedToNode), + ) + if err != nil { + t.Fatalf("unable to create profile: %v", err) + } + + // Validate the extension points of all registered plugins are properly detected + + diff := cmp.Diff(sets.NewString("DeschedulePlugin_0", "DeschedulePlugin_1", "DeschedulePlugin_2", "FakePlugin_0", "FakePlugin_1", "FakePlugin_2"), prfl.deschedule) + if diff != "" { + t.Errorf("check for deschedule failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + diff = cmp.Diff(sets.NewString("BalancePlugin_0", "BalancePlugin_1", "BalancePlugin_2", "FakePlugin_0", "FakePlugin_1", "FakePlugin_2"), prfl.balance) + if diff != "" { + t.Errorf("check for balance failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + diff = cmp.Diff(sets.NewString("DefaultEvictor", "FakePlugin_0", "FakePlugin_1", "FakePlugin_2", "FilterPlugin_0", "FilterPlugin_1", "FilterPlugin_2"), prfl.filter) + if diff != "" { + t.Errorf("check for filter failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + diff = cmp.Diff(sets.NewString("DefaultEvictor", "FakePlugin_0", "FakePlugin_1", "FakePlugin_2", "FilterPlugin_0", "FilterPlugin_1", "FilterPlugin_2"), prfl.preEvictionFilter) + if diff != "" { + t.Errorf("check for preEvictionFilter failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + + // One deschedule ep enabled + names := []string{} + for _, pl := range prfl.deschedulePlugins { + names = append(names, pl.Name()) + } + sort.Strings(names) + diff = cmp.Diff(sets.NewString("FakePlugin_0"), sets.NewString(names...)) + if diff != "" { + t.Errorf("check for deschedule failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + + // No balance ep enabled + names = []string{} + for _, pl := range prfl.balancePlugins { + names = append(names, pl.Name()) + } + sort.Strings(names) + diff = cmp.Diff(sets.NewString(), sets.NewString(names...)) + if diff != "" { + t.Errorf("check for balance failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + + // Two filter eps enabled + names = []string{} + for _, pl := range prfl.filterPlugins { + names = append(names, pl.Name()) + } + sort.Strings(names) + diff = cmp.Diff(sets.NewString("FilterPlugin_0", "FilterPlugin_1"), sets.NewString(names...)) + if diff != "" { + t.Errorf("check for filter failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } +} + +func TestProfileExtensionPointOrdering(t *testing.T) { + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + n1 := testutils.BuildTestNode("n1", 2000, 3000, 10, nil) + n2 := testutils.BuildTestNode("n2", 2000, 3000, 10, nil) + nodes := []*v1.Node{n1, n2} + + p1 := testutils.BuildTestPod(fmt.Sprintf("pod_1_%s", n1.Name), 200, 0, n1.Name, nil) + p1.ObjectMeta.OwnerReferences = []metav1.OwnerReference{{}} + + pluginregistry.PluginRegistry = pluginregistry.NewRegistry() + + filterInvocationOrder := []string{} + preEvictionFilterInvocationOrder := []string{} + descheduleInvocationOrder := []string{} + balanceInvocationOrder := []string{} + + for i := 0; i < 3; i++ { + pluginName := fmt.Sprintf("Filter_%v", i) + fakeFilterPlugin := &fakeplugin.FakeFilterPlugin{PluginName: pluginName} + fakeFilterPlugin.AddReactor(string(frameworktypes.FilterExtensionPoint), func(action fakeplugin.Action) (handled, filter bool, err error) { + if _, ok := action.(fakeplugin.FilterAction); ok { + filterInvocationOrder = append(filterInvocationOrder, pluginName+"_filter") + return true, true, nil + } + return false, false, nil + }) + + fakeFilterPlugin.AddReactor(string(frameworktypes.PreEvictionFilterExtensionPoint), func(action fakeplugin.Action) (handled, filter bool, err error) { + if _, ok := action.(fakeplugin.PreEvictionFilterAction); ok { + preEvictionFilterInvocationOrder = append(preEvictionFilterInvocationOrder, pluginName+"_preEvictionFilter") + return true, true, nil + } + return false, false, nil + }) + + // plugin implementing Filter extension point + pluginregistry.Register( + pluginName, + fakeplugin.NewFakeFilterPluginFncFromFake(fakeFilterPlugin), + &fakeplugin.FakeFilterPlugin{}, + &fakeplugin.FakeFilterPluginArgs{}, + fakeplugin.ValidateFakePluginArgs, + fakeplugin.SetDefaults_FakePluginArgs, + pluginregistry.PluginRegistry, + ) + + fakePluginName := fmt.Sprintf("FakePlugin_%v", i) + fakePlugin := fakeplugin.FakePlugin{} + idx := i + fakePlugin.AddReactor(string(frameworktypes.DescheduleExtensionPoint), func(action fakeplugin.Action) (handled, filter bool, err error) { + descheduleInvocationOrder = append(descheduleInvocationOrder, fakePluginName) + if idx == 0 { + if dAction, ok := action.(fakeplugin.DescheduleAction); ok { + // Invoke filters + dAction.Handle().Evictor().Filter(p1) + // Invoke pre-eviction filters + dAction.Handle().Evictor().PreEvictionFilter(p1) + return true, true, nil + } + return false, false, nil + } + return true, false, nil + }) + + fakePlugin.AddReactor(string(frameworktypes.BalanceExtensionPoint), func(action fakeplugin.Action) (handled, filter bool, err error) { + balanceInvocationOrder = append(balanceInvocationOrder, fakePluginName) + return true, false, nil + }) + + pluginregistry.Register( + fakePluginName, + fakeplugin.NewPluginFncFromFake(&fakePlugin), + &fakeplugin.FakePlugin{}, + &fakeplugin.FakePluginArgs{}, + fakeplugin.ValidateFakePluginArgs, + fakeplugin.SetDefaults_FakePluginArgs, + pluginregistry.PluginRegistry, + ) + } + + pluginregistry.Register( + defaultevictor.PluginName, + defaultevictor.New, + &defaultevictor.DefaultEvictor{}, + &defaultevictor.DefaultEvictorArgs{}, + defaultevictor.ValidateDefaultEvictorArgs, + defaultevictor.SetDefaults_DefaultEvictorArgs, + pluginregistry.PluginRegistry, + ) + + client := fakeclientset.NewSimpleClientset(n1, n2, p1) + var evictedPods []string + client.PrependReactor("create", "pods", podEvictionReactionFuc(&evictedPods)) + + sharedInformerFactory := informers.NewSharedInformerFactory(client, 0) + podInformer := sharedInformerFactory.Core().V1().Pods().Informer() + getPodsAssignedToNode, err := podutil.BuildGetPodsAssignedToNodeFunc(podInformer) + if err != nil { + t.Fatalf("build get pods assigned to node function error: %v", err) + } + + sharedInformerFactory.Start(ctx.Done()) + sharedInformerFactory.WaitForCacheSync(ctx.Done()) + + eventClient := fakeclientset.NewSimpleClientset(n1, n2, p1) + eventBroadcaster, eventRecorder := utils.GetRecorderAndBroadcaster(ctx, eventClient) + defer eventBroadcaster.Shutdown() + + podEvictor := evictions.NewPodEvictor(client, "policy/v1", false, nil, nil, nodes, true, eventRecorder) + + prfl, err := NewProfile( + api.DeschedulerProfile{ + Name: "strategy-test-profile", + PluginConfigs: []api.PluginConfig{ + { + Name: defaultevictor.PluginName, + Args: &defaultevictor.DefaultEvictorArgs{ + PriorityThreshold: &api.PriorityThreshold{ + Value: nil, + }, + }, + }, + { + Name: "FakePlugin_0", + Args: &fakeplugin.FakePluginArgs{}, + }, + { + Name: "FakePlugin_1", + Args: &fakeplugin.FakePluginArgs{}, + }, + { + Name: "FakePlugin_2", + Args: &fakeplugin.FakePluginArgs{}, + }, + { + Name: "Filter_0", + Args: &fakeplugin.FakeFilterPluginArgs{}, + }, + { + Name: "Filter_1", + Args: &fakeplugin.FakeFilterPluginArgs{}, + }, + { + Name: "Filter_2", + Args: &fakeplugin.FakeFilterPluginArgs{}, + }, + }, + Plugins: api.Plugins{ + Deschedule: api.PluginSet{ + Enabled: []string{"FakePlugin_2", "FakePlugin_0", "FakePlugin_1"}, + }, + Balance: api.PluginSet{ + Enabled: []string{"FakePlugin_1", "FakePlugin_0", "FakePlugin_2"}, + }, + Filter: api.PluginSet{ + Enabled: []string{"Filter_2", "Filter_1", "Filter_0"}, + }, + PreEvictionFilter: api.PluginSet{ + Enabled: []string{"Filter_2", "Filter_1", "Filter_0"}, + }, + Evict: api.PluginSet{ + Enabled: []string{defaultevictor.PluginName}, + }, + }, + }, + pluginregistry.PluginRegistry, + WithClientSet(client), + WithSharedInformerFactory(sharedInformerFactory), + WithPodEvictor(podEvictor), + WithGetPodsAssignedToNodeFnc(getPodsAssignedToNode), + ) + if err != nil { + t.Fatalf("unable to create profile: %v", err) + } + + prfl.RunDeschedulePlugins(ctx, nodes) + + diff := cmp.Diff([]string{"Filter_2_filter", "Filter_1_filter", "Filter_0_filter"}, filterInvocationOrder) + if diff != "" { + t.Errorf("check for filter invocation order failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + + diff = cmp.Diff([]string{"Filter_2_preEvictionFilter", "Filter_1_preEvictionFilter", "Filter_0_preEvictionFilter"}, preEvictionFilterInvocationOrder) + if diff != "" { + t.Errorf("check for filter invocation order failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + + diff = cmp.Diff([]string{"FakePlugin_2", "FakePlugin_0", "FakePlugin_1"}, descheduleInvocationOrder) + if diff != "" { + t.Errorf("check for deschedule invocation order failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } + + prfl.RunBalancePlugins(ctx, nodes) + + diff = cmp.Diff([]string{"FakePlugin_1", "FakePlugin_0", "FakePlugin_2"}, balanceInvocationOrder) + if diff != "" { + t.Errorf("check for balance invocation order failed. Results are not deep equal. mismatch (-want +got):\n%s", diff) + } +}