Merge pull request #108613 from Huang-Wei/fix-v1beta3-order

Fix a bug that out-of-tree plugin is misplaced when using scheduler v1beta3 config

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
+}

pkg/scheduler/framework/runtime/framework.go

@@ -403,6 +403,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) {
@@ -410,9 +441,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()
@@ -426,8 +457,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 {
@@ -449,23 +480,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)
+			multiPointEnabled.insert(ep.Name)
-
-			newPlugins := reflect.Append(plugins, reflect.ValueOf(pg))
-			plugins.Set(newPlugins)
 		}
+
+		// 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
 }

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{

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: