Merge pull request #88870 from alculquicondor/disallow_dup_plugin_config

Disallow duplicate PluginConfig in framework creation

pkg/scheduler/BUILD

@@ -46,6 +46,7 @@ go_library(
         "//staging/src/k8s.io/client-go/listers/core/v1:go_default_library",
         "//staging/src/k8s.io/client-go/listers/policy/v1beta1:go_default_library",
         "//staging/src/k8s.io/client-go/tools/cache:go_default_library",
+        "//vendor/github.com/google/go-cmp/cmp:go_default_library",
         "//vendor/k8s.io/klog:go_default_library",
     ],
 )

pkg/scheduler/factory.go

@@ -23,10 +23,12 @@ import (
 	"sort"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -312,9 +314,10 @@ func (c *Configurator) createFromConfig(policy schedulerapi.Policy) (*Scheduler,
 	})
 	defPlugins.Append(pluginsForPredicates)
 	defPlugins.Append(pluginsForPriorities)
-	var defPluginConfig []schedulerapi.PluginConfig
-	defPluginConfig = append(defPluginConfig, pluginConfigForPredicates...)
-	defPluginConfig = append(defPluginConfig, pluginConfigForPriorities...)
+	defPluginConfig, err := mergePluginConfigsFromPolicy(pluginConfigForPredicates, pluginConfigForPriorities)
+	if err != nil {
+		return nil, err
+	}
 	for i := range c.profiles {
 		prof := &c.profiles[i]
 		if prof.Plugins != nil {
@@ -332,6 +335,30 @@ func (c *Configurator) createFromConfig(policy schedulerapi.Policy) (*Scheduler,
 	return c.create()
 }
 
+// mergePluginConfigsFromPolicy merges the giving plugin configs ensuring that,
+// if a plugin name is repeated, the arguments are the same.
+func mergePluginConfigsFromPolicy(pc1, pc2 []schedulerapi.PluginConfig) ([]schedulerapi.PluginConfig, error) {
+	args := make(map[string]runtime.Unknown)
+	for _, c := range pc1 {
+		args[c.Name] = c.Args
+	}
+	for _, c := range pc2 {
+		if v, ok := args[c.Name]; ok && !cmp.Equal(v, c.Args) {
+			// This should be unreachable.
+			return nil, fmt.Errorf("inconsistent configuration produced for plugin %s", c.Name)
+		}
+		args[c.Name] = c.Args
+	}
+	pc := make([]schedulerapi.PluginConfig, 0, len(args))
+	for k, v := range args {
+		pc = append(pc, schedulerapi.PluginConfig{
+			Name: k,
+			Args: v,
+		})
+	}
+	return pc, nil
+}
+
 // getPriorityConfigs returns priorities configuration: ones that will run as priorities and ones that will run
 // as framework plugins. Specifically, a priority will run as a framework plugin if a plugin config producer was
 // registered for that priority.

pkg/scheduler/framework/v1alpha1/framework.go

@@ -202,7 +202,11 @@ func NewFramework(r Registry, plugins *config.Plugins, args []config.PluginConfi
 
 	pluginConfig := make(map[string]*runtime.Unknown, 0)
 	for i := range args {
-		pluginConfig[args[i].Name] = &args[i].Args
+		name := args[i].Name
+		if _, ok := pluginConfig[name]; ok {
+			return nil, fmt.Errorf("repeated config for plugin %s", name)
+		}
+		pluginConfig[name] = &args[i].Args
 	}
 
 	pluginsMap := make(map[string]Plugin)

pkg/scheduler/framework/v1alpha1/framework_test.go

@@ -410,25 +410,52 @@ func TestInitFrameworkWithScorePlugins(t *testing.T) {
 	}
 }
 
-func TestRegisterDuplicatePluginWouldFail(t *testing.T) {
-	plugin := config.Plugin{Name: duplicatePluginName, Weight: 1}
-
-	pluginSet := config.PluginSet{
-		Enabled: []config.Plugin{
-			plugin,
-			plugin,
+func TestNewFrameworkErrors(t *testing.T) {
+	tests := []struct {
+		name      string
+		plugins   *config.Plugins
+		pluginCfg []config.PluginConfig
+		wantErr   string
+	}{
+		{
+			name: "duplicate plugin name",
+			plugins: &config.Plugins{
+				PreFilter: &config.PluginSet{
+					Enabled: []config.Plugin{
+						{Name: duplicatePluginName, Weight: 1},
+						{Name: duplicatePluginName, Weight: 1},
+					},
+				},
+			},
+			pluginCfg: []config.PluginConfig{
+				{Name: duplicatePluginName},
+			},
+			wantErr: "already registered",
+		},
+		{
+			name: "duplicate plugin config",
+			plugins: &config.Plugins{
+				PreFilter: &config.PluginSet{
+					Enabled: []config.Plugin{
+						{Name: duplicatePluginName, Weight: 1},
+					},
+				},
+			},
+			pluginCfg: []config.PluginConfig{
+				{Name: duplicatePluginName},
+				{Name: duplicatePluginName},
+			},
+			wantErr: "repeated config for plugin",
 		},
 	}
-	plugins := config.Plugins{}
-	plugins.PreFilter = &pluginSet
 
-	_, err := NewFramework(registry, &plugins, emptyArgs)
-	if err == nil {
-		t.Fatal("Framework initialization should fail")
-	}
-
-	if err != nil && !strings.Contains(err.Error(), "already registered") {
-		t.Fatalf("Unexpected error, got %s, expect: plugin already registered", err.Error())
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := NewFramework(registry, tc.plugins, tc.pluginCfg)
+			if err == nil || !strings.Contains(err.Error(), tc.wantErr) {
+				t.Errorf("Unexpected error, got %v, expect: %s", err, tc.wantErr)
+			}
+		})
 	}
 }