From 13fa48e5924b866cec3e965a0c540f6296781cd1 Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Thu, 5 Mar 2020 13:52:40 -0500 Subject: [PATCH] Disallow duplicate PluginConfig in framework creation Signed-off-by: Aldo Culquicondor --- pkg/scheduler/BUILD | 1 + pkg/scheduler/factory.go | 33 ++++++++++- pkg/scheduler/framework/v1alpha1/framework.go | 6 +- .../framework/v1alpha1/framework_test.go | 59 ++++++++++++++----- 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/pkg/scheduler/BUILD b/pkg/scheduler/BUILD index e21840656d7..f9904c2931a 100644 --- a/pkg/scheduler/BUILD +++ b/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", ], ) diff --git a/pkg/scheduler/factory.go b/pkg/scheduler/factory.go index eb99eacbec7..5010eef7e51 100644 --- a/pkg/scheduler/factory.go +++ b/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" @@ -296,9 +298,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] plugins := &schedulerapi.Plugins{} @@ -315,6 +318,30 @@ func (c *Configurator) createFromConfig(policy schedulerapi.Policy) (*Scheduler, return c.create(extenders) } +// 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. diff --git a/pkg/scheduler/framework/v1alpha1/framework.go b/pkg/scheduler/framework/v1alpha1/framework.go index ededc1b091b..4924a51db2a 100644 --- a/pkg/scheduler/framework/v1alpha1/framework.go +++ b/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) diff --git a/pkg/scheduler/framework/v1alpha1/framework_test.go b/pkg/scheduler/framework/v1alpha1/framework_test.go index 440760a62c4..c88b77360e6 100644 --- a/pkg/scheduler/framework/v1alpha1/framework_test.go +++ b/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) + } + }) } }