From a2229691a99fc42396941e0cbc1e7cd58af4a512 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Mon, 25 Jul 2022 10:56:05 +0800 Subject: [PATCH 1/2] Revert "Cleanup: remove validation of removedPlugins" This reverts commit 0971577c3e6f7710a548630d5a42b4f11dae346f. --- .../apis/config/validation/validation.go | 73 ++++++++++++++++++- .../apis/config/validation/validation_test.go | 14 ++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/pkg/scheduler/apis/config/validation/validation.go b/pkg/scheduler/apis/config/validation/validation.go index ad1c34a3e9d..2aae138ce5a 100644 --- a/pkg/scheduler/apis/config/validation/validation.go +++ b/pkg/scheduler/apis/config/validation/validation.go @@ -33,6 +33,8 @@ import ( componentbasevalidation "k8s.io/component-base/config/validation" v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/scheduler/apis/config" + "k8s.io/kubernetes/pkg/scheduler/apis/config/v1beta2" + "k8s.io/kubernetes/pkg/scheduler/apis/config/v1beta3" ) // ValidateKubeSchedulerConfiguration ensures validation of the KubeSchedulerConfiguration struct @@ -115,6 +117,51 @@ func splitHostIntPort(s string) (string, int, error) { return host, portInt, err } +type removedPlugins struct { + schemeGroupVersion string + plugins []string +} + +// removedPluginsByVersion maintains a list of removed plugins in each version. +// Remember to add an entry to that list when creating a new component config +// version (even if the list of removed plugins is empty). +var removedPluginsByVersion = []removedPlugins{ + { + schemeGroupVersion: v1beta2.SchemeGroupVersion.String(), + plugins: []string{}, + }, + { + schemeGroupVersion: v1beta3.SchemeGroupVersion.String(), + plugins: []string{}, + }, +} + +// isPluginRemoved checks if a given plugin was removed in the given component +// config version or earlier. +func isPluginRemoved(apiVersion string, name string) (bool, string) { + for _, dp := range removedPluginsByVersion { + for _, plugin := range dp.plugins { + if name == plugin { + return true, dp.schemeGroupVersion + } + } + if apiVersion == dp.schemeGroupVersion { + break + } + } + return false, "" +} + +func validatePluginSetForRemovedPlugins(path *field.Path, apiVersion string, ps config.PluginSet) []error { + var errs []error + for i, plugin := range ps.Enabled { + if removed, removedVersion := isPluginRemoved(apiVersion, plugin.Name); removed { + errs = append(errs, field.Invalid(path.Child("enabled").Index(i), plugin.Name, fmt.Sprintf("was removed in version %q (KubeSchedulerConfiguration is version %q)", removedVersion, apiVersion))) + } + } + return errs +} + func validateKubeSchedulerProfile(path *field.Path, apiVersion string, profile *config.KubeSchedulerProfile) []error { var errs []error if len(profile.SchedulerName) == 0 { @@ -136,6 +183,28 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K "VolumeBinding": ValidateVolumeBindingArgs, } + if profile.Plugins != nil { + stagesToPluginSet := map[string]config.PluginSet{ + "queueSort": profile.Plugins.QueueSort, + "preFilter": profile.Plugins.PreFilter, + "filter": profile.Plugins.Filter, + "postFilter": profile.Plugins.PostFilter, + "preScore": profile.Plugins.PreScore, + "score": profile.Plugins.Score, + "reserve": profile.Plugins.Reserve, + "permit": profile.Plugins.Permit, + "preBind": profile.Plugins.PreBind, + "bind": profile.Plugins.Bind, + "postBind": profile.Plugins.PostBind, + } + + pluginsPath := path.Child("plugins") + for s, p := range stagesToPluginSet { + errs = append(errs, validatePluginSetForRemovedPlugins( + pluginsPath.Child(s), apiVersion, p)...) + } + } + seenPluginConfig := make(sets.String) for i := range profile.PluginConfig { @@ -147,7 +216,9 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K } else { seenPluginConfig.Insert(name) } - if validateFunc, ok := m[name]; ok { + 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")) diff --git a/pkg/scheduler/apis/config/validation/validation_test.go b/pkg/scheduler/apis/config/validation/validation_test.go index 3db58e2b1dc..a0dbd49896c 100644 --- a/pkg/scheduler/apis/config/validation/validation_test.go +++ b/pkg/scheduler/apis/config/validation/validation_test.go @@ -198,6 +198,9 @@ func TestValidateKubeSchedulerConfigurationV1beta2(t *testing.T) { BindVerb: "bar", }) + goodRemovedPlugins2 := validConfig.DeepCopy() + goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "PodTopologySpread", Weight: 2}) + scenarios := map[string]struct { expectedToFail bool config *config.KubeSchedulerConfiguration @@ -275,6 +278,10 @@ func TestValidateKubeSchedulerConfigurationV1beta2(t *testing.T) { expectedToFail: true, config: mismatchQueueSort, }, + "good-removed-plugins-2": { + expectedToFail: false, + config: goodRemovedPlugins2, + }, } for name, scenario := range scenarios { @@ -465,6 +472,9 @@ func TestValidateKubeSchedulerConfigurationV1beta3(t *testing.T) { BindVerb: "bar", }) + goodRemovedPlugins2 := validConfig.DeepCopy() + goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "PodTopologySpread", Weight: 2}) + scenarios := map[string]struct { expectedToFail bool config *config.KubeSchedulerConfiguration @@ -542,6 +552,10 @@ func TestValidateKubeSchedulerConfigurationV1beta3(t *testing.T) { expectedToFail: true, config: mismatchQueueSort, }, + "good-removed-plugins-2": { + expectedToFail: false, + config: goodRemovedPlugins2, + }, } for name, scenario := range scenarios { From aa04472f54f86432eb1d3a94f917da2610e8f364 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Mon, 25 Jul 2022 11:19:44 +0800 Subject: [PATCH 2/2] Refactor: rename removedPlugin to invalidPlugin Signed-off-by: kerthcet --- .../apis/config/validation/validation.go | 26 +++++++++---------- .../apis/config/validation/validation_test.go | 16 ++++++------ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/scheduler/apis/config/validation/validation.go b/pkg/scheduler/apis/config/validation/validation.go index 2aae138ce5a..69ea1aac074 100644 --- a/pkg/scheduler/apis/config/validation/validation.go +++ b/pkg/scheduler/apis/config/validation/validation.go @@ -117,15 +117,15 @@ func splitHostIntPort(s string) (string, int, error) { return host, portInt, err } -type removedPlugins struct { +type invalidPlugins struct { schemeGroupVersion string plugins []string } -// removedPluginsByVersion maintains a list of removed plugins in each version. +// invalidPluginsByVersion maintains a list of removed/deprecated plugins in each version. // Remember to add an entry to that list when creating a new component config -// version (even if the list of removed plugins is empty). -var removedPluginsByVersion = []removedPlugins{ +// version (even if the list of invalid plugins is empty). +var invalidPluginsByVersion = []invalidPlugins{ { schemeGroupVersion: v1beta2.SchemeGroupVersion.String(), plugins: []string{}, @@ -136,10 +136,10 @@ var removedPluginsByVersion = []removedPlugins{ }, } -// isPluginRemoved checks if a given plugin was removed in the given component +// isPluginInvalid checks if a given plugin was removed/deprecated in the given component // config version or earlier. -func isPluginRemoved(apiVersion string, name string) (bool, string) { - for _, dp := range removedPluginsByVersion { +func isPluginInvalid(apiVersion string, name string) (bool, string) { + for _, dp := range invalidPluginsByVersion { for _, plugin := range dp.plugins { if name == plugin { return true, dp.schemeGroupVersion @@ -152,11 +152,11 @@ func isPluginRemoved(apiVersion string, name string) (bool, string) { return false, "" } -func validatePluginSetForRemovedPlugins(path *field.Path, apiVersion string, ps config.PluginSet) []error { +func validatePluginSetForInvalidPlugins(path *field.Path, apiVersion string, ps config.PluginSet) []error { var errs []error for i, plugin := range ps.Enabled { - if removed, removedVersion := isPluginRemoved(apiVersion, plugin.Name); removed { - errs = append(errs, field.Invalid(path.Child("enabled").Index(i), plugin.Name, fmt.Sprintf("was removed in version %q (KubeSchedulerConfiguration is version %q)", removedVersion, apiVersion))) + if invalid, invalidVersion := isPluginInvalid(apiVersion, plugin.Name); invalid { + errs = append(errs, field.Invalid(path.Child("enabled").Index(i), plugin.Name, fmt.Sprintf("was invalid in version %q (KubeSchedulerConfiguration is version %q)", invalidVersion, apiVersion))) } } return errs @@ -200,7 +200,7 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K pluginsPath := path.Child("plugins") for s, p := range stagesToPluginSet { - errs = append(errs, validatePluginSetForRemovedPlugins( + errs = append(errs, validatePluginSetForInvalidPlugins( pluginsPath.Child(s), apiVersion, p)...) } } @@ -216,8 +216,8 @@ func validatePluginConfig(path *field.Path, apiVersion string, profile *config.K } 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))) + if invalid, invalidVersion := isPluginInvalid(apiVersion, name); invalid { + errs = append(errs, field.Invalid(pluginConfigPath, name, fmt.Sprintf("was invalid in version %q (KubeSchedulerConfiguration is version %q)", invalidVersion, 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) { diff --git a/pkg/scheduler/apis/config/validation/validation_test.go b/pkg/scheduler/apis/config/validation/validation_test.go index a0dbd49896c..4489fa78095 100644 --- a/pkg/scheduler/apis/config/validation/validation_test.go +++ b/pkg/scheduler/apis/config/validation/validation_test.go @@ -198,8 +198,8 @@ func TestValidateKubeSchedulerConfigurationV1beta2(t *testing.T) { BindVerb: "bar", }) - goodRemovedPlugins2 := validConfig.DeepCopy() - goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "PodTopologySpread", Weight: 2}) + goodInvalidPlugins := validConfig.DeepCopy() + goodInvalidPlugins.Profiles[0].Plugins.Score.Enabled = append(goodInvalidPlugins.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "PodTopologySpread", Weight: 2}) scenarios := map[string]struct { expectedToFail bool @@ -278,9 +278,9 @@ func TestValidateKubeSchedulerConfigurationV1beta2(t *testing.T) { expectedToFail: true, config: mismatchQueueSort, }, - "good-removed-plugins-2": { + "good-invalid-plugins": { expectedToFail: false, - config: goodRemovedPlugins2, + config: goodInvalidPlugins, }, } @@ -472,8 +472,8 @@ func TestValidateKubeSchedulerConfigurationV1beta3(t *testing.T) { BindVerb: "bar", }) - goodRemovedPlugins2 := validConfig.DeepCopy() - goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled = append(goodRemovedPlugins2.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "PodTopologySpread", Weight: 2}) + goodInvalidPlugins := validConfig.DeepCopy() + goodInvalidPlugins.Profiles[0].Plugins.Score.Enabled = append(goodInvalidPlugins.Profiles[0].Plugins.Score.Enabled, config.Plugin{Name: "PodTopologySpread", Weight: 2}) scenarios := map[string]struct { expectedToFail bool @@ -552,9 +552,9 @@ func TestValidateKubeSchedulerConfigurationV1beta3(t *testing.T) { expectedToFail: true, config: mismatchQueueSort, }, - "good-removed-plugins-2": { + "good-invalid-plugins": { expectedToFail: false, - config: goodRemovedPlugins2, + config: goodInvalidPlugins, }, }