Move predicates and priorities configuration creation to Policy mapping

For separation of concerns. Also make the producers receive the
existing configuration so they can take decisions based on them.

Change-Id: Id5db5588aabb90e7b8f087bd3f3c967e86dc4acc

Author: alculquicondor

pkg/scheduler/factory.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"sort"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
@@ -268,138 +267,56 @@ func (c *Configurator) createFromConfig(policy schedulerapi.Policy) (*Scheduler,
 
 	klog.V(2).Infof("Creating scheduler with fit predicates '%v' and priority functions '%v'", predicateKeys, priorityKeys)
 
-	pluginsForPredicates, pluginConfigForPredicates, err := getPredicateConfigs(predicateKeys, lr, args)
-	if err != nil {
-		return nil, err
-	}
-
-	pluginsForPriorities, pluginConfigForPriorities, err := getPriorityConfigs(priorityKeys, lr, args)
-	if err != nil {
-		return nil, err
-	}
 	// Combine all framework configurations. If this results in any duplication, framework
 	// instantiation should fail.
-	var defPlugins schedulerapi.Plugins
+
 	// "PrioritySort" and "DefaultBinder" were neither predicates nor priorities
 	// before. We add them by default.
-	defPlugins.Append(&schedulerapi.Plugins{
+	plugins := schedulerapi.Plugins{
 		QueueSort: &schedulerapi.PluginSet{
 			Enabled: []schedulerapi.Plugin{{Name: queuesort.Name}},
 		},
 		Bind: &schedulerapi.PluginSet{
 			Enabled: []schedulerapi.Plugin{{Name: defaultbinder.Name}},
 		},
-	})
-	defPlugins.Append(pluginsForPredicates)
-	defPlugins.Append(pluginsForPriorities)
-	defPluginConfig, err := mergePluginConfigsFromPolicy(pluginConfigForPredicates, pluginConfigForPriorities)
-	if err != nil {
+	}
+	var pluginConfig []schedulerapi.PluginConfig
+	var err error
+	if plugins, pluginConfig, err = lr.AppendPredicateConfigs(predicateKeys, args, plugins, pluginConfig); err != nil {
+		return nil, err
+	}
+	if plugins, pluginConfig, err = lr.AppendPriorityConfigs(priorityKeys, args, plugins, pluginConfig); err != nil {
+		return nil, err
+	}
+	if pluginConfig, err = dedupPluginConfigs(pluginConfig); err != nil {
 		return nil, err
 	}
 	for i := range c.profiles {
 		prof := &c.profiles[i]
-		// Plugins are empty when using Policy.
+		// Plugins and PluginConfig are empty when using Policy; overriding.
 		prof.Plugins = &schedulerapi.Plugins{}
-		prof.Plugins.Append(&defPlugins)
-
-		// PluginConfig is ignored when using Policy.
-		prof.PluginConfig = defPluginConfig
+		prof.Plugins.Append(&plugins)
+		prof.PluginConfig = pluginConfig
 	}
 
 	return c.create()
 }
 
-// mergePluginConfigsFromPolicy merges the giving plugin configs ensuring that,
+// dedupPluginConfigs removes duplicates from pluginConfig, ensuring that,
 // if a plugin name is repeated, the arguments are the same.
-func mergePluginConfigsFromPolicy(pc1, pc2 []schedulerapi.PluginConfig) ([]schedulerapi.PluginConfig, error) {
+func dedupPluginConfigs(pc []schedulerapi.PluginConfig) ([]schedulerapi.PluginConfig, error) {
 	args := make(map[string]runtime.Object)
-	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) {
+	result := make([]schedulerapi.PluginConfig, 0, len(pc))
+	for _, c := range pc {
+		if v, found := args[c.Name]; !found {
+			result = append(result, c)
+			args[c.Name] = c.Args
+		} else if !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.
-func getPriorityConfigs(keys map[string]int64, lr *frameworkplugins.LegacyRegistry, args *frameworkplugins.ConfigProducerArgs) (*schedulerapi.Plugins, []schedulerapi.PluginConfig, error) {
-	var plugins schedulerapi.Plugins
-	var pluginConfig []schedulerapi.PluginConfig
-
-	// Sort the keys so that it is easier for unit tests to do compare.
-	var sortedKeys []string
-	for k := range keys {
-		sortedKeys = append(sortedKeys, k)
-	}
-	sort.Strings(sortedKeys)
-
-	for _, priority := range sortedKeys {
-		weight := keys[priority]
-		producer, exist := lr.PriorityToConfigProducer[priority]
-		if !exist {
-			return nil, nil, fmt.Errorf("no config producer registered for %q", priority)
-		}
-		a := *args
-		a.Weight = int32(weight)
-		pl, plc := producer(a)
-		plugins.Append(&pl)
-		pluginConfig = append(pluginConfig, plc...)
-	}
-	return &plugins, pluginConfig, nil
-}
-
-// getPredicateConfigs returns predicates configuration: ones that will run as fitPredicates and ones that will run
-// as framework plugins. Specifically, a predicate will run as a framework plugin if a plugin config producer was
-// registered for that predicate.
-// Note that the framework executes plugins according to their order in the Plugins list, and so predicates run as plugins
-// are added to the Plugins list according to the order specified in predicates.Ordering().
-func getPredicateConfigs(keys sets.String, lr *frameworkplugins.LegacyRegistry, args *frameworkplugins.ConfigProducerArgs) (*schedulerapi.Plugins, []schedulerapi.PluginConfig, error) {
-	allPredicates := keys.Union(lr.MandatoryPredicates)
-
-	// Create the framework plugin configurations, and place them in the order
-	// that the corresponding predicates were supposed to run.
-	var plugins schedulerapi.Plugins
-	var pluginConfig []schedulerapi.PluginConfig
-
-	for _, predicateKey := range frameworkplugins.PredicateOrdering() {
-		if allPredicates.Has(predicateKey) {
-			producer, exist := lr.PredicateToConfigProducer[predicateKey]
-			if !exist {
-				return nil, nil, fmt.Errorf("no framework config producer registered for %q", predicateKey)
-			}
-			pl, plc := producer(*args)
-			plugins.Append(&pl)
-			pluginConfig = append(pluginConfig, plc...)
-			allPredicates.Delete(predicateKey)
-		}
-	}
-
-	// Third, add the rest in no specific order.
-	for predicateKey := range allPredicates {
-		producer, exist := lr.PredicateToConfigProducer[predicateKey]
-		if !exist {
-			return nil, nil, fmt.Errorf("no framework config producer registered for %q", predicateKey)
-		}
-		pl, plc := producer(*args)
-		plugins.Append(&pl)
-		pluginConfig = append(pluginConfig, plc...)
-	}
-
-	return &plugins, pluginConfig, nil
+	return result, nil
 }
 
 // MakeDefaultErrorFunc construct a function to handle pod scheduler error

pkg/scheduler/framework/plugins/BUILD

@@ -80,6 +80,9 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//pkg/scheduler/apis/config:go_default_library",
+        "//pkg/scheduler/framework/plugins/nodeunschedulable:go_default_library",
+        "//pkg/scheduler/framework/plugins/tainttoleration:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/github.com/google/go-cmp/cmp:go_default_library",
     ],
 )