diff --git a/pkg/api/v1alpha1/conversion.go b/pkg/api/v1alpha1/conversion.go index a805e3b9b..03f03eb54 100644 --- a/pkg/api/v1alpha1/conversion.go +++ b/pkg/api/v1alpha1/conversion.go @@ -1,5 +1,5 @@ /* -Copyright 2023 The Kubernetes Authors. +Copyright 2022 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -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{ - Value: &thresholdPriority, - }, + PriorityThreshold: priorityThreshold, }, }, *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{ - Profiles: profiles, - NodeSelector: deschedulerPolicy.NodeSelector, - MaxNoOfPodsToEvictPerNode: deschedulerPolicy.MaxNoOfPodsToEvictPerNode, - MaxNoOfPodsToEvictPerNamespace: deschedulerPolicy.MaxNoOfPodsToEvictPerNamespace, - }, nil + out.Profiles = profiles + out.NodeSelector = deschedulerPolicy.NodeSelector + out.MaxNoOfPodsToEvictPerNamespace = deschedulerPolicy.MaxNoOfPodsToEvictPerNamespace + out.MaxNoOfPodsToEvictPerNode = deschedulerPolicy.MaxNoOfPodsToEvictPerNode + + 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 +} diff --git a/pkg/api/v1alpha1/register.go b/pkg/api/v1alpha1/register.go index bf3f210d9..3c33664bf 100644 --- a/pkg/api/v1alpha1/register.go +++ b/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 { diff --git a/pkg/descheduler/descheduler.go b/pkg/descheduler/descheduler.go index b19009edb..a1b72eb09 100644 --- a/pkg/descheduler/descheduler.go +++ b/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 { - for _, pluginConfig := range pluginConfigs { +func GetPluginConfig(pluginName string, pluginConfigs []api.PluginConfig) (*api.PluginConfig, int) { + 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) { diff --git a/pkg/descheduler/descheduler_test.go b/pkg/descheduler/descheduler_test.go index fa7482308..5ee71d130 100644 --- a/pkg/descheduler/descheduler_test.go +++ b/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) } diff --git a/pkg/descheduler/policyconfig.go b/pkg/descheduler/policyconfig.go index 46beb0ddc..071fa46c2 100644 --- a/pkg/descheduler/policyconfig.go +++ b/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 { - // TODO: Right now we are checking this error string because we couldn't make a native - // 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) - } + 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 } diff --git a/pkg/descheduler/policyconfig_test.go b/pkg/descheduler/policyconfig_test.go index 719541b31..2717048cc 100644 --- a/pkg/descheduler/policyconfig_test.go +++ b/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, err := v1alpha1.V1alpha1ToInternal(client, tc.policy, pluginregistry.PluginRegistry) + result := &api.DeschedulerPolicy{} + 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, }, },