Merge pull request #84795 from ahg-g/ahg-custom

Convert multiple node label predicates to be a single filter plugin

pkg/scheduler/algorithm/predicates/predicates.go

@@ -940,16 +940,18 @@ func PodFitsHost(pod *v1.Pod, meta PredicateMetadata, nodeInfo *schedulernodeinf
 
 // NodeLabelChecker contains information to check node labels for a predicate.
 type NodeLabelChecker struct {
-	labels   []string
-	presence bool
+	// presentLabels should be present for the node to be considered a fit for hosting the pod
+	presentLabels []string
+	// absentLabels should be absent for the node to be considered a fit for hosting the pod
+	absentLabels []string
 }
 
 // NewNodeLabelPredicate creates a predicate which evaluates whether a pod can fit based on the
 // node labels which match a filter that it requests.
-func NewNodeLabelPredicate(labels []string, presence bool) FitPredicate {
+func NewNodeLabelPredicate(presentLabels []string, absentLabels []string) FitPredicate {
 	labelChecker := &NodeLabelChecker{
-		labels:   labels,
-		presence: presence,
+		presentLabels: presentLabels,
+		absentLabels:  absentLabels,
 	}
 	return labelChecker.CheckNodeLabelPresence
 }
@@ -972,15 +974,21 @@ func (n *NodeLabelChecker) CheckNodeLabelPresence(pod *v1.Pod, meta PredicateMet
 		return false, nil, fmt.Errorf("node not found")
 	}
 
-	var exists bool
 	nodeLabels := labels.Set(node.Labels)
-	for _, label := range n.labels {
-		exists = nodeLabels.Has(label)
-		if (exists && !n.presence) || (!exists && n.presence) {
-			return false, []PredicateFailureReason{ErrNodeLabelPresenceViolated}, nil
+	check := func(labels []string, presence bool) bool {
+		for _, label := range labels {
+			exists := nodeLabels.Has(label)
+			if (exists && !presence) || (!exists && presence) {
+				return false
+			}
 		}
+		return true
 	}
-	return true, nil, nil
+	if check(n.presentLabels, true) && check(n.absentLabels, false) {
+		return true, nil, nil
+	}
+
+	return false, []PredicateFailureReason{ErrNodeLabelPresenceViolated}, nil
 }
 
 // ServiceAffinity defines a struct used for creating service affinity predicates.

pkg/scheduler/algorithm/predicates/predicates_test.go

@@ -1658,47 +1658,41 @@ func TestPodFitsSelector(t *testing.T) {
 func TestNodeLabelPresence(t *testing.T) {
 	label := map[string]string{"foo": "bar", "bar": "foo"}
 	tests := []struct {
-		pod      *v1.Pod
-		labels   []string
-		presence bool
-		fits     bool
-		name     string
+		pod           *v1.Pod
+		presentLabels []string
+		absentLabels  []string
+		fits          bool
+		name          string
 	}{
 		{
-			labels:   []string{"baz"},
-			presence: true,
-			fits:     false,
-			name:     "label does not match, presence true",
+			presentLabels: []string{"baz"},
+			fits:          false,
+			name:          "label does not match, presence true",
 		},
 		{
-			labels:   []string{"baz"},
-			presence: false,
-			fits:     true,
-			name:     "label does not match, presence false",
+			absentLabels: []string{"baz"},
+			fits:         true,
+			name:         "label does not match, presence false",
 		},
 		{
-			labels:   []string{"foo", "baz"},
-			presence: true,
-			fits:     false,
-			name:     "one label matches, presence true",
+			presentLabels: []string{"foo", "baz"},
+			fits:          false,
+			name:          "one label matches, presence true",
 		},
 		{
-			labels:   []string{"foo", "baz"},
-			presence: false,
-			fits:     false,
-			name:     "one label matches, presence false",
+			absentLabels: []string{"foo", "baz"},
+			fits:         false,
+			name:         "one label matches, presence false",
 		},
 		{
-			labels:   []string{"foo", "bar"},
-			presence: true,
-			fits:     true,
-			name:     "all labels match, presence true",
+			presentLabels: []string{"foo", "bar"},
+			fits:          true,
+			name:          "all labels match, presence true",
 		},
 		{
-			labels:   []string{"foo", "bar"},
-			presence: false,
-			fits:     false,
-			name:     "all labels match, presence false",
+			absentLabels: []string{"foo", "bar"},
+			fits:         false,
+			name:         "all labels match, presence false",
 		},
 	}
 	expectedFailureReasons := []PredicateFailureReason{ErrNodeLabelPresenceViolated}
@@ -1709,7 +1703,7 @@ func TestNodeLabelPresence(t *testing.T) {
 			nodeInfo := schedulernodeinfo.NewNodeInfo()
 			nodeInfo.SetNode(&node)
 
-			labelChecker := NodeLabelChecker{test.labels, test.presence}
+			labelChecker := NodeLabelChecker{test.presentLabels, test.absentLabels}
 			fits, reasons, err := labelChecker.CheckNodeLabelPresence(test.pod, GetPredicateMetadata(test.pod, nil), nodeInfo)
 			if err != nil {
 				t.Errorf("unexpected error: %v", err)

pkg/scheduler/algorithm_factory.go

@@ -287,17 +287,23 @@ func RegisterCustomFitPredicate(policy schedulerapi.PredicatePolicy, args *plugi
 			}
 		} else if policy.Argument.LabelsPresence != nil {
 			// map LabelPresence policy to ConfigProducerArgs that's used to configure the NodeLabel plugin.
-			args.NodeLabelArgs = &nodelabel.Args{
-				Labels:   policy.Argument.LabelsPresence.Labels,
-				Presence: policy.Argument.LabelsPresence.Presence,
+			if args.NodeLabelArgs == nil {
+				args.NodeLabelArgs = &nodelabel.Args{}
 			}
-			predicateFactory = func(args PluginFactoryArgs) predicates.FitPredicate {
+			if policy.Argument.LabelsPresence.Presence {
+				args.NodeLabelArgs.PresentLabels = append(args.NodeLabelArgs.PresentLabels, policy.Argument.LabelsPresence.Labels...)
+			} else {
+				args.NodeLabelArgs.AbsentLabels = append(args.NodeLabelArgs.AbsentLabels, policy.Argument.LabelsPresence.Labels...)
+			}
+			predicateFactory = func(_ PluginFactoryArgs) predicates.FitPredicate {
 				return predicates.NewNodeLabelPredicate(
-					policy.Argument.LabelsPresence.Labels,
-					policy.Argument.LabelsPresence.Presence,
+					args.NodeLabelArgs.PresentLabels,
+					args.NodeLabelArgs.AbsentLabels,
 				)
 			}
-			// We do not allow specifying the name for custom plugins, see #83472
+			// We use the NodeLabel plugin name for all NodeLabel custom priorities.
+			// It may get called multiple times but we essentially only register one instance of NodeLabel predicate.
+			// This name is then used to find the registered plugin and run the plugin instead of the predicate.
 			name = nodelabel.Name
 		}
 	} else if predicateFactory, ok = fitPredicateMap[policy.Name]; ok {

pkg/scheduler/factory_test.go

@@ -92,6 +92,7 @@ func TestCreateFromConfig(t *testing.T) {
 		"predicates" : [
 			{"name" : "TestZoneAffinity", "argument" : {"serviceAffinity" : {"labels" : ["zone"]}}},
 			{"name" : "TestRequireZone", "argument" : {"labelsPresence" : {"labels" : ["zone"], "presence" : true}}},
+			{"name" : "TestNoFooLabel", "argument" : {"labelsPresence" : {"labels" : ["foo"], "presence" : false}}},
 			{"name" : "PredicateOne"},
 			{"name" : "PredicateTwo"}
 		],
@@ -123,7 +124,7 @@ func TestCreateFromConfig(t *testing.T) {
 	if err != nil {
 		t.Errorf("Failed to marshal %+v: %v", nodeLabelConfig, err)
 	}
-	want := `{"Name":"NodeLabel","Args":{"labels":["zone"],"presence":true}}`
+	want := `{"Name":"NodeLabel","Args":{"presentLabels":["zone"],"absentLabels":["foo"]}}`
 	if string(encoding) != want {
 		t.Errorf("Config for NodeLabel plugin mismatch. got: %v, want: %v", string(encoding), want)
 	}

pkg/scheduler/framework/plugins/nodelabel/node_label.go

@@ -18,6 +18,7 @@ package nodelabel
 
 import (
 	"context"
+	"fmt"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -32,11 +33,24 @@ const Name = "NodeLabel"
 
 // Args holds the args that are used to configure the plugin.
 type Args struct {
-	// The list of labels that identify node "groups"
-	// All of the labels should be either present (or absent) for the node to be considered a fit for hosting the pod
-	Labels []string `json:"labels,omitempty"`
-	// The boolean flag that indicates whether the labels should be present or absent from the node
-	Presence bool `json:"presence,omitempty"`
+	// PresentLabels should be present for the node to be considered a fit for hosting the pod
+	PresentLabels []string `json:"presentLabels,omitempty"`
+	// AbsentLabels should be absent for the node to be considered a fit for hosting the pod
+	AbsentLabels []string `json:"absentLabels,omitempty"`
+}
+
+// validateArgs validates that PresentLabels and AbsentLabels do not conflict.
+func validateArgs(args *Args) error {
+	presentLabels := make(map[string]struct{}, len(args.PresentLabels))
+	for _, l := range args.PresentLabels {
+		presentLabels[l] = struct{}{}
+	}
+	for _, l := range args.AbsentLabels {
+		if _, ok := presentLabels[l]; ok {
+			return fmt.Errorf("detecting at least one label (e.g., %q) that exist in both the present and absent label list: %+v", l, args)
+		}
+	}
+	return nil
 }
 
 // New initializes a new plugin and returns it.
@@ -45,8 +59,11 @@ func New(plArgs *runtime.Unknown, _ framework.FrameworkHandle) (framework.Plugin
 	if err := framework.DecodeInto(plArgs, args); err != nil {
 		return nil, err
 	}
+	if err := validateArgs(args); err != nil {
+		return nil, err
+	}
 	return &NodeLabel{
-		predicate: predicates.NewNodeLabelPredicate(args.Labels, args.Presence),
+		predicate: predicates.NewNodeLabelPredicate(args.PresentLabels, args.AbsentLabels),
 	}, nil
 }
 

pkg/scheduler/framework/plugins/nodelabel/node_label_test.go

@@ -27,42 +27,71 @@ import (
 	schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
 )
 
-func TestNodeLabelPresence(t *testing.T) {
-	label := map[string]string{"foo": "bar", "bar": "foo"}
+func TestValidateNodeLabelArgs(t *testing.T) {
+	// "bar" exists in both present and absent labels therefore validatio should fail.
+	args := &runtime.Unknown{Raw: []byte(`{"presentLabels" : ["foo", "bar"], "absentLabels" : ["bar", "baz"]}`)}
+	_, err := New(args, nil)
+	if err == nil {
+		t.Fatal("Plugin initialization should fail.")
+	}
+}
+
+func TestNodeLabelFilter(t *testing.T) {
+	label := map[string]string{"foo": "any value", "bar": "any value"}
+	var pod *v1.Pod
 	tests := []struct {
 		name    string
-		pod     *v1.Pod
 		rawArgs string
 		res     framework.Code
 	}{
 		{
-			name:    "label does not match, presence true",
-			rawArgs: `{"labels" : ["baz"], "presence" : true}`,
+			name:    "present label does not match",
+			rawArgs: `{"presentLabels" : ["baz"]}`,
 			res:     framework.UnschedulableAndUnresolvable,
 		},
 		{
-			name:    "label does not match, presence false",
-			rawArgs: `{"labels" : ["baz"], "presence" : false}`,
+			name:    "absent label does not match",
+			rawArgs: `{"absentLabels" : ["baz"]}`,
 			res:     framework.Success,
 		},
 		{
-			name:    "one label matches, presence true",
-			rawArgs: `{"labels" : ["foo", "baz"], "presence" : true}`,
+			name:    "one of two present labels matches",
+			rawArgs: `{"presentLabels" : ["foo", "baz"]}`,
 			res:     framework.UnschedulableAndUnresolvable,
 		},
 		{
-			name:    "one label matches, presence false",
-			rawArgs: `{"labels" : ["foo", "baz"], "presence" : false}`,
+			name:    "one of two absent labels matches",
+			rawArgs: `{"absentLabels" : ["foo", "baz"]}`,
 			res:     framework.UnschedulableAndUnresolvable,
 		},
 		{
-			name:    "all labels match, presence true",
-			rawArgs: `{"labels" : ["foo", "bar"], "presence" : true}`,
+			name:    "all present abels match",
+			rawArgs: `{"presentLabels" : ["foo", "bar"]}`,
 			res:     framework.Success,
 		},
 		{
-			name:    "all labels match, presence false",
-			rawArgs: `{"labels" : ["foo", "bar"], "presence" : false}`,
+			name:    "all absent labels match",
+			rawArgs: `{"absentLabels" : ["foo", "bar"]}`,
+			res:     framework.UnschedulableAndUnresolvable,
+		},
+		{
+			name:    "both present and absent label matches",
+			rawArgs: `{"presentLabels" : ["foo"], "absentLabels" : ["bar"]}`,
+			res:     framework.UnschedulableAndUnresolvable,
+		},
+		{
+			name:    "neither present nor absent label matches",
+			rawArgs: `{"presentLabels" : ["foz"], "absentLabels" : ["baz"]}`,
+			res:     framework.UnschedulableAndUnresolvable,
+		},
+		{
+			name:    "present label matches and absent label doesn't match",
+			rawArgs: `{"presentLabels" : ["foo"], "absentLabels" : ["baz"]}`,
+			res:     framework.Success,
+		},
+		{
+			name:    "present label doesn't match and absent label matches",
+			rawArgs: `{"presentLabels" : ["foz"], "absentLabels" : ["bar"]}`,
 			res:     framework.UnschedulableAndUnresolvable,
 		},
 	}
@@ -79,7 +108,7 @@ func TestNodeLabelPresence(t *testing.T) {
 				t.Fatalf("Failed to create plugin: %v", err)
 			}
 
-			status := p.(framework.FilterPlugin).Filter(context.TODO(), nil, test.pod, nodeInfo)
+			status := p.(framework.FilterPlugin).Filter(context.TODO(), nil, pod, nodeInfo)
 			if status.Code() != test.res {
 				t.Errorf("Status mismatch. got: %v, want: %v", status.Code(), test.res)
 			}

test/integration/scheduler_perf/scheduler_bench_test.go

@@ -48,7 +48,7 @@ var (
 
 // BenchmarkScheduling benchmarks the scheduling rate when the cluster has
 // various quantities of nodes and scheduled pods.
-func BenchmarkScheduling(b *testing.B) {
+func BenchmarkSchedulingV(b *testing.B) {
 	tests := []struct{ nodes, existingPods, minPods int }{
 		{nodes: 100, existingPods: 0, minPods: 100},
 		{nodes: 100, existingPods: 1000, minPods: 100},