move NodeLabel predicate logic to its Filter plugin

Author: ahg-g

pkg/scheduler/algorithm/predicates/predicates.go

@@ -939,59 +939,6 @@ func PodFitsHost(pod *v1.Pod, meta Metadata, nodeInfo *schedulernodeinfo.NodeInf
 	return false, []PredicateFailureReason{ErrPodNotMatchHostName}, nil
 }
 
-// NodeLabelChecker contains information to check node labels for a predicate.
-type NodeLabelChecker struct {
-	// 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(presentLabels []string, absentLabels []string) FitPredicate {
-	labelChecker := &NodeLabelChecker{
-		presentLabels: presentLabels,
-		absentLabels:  absentLabels,
-	}
-	return labelChecker.CheckNodeLabelPresence
-}
-
-// CheckNodeLabelPresence checks whether all of the specified labels exists on a node or not, regardless of their value
-// If "presence" is false, then returns false if any of the requested labels matches any of the node's labels,
-// otherwise returns true.
-// If "presence" is true, then returns false if any of the requested labels does not match any of the node's labels,
-// otherwise returns true.
-//
-// Consider the cases where the nodes are placed in regions/zones/racks and these are identified by labels
-// In some cases, it is required that only nodes that are part of ANY of the defined regions/zones/racks be selected
-//
-// Alternately, eliminating nodes that have a certain label, regardless of value, is also useful
-// A node may have a label with "retiring" as key and the date as the value
-// and it may be desirable to avoid scheduling new pods on this node.
-func (n *NodeLabelChecker) CheckNodeLabelPresence(pod *v1.Pod, meta Metadata, nodeInfo *schedulernodeinfo.NodeInfo) (bool, []PredicateFailureReason, error) {
-	node := nodeInfo.Node()
-	if node == nil {
-		return false, nil, fmt.Errorf("node not found")
-	}
-
-	nodeLabels := labels.Set(node.Labels)
-	check := func(labels []string, presence bool) bool {
-		for _, label := range labels {
-			exists := nodeLabels.Has(label)
-			if (exists && !presence) || (!exists && presence) {
-				return false
-			}
-		}
-		return true
-	}
-	if check(n.presentLabels, true) && check(n.absentLabels, false) {
-		return true, nil, nil
-	}
-
-	return false, []PredicateFailureReason{ErrNodeLabelPresenceViolated}, nil
-}
-
 // PodFitsHostPorts is a wrapper around PodFitsHostPortsPredicate. This is needed until
 // we are able to get rid of the FitPredicate function signature.
 // TODO(#85822): remove this function once predicate registration logic is deleted.

pkg/scheduler/algorithm/predicates/predicates_test.go

@@ -1651,69 +1651,6 @@ func TestPodFitsSelector(t *testing.T) {
 	}
 }
 
