From d330f4dcb4fcf7725b1fe64041c6b68a729e4442 Mon Sep 17 00:00:00 2001 From: Wei Huang Date: Mon, 21 Mar 2022 09:30:41 -0700 Subject: [PATCH] Fix a bug that out-of-tree plugin is misplaced when using scheduler v1beta3 config --- cmd/kube-scheduler/app/server_test.go | 120 +++++++++++++++++- pkg/scheduler/framework/runtime/framework.go | 71 +++++++++-- .../framework/runtime/framework_test.go | 38 ++++++ test/integration/scheduler/framework_test.go | 5 +- 4 files changed, 222 insertions(+), 12 deletions(-) diff --git a/cmd/kube-scheduler/app/server_test.go b/cmd/kube-scheduler/app/server_test.go index b6bd1946471..4edb0d07ba4 100644 --- a/cmd/kube-scheduler/app/server_test.go +++ b/cmd/kube-scheduler/app/server_test.go @@ -29,7 +29,9 @@ import ( "github.com/google/go-cmp/cmp" "github.com/spf13/pflag" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/util/feature" componentbaseconfig "k8s.io/component-base/config" "k8s.io/component-base/featuregate" @@ -39,6 +41,7 @@ import ( "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/scheduler/apis/config" "k8s.io/kubernetes/pkg/scheduler/apis/config/testing/defaults" + "k8s.io/kubernetes/pkg/scheduler/framework" ) func TestSetup(t *testing.T) { @@ -154,6 +157,44 @@ profiles: t.Fatal(err) } + // out-of-tree plugin config v1beta3 + outOfTreePluginConfigFilev1beta3 := filepath.Join(tmpDir, "outOfTreePluginv1beta3.yaml") + if err := os.WriteFile(outOfTreePluginConfigFilev1beta3, []byte(fmt.Sprintf(` +apiVersion: kubescheduler.config.k8s.io/v1beta3 +kind: KubeSchedulerConfiguration +clientConnection: + kubeconfig: "%s" +profiles: +- plugins: + preFilter: + enabled: + - name: Foo + filter: + enabled: + - name: Foo +`, configKubeconfig)), os.FileMode(0600)); err != nil { + t.Fatal(err) + } + + // out-of-tree plugin config v1beta2 + outOfTreePluginConfigFilev1beta2 := filepath.Join(tmpDir, "outOfTreePluginv1beta2.yaml") + if err := os.WriteFile(outOfTreePluginConfigFilev1beta2, []byte(fmt.Sprintf(` +apiVersion: kubescheduler.config.k8s.io/v1beta2 +kind: KubeSchedulerConfiguration +clientConnection: + kubeconfig: "%s" +profiles: +- plugins: + preFilter: + enabled: + - name: Foo + filter: + enabled: + - name: Foo +`, configKubeconfig)), os.FileMode(0600)); err != nil { + t.Fatal(err) + } + // multiple profiles config multiProfilesConfig := filepath.Join(tmpDir, "multi-profiles.yaml") if err := os.WriteFile(multiProfilesConfig, []byte(fmt.Sprintf(` @@ -211,6 +252,7 @@ leaderElection: testcases := []struct { name string flags []string + registryOptions []Option restoreFeatures map[featuregate.Feature]bool wantPlugins map[string]*config.Plugins wantLeaderElection *componentbaseconfig.LeaderElectionConfiguration @@ -330,6 +372,56 @@ leaderElection: }, }, }, + { + name: "out-of-tree component configuration v1beta2", + flags: []string{ + "--config", outOfTreePluginConfigFilev1beta2, + "--kubeconfig", configKubeconfig, + }, + registryOptions: []Option{WithPlugin("Foo", newFoo)}, + wantPlugins: map[string]*config.Plugins{ + "default-scheduler": { + Bind: defaults.PluginsV1beta2.Bind, + Filter: config.PluginSet{ + Enabled: append(defaults.PluginsV1beta2.Filter.Enabled, config.Plugin{Name: "Foo"}), + }, + PreFilter: config.PluginSet{ + Enabled: append(defaults.PluginsV1beta2.PreFilter.Enabled, config.Plugin{Name: "Foo"}), + }, + PostFilter: defaults.PluginsV1beta2.PostFilter, + PreScore: defaults.PluginsV1beta2.PreScore, + QueueSort: defaults.PluginsV1beta2.QueueSort, + Score: defaults.PluginsV1beta2.Score, + Reserve: defaults.PluginsV1beta2.Reserve, + PreBind: defaults.PluginsV1beta2.PreBind, + }, + }, + }, + { + name: "out-of-tree component configuration v1beta3", + flags: []string{ + "--config", outOfTreePluginConfigFilev1beta3, + "--kubeconfig", configKubeconfig, + }, + registryOptions: []Option{WithPlugin("Foo", newFoo)}, + wantPlugins: map[string]*config.Plugins{ + "default-scheduler": { + Bind: defaults.ExpandedPluginsV1beta3.Bind, + Filter: config.PluginSet{ + Enabled: append(defaults.ExpandedPluginsV1beta3.Filter.Enabled, config.Plugin{Name: "Foo"}), + }, + PreFilter: config.PluginSet{ + Enabled: append(defaults.ExpandedPluginsV1beta3.PreFilter.Enabled, config.Plugin{Name: "Foo"}), + }, + PostFilter: defaults.ExpandedPluginsV1beta3.PostFilter, + PreScore: defaults.ExpandedPluginsV1beta3.PreScore, + QueueSort: defaults.ExpandedPluginsV1beta3.QueueSort, + Score: defaults.ExpandedPluginsV1beta3.Score, + Reserve: defaults.ExpandedPluginsV1beta3.Reserve, + PreBind: defaults.ExpandedPluginsV1beta3.PreBind, + }, + }, + }, { name: "leader election CLI args, along with --config arg", flags: []string{ @@ -437,7 +529,7 @@ leaderElection: ctx, cancel := context.WithCancel(context.Background()) defer cancel() - _, sched, err := Setup(ctx, opts) + _, sched, err := Setup(ctx, opts, tc.registryOptions...) if err != nil { t.Fatal(err) } @@ -462,3 +554,29 @@ leaderElection: }) } } + +// Simulates an out-of-tree plugin. +type foo struct{} + +var _ framework.PreFilterPlugin = &foo{} +var _ framework.FilterPlugin = &foo{} + +func (*foo) Name() string { + return "Foo" +} + +func newFoo(_ runtime.Object, _ framework.Handle) (framework.Plugin, error) { + return &foo{}, nil +} + +func (*foo) PreFilter(_ context.Context, _ *framework.CycleState, _ *v1.Pod) (*framework.PreFilterResult, *framework.Status) { + return nil, nil +} + +func (*foo) PreFilterExtensions() framework.PreFilterExtensions { + return nil +} + +func (*foo) Filter(_ context.Context, _ *framework.CycleState, _ *v1.Pod, nodeInfo *framework.NodeInfo) *framework.Status { + return nil +} diff --git a/pkg/scheduler/framework/runtime/framework.go b/pkg/scheduler/framework/runtime/framework.go index aa449e7d6e3..e4d0fe3d49d 100644 --- a/pkg/scheduler/framework/runtime/framework.go +++ b/pkg/scheduler/framework/runtime/framework.go @@ -417,6 +417,37 @@ func getScoreWeights(f *frameworkImpl, pluginsMap map[string]framework.Plugin, p return nil } +type orderedSet struct { + set map[string]int + list []string + deletionCnt int +} + +func newOrderedSet() *orderedSet { + return &orderedSet{set: make(map[string]int)} +} + +func (os *orderedSet) insert(s string) { + if os.has(s) { + return + } + os.set[s] = len(os.list) + os.list = append(os.list, s) +} + +func (os *orderedSet) has(s string) bool { + _, found := os.set[s] + return found +} + +func (os *orderedSet) delete(s string) { + if i, found := os.set[s]; found { + delete(os.set, s) + os.list = append(os.list[:i-os.deletionCnt], os.list[i+1-os.deletionCnt:]...) + os.deletionCnt++ + } +} + func (f *frameworkImpl) expandMultiPointPlugins(profile *config.KubeSchedulerProfile, pluginsMap map[string]framework.Plugin) error { // initialize MultiPoint plugins for _, e := range f.getExtensionPoints(profile.Plugins) { @@ -424,9 +455,9 @@ func (f *frameworkImpl) expandMultiPointPlugins(profile *config.KubeSchedulerPro pluginType := plugins.Type().Elem() // build enabledSet of plugins already registered via normal extension points // to check double registration - enabledSet := sets.NewString() + enabledSet := newOrderedSet() for _, plugin := range e.plugins.Enabled { - enabledSet.Insert(plugin.Name) + enabledSet.insert(plugin.Name) } disabledSet := sets.NewString() @@ -440,8 +471,8 @@ func (f *frameworkImpl) expandMultiPointPlugins(profile *config.KubeSchedulerPro // track plugins enabled via multipoint separately from those enabled by specific extensions, // so that we can distinguish between double-registration and explicit overrides - multiPointEnabled := sets.NewString() - + multiPointEnabled := newOrderedSet() + overridePlugins := newOrderedSet() for _, ep := range profile.Plugins.MultiPoint.Enabled { pg, ok := pluginsMap[ep.Name] if !ok { @@ -463,23 +494,43 @@ func (f *frameworkImpl) expandMultiPointPlugins(profile *config.KubeSchedulerPro // the user intent is to override the default plugin or make some other explicit setting. // Either way, discard the MultiPoint value for this plugin. // This maintains expected behavior for overriding default plugins (see https://github.com/kubernetes/kubernetes/pull/99582) - if enabledSet.Has(ep.Name) { + if enabledSet.has(ep.Name) { + overridePlugins.insert(ep.Name) klog.InfoS("MultiPoint plugin is explicitly re-configured; overriding", "plugin", ep.Name) continue } // if this plugin is already registered via MultiPoint, then this is // a double registration and an error in the config. - if multiPointEnabled.Has(ep.Name) { + if multiPointEnabled.has(ep.Name) { return fmt.Errorf("plugin %q already registered as %q", ep.Name, pluginType.Name()) } // we only need to update the multipoint set, since we already have the specific extension set from above - multiPointEnabled.Insert(ep.Name) - - newPlugins := reflect.Append(plugins, reflect.ValueOf(pg)) - plugins.Set(newPlugins) + multiPointEnabled.insert(ep.Name) } + + // Reorder plugins. Here is the expected order: + // - part 1: overridePlugins. Their order stay intact as how they're specified in regular extension point. + // - part 2: multiPointEnabled - i.e., plugin defined in multipoint but not in regular extension point. + // - part 3: other plugins (excluded by part 1 & 2) in regular extension point. + newPlugins := reflect.New(reflect.TypeOf(e.slicePtr).Elem()).Elem() + // part 1 + for _, name := range enabledSet.list { + if overridePlugins.has(name) { + newPlugins = reflect.Append(newPlugins, reflect.ValueOf(pluginsMap[name])) + enabledSet.delete(name) + } + } + // part 2 + for _, name := range multiPointEnabled.list { + newPlugins = reflect.Append(newPlugins, reflect.ValueOf(pluginsMap[name])) + } + // part 3 + for _, name := range enabledSet.list { + newPlugins = reflect.Append(newPlugins, reflect.ValueOf(pluginsMap[name])) + } + plugins.Set(newPlugins) } return nil } diff --git a/pkg/scheduler/framework/runtime/framework_test.go b/pkg/scheduler/framework/runtime/framework_test.go index a1672979a68..ed03d631620 100644 --- a/pkg/scheduler/framework/runtime/framework_test.go +++ b/pkg/scheduler/framework/runtime/framework_test.go @@ -656,6 +656,44 @@ func TestNewFrameworkMultiPointExpansion(t *testing.T) { PostBind: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, }, }, + { + name: "Reorder MultiPoint plugins (specified extension only takes precedence when it exists in MultiPoint)", + plugins: &config.Plugins{ + MultiPoint: config.PluginSet{ + Enabled: []config.Plugin{ + {Name: testPlugin}, + {Name: scorePlugin1}, + }, + }, + Score: config.PluginSet{ + Enabled: []config.Plugin{ + {Name: scoreWithNormalizePlugin1}, + {Name: scorePlugin1}, + {Name: testPlugin}, + }, + }, + }, + wantPlugins: &config.Plugins{ + QueueSort: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + PreFilter: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + Filter: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + PostFilter: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + PreScore: config.PluginSet{Enabled: []config.Plugin{ + {Name: testPlugin}, + {Name: scorePlugin1}, + }}, + Score: config.PluginSet{Enabled: []config.Plugin{ + {Name: scorePlugin1, Weight: 1}, + {Name: testPlugin, Weight: 1}, + {Name: scoreWithNormalizePlugin1, Weight: 1}, + }}, + Reserve: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + Permit: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + PreBind: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + Bind: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + PostBind: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}}, + }, + }, { name: "Override MultiPoint plugins weights", plugins: &config.Plugins{ diff --git a/test/integration/scheduler/framework_test.go b/test/integration/scheduler/framework_test.go index a34912230a7..ef1d82903c0 100644 --- a/test/integration/scheduler/framework_test.go +++ b/test/integration/scheduler/framework_test.go @@ -1420,7 +1420,7 @@ func TestUnReserveBindPlugins(t *testing.T) { } if test.plugin.numBindCalled != 1 { - t.Errorf("Expected the Prebind plugin to be called.") + t.Errorf("Expected the Bind plugin to be called.") } testutils.CleanupPods(testCtx.ClientSet, t, []*v1.Pod{pod}) @@ -2448,6 +2448,9 @@ func initRegistryAndConfig(t *testing.T, plugins ...framework.Plugin) (framework pls.PreBind.Enabled = append(pls.PreBind.Enabled, plugin) case *BindPlugin: pls.Bind.Enabled = append(pls.Bind.Enabled, plugin) + // It's intentional to disable the DefaultBind plugin. Otherwise, DefaultBinder's failure would fail + // a pod's scheduling, as well as the test BindPlugin's execution. + pls.Bind.Disabled = []v1beta3.Plugin{{Name: defaultbinder.Name}} case *PostBindPlugin: pls.PostBind.Enabled = append(pls.PostBind.Enabled, plugin) case *PermitPlugin: