From c6c2e2d7fc31fc38f01be1ef7ddbb0604cf2281b Mon Sep 17 00:00:00 2001 From: Amir Alavi Date: Thu, 11 May 2023 11:25:03 -0400 Subject: [PATCH] fix plugin arg conversion when using multiple profiles with same plugin (#1143) * fix plugin arg conversion when using multiple profiles with same plugin Signed-off-by: Amir Alavi * PR feedback to refactor validateDeschedulerConfiguration error handling --------- Signed-off-by: Amir Alavi --- pkg/api/v1alpha2/conversion.go | 2 +- pkg/descheduler/policyconfig.go | 33 ++++++++++++---------------- pkg/descheduler/policyconfig_test.go | 2 +- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/pkg/api/v1alpha2/conversion.go b/pkg/api/v1alpha2/conversion.go index 36c95e5ca..6d8ee01f6 100644 --- a/pkg/api/v1alpha2/conversion.go +++ b/pkg/api/v1alpha2/conversion.go @@ -88,7 +88,7 @@ func convertToInternalPluginConfigArgs(out *api.DeschedulerPolicy) error { func Convert_v1alpha2_PluginConfig_To_api_PluginConfig(in *PluginConfig, out *api.PluginConfig, s conversion.Scope) error { out.Name = in.Name if _, ok := pluginregistry.PluginRegistry[in.Name]; ok { - out.Args = pluginregistry.PluginRegistry[in.Name].PluginArgInstance + out.Args = pluginregistry.PluginRegistry[in.Name].PluginArgInstance.DeepCopyObject() if in.Args.Raw != nil { _, _, err := Codecs.UniversalDecoder().Decode(in.Args.Raw, nil, out.Args) if err != nil { diff --git a/pkg/descheduler/policyconfig.go b/pkg/descheduler/policyconfig.go index cd7b7d2e0..aa9c17405 100644 --- a/pkg/descheduler/policyconfig.go +++ b/pkg/descheduler/policyconfig.go @@ -21,6 +21,8 @@ import ( "fmt" "os" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/runtime" clientset "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" @@ -133,29 +135,22 @@ func setDefaultEvictor(profile api.DeschedulerProfile, client clientset.Interfac } func validateDeschedulerConfiguration(in api.DeschedulerPolicy, registry pluginregistry.Registry) error { - var errorsInProfiles error + var errorsInProfiles []error for _, profile := range in.Profiles { for _, pluginConfig := range profile.PluginConfigs { - if _, ok := registry[pluginConfig.Name]; ok { - if _, ok := registry[pluginConfig.Name]; !ok { - errorsInProfiles = fmt.Errorf("%w: %s", errorsInProfiles, fmt.Sprintf("in profile %s: plugin %s in pluginConfig not registered", profile.Name, pluginConfig.Name)) - continue - } + if _, ok := registry[pluginConfig.Name]; !ok { + errorsInProfiles = append(errorsInProfiles, fmt.Errorf("in profile %s: plugin %s in pluginConfig not registered", profile.Name, pluginConfig.Name)) + continue + } - pluginUtilities := registry[pluginConfig.Name] - if pluginUtilities.PluginArgValidator == nil { - continue - } - err := pluginUtilities.PluginArgValidator(pluginConfig.Args) - if err != nil { - if errorsInProfiles == nil { - errorsInProfiles = fmt.Errorf("in profile %s: %s", profile.Name, err.Error()) - } else { - errorsInProfiles = fmt.Errorf("%w: %s", errorsInProfiles, fmt.Sprintf("in profile %s: %s", profile.Name, err.Error())) - } - } + pluginUtilities := registry[pluginConfig.Name] + if pluginUtilities.PluginArgValidator == nil { + continue + } + if err := pluginUtilities.PluginArgValidator(pluginConfig.Args); err != nil { + errorsInProfiles = append(errorsInProfiles, fmt.Errorf("in profile %s: %s", profile.Name, err.Error())) } } } - return errorsInProfiles + return utilerrors.NewAggregate(errorsInProfiles) } diff --git a/pkg/descheduler/policyconfig_test.go b/pkg/descheduler/policyconfig_test.go index 8c9e95f28..629bb3f40 100644 --- a/pkg/descheduler/policyconfig_test.go +++ b/pkg/descheduler/policyconfig_test.go @@ -985,7 +985,7 @@ func TestValidateDeschedulerConfiguration(t *testing.T) { }, }, }, - result: fmt.Errorf("in profile RemoveFailedPods: only one of Include/Exclude namespaces can be set: in profile RemovePodsViolatingTopologySpreadConstraint: only one of Include/Exclude namespaces can be set"), + result: fmt.Errorf("[in profile RemoveFailedPods: only one of Include/Exclude namespaces can be set, in profile RemovePodsViolatingTopologySpreadConstraint: only one of Include/Exclude namespaces can be set]"), }, }