Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return field.Errors from node affinity parsing #96522

Merged
merged 1 commit into from Dec 10, 2020
Merged

Return field.Errors from node affinity parsing #96522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 8 additions & 9 deletions pkg/scheduler/apis/config/validation/validation_pluginargs.go
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ import (

v1 "k8s.io/api/core/v1"
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
"k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/component-helpers/scheduling/corev1/nodeaffinity"
Expand Down Expand Up @@ -280,22 +281,20 @@ func ValidateNodeAffinityArgs(args *config.NodeAffinityArgs) error {
return nil
}
affinity := args.AddedAffinity
f := field.NewPath("addedAffinity")
var allErrs field.ErrorList
path := field.NewPath("addedAffinity")
var errs []error
if ns := affinity.RequiredDuringSchedulingIgnoredDuringExecution; ns != nil {
_, err := nodeaffinity.NewNodeSelector(ns)
_, err := nodeaffinity.NewNodeSelector(ns, nodeaffinity.WithPath(path.Child("requiredDuringSchedulingIgnoredDuringExecution")))
if err != nil {
// TODO(#96167): Expand all field.Error(s) returned once constructor use them.
allErrs = append(allErrs, field.Invalid(f.Child("requiredDuringSchedulingIgnoredDuringExecution"), affinity.RequiredDuringSchedulingIgnoredDuringExecution, err.Error()))
errs = append(errs, err)
}
}
// TODO: Add validation for requiredDuringSchedulingRequiredDuringExecution when it gets added to the API.
if terms := affinity.PreferredDuringSchedulingIgnoredDuringExecution; len(terms) != 0 {
_, err := nodeaffinity.NewPreferredSchedulingTerms(terms)
_, err := nodeaffinity.NewPreferredSchedulingTerms(terms, nodeaffinity.WithPath(path.Child("preferredDuringSchedulingIgnoredDuringExecution")))
if err != nil {
// TODO(#96167): Expand all field.Error(s) returned once constructor use them.
allErrs = append(allErrs, field.Invalid(f.Child("preferredDuringSchedulingIgnoredDuringExecution"), affinity.PreferredDuringSchedulingIgnoredDuringExecution, err.Error()))
errs = append(errs, err)
}
}
return allErrs.ToAggregate()
return errors.Flatten(errors.NewAggregate(errs))
}
12 changes: 9 additions & 3 deletions pkg/scheduler/apis/config/validation/validation_pluginargs_test.go
View file
Open in desktop
Expand Up @@ -925,15 +925,21 @@ func TestValidateNodeAffinityArgs(t *testing.T) {
},
},
wantErr: field.ErrorList{
field.Invalid(field.NewPath("addedAffinity", "requiredDuringSchedulingIgnoredDuringExecution"), nil, ""),
field.Invalid(field.NewPath("addedAffinity", "preferredDuringSchedulingIgnoredDuringExecution"), nil, ""),
&field.Error{
Type: field.ErrorTypeInvalid,
Field: "addedAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0]",
},
&field.Error{
Type: field.ErrorTypeInvalid,
Field: "addedAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].matchFields[0].values",
},
}.ToAggregate(),
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
err := ValidateNodeAffinityArgs(&tc.args)
if diff := cmp.Diff(err, tc.wantErr, ignoreBadValueDetail); diff != "" {
if diff := cmp.Diff(tc.wantErr, err, ignoreBadValueDetail); diff != "" {
t.Fatalf("ValidatedNodeAffinityArgs returned err (-want,+got):\n%s", diff)
}
})
Expand Down
1 change: 1 addition & 0 deletions staging/src/k8s.io/component-helpers/go.mod
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ module k8s.io/component-helpers
go 1.15

require (
github.com/google/go-cmp v0.5.2
k8s.io/api v0.0.0
k8s.io/apimachinery v0.0.0
k8s.io/client-go v0.0.0
Expand Down
6 changes: 4 additions & 2 deletions staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@ go_library(
"//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/selection:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
],
)

Expand All @@ -37,6 +37,8 @@ go_test(
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//vendor/github.com/google/go-cmp/cmp:go_default_library",
"//vendor/github.com/google/go-cmp/cmp/cmpopts:go_default_library",
],
)
138 changes: 91 additions & 47 deletions staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go
View file
Open in desktop
Expand Up @@ -17,15 +17,16 @@ limitations under the License.
package nodeaffinity

import (
"fmt"

v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/validation/field"
)

// ParseOption is an option for parsing.
type ParseOption func(*parseOptions)

