Simplified validation logic and deferred correctness checks to requir…

…ements.Compatible()

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 <In, NotIn> 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

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{}

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, <NotIn, DoesNotExist> operator, conflicting", func() {
+		It("A should be compatible to B, <NotIn, DoesNotExist> 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, <NotIn, Empty> operator", func() {
 			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})

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},

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")