Merge pull request #1051 from gustavomfc/fix/conversion_v1alpha1_

Fix v1alpha1 conversion to use universal decoder

pkg/api/v1alpha1/conversion.go

@@ -21,6 +21,9 @@ import (
 	"fmt"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/client-go/informers"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
@@ -30,7 +33,15 @@ import (
 	"sigs.k8s.io/descheduler/pkg/framework"
 	"sigs.k8s.io/descheduler/pkg/framework/pluginregistry"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/defaultevictor"
-	"sigs.k8s.io/descheduler/pkg/utils"
+)
+
+var (
+	// pluginArgConversionScheme is a scheme with internal and v1alpha2 registered,
+	// used for defaulting/converting typed PluginConfig Args.
+	// Access via getPluginArgConversionScheme()
+
+	Scheme = runtime.NewScheme()
+	Codecs = serializer.NewCodecFactory(Scheme, serializer.EnableStrict)
 )
 
 // evictorImpl implements the Evictor interface so plugins
@@ -91,11 +102,20 @@ func (hi *handleImpl) Evictor() framework.Evictor {
 	return hi.evictor
 }
 
+func Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy(in *DeschedulerPolicy, out *api.DeschedulerPolicy, s conversion.Scope) error {
+	err := V1alpha1ToInternal(in, pluginregistry.PluginRegistry, out, s)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func V1alpha1ToInternal(
-	client clientset.Interface,
 	deschedulerPolicy *DeschedulerPolicy,
 	registry pluginregistry.Registry,
-) (*api.DeschedulerPolicy, error) {
+	out *api.DeschedulerPolicy,
+	s conversion.Scope,
+) error {
 	var evictLocalStoragePods bool
 	if deschedulerPolicy.EvictLocalStoragePods != nil {
 		evictLocalStoragePods = *deschedulerPolicy.EvictLocalStoragePods
@@ -140,7 +160,7 @@ func V1alpha1ToInternal(
 
 				if params.ThresholdPriority != nil && params.ThresholdPriorityClassName != "" {
 					klog.ErrorS(fmt.Errorf("priority threshold misconfigured"), "only one of priorityThreshold fields can be set", "pluginName", name)
-					return nil, fmt.Errorf("priority threshold misconfigured for plugin %v", name)
+					return fmt.Errorf("priority threshold misconfigured for plugin %v", name)
 				}
 
 				var priorityThreshold *api.PriorityThreshold
@@ -150,22 +170,18 @@ func V1alpha1ToInternal(
 						Name:  strategy.Params.ThresholdPriorityClassName,
 					}
 				}
-				thresholdPriority, err := utils.GetPriorityFromStrategyParams(context.TODO(), client, priorityThreshold)
-				if err != nil {
-					klog.ErrorS(err, "Failed to get threshold priority from strategy's params")
-					return nil, fmt.Errorf("failed to get threshold priority from strategy's params: %v", err)
-				}
 
 				var pluginConfig *api.PluginConfig
+				var err error
 				if pcFnc, exists := StrategyParamsToPluginArgs[string(name)]; exists {
 					pluginConfig, err = pcFnc(params)
 					if err != nil {
 						klog.ErrorS(err, "skipping strategy", "strategy", name)
-						return nil, fmt.Errorf("failed to get plugin config for strategy %v: %v", name, err)
+						return fmt.Errorf("failed to get plugin config for strategy %v: %v", name, err)
 					}
 				} else {
 					klog.ErrorS(fmt.Errorf("unknown strategy name"), "skipping strategy", "strategy", name)
-					return nil, fmt.Errorf("unknown strategy name: %v", name)
+					return fmt.Errorf("unknown strategy name: %v", name)
 				}
 
 				profile := api.Profile{
@@ -179,9 +195,7 @@ func V1alpha1ToInternal(
 								IgnorePvcPods:           ignorePvcPods,
 								EvictFailedBarePods:     evictBarePods,
 								NodeFit:                 nodeFit,
-								PriorityThreshold: &api.PriorityThreshold{
+								PriorityThreshold:       priorityThreshold,
-									Value: &thresholdPriority,
-								},
 							},
 						},
 						*pluginConfig,
@@ -197,7 +211,7 @@ func V1alpha1ToInternal(
 				pluginInstance, err := registry[string(name)].PluginBuilder(pluginArgs, &handleImpl{})
 				if err != nil {
 					klog.ErrorS(fmt.Errorf("could not build plugin"), "plugin build error", "plugin", name)
-					return nil, fmt.Errorf("could not build plugin: %v", name)
+					return fmt.Errorf("could not build plugin: %v", name)
 				}
 
 				// pluginInstance can be of any of each type, or both
@@ -207,16 +221,16 @@ func V1alpha1ToInternal(
 			}
 		} else {
 			klog.ErrorS(fmt.Errorf("unknown strategy name"), "skipping strategy", "strategy", name)
-			return nil, fmt.Errorf("unknown strategy name: %v", name)
+			return fmt.Errorf("unknown strategy name: %v", name)
 		}
 	}
 
-	return &api.DeschedulerPolicy{
+	out.Profiles = profiles
-		Profiles:                       profiles,
+	out.NodeSelector = deschedulerPolicy.NodeSelector
-		NodeSelector:                   deschedulerPolicy.NodeSelector,
+	out.MaxNoOfPodsToEvictPerNamespace = deschedulerPolicy.MaxNoOfPodsToEvictPerNamespace
-		MaxNoOfPodsToEvictPerNode:      deschedulerPolicy.MaxNoOfPodsToEvictPerNode,
+	out.MaxNoOfPodsToEvictPerNode = deschedulerPolicy.MaxNoOfPodsToEvictPerNode
-		MaxNoOfPodsToEvictPerNamespace: deschedulerPolicy.MaxNoOfPodsToEvictPerNamespace,
+
-	}, nil
+	return nil
 }
 
 func enableProfilePluginsByType(profilePlugins api.Plugins, pluginInstance framework.Plugin, pluginConfig *api.PluginConfig) api.Plugins {
@@ -242,3 +256,13 @@ func checkDeschedule(profilePlugins api.Plugins, pluginInstance framework.Plugin
 	}
 	return profilePlugins
 }
+
+// Register Conversions
+func RegisterConversions(s *runtime.Scheme) error {
+	if err := s.AddGeneratedConversionFunc((*DeschedulerPolicy)(nil), (*api.DeschedulerPolicy)(nil), func(a, b interface{}, scope conversion.Scope) error {
+		return Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy(a.(*DeschedulerPolicy), b.(*api.DeschedulerPolicy), scope)
+	}); err != nil {
+		return err
+	}
+	return nil
+}

pkg/api/v1alpha1/register.go

@@ -50,7 +50,7 @@ func init() {
 	// We only register manually written functions here. The registration of the
 	// generated functions takes place in the generated files. The separation
 	// makes the code compile even when the generated files are missing.
-	localSchemeBuilder.Register(addKnownTypes, addDefaultingFuncs)
+	localSchemeBuilder.Register(addKnownTypes, addDefaultingFuncs, RegisterConversions)
 }
 
 func addKnownTypes(scheme *runtime.Scheme) error {

pkg/descheduler/descheduler.go

@@ -328,7 +328,7 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 
 		// Build plugins
 		for _, profile := range deschedulerPolicy.Profiles {
-			pc := getPluginConfig(defaultevictor.PluginName, profile.PluginConfigs)
+			pc, _ := GetPluginConfig(defaultevictor.PluginName, profile.PluginConfigs)
 			if pc == nil {
 				klog.ErrorS(fmt.Errorf("unable to get plugin config"), "skipping plugin", "plugin", defaultevictor.PluginName, "profile", profile.Name)
 				continue
@@ -358,7 +358,7 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 			// 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(profile.Plugins.Deschedule.Enabled, profile.Plugins.Balance.Enabled...) {
-				pc := getPluginConfig(plugin, profile.PluginConfigs)
+				pc, _ := GetPluginConfig(plugin, profile.PluginConfigs)
 				if pc == nil {
 					klog.ErrorS(fmt.Errorf("unable to get plugin config"), "skipping plugin", "plugin", plugin)
 					continue
@@ -437,13 +437,13 @@ func includeBalance(enabledBalancePlugins []framework.BalancePlugin, pg framewor
 	return enabledBalancePlugins
 }
 
-func getPluginConfig(pluginName string, pluginConfigs []api.PluginConfig) *api.PluginConfig {
+func GetPluginConfig(pluginName string, pluginConfigs []api.PluginConfig) (*api.PluginConfig, int) {
-	for _, pluginConfig := range pluginConfigs {
+	for idx, pluginConfig := range pluginConfigs {
 		if pluginConfig.Name == pluginName {
-			return &pluginConfig
+			return &pluginConfig, idx
 		}
 	}
-	return nil
+	return nil, 0
 }
 
 func createClients(clientConnection componentbaseconfig.ClientConnectionConfiguration) (clientset.Interface, clientset.Interface, error) {

pkg/descheduler/descheduler_test.go

@@ -9,6 +9,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	policy "k8s.io/api/policy/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/runtime"
 	fakeclientset "k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
@@ -21,6 +22,22 @@ import (
 	"sigs.k8s.io/descheduler/test"
 )
 
+// scope contains information about an ongoing conversion.
+type scope struct {
+	converter *conversion.Converter
+	meta      *conversion.Meta
+}
+
+// Convert continues a conversion.
+func (s scope) Convert(src, dest interface{}) error {
+	return s.converter.Convert(src, dest, s.meta)
+}
+
+// Meta returns the meta object that was originally passed to Convert.
+func (s scope) Meta() *conversion.Meta {
+	return s.meta
+}
+
 func TestTaintsUpdated(t *testing.T) {
 	pluginregistry.PluginRegistry = pluginregistry.NewRegistry()
 	pluginregistry.Register(removepodsviolatingnodetaints.PluginName, removepodsviolatingnodetaints.New, &removepodsviolatingnodetaints.RemovePodsViolatingNodeTaintsArgs{}, removepodsviolatingnodetaints.ValidateRemovePodsViolatingNodeTaintsArgs, removepodsviolatingnodetaints.SetDefaults_RemovePodsViolatingNodeTaintsArgs, pluginregistry.PluginRegistry)
@@ -74,7 +91,9 @@ func TestTaintsUpdated(t *testing.T) {
 	var evictedPods []string
 	client.PrependReactor("create", "pods", podEvictionReactionFuc(&evictedPods))
 
-	internalDeschedulerPolicy, err := v1alpha1.V1alpha1ToInternal(client, dp, pluginregistry.PluginRegistry)
+	internalDeschedulerPolicy := &api.DeschedulerPolicy{}
+	scope := scope{}
+	err = v1alpha1.V1alpha1ToInternal(dp, pluginregistry.PluginRegistry, internalDeschedulerPolicy, scope)
 	if err != nil {
 		t.Fatalf("Unable to convert v1alpha1 to v1alpha2: %v", err)
 	}
@@ -136,7 +155,9 @@ func TestDuplicate(t *testing.T) {
 	var evictedPods []string
 	client.PrependReactor("create", "pods", podEvictionReactionFuc(&evictedPods))
 
-	internalDeschedulerPolicy, err := v1alpha1.V1alpha1ToInternal(client, dp, pluginregistry.PluginRegistry)
+	internalDeschedulerPolicy := &api.DeschedulerPolicy{}
+	scope := scope{}
+	err = v1alpha1.V1alpha1ToInternal(dp, pluginregistry.PluginRegistry, internalDeschedulerPolicy, scope)
 	if err != nil {
 		t.Fatalf("Unable to convert v1alpha1 to v1alpha2: %v", err)
 	}

pkg/descheduler/policyconfig.go

@@ -17,8 +17,9 @@ limitations under the License.
 package descheduler
 
 import (
+	"context"
 	"fmt"
-	"os"
+	"io/ioutil"
 
 	"k8s.io/apimachinery/pkg/runtime"
 	clientset "k8s.io/client-go/kubernetes"
@@ -30,6 +31,7 @@ import (
 	"sigs.k8s.io/descheduler/pkg/descheduler/scheme"
 	"sigs.k8s.io/descheduler/pkg/framework/pluginregistry"
 	"sigs.k8s.io/descheduler/pkg/framework/plugins/defaultevictor"
+	"sigs.k8s.io/descheduler/pkg/utils"
 )
 
 func LoadPolicyConfig(policyConfigFile string, client clientset.Interface, registry pluginregistry.Registry) (*api.DeschedulerPolicy, error) {
@@ -38,7 +40,7 @@ func LoadPolicyConfig(policyConfigFile string, client clientset.Interface, regis
 		return nil, nil
 	}
 
-	policy, err := os.ReadFile(policyConfigFile)
+	policy, err := ioutil.ReadFile(policyConfigFile)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read policy config file %q: %+v", policyConfigFile, err)
 	}
@@ -47,35 +49,12 @@ func LoadPolicyConfig(policyConfigFile string, client clientset.Interface, regis
 }
 
 func decode(policyConfigFile string, policy []byte, client clientset.Interface, registry pluginregistry.Registry) (*api.DeschedulerPolicy, error) {
-	versionedPolicy := &v1alpha1.DeschedulerPolicy{}
 	internalPolicy := &api.DeschedulerPolicy{}
 	var err error
 
 	decoder := scheme.Codecs.UniversalDecoder(v1alpha1.SchemeGroupVersion, v1alpha2.SchemeGroupVersion, api.SchemeGroupVersion)
-	if err := runtime.DecodeInto(decoder, policy, versionedPolicy); err != nil {
+	if err := runtime.DecodeInto(decoder, policy, internalPolicy); err != nil {
-		// TODO: Right now we are checking this error string because we couldn't make a native
+		return nil, fmt.Errorf("failed decoding descheduler's policy config %q: %v", policyConfigFile, err)
-		// Convert_v1alpha1_DeschedulerPolicy_To_api_DeschedulerPolicy in conversion.go
-		// and we are just calling  V1alpha1ToInternal directly (since it needs a client as an argument).
-		// We need to make V1alpha1ToInternal stop needing a client, use a native conversion function
-		// and just rely in a DecodeInto that would pick up any policy file directly into our internal api.
-		// An attempt of doing that is in b25a44c51c0fa31a7831b899e73d83abd12a789a. Relevant discussions in following comments:
-		// https://github.com/kubernetes-sigs/descheduler/pull/1006#discussion_r1062630128
-		if err.Error() == "converting (v1alpha2.DeschedulerPolicy) to (v1alpha1.DeschedulerPolicy): unknown conversion" {
-			klog.V(1).InfoS("Tried reading v1alpha2.DeschedulerPolicy and failed. Trying legacy conversion now.")
-		} else {
-			return nil, fmt.Errorf("failed decoding descheduler's policy config %q: %v", policyConfigFile, err)
-		}
-	}
-	if versionedPolicy.APIVersion == "descheduler/v1alpha1" {
-		// Build profiles
-		internalPolicy, err = v1alpha1.V1alpha1ToInternal(client, versionedPolicy, registry)
-		if err != nil {
-			return nil, fmt.Errorf("failed converting versioned policy to internal policy version: %v", err)
-		}
-	} else {
-		if err := runtime.DecodeInto(decoder, policy, internalPolicy); err != nil {
-			return nil, fmt.Errorf("failed decoding descheduler's policy config %q: %v", policyConfigFile, err)
-		}
 	}
 
 	err = validateDeschedulerConfiguration(*internalPolicy, registry)
@@ -83,15 +62,15 @@ func decode(policyConfigFile string, policy []byte, client clientset.Interface,
 		return nil, err
 	}
 
-	setDefaults(*internalPolicy, registry)
+	setDefaults(*internalPolicy, registry, client)
 
 	return internalPolicy, nil
 }
 
-func setDefaults(in api.DeschedulerPolicy, registry pluginregistry.Registry) *api.DeschedulerPolicy {
+func setDefaults(in api.DeschedulerPolicy, registry pluginregistry.Registry, client clientset.Interface) *api.DeschedulerPolicy {
 	for idx, profile := range in.Profiles {
 		// If we need to set defaults coming from loadtime in each profile we do it here
-		in.Profiles[idx] = setDefaultEvictor(profile)
+		in.Profiles[idx] = setDefaultEvictor(profile, client)
 		for _, pluginConfig := range profile.PluginConfigs {
 			setDefaultsPluginConfig(&pluginConfig, registry)
 		}
@@ -108,7 +87,7 @@ func setDefaultsPluginConfig(pluginConfig *api.PluginConfig, registry pluginregi
 	}
 }
 
-func setDefaultEvictor(profile api.Profile) api.Profile {
+func setDefaultEvictor(profile api.Profile, client clientset.Interface) api.Profile {
 	newPluginConfig := api.PluginConfig{
 		Name: defaultevictor.PluginName,
 		Args: &defaultevictor.DefaultEvictorArgs{
@@ -122,6 +101,17 @@ func setDefaultEvictor(profile api.Profile) api.Profile {
 		profile.Plugins.Evict.Enabled = append(profile.Plugins.Evict.Enabled, defaultevictor.PluginName)
 		profile.PluginConfigs = append(profile.PluginConfigs, newPluginConfig)
 	}
+	var thresholdPriority int32
+	var err error
+	defaultevictorPluginConfig, idx := GetPluginConfig(defaultevictor.PluginName, profile.PluginConfigs)
+	if defaultevictorPluginConfig != nil {
+		thresholdPriority, err = utils.GetPriorityFromStrategyParams(context.TODO(), client, defaultevictorPluginConfig.Args.(*defaultevictor.DefaultEvictorArgs).PriorityThreshold)
+	}
+	if err != nil {
+		klog.Error(err, "Failed to get threshold priority from args")
+	}
+	profile.PluginConfigs[idx].Args.(*defaultevictor.DefaultEvictorArgs).PriorityThreshold = &api.PriorityThreshold{}
+	profile.PluginConfigs[idx].Args.(*defaultevictor.DefaultEvictorArgs).PriorityThreshold.Value = &thresholdPriority
 	return profile
 }
 

pkg/descheduler/policyconfig_test.go

@@ -45,7 +45,7 @@ func TestV1alpha1ToV1alpha2(t *testing.T) {
 		Name: defaultevictor.PluginName,
 		Args: &defaultevictor.DefaultEvictorArgs{
 			PriorityThreshold: &api.PriorityThreshold{
-				Value: utilpointer.Int32(utils.SystemCriticalPriority),
+				Value: nil,
 			},
 		},
 	}
@@ -682,8 +682,9 @@ func TestV1alpha1ToV1alpha2(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			client := fakeclientset.NewSimpleClientset()
+			result := &api.DeschedulerPolicy{}
-			result, err := v1alpha1.V1alpha1ToInternal(client, tc.policy, pluginregistry.PluginRegistry)
+			scope := scope{}
+			err := v1alpha1.V1alpha1ToInternal(tc.policy, pluginregistry.PluginRegistry, result, scope)
 			if err != nil {
 				if err.Error() != tc.err.Error() {
 					t.Errorf("unexpected error: %s", err.Error())
@@ -763,7 +764,7 @@ strategies:
 			},
 		},
 		{
-			description: "v1aplha2 to internal",
+			description: "v1alpha2 to internal",
 			policy: []byte(`apiVersion: "descheduler/v1alpha2"
 kind: "DeschedulerPolicy"
 profiles:
@@ -801,6 +802,7 @@ profiles:
 									EvictSystemCriticalPods: true,
 									EvictFailedBarePods:     true,
 									EvictLocalStoragePods:   true,
+									PriorityThreshold:       &api.PriorityThreshold{Value: utilpointer.Int32(2000000000)},
 									NodeFit:                 true,
 								},
 							},
@@ -955,6 +957,7 @@ profiles:
 									EvictSystemCriticalPods: true,
 									EvictFailedBarePods:     true,
 									EvictLocalStoragePods:   true,
+									PriorityThreshold:       &api.PriorityThreshold{Value: utilpointer.Int32(2000000000)},
 									NodeFit:                 true,
 								},
 							},