From 5ea9460234a9da71e5641212c219bc9bb338bde1 Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Mon, 5 Jul 2021 17:27:46 +0800 Subject: [PATCH 1/2] Readable error message on the plugin configs of the removed plugins Several plugins are removed in the v1beta2, but the legacy scheduler config would still have the plugin configs of those removed plugins. It was throwing raw byte data when those plugin configs are still in place which will hard to read and understand. Fix it by checking the removed plugin config before the validation of the plugin args. Signed-off-by: Dave Chen --- .../apis/config/validation/validation.go | 55 ++++++++++--------- .../apis/config/validation/validation_test.go | 11 ++++ 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/pkg/scheduler/apis/config/validation/validation.go b/pkg/scheduler/apis/config/validation/validation.go index 1c9fc2806af..e5b131df694 100644 --- a/pkg/scheduler/apis/config/validation/validation.go +++ b/pkg/scheduler/apis/config/validation/validation.go @@ -201,31 +201,6 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K "VolumeBinding": ValidateVolumeBindingArgs, } - seenPluginConfig := make(sets.String) - for i := range profile.PluginConfig { - pluginConfigPath := path.Child("pluginConfig").Index(i) - name := profile.PluginConfig[i].Name - args := profile.PluginConfig[i].Args - if seenPluginConfig.Has(name) { - errs = append(errs, field.Duplicate(pluginConfigPath, name)) - } else { - seenPluginConfig.Insert(name) - } - if validateFunc, ok := m[name]; ok { - // type mismatch, no need to validate the `args`. - if reflect.TypeOf(args) != reflect.ValueOf(validateFunc).Type().In(1) { - errs = append(errs, field.Invalid(pluginConfigPath.Child("args"), args, "has to match plugin args")) - return errs - } - in := []reflect.Value{reflect.ValueOf(pluginConfigPath.Child("args")), reflect.ValueOf(args)} - res := reflect.ValueOf(validateFunc).Call(in) - // It's possible that validation function return a Aggregate, just append here and it will be flattened at the end of CC validation. - if res[0].Interface() != nil { - errs = append(errs, res[0].Interface().(error)) - } - } - } - if profile.Plugins != nil { stagesToPluginSet := map[string]config.PluginSet{ "queueSort": profile.Plugins.QueueSort, @@ -249,6 +224,36 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K errs = append(errs, validateScorePluginSetForConflictPlugins( pluginsPath.Child("score"), apiVersion, profile)...) } + + seenPluginConfig := make(sets.String) + + // validate plugin config at the end because we will return the aggregated errors early if type of args doesn't match. + // validate plugin config at the end will will help to collect as much as misconfiguration as possible. + for i := range profile.PluginConfig { + pluginConfigPath := path.Child("pluginConfig").Index(i) + name := profile.PluginConfig[i].Name + args := profile.PluginConfig[i].Args + if seenPluginConfig.Has(name) { + errs = append(errs, field.Duplicate(pluginConfigPath, name)) + } else { + seenPluginConfig.Insert(name) + } + if removed, removedVersion := isPluginRemoved(apiVersion, name); removed { + errs = append(errs, field.Invalid(pluginConfigPath, name, fmt.Sprintf("was removed in version %q (KubeSchedulerConfiguration is version %q)", removedVersion, apiVersion))) + } else if validateFunc, ok := m[name]; ok { + // type mismatch, no need to validate the `args`. + if reflect.TypeOf(args) != reflect.ValueOf(validateFunc).Type().In(1) { + errs = append(errs, field.Invalid(pluginConfigPath.Child("args"), args, "has to match plugin args")) + return errs + } + in := []reflect.Value{reflect.ValueOf(pluginConfigPath.Child("args")), reflect.ValueOf(args)} + res := reflect.ValueOf(validateFunc).Call(in) + // It's possible that validation function return a Aggregate, just append here and it will be flattened at the end of CC validation. + if res[0].Interface() != nil { + errs = append(errs, res[0].Interface().(error)) + } + } + } return errs } diff --git a/pkg/scheduler/apis/config/validation/validation_test.go b/pkg/scheduler/apis/config/validation/validation_test.go index 5cef752b316..d7110aa476b 100644 --- a/pkg/scheduler/apis/config/validation/validation_test.go +++ b/pkg/scheduler/apis/config/validation/validation_test.go @@ -254,6 +254,12 @@ func TestValidateKubeSchedulerConfiguration(t *testing.T) { goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "NodeResourcesMostAllocated", Weight: 2}) goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "RequestedToCapacityRatio", Weight: 2}) + badPluginsConfig := validConfig.DeepCopy() + badPluginsConfig.Profiles[0].PluginConfig = append(badPluginsConfig.Profiles[0].PluginConfig, config.PluginConfig{ + Name: "NodeResourcesLeastAllocated", + Args: &config.NodeResourcesLeastAllocatedArgs{}, + }) + scenarios := map[string]struct { expectedToFail bool config *config.KubeSchedulerConfiguration @@ -384,6 +390,11 @@ func TestValidateKubeSchedulerConfiguration(t *testing.T) { expectedToFail: false, config: goodConflictPlugins2, }, + "bad-plugins-config": { + expectedToFail: true, + config: badPluginsConfig, + errorString: "profiles[0].pluginConfig[1]: Invalid value: \"NodeResourcesLeastAllocated\": was removed in version \"kubescheduler.config.k8s.io/v1beta2\" (KubeSchedulerConfiguration is version \"kubescheduler.config.k8s.io/v1beta2\")", + }, } for name, scenario := range scenarios { From 823a0f101acb15e6c87f0725f17493f054424637 Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Tue, 6 Jul 2021 11:21:58 +0800 Subject: [PATCH 2/2] Don't return in api validation --- .../apis/config/validation/validation.go | 16 +++++++--------- .../apis/config/validation/validation_test.go | 6 +++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/scheduler/apis/config/validation/validation.go b/pkg/scheduler/apis/config/validation/validation.go index e5b131df694..67e25c92cbe 100644 --- a/pkg/scheduler/apis/config/validation/validation.go +++ b/pkg/scheduler/apis/config/validation/validation.go @@ -227,8 +227,6 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K seenPluginConfig := make(sets.String) - // validate plugin config at the end because we will return the aggregated errors early if type of args doesn't match. - // validate plugin config at the end will will help to collect as much as misconfiguration as possible. for i := range profile.PluginConfig { pluginConfigPath := path.Child("pluginConfig").Index(i) name := profile.PluginConfig[i].Name @@ -244,13 +242,13 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K // type mismatch, no need to validate the `args`. if reflect.TypeOf(args) != reflect.ValueOf(validateFunc).Type().In(1) { errs = append(errs, field.Invalid(pluginConfigPath.Child("args"), args, "has to match plugin args")) - return errs - } - in := []reflect.Value{reflect.ValueOf(pluginConfigPath.Child("args")), reflect.ValueOf(args)} - res := reflect.ValueOf(validateFunc).Call(in) - // It's possible that validation function return a Aggregate, just append here and it will be flattened at the end of CC validation. - if res[0].Interface() != nil { - errs = append(errs, res[0].Interface().(error)) + } else { + in := []reflect.Value{reflect.ValueOf(pluginConfigPath.Child("args")), reflect.ValueOf(args)} + res := reflect.ValueOf(validateFunc).Call(in) + // It's possible that validation function return a Aggregate, just append here and it will be flattened at the end of CC validation. + if res[0].Interface() != nil { + errs = append(errs, res[0].Interface().(error)) + } } } } diff --git a/pkg/scheduler/apis/config/validation/validation_test.go b/pkg/scheduler/apis/config/validation/validation_test.go index d7110aa476b..dee8b96567b 100644 --- a/pkg/scheduler/apis/config/validation/validation_test.go +++ b/pkg/scheduler/apis/config/validation/validation_test.go @@ -254,8 +254,8 @@ func TestValidateKubeSchedulerConfiguration(t *testing.T) { goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "NodeResourcesMostAllocated", Weight: 2}) goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodConflictPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "RequestedToCapacityRatio", Weight: 2}) - badPluginsConfig := validConfig.DeepCopy() - badPluginsConfig.Profiles[0].PluginConfig = append(badPluginsConfig.Profiles[0].PluginConfig, config.PluginConfig{ + deprecatedPluginsConfig := validConfig.DeepCopy() + deprecatedPluginsConfig.Profiles[0].PluginConfig = append(deprecatedPluginsConfig.Profiles[0].PluginConfig, config.PluginConfig{ Name: "NodeResourcesLeastAllocated", Args: &config.NodeResourcesLeastAllocatedArgs{}, }) @@ -392,7 +392,7 @@ func TestValidateKubeSchedulerConfiguration(t *testing.T) { }, "bad-plugins-config": { expectedToFail: true, - config: badPluginsConfig, + config: deprecatedPluginsConfig, errorString: "profiles[0].pluginConfig[1]: Invalid value: \"NodeResourcesLeastAllocated\": was removed in version \"kubescheduler.config.k8s.io/v1beta2\" (KubeSchedulerConfiguration is version \"kubescheduler.config.k8s.io/v1beta2\")", }, }