From 1727cea64c1d53f7badbc03b0ca77543283e6157 Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Thu, 8 Jul 2021 13:00:23 +0800 Subject: [PATCH 1/2] Fix index out of range if multiple default plugins are overridden Signed-off-by: Dave Chen --- .../apis/config/v1beta2/default_plugins.go | 18 ++++++--- .../config/v1beta2/default_plugins_test.go | 40 ++++++++++++++++++- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/pkg/scheduler/apis/config/v1beta2/default_plugins.go b/pkg/scheduler/apis/config/v1beta2/default_plugins.go index 7eaa358143f..1d63003aa8b 100644 --- a/pkg/scheduler/apis/config/v1beta2/default_plugins.go +++ b/pkg/scheduler/apis/config/v1beta2/default_plugins.go @@ -156,6 +156,8 @@ type pluginIndex struct { func mergePluginSet(defaultPluginSet, customPluginSet v1beta2.PluginSet) v1beta2.PluginSet { disabledPlugins := sets.NewString() enabledCustomPlugins := make(map[string]pluginIndex) + // replacedPluginIndex is a set of index of plugins, which have replaced the default plugins. + replacedPluginIndex := sets.NewInt() for _, disabledPlugin := range customPluginSet.Disabled { disabledPlugins.Insert(disabledPlugin.Name) } @@ -170,16 +172,22 @@ func mergePluginSet(defaultPluginSet, customPluginSet v1beta2.PluginSet) v1beta2 } // The default plugin is explicitly re-configured, update the default plugin accordingly. if customPlugin, ok := enabledCustomPlugins[defaultEnabledPlugin.Name]; ok { - klog.InfoS("Defaut plugin is explicitly re-configured and is overriding.", "plugin", defaultEnabledPlugin.Name) - // update the default plugin in place to preserve order + klog.InfoS("Default plugin is explicitly re-configured; overriding", "plugin", defaultEnabledPlugin.Name) + // Update the default plugin in place to preserve order. defaultEnabledPlugin = customPlugin.plugin - // kick the plugin from the enabled list of custom plugins - customPluginSet.Enabled = append(customPluginSet.Enabled[:customPlugin.index], customPluginSet.Enabled[customPlugin.index+1:]...) + replacedPluginIndex.Insert(customPlugin.index) } enabledPlugins = append(enabledPlugins, defaultEnabledPlugin) } } - enabledPlugins = append(enabledPlugins, customPluginSet.Enabled...) + // Append all the custom plugins which haven't replaced any default plugins. + // Note: duplicated custom plugins will still be appended here. + // If so, the instantiation of scheduler framework will detect it and abort. + for index, plugin := range customPluginSet.Enabled { + if !replacedPluginIndex.Has(index) { + enabledPlugins = append(enabledPlugins, plugin) + } + } return v1beta2.PluginSet{Enabled: enabledPlugins} } diff --git a/pkg/scheduler/apis/config/v1beta2/default_plugins_test.go b/pkg/scheduler/apis/config/v1beta2/default_plugins_test.go index 417fa0a8a4d..934588833d7 100644 --- a/pkg/scheduler/apis/config/v1beta2/default_plugins_test.go +++ b/pkg/scheduler/apis/config/v1beta2/default_plugins_test.go @@ -367,6 +367,7 @@ func TestMergePlugins(t *testing.T) { Filter: v1beta2.PluginSet{ Enabled: []v1beta2.Plugin{ {Name: "Plugin1", Weight: pointer.Int32Ptr(2)}, + {Name: "Plugin3", Weight: pointer.Int32Ptr(3)}, }, }, }, @@ -374,6 +375,8 @@ func TestMergePlugins(t *testing.T) { Filter: v1beta2.PluginSet{ Enabled: []v1beta2.Plugin{ {Name: "Plugin1"}, + {Name: "Plugin2"}, + {Name: "Plugin3"}, }, }, }, @@ -381,6 +384,8 @@ func TestMergePlugins(t *testing.T) { Filter: v1beta2.PluginSet{ Enabled: []v1beta2.Plugin{ {Name: "Plugin1", Weight: pointer.Int32Ptr(2)}, + {Name: "Plugin2"}, + {Name: "Plugin3", Weight: pointer.Int32Ptr(3)}, }, }, }, @@ -391,6 +396,38 @@ func TestMergePlugins(t *testing.T) { Filter: v1beta2.PluginSet{ Enabled: []v1beta2.Plugin{ {Name: "Plugin2", Weight: pointer.Int32Ptr(2)}, + {Name: "Plugin1", Weight: pointer.Int32Ptr(1)}, + }, + }, + }, + defaultPlugins: &v1beta2.Plugins{ + Filter: v1beta2.PluginSet{ + Enabled: []v1beta2.Plugin{ + {Name: "Plugin1"}, + {Name: "Plugin2"}, + {Name: "Plugin3"}, + }, + }, + }, + expectedPlugins: &v1beta2.Plugins{ + Filter: v1beta2.PluginSet{ + Enabled: []v1beta2.Plugin{ + {Name: "Plugin1", Weight: pointer.Int32Ptr(1)}, + {Name: "Plugin2", Weight: pointer.Int32Ptr(2)}, + {Name: "Plugin3"}, + }, + }, + }, + }, + { + name: "RepeatedCustomPlugin", + customPlugins: &v1beta2.Plugins{ + Filter: v1beta2.PluginSet{ + Enabled: []v1beta2.Plugin{ + {Name: "Plugin1"}, + {Name: "Plugin2", Weight: pointer.Int32Ptr(2)}, + {Name: "Plugin3"}, + {Name: "Plugin2", Weight: pointer.Int32Ptr(4)}, }, }, }, @@ -407,8 +444,9 @@ func TestMergePlugins(t *testing.T) { Filter: v1beta2.PluginSet{ Enabled: []v1beta2.Plugin{ {Name: "Plugin1"}, - {Name: "Plugin2", Weight: pointer.Int32Ptr(2)}, + {Name: "Plugin2", Weight: pointer.Int32Ptr(4)}, {Name: "Plugin3"}, + {Name: "Plugin2", Weight: pointer.Int32Ptr(2)}, }, }, }, From 5918869ed6c2d5b5d6c4db5be77f846b6a7b5422 Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Thu, 8 Jul 2021 13:25:53 +0800 Subject: [PATCH 2/2] Revert 103327: "kube-scheduler: ensure the default config output of --write-to-config is usable" We don't need to maually disable all the default plugins anymore Signed-off-by: Dave Chen dave.chen@arm.com --- cmd/kube-scheduler/app/options/configfile.go | 24 -------------------- 1 file changed, 24 deletions(-) diff --git a/cmd/kube-scheduler/app/options/configfile.go b/cmd/kube-scheduler/app/options/configfile.go index ea0c9644271..384e86fb480 100644 --- a/cmd/kube-scheduler/app/options/configfile.go +++ b/cmd/kube-scheduler/app/options/configfile.go @@ -87,30 +87,6 @@ func LogOrWriteConfig(fileName string, cfg *config.KubeSchedulerConfiguration, c } cfg.Profiles = completedProfiles - if len(fileName) > 0 { - // Since the default component config lists all the default plugins as enabled - // without disabling them, we must explicitly disable all the plugins for each - // extension point using the "*" expression to ensure that the generated config - // file is usable - disabledPlugins := []config.Plugin{{Name: "*"}} - for i := range cfg.Profiles { - if cfg.Profiles[i].Plugins == nil { - continue - } - cfg.Profiles[i].Plugins.QueueSort.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.PreFilter.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.Filter.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.PostFilter.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.PreScore.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.Score.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.Reserve.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.Permit.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.PreBind.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.Bind.Disabled = disabledPlugins - cfg.Profiles[i].Plugins.PostBind.Disabled = disabledPlugins - } - } - buf, err := encodeConfig(cfg) if err != nil { return err