add v1alpha2 registry based conversion #1006

knelasevero · 2022-12-01T17:54:41Z

Replaces #974

~~I am adding a single commit since I had the other PR (#974) as reference~~. Please let me know if somthing is not clear.

In any case the conversion is way simpler now that we can use the registry to also register: validators, defaulters and argtypes.

I also changed the main loop to also consider that plugins can be of multiple types (one plugin with both types for example).

I changed the name of the pluginbuilder package to pluginregistry since we can hold more stuff into it.

Hope we can merge this soon to go to the other steps next! 🚀

knelasevero · 2022-12-02T16:28:03Z

pkg/framework/plugins/removefailedpods/defaults.go

- if obj.MinPodLifetimeSeconds == nil {
- obj.MinPodLifetimeSeconds = nil
+ if args.MinPodLifetimeSeconds == nil {
+ args.MinPodLifetimeSeconds = utilpointer.Uint(3600)


I am setting our first explicit default here.

I think we wont get any feedback from the community regarding defaults if we don't start setting them. Waiting for the community feedback to start setting won't work I think. We will need to start experimenting here. (Using values that are already on the readme seems safe enough, as I think most people are just using those anyways)

Also I needed at least one parameter defaulted to something that is different to how golang initializes variables by default to have a unit test that actually tests something

knelasevero · 2022-12-02T19:58:28Z

Docs are missing (namely our readme).

Should we already update all of it and encourage v1alpha2 usage? Keep both there? Separate the docs?

knelasevero · 2022-12-06T13:17:51Z

@ingvagabund @damemi @a7i and anyone around, please have a look when you have some time :D

pkg/api/conversion.go

pkg/api/v1alpha1/conversion.go

pkg/api/v1alpha2/conversion.go

pkg/api/v1alpha2/defaults.go

pkg/api/v1alpha2/doc.go

pkg/descheduler/descheduler.go

pkg/descheduler/policyconfig.go

damemi · 2022-12-06T14:04:09Z

pkg/descheduler/policyconfig.go

+ if versionedPolicy.APIVersion == "descheduler/v1alpha1" {
+ // Build profiles
+ internalPolicy, err = V1alpha1ToInternal(client, versionedPolicy, registry)


maybe I'm wrong but I thought with conversion funcs set up we should just be able to pass either one to decode and let the conversion functions handle it. seems odd that we still have that extra layer here. Either way, I think the V1Alpha1ToInternal function should be in the api package, not here (if we do need it)

So, we have most of the auto generated conversions working for v1alpha2 to api because they are very similar (with exception to the runtime.Objects/RawExtensions args, which made us do some custom code).

v1alpha1 and api are very different, and none of it can be auto-generated.

Also we have some very custom ways to set defaults, and specially, setting thresholdPriorities. Right now we depend on having a client passed around.

The other thing is strategyParamsToPluginArgs map, that lives in strategy_migration.go, and we cant import descheduler.go in v1alpha1, as descheduler.go imports v1alpha1.

So moving this function to v1alpha1 would require some serious moving around. I think it is easier to keep it in the policyconfig.go file since it is temporary, and also very custom with all the conversion from strategies to profiles.

I tried making V1alpha1ToInternal just become a normal Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy in v1alpha1/conversion.go, but gave up mid tryin it 😅

@damemi @ingvagabund what do you think? And also, if we want to go forward with moving it around, do you have suggestions?

Ok, so, I tried moving things around to avoid import cycles. We still have V1Alpha1ToInternal func, I don't want to create another client inside a default Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy func (a func with that name is called by decodes) ... so better to have this as something that we call explicitly so we can pass registries+clients and stuff around

knelasevero · 2022-12-08T15:54:12Z

@damemi @ingvagabund this is ready for another round when you have some time

a7i · 2022-12-28T03:11:23Z

pkg/api/v1alpha1/conversion.go

+ if deschedulerPolicy.EvictFailedBarePods != nil {
+ evictBarePods = *deschedulerPolicy.EvictFailedBarePods
+ if evictBarePods {
+ klog.V(1).InfoS("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")


no keysAndValues args so not structured logs.

Suggested change

klog.V(1).InfoS("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")

klog.V(1).Info("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")

a7i · 2022-12-28T03:11:48Z

pkg/api/v1alpha1/conversion.go

+ if deschedulerPolicy.EvictSystemCriticalPods != nil {
+ evictSystemCriticalPods = *deschedulerPolicy.EvictSystemCriticalPods
+ if evictSystemCriticalPods {
+ klog.V(1).InfoS("Warning: EvictSystemCriticalPods is set to True. This could cause eviction of Kubernetes system pods.")


Suggested change

klog.V(1).InfoS("Warning: EvictSystemCriticalPods is set to True. This could cause eviction of Kubernetes system pods.")

klog.V(1).Info("Warning: EvictSystemCriticalPods is set to True. This could cause eviction of Kubernetes system pods.")

a7i · 2022-12-28T03:24:51Z

/lgtm
/assign @damemi

ingvagabund · 2022-12-13T21:23:16Z

pkg/descheduler/setupplugins.go

- pluginbuilder.Register(removepodsviolatingnodetaints.PluginName, removepodsviolatingnodetaints.New, &removepodsviolatingnodetaints.RemovePodsViolatingNodeTaintsArgs{}, registry)
- pluginbuilder.Register(removepodsviolatingtopologyspreadconstraint.PluginName, removepodsviolatingtopologyspreadconstraint.New, &removepodsviolatingtopologyspreadconstraint.RemovePodsViolatingTopologySpreadConstraintArgs{}, registry)
+func RegisterDefaultPlugins(registry pluginregistry.Registry) {
+ pluginregistry.Register(defaultevictor.PluginName, defaultevictor.New, &defaultevictor.DefaultEvictorArgs{}, defaultevictor.ValidateDefaultEvictorArgs, defaultevictor.SetDefaults_DefaultEvictorArgs, registry)


What about to rename all validating functions into ValidatePluginArgs or similar, turn them into args methods and invoke the method always? In case the validation is not needed, the method stays empty. The same for renaming the defaulting functions into SetDefaults or similar, turn them into args method and invoke it in the same fashion? This way one does not need to pass either of the functions when registering a plugin.

This makes us go back to having switch cases in validateDeschedulerConfiguration and setDefaultsPluginConfig. Since we are dealing with runtime.Objects there is no way to invoke SetDefaults before checking the type of PluginArgInstance. Even if they are args methods, were are dealing with runtime.Objects, and you would not have access to its methods withough casting it first to the correct arg type. I personally prefer to keep this being registered just one time in the registry, and keeping validateDeschedulerConfiguration and setDefaultsPluginConfig simple, without having to verify types in policyconfig.go

pkg/descheduler/policyconfig.go

ingvagabund · 2022-12-13T21:29:57Z

pkg/descheduler/policyconfig.go

 if err := runtime.DecodeInto(decoder, policy, versionedPolicy); err != nil {
- return nil, fmt.Errorf("failed decoding descheduler's policy config %q: %v", policyConfigFile, err)
+ if err.Error() == "converting (v1alpha2.DeschedulerPolicy) to (v1alpha1.DeschedulerPolicy): unknown conversion" {


More preferable to return an error constant and compare for that. The error string can change in time and cause unexpected behavior.

What do you mean by returning an error constant?

This error is returned from the runtime library. I can't make it return a constant. And since they are just using fmt.Errorf, error.Is won't work.

https://github.com/kubernetes/apimachinery/blob/6c409361e35e40e38c4056ba0b86647d4244c047/pkg/runtime/codec.go#L69

If I create a constant error and just compare the strings (the only thing that will be equal), I wouldn't be doing anything differently really

I agree that checking the error string isn't optimal, but I'm wondering if there is a way to reorganize this so that's not necessary.

It seems weird to me that v1alpha1->internal has to be handled differently than v1alpha2->internal, ie that they both can't be handled by the Decoder. I would expect a function like Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy() to call v1alpha1.V1alpha1ToInternal() (to handle the necessary manual conversion), and rely on the automated tooling.

If that's not possible, maybe this section can be refactored to not rely on the exact output of the conversion error to determine the branch

I responded this here: #1006 (comment)
v1alpha1 is too different from internal api, and we do too many custom stuff (like how we set thresholdPriorities).

If that's not possible, maybe this section can be refactored to not rely on the exact output of the conversion error to determine the branch

I can try that!

So, if we can drop the way that we set threshold priorities (that requires a client to be passed around) , Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy calling V1alpha1ToInternal would be possible. Can we do that?

Yeah, calling manually-written code is not necessarily the problem, but the arguments required by that code, and I guess we don't want to create clients at the the conversion level, or inject clients from the upper level.

Ohh, yeah needing a client makes that a lot tougher. My gut tells me that we should look more into how we do that, but it feels like getting close to yak shaving to focus too much on it. So I'm fine with the manual conversion the way it is for now, but would still like to look into the error string check

Ack. Let me see what I can do here :)

@knelasevero were you able to take another look at this to see if we could avoid the error string literal check?

if not, could you add a comment briefly explaining it (w/ a link to this thread) and a TODO?

Added a TODO comment that points back to this discussion and also points to my commit in another branch trying to move things around. I could not make it work, but if someone manages to make that work I think that would be the direction to go:
TODO: eec534a

Commit in another branch: knelasevero@b25a44c

pkg/descheduler/policyconfig.go

ingvagabund · 2022-12-15T12:18:48Z

pkg/descheduler/policyconfig.go

+
+func setDefaultEvictor(profile api.Profile) api.Profile {
+ if len(profile.Plugins.Evict.Enabled) == 0 {
+ profile.Plugins.Evict.Enabled = append(profile.Plugins.Evict.Enabled, defaultevictor.PluginName)


This needs to be set only when defaultevictor.PluginName plugin config is not present. I.e. we start from a clean slate. In case the plugin is already there but not in the list of enabled plugins, it might get re-enabled unintentionally. E.g. a profile has a plugin configuration for both the default and some custom evictor plugin but the list of enabled evict plugins is intentionally empty (e.g. user forgot to extend it with the custom evictor plugin name). By extending the list now the default evictor plugin could get enabled without user knowing when the custom plugin is expected. Better to error and have a user set the right plugin.

Since we are unable to detect whether there's a pluginConfig for a custom evictor plugin, the checks performed here are quite limited. Yet, better to make sure we enable the default evictor plugin with the most restrictions we can find.

We can have a discussion whether we enable two or more evict extension points simultaneously later. Let's only focus on the conversion at the moment.

✔️

Please have a look, I made some changes

ingvagabund · 2022-12-28T09:39:31Z

pkg/api/v1alpha1/strategymigration.go

@@ -14,14 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.


Why renaming the file?

I don't remember now 😅 I think because all other files have two words together and none of the other files have underscores.

ingvagabund · 2022-12-28T09:40:42Z

pkg/api/v1alpha1/conversion.go

+
+// evictorImpl implements the Evictor interface so plugins
+// can evict a pod without importing a specific pod evictor
+type evictorImpl struct {


Why do evictorImpl and other types need to be present under pkg/api/v1alpha1 if they are expected to be used solely by the framework?

I moved things here by Mike's request: #1006 (comment)

/*197*/ pluginInstance, err := registry[string(name)].PluginBuilder(pluginArgs, &handleImpl{})

These lines need it. And I can't import it from somewhere else since basically all other packages import v1alpha1 and this would generate a cycle. When this was back at policyconfig.go we could just use it.

ingvagabund · 2023-01-10T11:08:42Z

In overall the conversions from v1alpha1 -> internal and v1alpha2 -> internal are implemented. The main functionality is here. We have been going forth and back during the reviews. Any more improvements and refactoring would produce another delay. Let's merge the PR as it is unless there are reasonable objections not to. So we can move forward implementing other parts on top of the conversion. While dealing with improvement in parallel. The conversion code itself is pretty orthogonal to the rest of the framework. The mechanics for validation and defaulting are in place. The conversion produces an internal representation which we can build on top of.

/approve

k8s-ci-robot · 2023-01-10T11:09:10Z

[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

ingvagabund · 2023-01-12T12:32:00Z

@a7i @damemi anything else that you have in mind that needs to be done here?

a7i · 2023-01-16T16:45:13Z

/lgtm

/hold for @damemi to get a chance to 👍

damemi · 2023-01-17T15:04:25Z

pkg/api/conversion.go

@@ -0,0 +1,17 @@
+/*
+Copyright 2022 The Kubernetes Authors.


hate to do this, but these need to be updated to 2023 now in each new file

damemi · 2023-01-17T15:08:14Z

lgmt, just need to update the copyright year on the new files and I had one question about the error handling check we were discussing before

knelasevero · 2023-01-17T16:01:16Z

Addressed the copyright year bump and the TODO for the error handling code

damemi · 2023-01-17T16:04:24Z

thanks @knelasevero
/hold cancel
/lgtm

knelasevero · 2023-01-17T16:07:29Z

/label tide/merge-method-squash

add v1alpha2 registry based conversion

8b2cb93

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 1, 2022

k8s-ci-robot requested review from damemi and seanmalloy December 1, 2022 17:55

knelasevero commented Dec 2, 2022

View reviewed changes

test defaults, set our 1st explicit default

ef1c223

knelasevero force-pushed the conversion_using_registry branch from e8b666b to ef1c223 Compare December 2, 2022 16:34

damemi reviewed Dec 6, 2022

View reviewed changes

knelasevero added 5 commits December 7, 2022 17:30

fix typos and dates

5abb6b0

move pluginregistry to its own dir

7802034

remove unused v1alpha2.Namespace type

e544e0a

move migration code folders, remove switch

33f81b6

validate internalPolicy a single time

9e97411

knelasevero force-pushed the conversion_using_registry branch from b0c0e53 to 9e97411 Compare December 8, 2022 15:30

a7i approved these changes Dec 28, 2022

View reviewed changes

k8s-ci-robot assigned damemi and a7i Dec 28, 2022

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

ingvagabund reviewed Dec 28, 2022

View reviewed changes

knelasevero added 4 commits January 5, 2023 16:05

remove structured logs

da590c0

simplify return

a626085

check for nil methods

e770f67

properly check before adding default evictor

0a45b0a

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2023

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 16, 2023

damemi reviewed Jan 17, 2023

View reviewed changes

knelasevero added 2 commits January 17, 2023 16:46

add TODO comment

eec534a

bump copyright year

e13f3fe

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 17, 2023

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 17, 2023

k8s-ci-robot merged commit 137f3b2 into kubernetes-sigs:master Jan 17, 2023

ingvagabund mentioned this pull request Feb 5, 2023

Descheduler strategies as plugins #557

Closed

	klog.V(1).InfoS("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")
	klog.V(1).Info("Warning: EvictFailedBarePods is set to True. This could cause eviction of pods without ownerReferences.")

	klog.V(1).InfoS("Warning: EvictSystemCriticalPods is set to True. This could cause eviction of Kubernetes system pods.")
	klog.V(1).Info("Warning: EvictSystemCriticalPods is set to True. This could cause eviction of Kubernetes system pods.")

		@@ -14,14 +14,14 @@ See the License for the specific language governing permissions and
		limitations under the License.

add v1alpha2 registry based conversion #1006

add v1alpha2 registry based conversion #1006

Conversation

knelasevero commented Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelasevero commented Dec 2, 2022

knelasevero commented Dec 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelasevero Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

knelasevero Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

knelasevero commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a7i commented Dec 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelasevero Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

knelasevero Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelasevero Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

ingvagabund commented Jan 10, 2023

k8s-ci-robot commented Jan 10, 2023

ingvagabund commented Jan 12, 2023

a7i commented Jan 16, 2023

Choose a reason for hiding this comment

damemi commented Jan 17, 2023

knelasevero commented Jan 17, 2023

damemi commented Jan 17, 2023

knelasevero commented Jan 17, 2023

knelasevero commented Dec 1, 2022 •

edited

Loading

knelasevero Dec 7, 2022 •

edited

Loading

knelasevero Dec 7, 2022 •

edited

Loading

knelasevero Jan 5, 2023 •

edited

Loading

damemi Jan 17, 2023 •

edited

Loading

knelasevero Jan 17, 2023 •

edited

Loading

knelasevero Jan 5, 2023 •

edited

Loading