Merge pull request #96126 from alculquicondor/parse-preferred-node-aff

Add runtime representation of []v1.PreferredSchedulingTerm

pkg/scheduler/framework/plugins/helper/node_affinity.go

@@ -70,7 +70,7 @@ func PodMatchesNodeSelectorAndAffinityTerms(pod *v1.Pod, node *v1.Node) bool {
 // nodeMatchesNodeSelectorTerms checks if a node's labels satisfy a list of node selector terms,
 // terms are ORed, and an empty list of terms will match nothing.
 func nodeMatchesNodeSelectorTerms(node *v1.Node, nodeSelector *v1.NodeSelector) bool {
-	// TODO(@alculquicondor, #95738): parse this error earlier in the plugin so we only need to do it once per pod
+	// TODO(#96164): parse this error earlier in the plugin so we only need to do it once per Pod.
 	matches, _ := corev1.MatchNodeSelectorTerms(node, nodeSelector)
 	return matches
 }

pkg/scheduler/framework/plugins/nodeaffinity/BUILD

@@ -10,7 +10,7 @@ go_library(
         "//pkg/scheduler/framework/plugins/helper:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
-        "//staging/src/k8s.io/component-helpers/scheduling/corev1:go_default_library",
+        "//staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity:go_default_library",
     ],
 )
 

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go

@@ -22,7 +22,7 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/component-helpers/scheduling/corev1"
+	"k8s.io/component-helpers/scheduling/corev1/nodeaffinity"
 	"k8s.io/kubernetes/pkg/scheduler/framework"
 	pluginhelper "k8s.io/kubernetes/pkg/scheduler/framework/plugins/helper"
 )
