fix plugin arg conversion when using multiple profiles with same plug…

…in (kubernetes-sigs#1143) * fix plugin arg conversion when using multiple profiles with same plugin Signed-off-by: Amir Alavi <amiralavi7@gmail.com> * PR feedback to refactor validateDeschedulerConfiguration error handling --------- Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
a7i · May 25, 2023 · c6c2e2d · c6c2e2d
1 parent 4cae47f
commit c6c2e2d
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 22 deletions.
diff --git 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
@@ -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
-				}
-
-				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()))
-					}
-				}
+			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
+			}
+			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
@@ -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]"),
 		},
 	}