Add new DefaultEvictor plugin with args #929

knelasevero · 2022-08-29T18:18:24Z

Add new DefaultEvictor plugin with args, and only moving the Filter extension point to it for now.

Follow ups (other PRs):

PreEvictFilter, Sort, PreEvictSort for the default Evictor plugin

knelasevero · 2022-08-29T18:18:43Z

/hold

pkg/framework/plugins/defaultevictorfilter/defaultevictorfilter.go

pkg/apis/componentconfig/types_pluginargs.go

pkg/framework/plugins/defaultevictorfilter/defaultevictorfilter.go

pkg/framework/types.go

knelasevero · 2022-08-31T14:28:41Z

/unhold

pkg/framework/plugins/defaultevictor/defaultevictor.go

knelasevero · 2022-09-02T13:33:26Z

Had a internal discussion with Jan about naming.

This name that we have right now is a bit confusing. This plugin interface that we have right now is called EvictorPlugin, and the first default plugin is called DefaultEvictor. But this plugin is not really meant to actually evict pods, only filter/sort/etc, which will affect eviction, but is not the eviction itself. It is something like EvictorCustomizer, or EvictorBehaviour... or something...

From the user perspective, this to be called DefaultEvictor makes sense, since it feels like the user is configuring the evictor itself and how it evicts and decides to evict pods. But in code we will have one thing that is the actual evictor, and a thing that will change how the evictor filters/sorts/etc.

Just wanted to throw this here. It is confusing, but I can't think of a better name. I added comments both in defaultevictor.go and the framework types.go:

// EvictorPlugin defines extension points for a general evictor behavior
// Even though we name this plugin interface EvictorPlugin, it does not actually evict anything,
// This plugin is only meant to customize other actions (extension points) of the evictor,
// like filtering, sorting, and other ones that might be relevant in the future

I think it is sufficient, but if anybody has a better name feel free to chime in :)

There is yet another naming discussion that I will bring in the other issue #924.

pkg/apis/componentconfig/types_pluginargs.go

ingvagabund · 2022-09-05T08:19:22Z

pkg/apis/componentconfig/types_pluginargs.go

