From d572249d3056b06877dff30c8ddbe30e75d3d353 Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Mon, 2 Nov 2020 17:21:13 -0500 Subject: [PATCH] Add runtime representation of []v1.PreferredSchedulingTerm to be used for repeatedly scoring nodes. Change-Id: Ib1a0866979ce6cf75d1d9668c4bf8f6fb57298b2 --- .../framework/plugins/helper/node_affinity.go | 2 +- .../framework/plugins/nodeaffinity/BUILD | 2 +- .../plugins/nodeaffinity/node_affinity.go | 23 +--- .../corev1/nodeaffinity/nodeaffinity.go | 95 +++++++++++--- .../corev1/nodeaffinity/nodeaffinity_test.go | 123 ++++++++++++++++++ 5 files changed, 210 insertions(+), 35 deletions(-) diff --git a/pkg/scheduler/framework/plugins/helper/node_affinity.go b/pkg/scheduler/framework/plugins/helper/node_affinity.go index e5b3f1f885d..a3434618386 100644 --- a/pkg/scheduler/framework/plugins/helper/node_affinity.go +++ b/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 } diff --git a/pkg/scheduler/framework/plugins/nodeaffinity/BUILD b/pkg/scheduler/framework/plugins/nodeaffinity/BUILD index ea313c522d2..6b7d95ee85f 100644 --- a/pkg/scheduler/framework/plugins/nodeaffinity/BUILD +++ b/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", ], ) diff --git a/pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go b/pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go index 466a2594f2c..aad73c0526b 100644 --- a/pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go +++ b/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. - for i := range 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) - } - - if matches { - count += int64(preferredSchedulingTerm.Weight) - } + // TODO(#96164): Do this in PreScore to avoid computing it for all nodes. + preferredNodeAffinity, err := nodeaffinity.NewPreferredSchedulingTerms(affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution) + if err != nil { + return 0, framework.AsStatus(err) } + count += preferredNodeAffinity.Score(node) } return count, nil diff --git a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go index 81189c9ef3a..efca431e615 100644 --- a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go +++ b/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{}) - 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) - } + parsedTerms = append(parsedTerms, newNodeSelectorTerm(&term)) } 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) - if len(node.Name) > 0 { - nodeFields["metadata.name"] = node.Name - } + nodeFields := extractNodeFields(node) 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 +} diff --git a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go index 65879544bf9..738e3b4b69b 100644 --- a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go +++ b/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",