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

Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50ce3a8
Resolved an issue with NotExists requirements and simplified intersec…
ellistarn Mar 19, 2022
dff9703
Simplified validation logic and deferred correctness checks to requir…
ellistarn Mar 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/apis/provisioning/v1alpha5/constraints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ 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).IsEmpty() {
if c.Requirements.Get(key).Len() == 0 {
continue
}
labels[key] = c.Requirements.Label(key)
Expand Down
80 changes: 31 additions & 49 deletions pkg/apis/provisioning/v1alpha5/requirements.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,23 @@ func (r Requirements) Add(requirements ...v1.NodeSelectorRequirement) Requiremen
continue
}
r.Requirements = append(r.Requirements, requirement)
var values sets.Set
switch requirement.Operator {
case v1.NodeSelectorOpIn:
r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewSet(requirement.Values...))
values = sets.NewSet(requirement.Values...)
case v1.NodeSelectorOpNotIn:
r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewComplementSet(requirement.Values...))
values = sets.NewComplementSet(requirement.Values...)
case v1.NodeSelectorOpExists:
r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewComplementSet())
values = sets.NewComplementSet()
case v1.NodeSelectorOpDoesNotExist:
r.requirements[requirement.Key] = sets.NewSet()
values = sets.NewSet()
default:
values = sets.NewSet()
Copy link
Contributor

Choose a reason for hiding this comment

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

This default case should never happen given what we currently allow, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I'll rip this out. I only needed it when this was a helper function.

}
if existing, ok := r.requirements[requirement.Key]; ok {
Copy link
Contributor Author

@ellistarn ellistarn Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This change was necessary due to the change on line 134. Now that we're considering the EmptySet and NullSet as equivalent, we should only intersect values if they're already defined.

values = values.Intersection(existing)
}
r.requirements[requirement.Key] = values
}
return r
}
Expand Down Expand Up @@ -128,12 +135,12 @@ func (r Requirements) Label(key string) string {
return values.Values().UnsortedList()[0]
}

// Get returns the sets of values allowed by all included requirements
// following a denylist method. Values are allowed except specified
func (r Requirements) Has(key string) bool {
_, ok := r.requirements[key]
return ok
}

func (r Requirements) Get(key string) sets.Set {
if _, ok := r.requirements[key]; !ok {
Copy link
Contributor Author

@ellistarn ellistarn Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This bit of set theory is inaccurate and was causing problems. The empty set and undefined set are both equivalent to {}.

return sets.NewComplementSet()
}
return r.requirements[key]
}