+type DefaultEvictorArgs struct {
+ metav1.TypeMeta
+
+ Nodes []*v1.Node


Nodes param is a tricky param. It can not be passed through the plugin arguments since a plugin is initialized before its extension point is invoked. In my PoC implementation I reached for the current nodes through the shared informer: https://github.com/kubernetes-sigs/descheduler/pull/781/files#diff-66db71b98f61a3c06ca8d17cbed99626fc23f8bab081cd71a8baf0867613d4d5R111. I am open to suggestions on how to make it better. Using the shared node informer directly is acceptable solution for the moment. Nevertheless, once the descheduler is migrated into the framework completely, we need a single method for listing ready nodes which will be invoked by this plugin and the main descheduling loop. And possibly by others who needs to have identical list of ready nodes.

Created an issue to track, and I am getting nodes from shared informers for now.

#941

pkg/apis/componentconfig/types_pluginargs.go

pkg/framework/plugins/defaultevictor/defaultevictor_test.go

ingvagabund · 2022-09-05T08:36:42Z

pkg/framework/plugins/defaultevictor/defaultevictor.go

+
+const (
+ PluginName = "DefaultEvictor"
+ evictPodAnnotationKey = "descheduler.alpha.kubernetes.io/evict"


@damemi the annotation has been alpha for some time. I wonder whether the annotation is still in use or whether we could drop it.

definitely don't drop it, if anything we should probably promote it. I think this feature has shown to be useful

pkg/framework/plugins/defaultevictor/defaultevictor.go

ingvagabund · 2022-09-05T08:48:31Z

I think it is sufficient, but if anybody has a better name feel free to chime in :)

@knelasevero thank you for putting the comments down. Right before we introduce the v1alpha2 configuration type we will have a discussion about naming of all plugins. At that point we will need to decide which name will be the least confusing yet still sufficiently descriptive.

pkg/framework/plugins/defaultevictor/defaultevictor.go

pkg/descheduler/descheduler.go

knelasevero · 2022-09-06T16:14:10Z

/retest

knelasevero · 2022-09-06T16:18:49Z

hack/verify-deep-copies.sh

@@ -20,7 +20,7 @@ go build -o "${OS_OUTPUT_BINPATH}/deepcopy-gen" "k8s.io/code-generator/cmd/deepc

 ${OS_OUTPUT_BINPATH}/deepcopy-gen \
 --go-header-file "hack/boilerplate/boilerplate.go.txt" \
- --input-dirs "./pkg/apis/componentconfig,./pkg/apis/componentconfig/v1alpha1,./pkg/api,./pkg/api/v1alpha1" \
+ --input-dirs "./pkg/apis/componentconfig,./pkg/apis/componentconfig/v1alpha1,./pkg/api,./pkg/api/v1alpha1,./pkg/framework/plugins/defaultevictor/" \


We need a better way to get dirs where we have to generate code. I can work on that in another PR. For now I am adding manually

+1 definitely, this is part of the reason I was okay closing #900 and holding off on these moves for now. But if you have a way to make it better, please feel free

pkg/framework/plugins/defaultevictor/defaultevictor.go

ingvagabund · 2022-09-12T16:19:15Z

/approve
/lgtm

k8s-ci-robot · 2022-09-12T16:19:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 29, 2022

knelasevero marked this pull request as draft August 29, 2022 18:18

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 29, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2022

k8s-ci-robot requested review from JaneLiuL and lixiang233 August 29, 2022 18:18

knelasevero changed the title ~~Add new preevectionfilter plugin with args~~ [WIP] Add new preevectionfilter plugin with args Aug 29, 2022

ingvagabund requested changes Aug 30, 2022

View reviewed changes

knelasevero force-pushed the ev-filter-plugin branch 3 times, most recently from ea1a3aa to 85df5f8 Compare August 31, 2022 14:21

knelasevero changed the title ~~[WIP] Add new preevectionfilter plugin with args~~ Add new preevectionfilter plugin with args Aug 31, 2022

knelasevero marked this pull request as ready for review August 31, 2022 14:24

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022

k8s-ci-robot requested a review from a7i August 31, 2022 14:24

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2022

a7i reviewed Aug 31, 2022

View reviewed changes

pkg/framework/plugins/defaultevictor/defaultevictor.go Show resolved Hide resolved

a7i reviewed Aug 31, 2022

View reviewed changes

pkg/framework/plugins/defaultevictor/defaultevictor.go Outdated Show resolved Hide resolved

knelasevero changed the title ~~Add new preevectionfilter plugin with args~~ Add new EvectionFilter plugin with args Aug 31, 2022

knelasevero changed the title ~~Add new EvectionFilter plugin with args~~ Add new DefaultEvictor plugin with args Aug 31, 2022

knelasevero force-pushed the ev-filter-plugin branch 3 times, most recently from fee0975 to f199d2a Compare September 2, 2022 13:24

ingvagabund reviewed Sep 3, 2022

View reviewed changes

pkg/apis/componentconfig/types_pluginargs.go Outdated Show resolved Hide resolved

ingvagabund reviewed Sep 5, 2022

View reviewed changes

pkg/apis/componentconfig/types_pluginargs.go Outdated Show resolved Hide resolved

ingvagabund reviewed Sep 5, 2022

View reviewed changes

pkg/apis/componentconfig/types_pluginargs.go Outdated Show resolved Hide resolved

ingvagabund requested changes Sep 5, 2022

View reviewed changes

ingvagabund reviewed Sep 5, 2022

View reviewed changes

pkg/framework/plugins/defaultevictor/defaultevictor.go Outdated Show resolved Hide resolved

knelasevero force-pushed the ev-filter-plugin branch 2 times, most recently from a4369db to d51e5aa Compare September 6, 2022 15:51

knelasevero mentioned this pull request Sep 6, 2022

We need a single method for listing ready nodes which will be invoked by plugins #941

Open

knelasevero force-pushed the ev-filter-plugin branch 2 times, most recently from d4cb211 to 3e9e9b3 Compare September 6, 2022 16:02

damemi reviewed Sep 6, 2022

View reviewed changes

pkg/descheduler/descheduler.go Show resolved Hide resolved

knelasevero force-pushed the ev-filter-plugin branch from 3e9e9b3 to 861487c Compare September 6, 2022 16:10

knelasevero force-pushed the ev-filter-plugin branch from 861487c to 7e53b0f Compare September 6, 2022 16:16

knelasevero commented Sep 6, 2022

View reviewed changes

ingvagabund requested changes Sep 7, 2022

View reviewed changes

knelasevero force-pushed the ev-filter-plugin branch from 7e53b0f to bbde246 Compare September 9, 2022 11:56

knelasevero requested review from ingvagabund and removed request for lixiang233 and JaneLiuL September 12, 2022 09:55

add new preevectionfilter plugin with args

f47c2c4

knelasevero force-pushed the ev-filter-plugin branch from bbde246 to f47c2c4 Compare September 12, 2022 14:56

k8s-ci-robot assigned ingvagabund Sep 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2022

k8s-ci-robot merged commit 72bf50f into kubernetes-sigs:master Sep 12, 2022

damemi mentioned this pull request Apr 25, 2023

Support labelSelector in RemoveDuplicates #654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new DefaultEvictor plugin with args #929

Add new DefaultEvictor plugin with args #929

knelasevero commented Aug 29, 2022 •

edited

Loading

knelasevero commented Aug 29, 2022

knelasevero commented Aug 31, 2022

knelasevero commented Sep 2, 2022 •

edited

Loading

ingvagabund Sep 5, 2022

knelasevero Sep 6, 2022

ingvagabund Sep 5, 2022

damemi Sep 6, 2022

ingvagabund commented Sep 5, 2022

knelasevero commented Sep 6, 2022

knelasevero Sep 6, 2022

damemi Sep 6, 2022

ingvagabund commented Sep 12, 2022

k8s-ci-robot commented Sep 12, 2022

Add new DefaultEvictor plugin with args #929

Add new DefaultEvictor plugin with args #929

Conversation

knelasevero commented Aug 29, 2022 • edited Loading

knelasevero commented Aug 29, 2022

knelasevero commented Aug 31, 2022

knelasevero commented Sep 2, 2022 • edited Loading

ingvagabund Sep 5, 2022

Choose a reason for hiding this comment

knelasevero Sep 6, 2022

Choose a reason for hiding this comment

ingvagabund Sep 5, 2022

Choose a reason for hiding this comment

damemi Sep 6, 2022

Choose a reason for hiding this comment

ingvagabund commented Sep 5, 2022

knelasevero commented Sep 6, 2022

knelasevero Sep 6, 2022

Choose a reason for hiding this comment

damemi Sep 6, 2022

Choose a reason for hiding this comment

ingvagabund commented Sep 12, 2022

k8s-ci-robot commented Sep 12, 2022

knelasevero commented Aug 29, 2022 •

edited

Loading

knelasevero commented Sep 2, 2022 •

edited

Loading