// NodeSelector is a runtime representation of v1.NodeSelector.
type NodeSelector struct {
lazy LazyErrorNodeSelector
Expand All @@ -37,31 +38,44 @@ type LazyErrorNodeSelector struct {
terms []nodeSelectorTerm
}

// NewNodeSelector returns a NodeSelector or all parsing errors found.
func NewNodeSelector(ns *v1.NodeSelector) (*NodeSelector, error) {
lazy := NewLazyErrorNodeSelector(ns)
var errs []error
// WithPath sets a field.Path to be used as root for parse errors.
func WithPath(p *field.Path) ParseOption {
return func(o *parseOptions) {
o.path = p
}
}

// NewNodeSelector returns a NodeSelector or aggregate parsing errors found.
func NewNodeSelector(ns *v1.NodeSelector, opts ...ParseOption) (*NodeSelector, error) {
lazy := NewLazyErrorNodeSelector(ns, opts...)
var errs field.ErrorList
for _, term := range lazy.terms {
if term.parseErr != nil {
errs = append(errs, term.parseErr)
if len(term.parseErrs) > 0 {
errs = append(errs, term.parseErrs...)
}
}
if len(errs) != 0 {
return nil, errors.NewAggregate(errs)
return nil, errs.ToAggregate()
}
return &NodeSelector{lazy: *lazy}, nil
}

// NewLazyErrorNodeSelector creates a NodeSelector that only reports parse
// errors when no terms match.
func NewLazyErrorNodeSelector(ns *v1.NodeSelector) *LazyErrorNodeSelector {
func NewLazyErrorNodeSelector(ns *v1.NodeSelector, opts ...ParseOption) *LazyErrorNodeSelector {
o := parseOptions{}
for _, opt := range opts {
opt(&o)
}
parsedTerms := make([]nodeSelectorTerm, 0, len(ns.NodeSelectorTerms))
for _, term := range ns.NodeSelectorTerms {
path := o.path.Child("nodeSelectorTerms")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o.path could be nil right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye, but (nil).Child actually works :)

for i, term := range ns.NodeSelectorTerms {
// nil or empty term selects no objects
if isEmptyNodeSelectorTerm(&term) {
continue
}
parsedTerms = append(parsedTerms, newNodeSelectorTerm(&term))
p := path.Index(i)
parsedTerms = append(parsedTerms, newNodeSelectorTerm(&term, p))
}
return &LazyErrorNodeSelector{
terms: parsedTerms,
Expand All @@ -86,18 +100,18 @@ func (ns *LazyErrorNodeSelector) Match(node *v1.Node) (bool, error) {
nodeLabels := labels.Set(node.Labels)
nodeFields := extractNodeFields(node)

var errs []error
var errs field.ErrorList
for _, term := range ns.terms {
match, err := term.match(nodeLabels, nodeFields)
if err != nil {
errs = append(errs, term.parseErr)
match, tErrs := term.match(nodeLabels, nodeFields)
if len(tErrs) > 0 {
errs = append(errs, tErrs...)
continue
}
if match {
return true, nil
}
}
return false, errors.NewAggregate(errs)
return false, errs.ToAggregate()
}

// PreferredSchedulingTerms is a runtime representation of []v1.PreferredSchedulingTerms.
Expand All @@ -107,25 +121,30 @@ type PreferredSchedulingTerms struct {

// 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
func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm, opts ...ParseOption) (*PreferredSchedulingTerms, error) {
o := parseOptions{}
for _, opt := range opts {
opt(&o)
}
var errs field.ErrorList
parsedTerms := make([]preferredSchedulingTerm, 0, len(terms))
for _, term := range terms {
for i, term := range terms {
path := o.path.Index(i)
alculquicondor marked this conversation as resolved.
Show resolved Hide resolved
if term.Weight == 0 || isEmptyNodeSelectorTerm(&term.Preference) {
continue
}
parsedTerm := preferredSchedulingTerm{
nodeSelectorTerm: newNodeSelectorTerm(&term.Preference),
nodeSelectorTerm: newNodeSelectorTerm(&term.Preference, path),
weight: int(term.Weight),
}
if parsedTerm.parseErr != nil {
errs = append(errs, parsedTerm.parseErr)
if len(parsedTerm.parseErrs) > 0 {
errs = append(errs, parsedTerm.parseErrs...)
} else {
parsedTerms = append(parsedTerms, parsedTerm)
}
}
if len(errs) != 0 {
return nil, errors.NewAggregate(errs)
return nil, errs.ToAggregate()
}
return &PreferredSchedulingTerms{terms: parsedTerms}, nil
}
Expand Down Expand Up @@ -160,26 +179,28 @@ func extractNodeFields(n *v1.Node) fields.Set {
type nodeSelectorTerm struct {
matchLabels labels.Selector
matchFields fields.Selector
parseErr error
parseErrs field.ErrorList
}

func newNodeSelectorTerm(term *v1.NodeSelectorTerm) nodeSelectorTerm {
func newNodeSelectorTerm(term *v1.NodeSelectorTerm, path *field.Path) nodeSelectorTerm {
var parsedTerm nodeSelectorTerm
if len(term.MatchExpressions) != 0 {
parsedTerm.matchLabels, parsedTerm.parseErr = nodeSelectorRequirementsAsSelector(term.MatchExpressions)
if parsedTerm.parseErr != nil {
p := path.Child("matchExpressions")
parsedTerm.matchLabels, parsedTerm.parseErrs = nodeSelectorRequirementsAsSelector(term.MatchExpressions, p)
if parsedTerm.parseErrs != nil {
return parsedTerm
}
}
if len(term.MatchFields) != 0 {
parsedTerm.matchFields, parsedTerm.parseErr = nodeSelectorRequirementsAsFieldSelector(term.MatchFields)
p := path.Child("matchFields")
parsedTerm.matchFields, parsedTerm.parseErrs = nodeSelectorRequirementsAsFieldSelector(term.MatchFields, p)
}
return parsedTerm
}

func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, error) {
if t.parseErr != nil {
return false, t.parseErr
func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, field.ErrorList) {
if t.parseErrs != nil {
return false, t.parseErrs
}
if t.matchLabels != nil && !t.matchLabels.Matches(nodeLabels) {
return false, nil
Expand All @@ -192,12 +213,14 @@ func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (

// nodeSelectorRequirementsAsSelector converts the []NodeSelectorRequirement api type into a struct that implements
// labels.Selector.
func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement) (labels.Selector, error) {
func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement, path *field.Path) (labels.Selector, field.ErrorList) {
if len(nsm) == 0 {
return labels.Nothing(), nil
}
var errs field.ErrorList
selector := labels.NewSelector()
for _, expr := range nsm {
for i, expr := range nsm {
p := path.Index(i)
var op selection.Operator
switch expr.Operator {
case v1.NodeSelectorOpIn:
Expand All @@ -213,50 +236,71 @@ func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement) (label
case v1.NodeSelectorOpLt:
op = selection.LessThan
default:
return nil, fmt.Errorf("%q is not a valid node selector operator", expr.Operator)
errs = append(errs, field.NotSupported(p.Child("operator"), expr.Operator, nil))
continue
}
r, err := labels.NewRequirement(expr.Key, op, expr.Values)
if err != nil {
return nil, err
// TODO(#96167): Use error as returned by labels.NewRequirement once it
// is a field.Error.
errs = append(errs, field.Invalid(p, expr, err.Error()))
} else {
selector = selector.Add(*r)
}
selector = selector.Add(*r)
}
if len(errs) != 0 {
return nil, errs
}
return selector, nil
}

var validFieldSelectorOperators = []string{
string(v1.NodeSelectorOpIn),
string(v1.NodeSelectorOpNotIn),
}

// nodeSelectorRequirementsAsFieldSelector converts the []NodeSelectorRequirement core type into a struct that implements
// fields.Selector.
func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement) (fields.Selector, error) {
func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement, path *field.Path) (fields.Selector, field.ErrorList) {
if len(nsr) == 0 {
return fields.Nothing(), nil
}
var errs field.ErrorList

var selectors []fields.Selector
for _, expr := range nsr {
for i, expr := range nsr {
p := path.Index(i)
switch expr.Operator {
case v1.NodeSelectorOpIn:
if len(expr.Values) != 1 {
return nil, fmt.Errorf("unexpected number of value (%d) for node field selector operator %q",
len(expr.Values), expr.Operator)
errs = append(errs, field.Invalid(p.Child("values"), expr.Values, "must have one element"))
} else {
selectors = append(selectors, fields.OneTermEqualSelector(expr.Key, expr.Values[0]))
}
selectors = append(selectors, fields.OneTermEqualSelector(expr.Key, expr.Values[0]))

case v1.NodeSelectorOpNotIn:
if len(expr.Values) != 1 {
return nil, fmt.Errorf("unexpected number of value (%d) for node field selector operator %q",
len(expr.Values), expr.Operator)
errs = append(errs, field.Invalid(p.Child("values"), expr.Values, "must have one element"))
} else {
selectors = append(selectors, fields.OneTermNotEqualSelector(expr.Key, expr.Values[0]))
}
selectors = append(selectors, fields.OneTermNotEqualSelector(expr.Key, expr.Values[0]))

default:
return nil, fmt.Errorf("%q is not a valid node field selector operator", expr.Operator)
errs = append(errs, field.NotSupported(p.Child("operator"), expr.Operator, validFieldSelectorOperators))
}
}

if len(errs) != 0 {
return nil, errs
}
return fields.AndSelectors(selectors...), nil
}

type preferredSchedulingTerm struct {
nodeSelectorTerm
weight int
}

type parseOptions struct {
path *field.Path
}