From bedcf28811f288d3dd3c8e6d09177a424f50210a Mon Sep 17 00:00:00 2001 From: Ellis Tarn Date: Sun, 20 Mar 2022 11:27:44 -0700 Subject: [PATCH] Simplified validation logic and deferred correctness checks to requirements.Compatible() --- pkg/apis/provisioning/v1alpha5/constraints.go | 11 +-- .../provisioning/v1alpha5/requirements.go | 68 ++++--------------- pkg/apis/provisioning/v1alpha5/suite_test.go | 4 +- .../provisioning/scheduling/suite_test.go | 2 +- pkg/utils/sets/sets.go | 2 +- 5 files changed, 24 insertions(+), 63 deletions(-) diff --git a/pkg/apis/provisioning/v1alpha5/constraints.go b/pkg/apis/provisioning/v1alpha5/constraints.go index 95802008f665..03dc9fb504ef 100644 --- a/pkg/apis/provisioning/v1alpha5/constraints.go +++ b/pkg/apis/provisioning/v1alpha5/constraints.go @@ -19,6 +19,8 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + + "github.com/aws/karpenter/pkg/utils/rand" ) // Constraints are applied to all nodes created by the provisioner. @@ -70,11 +72,12 @@ func (c *Constraints) GenerateLabels() map[string]string { } for key := range c.Requirements.Keys() { if !IsRestrictedNodeLabel(key) { - // Ignore cases when values set is empty (i.e., DoesNotExist or cancling out) - if c.Requirements.Get(key).Len() == 0 { - continue + switch c.Requirements.Get(key).Type() { + case v1.NodeSelectorOpIn: + labels[key] = c.Requirements.Get(key).Values().UnsortedList()[0] + case v1.NodeSelectorOpExists: + labels[key] = rand.String(10) } - labels[key] = c.Requirements.Label(key) } } return labels diff --git a/pkg/apis/provisioning/v1alpha5/requirements.go b/pkg/apis/provisioning/v1alpha5/requirements.go index 1b40d70418d6..449e5e2af0eb 100644 --- a/pkg/apis/provisioning/v1alpha5/requirements.go +++ b/pkg/apis/provisioning/v1alpha5/requirements.go @@ -24,7 +24,6 @@ import ( stringsets "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" - "github.com/aws/karpenter/pkg/utils/rand" "github.com/aws/karpenter/pkg/utils/sets" ) @@ -100,8 +99,6 @@ func (r Requirements) Add(requirements ...v1.NodeSelectorRequirement) Requiremen values = sets.NewComplementSet() case v1.NodeSelectorOpDoesNotExist: values = sets.NewSet() - default: - values = sets.NewSet() } if existing, ok := r.requirements[requirement.Key]; ok { values = values.Intersection(existing) @@ -120,21 +117,6 @@ func (r Requirements) Keys() stringsets.String { return keys } -// Labels returns value realization for the provided key. -// If the set is a complement set, return a randomly generated value. -// If the set is not a complement set, return the first value in the set. -func (r Requirements) Label(key string) string { - values := r.Get(key) - if values.IsComplement() { - label := rand.String(10) - for !values.Has(label) { - label = rand.String(10) - } - return label - } - return values.Values().UnsortedList()[0] -} - func (r Requirements) Has(key string) bool { _, ok := r.requirements[key] return ok @@ -180,18 +162,9 @@ func (r Requirements) Validate() (errs error) { if !SupportedNodeSelectorOps.Has(string(requirement.Operator)) { errs = multierr.Append(errs, fmt.Errorf("operator %s not in %s for key %s", requirement.Operator, SupportedNodeSelectorOps.UnsortedList(), requirement.Key)) } - // Excludes cases when DoesNotExists appears together with In, NotIn, Exists - if requirement.Operator == v1.NodeSelectorOpDoesNotExist { - if r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpIn)) || - r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpNotIn)) || - r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpExists)) { - errs = multierr.Append(errs, fmt.Errorf("operator %s cannot coexist with other operators for key %s", v1.NodeSelectorOpDoesNotExist, requirement.Key)) - } - } - } - for key := range r.Keys() { - if r.Get(key).Len() == 0 && !r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) { - errs = multierr.Append(errs, fmt.Errorf("no feasible value for key %s", key)) + // Combined requirements must have some possible value unless Operator=DoesNotExist. + if values := r.Get(requirement.Key); values.Len() == 0 && requirement.Operator != v1.NodeSelectorOpDoesNotExist { + errs = multierr.Append(errs, fmt.Errorf("no feasible value for key %s", requirement.Key)) } } return errs @@ -200,37 +173,22 @@ func (r Requirements) Validate() (errs error) { // Compatible ensures the provided requirements can be met. func (r Requirements) Compatible(requirements Requirements) (errs error) { for key, requirement := range requirements.requirements { - if requirement.Type() == v1.NodeSelectorOpIn && requirement.Intersection(r.Get(key)).Len() == 0 { - errs = multierr.Append(errs, fmt.Errorf("%s not in %s, key %s", requirement, r.Get(key), key)) - } - if requirement.Type() == v1.NodeSelectorOpExists && requirement.Intersection(r.Get(key)).Len() == 0 { - errs = multierr.Append(errs, fmt.Errorf("%s not in %s, key %s", requirement, r.Get(key), key)) - } - if requirement.Type() == v1.NodeSelectorOpNotIn && r.Get(key).Type() == v1.NodeSelectorOpIn && requirement.Intersection(r.Get(key)).Len() == 0 { + intersection := requirement.Intersection(r.Get(key)) + // There must be some value, except in these cases + if intersection.Len() == 0 { + // Where incoming requirement has operator { NotIn, DoesNotExist } + if requirement.Type() == v1.NodeSelectorOpNotIn || requirement.Type() == v1.NodeSelectorOpDoesNotExist { + // And existing requirement has operator { NotIn, DoesNotExist } + if !r.Has(key) || r.Get(key).Type() == v1.NodeSelectorOpNotIn || r.Get(key).Type() == v1.NodeSelectorOpDoesNotExist { + continue + } + } errs = multierr.Append(errs, fmt.Errorf("%s not in %s, key %s", requirement, r.Get(key), key)) } - if requirement.Type() == v1.NodeSelectorOpDoesNotExist && r.Get(key).Len() > 0 { - errs = multierr.Append(errs, fmt.Errorf("operator DoesNotExist prohibits %s, key %s", r.Get(key), key)) - } } return errs } -func (r Requirements) hasRequirement(f func(v1.NodeSelectorRequirement) bool) bool { - for _, requirement := range r.Requirements { - if f(requirement) { - return true - } - } - return false -} - -func withKeyAndOperator(key string, operator v1.NodeSelectorOperator) func(v1.NodeSelectorRequirement) bool { - return func(requirement v1.NodeSelectorRequirement) bool { - return key == requirement.Key && requirement.Operator == operator - } -} - func (r *Requirements) MarshalJSON() ([]byte, error) { if r.Requirements == nil { r.Requirements = []v1.NodeSelectorRequirement{} diff --git a/pkg/apis/provisioning/v1alpha5/suite_test.go b/pkg/apis/provisioning/v1alpha5/suite_test.go index e0418d0dfbc6..1de7e0787277 100644 --- a/pkg/apis/provisioning/v1alpha5/suite_test.go +++ b/pkg/apis/provisioning/v1alpha5/suite_test.go @@ -284,10 +284,10 @@ var _ = Describe("Validation", func() { B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists}) Expect(A.Compatible(B)).To(Succeed()) }) - It("A should fail to be compatible to B, operator, conflicting", func() { + It("A should be compatible to B, operator, conflicting", func() { A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"test", "foo"}}) B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist}) - Expect(A.Compatible(B)).ToNot(Succeed()) + Expect(A.Compatible(B)).To(Succeed()) }) It("A should be compatible to B, operator", func() { A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}}) diff --git a/pkg/controllers/provisioning/scheduling/suite_test.go b/pkg/controllers/provisioning/scheduling/suite_test.go index 40ed80751f6f..75fc3c5a91bd 100644 --- a/pkg/controllers/provisioning/scheduling/suite_test.go +++ b/pkg/controllers/provisioning/scheduling/suite_test.go @@ -401,7 +401,7 @@ var _ = Describe("Custom Constraints", func() { }}))[0] ExpectNotScheduled(ctx, env.Client, pod) }) - It("should schedule pods that have node selectors with DoesNotExists operator and undefined key", func() { + It("should schedule pods that with DoesNotExists operator and undefined key", func() { pod := ExpectProvisioned(ctx, env.Client, selectionController, provisioners, provisioner, test.UnschedulablePod( test.PodOptions{NodeRequirements: []v1.NodeSelectorRequirement{ {Key: "test-key", Operator: v1.NodeSelectorOpDoesNotExist}, diff --git a/pkg/utils/sets/sets.go b/pkg/utils/sets/sets.go index cffb09cc49b8..9869a529353c 100644 --- a/pkg/utils/sets/sets.go +++ b/pkg/utils/sets/sets.go @@ -73,7 +73,7 @@ func (s Set) Type() v1.NodeSelectorOperator { } // Values returns the values of the set. -// If the set has an infinite size, it will panic +// If the set is negatively defined, it will panic func (s Set) Values() sets.String { if s.complement { panic("infinite set")