-func TestNodeLabelPresence(t *testing.T) {
-	label := map[string]string{"foo": "bar", "bar": "foo"}
-	tests := []struct {
-		pod           *v1.Pod
-		presentLabels []string
-		absentLabels  []string
-		fits          bool
-		name          string
-	}{
-		{
-			presentLabels: []string{"baz"},
-			fits:          false,
-			name:          "label does not match, presence true",
-		},
-		{
-			absentLabels: []string{"baz"},
-			fits:         true,
-			name:         "label does not match, presence false",
-		},
-		{
-			presentLabels: []string{"foo", "baz"},
-			fits:          false,
-			name:          "one label matches, presence true",
-		},
-		{
-			absentLabels: []string{"foo", "baz"},
-			fits:         false,
-			name:         "one label matches, presence false",
-		},
-		{
-			presentLabels: []string{"foo", "bar"},
-			fits:          true,
-			name:          "all labels match, presence true",
-		},
-		{
-			absentLabels: []string{"foo", "bar"},
-			fits:         false,
-			name:         "all labels match, presence false",
-		},
-	}
-	expectedFailureReasons := []PredicateFailureReason{ErrNodeLabelPresenceViolated}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			node := v1.Node{ObjectMeta: metav1.ObjectMeta{Labels: label}}
-			nodeInfo := schedulernodeinfo.NewNodeInfo()
-			nodeInfo.SetNode(&node)
-
-			labelChecker := NodeLabelChecker{test.presentLabels, test.absentLabels}
-			fits, reasons, err := labelChecker.CheckNodeLabelPresence(test.pod, nil, nodeInfo)
-			if err != nil {
-				t.Errorf("unexpected error: %v", err)
-			}
-			if !fits && !reflect.DeepEqual(reasons, expectedFailureReasons) {
-				t.Errorf("unexpected failure reasons: %v, want: %v", reasons, expectedFailureReasons)
-			}
-			if fits != test.fits {
-				t.Errorf("expected: %v got %v", test.fits, fits)
-			}
-		})
-	}
-}
-
 func newPodWithPort(hostPorts ...int) *v1.Pod {
 	networkPorts := []v1.ContainerPort{}
 	for _, port := range hostPorts {

pkg/scheduler/algorithm_factory.go

@@ -278,10 +278,7 @@ func RegisterCustomFitPredicate(policy schedulerapi.PredicatePolicy, pluginArgs
 				pluginArgs.NodeLabelArgs.AbsentLabels = append(pluginArgs.NodeLabelArgs.AbsentLabels, policy.Argument.LabelsPresence.Labels...)
 			}
 			predicateFactory = func(_ AlgorithmFactoryArgs) predicates.FitPredicate {
-				return predicates.NewNodeLabelPredicate(
-					pluginArgs.NodeLabelArgs.PresentLabels,
-					pluginArgs.NodeLabelArgs.AbsentLabels,
-				)
+				return nil
 			}
 		}
 	} else if predicateFactory, ok = fitPredicateMap[policyName]; ok {

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

@@ -71,16 +71,14 @@ func New(plArgs *runtime.Unknown, handle framework.FrameworkHandle) (framework.P
 		return nil, err
 	}
 	return &NodeLabel{
-		handle:    handle,
-		predicate: predicates.NewNodeLabelPredicate(args.PresentLabels, args.AbsentLabels),
-		Args:      args,
+		handle: handle,
+		Args:   args,
 	}, nil
 }
 
 // NodeLabel checks whether a pod can fit based on the node labels which match a filter that it requests.
 type NodeLabel struct {
-	handle    framework.FrameworkHandle
-	predicate predicates.FitPredicate
+	handle framework.FrameworkHandle
 	Args
 }
 
@@ -93,10 +91,31 @@ func (pl *NodeLabel) Name() string {
 }
 
 // Filter invoked at the filter extension point.
+// It checks whether all of the specified labels exists on a node or not, regardless of their value
+//
+// Consider the cases where the nodes are placed in regions/zones/racks and these are identified by labels
+// In some cases, it is required that only nodes that are part of ANY of the defined regions/zones/racks be selected
+//
+// Alternately, eliminating nodes that have a certain label, regardless of value, is also useful
+// A node may have a label with "retiring" as key and the date as the value
+// and it may be desirable to avoid scheduling new pods on this node.
 func (pl *NodeLabel) Filter(ctx context.Context, _ *framework.CycleState, pod *v1.Pod, nodeInfo *nodeinfo.NodeInfo) *framework.Status {
-	// Note that NodeLabelPredicate doesn't use predicate metadata, hence passing nil here.
-	_, reasons, err := pl.predicate(pod, nil, nodeInfo)
-	return migration.PredicateResultToFrameworkStatus(reasons, err)
+	node := nodeInfo.Node()
+	nodeLabels := labels.Set(node.Labels)
+	check := func(labels []string, presence bool) bool {
+		for _, label := range labels {
+			exists := nodeLabels.Has(label)
+			if (exists && !presence) || (!exists && presence) {
+				return false
+			}
+		}
+		return true
+	}
+	if check(pl.PresentLabels, true) && check(pl.AbsentLabels, false) {
+		return nil
+	}
+
+	return migration.PredicateResultToFrameworkStatus([]predicates.PredicateFailureReason{predicates.ErrNodeLabelPresenceViolated}, nil)
 }
 
 // Score invoked at the score extension point.