Expand Down Expand Up @@ -174,10 +181,12 @@ func (r Requirements) Validate() (errs error) {
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 && (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))
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() {
Expand All @@ -189,42 +198,19 @@ func (r Requirements) Validate() (errs error) {
}

// Compatible ensures the provided requirements can be met.
//gocyclo:ignore
func (r Requirements) Compatible(requirements Requirements) (errs error) {
felix-zhe-huang marked this conversation as resolved.
Show resolved Hide resolved
for _, key := range r.Keys().Union(requirements.Keys()).UnsortedList() {
// Key must be defined if required
if values := requirements.Get(key); values.Len() != 0 && !values.IsComplement() && !r.hasRequirement(withKey(key)) {
errs = multierr.Append(errs, fmt.Errorf("require values for key %s but is not defined", key))
}
// Values must overlap except DoesNotExist operator
// Both DoesNotExist and conflicting { In, NotIn } rules are represented by the empty set. DoesNotExist is a valid configuration, but conflicting { In, NotIn } is not.
if values := r.Get(key); values.Intersection(requirements.Get(key)).Len() == 0 && !r.Get(key).IsEmpty() && !requirements.Get(key).IsEmpty() {
errs = multierr.Append(errs, fmt.Errorf("%s not in %s, key %s", values, requirements.Get(key), key))
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))
}
// Exists incompatible with DoesNotExist or undefined
if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) {
if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key)) {
errs = multierr.Append(errs, fmt.Errorf("%s prohibits %s, key %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist, 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))
}
// DoesNotExist requires DoesNotExist or undefined
if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
if !(r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key))) {
errs = multierr.Append(errs, fmt.Errorf("%s requires %s, key %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, key))
}
if requirement.Type() == v1.NodeSelectorOpNotIn && r.Get(key).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))
}
// Repeat for the other direction
// Exists incompatible with DoesNotExist or undefined
if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) {
if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !requirements.hasRequirement(withKey(key)) {
errs = multierr.Append(errs, fmt.Errorf("%s prohibits %s, key %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist, key))
}
}
// DoesNotExist requires DoesNotExist or undefined
if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
if !(requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !requirements.hasRequirement(withKey(key))) {
errs = multierr.Append(errs, fmt.Errorf("%s requires %s, key %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, 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
Expand All @@ -239,10 +225,6 @@ func (r Requirements) hasRequirement(f func(v1.NodeSelectorRequirement) bool) bo
return false
}

func withKey(key string) func(v1.NodeSelectorRequirement) bool {
return func(requirement v1.NodeSelectorRequirement) bool { return requirement.Key == key }
}

func withKeyAndOperator(key string, operator v1.NodeSelectorOperator) func(v1.NodeSelectorRequirement) bool {
return func(requirement v1.NodeSelectorRequirement) bool {
return key == requirement.Key && requirement.Operator == operator
Expand Down
14 changes: 7 additions & 7 deletions pkg/apis/provisioning/v1alpha5/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,20 +314,20 @@ var _ = Describe("Validation", func() {
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should fail to be compatible to B, <Exists, Empty> operator, conflicting", func() {
It("A should be compatible to B, <Exists, Empty> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
B := NewRequirements()
Expect(A.Compatible(B)).ToNot(Succeed())
Expect(A.Compatible(B)).To(Succeed())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was incorrect. Pods are not required to have a nodeSelector in order to schedule onto a node.

})
It("A should fail to be compatible to B, <DoesNotExist, In> operator, conflicting", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should fail to be compatible to B, <DoesNotExist, NotIn> operator, conflicting", func() {
It("A should be compatible to B, <DoesNotExist, NotIn> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).ToNot(Succeed())
Expect(A.Compatible(B)).To(Succeed())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the second bullet at https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#set-based-requirement, DoesNotExist and NotIn are compatible. A Pod that says foo not in bar may schedule to a node without any label with key foo.

})
It("A should fail to be compatible to B, <DoesNotExists, Exists> operator, conflicting", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
Expand All @@ -340,7 +340,7 @@ var _ = Describe("Validation", func() {
Expect(A.Compatible(B)).To(Succeed())
})
It("A should be compatible to B, <DoesNotExist, Empty> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}})
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
B := NewRequirements()
Expect(A.Compatible(B)).To(Succeed())
})
Expand All @@ -354,14 +354,14 @@ var _ = Describe("Validation", func() {
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should fail to be compatible to B,, <Empty, Exists> operator, conflicting", func() {
It("A should fail to be compatible to B, <Empty, Exists> operator, conflicting", func() {
A := NewRequirements()
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should be compatible to B, <Empty, DoesNotExist> operator", func() {
A := NewRequirements()
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
Expect(A.Compatible(B)).To(Succeed())
})
})
Expand Down
5 changes: 2 additions & 3 deletions pkg/controllers/provisioning/scheduling/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,10 @@ package scheduling_test

import (
"context"
"k8s.io/apimachinery/pkg/api/resource"
"strings"
"testing"
"time"

"k8s.io/apimachinery/pkg/util/sets"

"github.com/Pallinder/go-randomdata"
"github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5"
"github.com/aws/karpenter/pkg/cloudprovider/fake"
Expand All @@ -31,6 +28,8 @@ import (
"github.com/aws/karpenter/pkg/controllers/provisioning/scheduling"
"github.com/aws/karpenter/pkg/controllers/selection"
"github.com/aws/karpenter/pkg/test"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/sets"
"sigs.k8s.io/controller-runtime/pkg/client"

v1 "k8s.io/api/core/v1"
Expand Down
17 changes: 13 additions & 4 deletions pkg/utils/sets/sets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"fmt"
"math"

v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
)

Expand Down Expand Up @@ -53,14 +54,22 @@ func (s Set) DeepCopy() Set {
}
}

// IsComplement returns whether the set is a complement set.
// IsComplement returns whether the set is a complement set.
func (s Set) IsComplement() bool {
return s.complement
}

// IsEmpty returns whether the set is an empty set.
func (s Set) IsEmpty() bool {
return !s.complement && s.values.Len() == 0
func (s Set) Type() v1.NodeSelectorOperator {
if s.IsComplement() {
if s.Len() < math.MaxInt64 {
return v1.NodeSelectorOpNotIn
}
return v1.NodeSelectorOpExists
}
if s.Len() > 0 {
return v1.NodeSelectorOpIn
}
return v1.NodeSelectorOpDoesNotExist
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat!


// Values returns the values of the set.
Expand Down
3 changes: 0 additions & 3 deletions pkg/utils/sets/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ var _ = Describe("Set", func() {
})
})
Context("Functional Correctness", func() {
It("A should not be empty", func() {
Expect(setA.IsEmpty()).To(BeFalse())
})
It("size of AB should be 2", func() {
Expect(setAB.Len()).To(Equal(2))
})
Expand Down