Skip to content

Commit

Permalink
New refactoring iteration, captures more cases
Browse files Browse the repository at this point in the history
  • Loading branch information
@felix-zhe-huang
felix-zhe-huang committed Feb 3, 2022
1 parent 97e9714 commit c5552b7
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 27 deletions.
3 changes: 1 addition & 2 deletions charts/karpenter/crds/karpenter.sh_provisioners.yaml
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@

---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.7.0
controller-gen.kubebuilder.io/version: v0.8.0
creationTimestamp: null
name: provisioners.karpenter.sh
spec:
Expand Down
30 changes: 20 additions & 10 deletions pkg/apis/provisioning/v1alpha5/requirements.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,10 @@ func (r Requirements) Validate() (errs *apis.FieldError) {
if !functional.ContainsString(SupportedNodeSelectorOps, string(requirement.Operator)) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", requirement.Operator, SupportedNodeSelectorOps), "operator"))
}
if requirement.Operator == v1.NodeSelectorOpDoesNotExist && !r.Values(requirement.Key).Complement {
// 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 = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("operator %s and %s conflict", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator"))
}
}
Expand All @@ -236,17 +239,24 @@ func (r Requirements) Compatible(requirements Requirements) (errs *apis.FieldErr
if values := r.Values(key); values.Intersection(requirements.Values(key)).Len() == 0 {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", values, requirements.Values(key)), "values")).ViaFieldIndex("requirements", i)
}
// 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 = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i)

for r1, r2 := range map[*Requirements]*Requirements{
&r: &requirements,
&requirements: &r,
} {
// Exists incompatible with DoesNotExist or undefined
if r1.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) {
if r2.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r2.hasRequirement(withKey(key)) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i)
}
}
}
// DoesNotExist requires DoesNotExist or undefined
if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
if !(r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key))) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i)
// DoesNotExist requires DoesNotExist or undefined
if r1.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) {
if !(r2.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r2.hasRequirement(withKey(key))) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i)
}
}

}
}
return errs
Expand Down
121 changes: 107 additions & 14 deletions pkg/apis/provisioning/v1alpha5/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,13 @@ var _ = Describe("Validation", func() {
provisioner.Spec.Requirements = NewRequirements()
Expect(provisioner.Validate(ctx)).To(Succeed())
})
It("should fail because In and DoesNotExists conflicting", func() {
provisioner.Spec.Requirements = NewRequirements(
v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}},
v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist},
)
It("should fail because DoesNotExists conflicting", func() {
for _, op := range []v1.NodeSelectorOperator{v1.NodeSelectorOpIn, v1.NodeSelectorOpNotIn, v1.NodeSelectorOpExists} {
provisioner.Spec.Requirements = NewRequirements(
v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOperator(op), Values: []string{"test"}},
v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist},
)
}
Expect(provisioner.Validate(ctx)).ToNot(Succeed())
})
It("should normalize aliased labels", func() {
Expand All @@ -199,7 +201,7 @@ var _ = Describe("Validation", func() {
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should fail to be compatible to B, <In, In> operator", func() {
It("A should fail to be compatible to B, <In, In> operaton, no overlap", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"bar"}})
Expect(A.Compatible(B)).ToNot(Succeed())
Expand All @@ -209,35 +211,126 @@ 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, <In, NotIn> operator", func() {
It("A should fail to be compatible to B, <In, NotIn> operator, cancel out", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should be compatible to B, <In, Exist> operator", func() {
It("A should be compatible to B, <In, Exists> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
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, <In, DoesNotExist> operator", func() {
It("A should fail to be compatible to B, <In, DoesNotExist> operator, conflicting", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should be compatible to B, <NotIn, In> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should fail to be compatible to B, <NotIn, In> operator, cancel out", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should be compatible to B, <NotIn, NotIn> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"test", "foo"}})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should be compatible to B, <NotIn, Exists> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"test", "foo"}})
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() {
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())
})

It("A should be compatible to B, <Exists, In> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should be compatible to B, <Exists, NotIn> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should be compatible to B, <Exists, Exists> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
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, <Exists, DoesNotExist> operaton, conflicting", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
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, <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() {
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())
})
It("A should fail to be compatible to B, <DoesNotExists, Exists> operator, conflicting", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should be compatible to B, <DoesNotExist, DoesNotExists> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
Expect(A.Compatible(B)).To(Succeed())
})
It("A should be compatible to B, <In, Empty> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
B := NewRequirements()
Expect(A.Compatible(B)).To(Succeed())
})
It("A should fail to be compatible to B, <Empty, In> operator, indirectional", func() {
A := NewRequirements()
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should be compatible to B, <Empty, NotIn> operator", func() {
A := NewRequirements()
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, In> operator", func() {
It("A should be compatible to B, <NotIn, Empty> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}})
B := NewRequirements()
Expect(A.Compatible(B)).To(Succeed())
})
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.NodeSelectorOpIn, Values: []string{"foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
Expect(A.Compatible(B)).ToNot(Succeed())
})
It("A should not be compatible to B, <In, DoesNotExist> operator", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}})
B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist})
It("A should fail to be compatible to B, <Exists, Empty> operator, conflicting", func() {
A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
B := NewRequirements()
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"}})
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"}})
B := NewRequirements()
Expect(A.Compatible(B)).To(Succeed())
})
})
})
1 change: 0 additions & 1 deletion pkg/controllers/provisioning/scheduling/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ var _ = AfterEach(func() {
ExpectProvisioningCleanedUp(ctx, env.Client, provisioners)
})


var _ = Describe("Constraints", func() {
Context("Custom Labels", func() {
It("should schedule unconstrained pods that don't have matching node selectors", func() {
Expand Down

0 comments on commit c5552b7

Please sign in to comment.