@@ -79,23 +79,12 @@ func (pl *NodeAffinity) Score(ctx context.Context, state *framework.CycleState,
 	// An element of PreferredDuringSchedulingIgnoredDuringExecution that refers to an
 	// empty PreferredSchedulingTerm matches all objects.
 	if affinity != nil && affinity.NodeAffinity != nil && affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil {
-		// Match PreferredDuringSchedulingIgnoredDuringExecution term by term.
+		// TODO(#96164): Do this in PreScore to avoid computing it for all nodes.
-		for i := range affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution {
+		preferredNodeAffinity, err := nodeaffinity.NewPreferredSchedulingTerms(affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution)
-			preferredSchedulingTerm := &affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution[i]
-			if preferredSchedulingTerm.Weight == 0 {
-				continue
-			}
-
-			// TODO: Avoid computing it for all nodes if this becomes a performance problem.
-			matches, err := corev1.MatchNodeSelectorTerms(node, &v1.NodeSelector{NodeSelectorTerms: []v1.NodeSelectorTerm{preferredSchedulingTerm.Preference}})
 		if err != nil {
 			return 0, framework.AsStatus(err)
 		}
-
+		count += preferredNodeAffinity.Score(node)
-			if matches {
-				count += int64(preferredSchedulingTerm.Weight)
-			}
-		}
 	}
 
 	return count, nil

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go

@@ -58,20 +58,10 @@ func NewLazyErrorNodeSelector(ns *v1.NodeSelector) *LazyErrorNodeSelector {
 	parsedTerms := make([]nodeSelectorTerm, 0, len(ns.NodeSelectorTerms))
 	for _, term := range ns.NodeSelectorTerms {
 		// nil or empty term selects no objects
-		if len(term.MatchExpressions) == 0 && len(term.MatchFields) == 0 {
+		if isEmptyNodeSelectorTerm(&term) {
 			continue
 		}
-		parsedTerms = append(parsedTerms, nodeSelectorTerm{})
+		parsedTerms = append(parsedTerms, newNodeSelectorTerm(&term))
-		parsedTerm := &parsedTerms[len(parsedTerms)-1]
-		if len(term.MatchExpressions) != 0 {
-			parsedTerm.matchLabels, parsedTerm.parseErr = nodeSelectorRequirementsAsSelector(term.MatchExpressions)
-			if parsedTerm.parseErr != nil {
-				continue
-			}
-		}
-		if len(term.MatchFields) != 0 {
-			parsedTerm.matchFields, parsedTerm.parseErr = nodeSelectorRequirementsAsFieldSelector(term.MatchFields)
-		}
 	}
 	return &LazyErrorNodeSelector{
 		terms: parsedTerms,
@@ -94,10 +84,7 @@ func (ns *LazyErrorNodeSelector) Match(node *v1.Node) (bool, error) {
 		return false, nil
 	}
 	nodeLabels := labels.Set(node.Labels)
-	nodeFields := make(fields.Set)
+	nodeFields := extractNodeFields(node)
-	if len(node.Name) > 0 {
-		nodeFields["metadata.name"] = node.Name
-	}
 
 	var errs []error
 	for _, term := range ns.terms {
@@ -113,12 +100,83 @@ func (ns *LazyErrorNodeSelector) Match(node *v1.Node) (bool, error) {
 	return false, errors.NewAggregate(errs)
 }
 
+// PreferredSchedulingTerms is a runtime representation of []v1.PreferredSchedulingTerms.
+type PreferredSchedulingTerms struct {
+	terms []preferredSchedulingTerm
+}
+
+// NewPreferredSchedulingTerms returns a PreferredSchedulingTerms or all the parsing errors found.
+// If a v1.PreferredSchedulingTerm has a 0 weight, its parsing is skipped.
+func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm) (*PreferredSchedulingTerms, error) {
+	var errs []error
+	parsedTerms := make([]preferredSchedulingTerm, 0, len(terms))
+	for _, term := range terms {
+		if term.Weight == 0 || isEmptyNodeSelectorTerm(&term.Preference) {
+			continue
+		}
+		parsedTerm := preferredSchedulingTerm{
+			nodeSelectorTerm: newNodeSelectorTerm(&term.Preference),
+			weight:           int(term.Weight),
+		}
+		if parsedTerm.parseErr != nil {
+			errs = append(errs, parsedTerm.parseErr)
+		} else {
+			parsedTerms = append(parsedTerms, parsedTerm)
+		}
+	}
+	if len(errs) != 0 {
+		return nil, errors.NewAggregate(errs)
+	}
+	return &PreferredSchedulingTerms{terms: parsedTerms}, nil
+}
+
+// Score returns a score for a Node: the sum of the weights of the terms that
+// match the Node.
+func (t *PreferredSchedulingTerms) Score(node *v1.Node) int64 {
+	var score int64
+	nodeLabels := labels.Set(node.Labels)
+	nodeFields := extractNodeFields(node)
+	for _, term := range t.terms {
+		// parse errors are reported in NewPreferredSchedulingTerms.
+		if ok, _ := term.match(nodeLabels, nodeFields); ok {
+			score += int64(term.weight)
+		}
+	}
+	return score
+}
+
+func isEmptyNodeSelectorTerm(term *v1.NodeSelectorTerm) bool {
+	return len(term.MatchExpressions) == 0 && len(term.MatchFields) == 0
+}
+
+func extractNodeFields(n *v1.Node) fields.Set {
+	f := make(fields.Set)
+	if len(n.Name) > 0 {
+		f["metadata.name"] = n.Name
+	}
+	return f
+}
+
 type nodeSelectorTerm struct {
 	matchLabels labels.Selector
 	matchFields fields.Selector
 	parseErr    error
 }
 
+func newNodeSelectorTerm(term *v1.NodeSelectorTerm) nodeSelectorTerm {
+	var parsedTerm nodeSelectorTerm
+	if len(term.MatchExpressions) != 0 {
+		parsedTerm.matchLabels, parsedTerm.parseErr = nodeSelectorRequirementsAsSelector(term.MatchExpressions)
+		if parsedTerm.parseErr != nil {
+			return parsedTerm
+		}
+	}
+	if len(term.MatchFields) != 0 {
+		parsedTerm.matchFields, parsedTerm.parseErr = nodeSelectorRequirementsAsFieldSelector(term.MatchFields)
+	}
+	return parsedTerm
+}
+
 func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, error) {
 	if t.parseErr != nil {
 		return false, t.parseErr
@@ -197,3 +255,8 @@ func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement) (
 
 	return fields.AndSelectors(selectors...), nil
 }
+
+type preferredSchedulingTerm struct {
+	nodeSelectorTerm
+	weight int
+}

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go

@@ -150,6 +150,129 @@ func TestNodeSelectorMatch(t *testing.T) {
 	}
 }
 
+func TestPreferredSchedulingTermsScore(t *testing.T) {
+	tests := []struct {
+		name           string
+		prefSchedTerms []v1.PreferredSchedulingTerm
+		node           *v1.Node
+		wantErr        error
+		wantScore      int64
+	}{
+		{
+			name: "invalid field selector and label selector",
+			prefSchedTerms: []v1.PreferredSchedulingTerm{
+				{
+					Weight: 1,
+					Preference: v1.NodeSelectorTerm{
+						MatchFields: []v1.NodeSelectorRequirement{{
+							Key:      "metadata.name",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"host_1", "host_2"},
+						}},
+					},
+				},
+				{
+					Weight: 1,
+					Preference: v1.NodeSelectorTerm{
+						MatchExpressions: []v1.NodeSelectorRequirement{{
+							Key:      "label_1",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"label_1_val"},
+						}},
+						MatchFields: []v1.NodeSelectorRequirement{{
+							Key:      "metadata.name",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"host_1"},
+						}},
+					},
+				},
+				{
+					Weight: 1,
+					Preference: v1.NodeSelectorTerm{
+						MatchExpressions: []v1.NodeSelectorRequirement{{
+							Key:      "invalid key",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"label_value"},
+						}},
+					},
+				},
+			},
+			wantErr: apierrors.NewAggregate([]error{
+				errors.New(`unexpected number of value (2) for node field selector operator "In"`),
+				errors.New(`invalid label key "invalid key": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`),
+			}),
+		},
+		{
+			name: "invalid field selector but no weight, error not reported",
+			prefSchedTerms: []v1.PreferredSchedulingTerm{
+				{
+					Weight: 0,
+					Preference: v1.NodeSelectorTerm{
+						MatchFields: []v1.NodeSelectorRequirement{{
+							Key:      "metadata.name",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"host_1", "host_2"},
+						}},
+					},
+				},
+			},
+			node: &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "host_1"}},
+		},
+		{
+			name: "first and third term match",
+			prefSchedTerms: []v1.PreferredSchedulingTerm{
+				{
+					Weight: 5,
+					Preference: v1.NodeSelectorTerm{
+						MatchFields: []v1.NodeSelectorRequirement{{
+							Key:      "metadata.name",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"host_1"},
+						}},
+					},
+				},
+				{
+					Weight: 7,
+					Preference: v1.NodeSelectorTerm{
+						MatchExpressions: []v1.NodeSelectorRequirement{{
+							Key:      "unknown_label",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"unknown_label_val"},
+						}},
+					},
+				},
+				{
+					Weight: 11,
+					Preference: v1.NodeSelectorTerm{
+						MatchExpressions: []v1.NodeSelectorRequirement{{
+							Key:      "label_1",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"label_1_val"},
+						}},
+					},
+				},
+			},
+			node:      &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "host_1", Labels: map[string]string{"label_1": "label_1_val"}}},
+			wantScore: 16,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			prefSchedTerms, err := NewPreferredSchedulingTerms(tt.prefSchedTerms)
+			if !reflect.DeepEqual(err, tt.wantErr) {
+				t.Fatalf("NewPreferredSchedulingTerms returned error %q, want %q", err, tt.wantErr)
+			}
+			if tt.wantErr != nil {
+				return
+			}
+			score := prefSchedTerms.Score(tt.node)
+			if score != tt.wantScore {
+				t.Errorf("PreferredSchedulingTerms.Score returned %d, want %d", score, tt.wantScore)
+			}
+		})
+	}
+}
+
 func TestNodeSelectorRequirementsAsSelector(t *testing.T) {
 	matchExpressions := []v1.NodeSelectorRequirement{{
 		Key:      "foo",