Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

ellistarn · 2022-03-19T19:00:16Z

Description of changes

Fixed: A should be compatible to B, <DoesNotExist, NotIn> operator

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

Previously, we required the node to have the label foo with a value of something other than bar.

Fixed: A should be compatible to B, <Exists, Empty> operator

Node labels cannot repel a pod (see: taints), unless a pod has conflicting requirements. In this case, a pod with empty requirements should happily schedule to a node with an Exists requirement on a node.

Previously, the pod needed to have a requirement for label foo in order to schedule onto node with requirement foo: Exists.

Note: provisioner only supports [In NotIn Exists], so it wasn't currently possible to run into this bug.

Cleanup: NullSet == EmptySet

Previously, we treated the undefined set to be the same as the universal set. In set theory, the nil set and the empty set are typically modeled equivalently (https://en.wikipedia.org/wiki/Empty_set). Treating NullSet == UniversalSet set was relied upon in requirements.Add, but significantly increased the complexity of our compatibility checks. Instead, we intersect requirements in requirements.Add if a requirement for the given key already exists, otherwise we use the added requirement as-is.

Cleanup: Compatibility Checks

Previously we oriented the logic around NodeSelectorOp rules via requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)). Instead, we can use the properties of the underlying set (e.g. Full, Empty, Positive, Negative). This collapses O(n^2) iteration through requirements to O(n), and simplifies the predicates.

Cleanup: Requirement Validation

Previously, requirement validation logic redundantly checked that DoesNotExist operators did not coexist with other operators as well as ensuring that valid values exist for any requirement with an operator other than DoesNotExist. This has been collapsed into a single check, and no longer relies on requirements.hasRequirement(withKeyAndOperator. This also collapses O(n^2) iteration through requirements to O(n), and simplifies the predicates.

3. How was this change tested?

make test
Manually

kind: Provisioner
metadata:
  name: default
spec:
  requirements:
  - key: isolation
    operator: Exists

apiVersion: v1
kind: Pod
metadata:
  name: inflate-6d7d985f4d-8pkgx
  namespace: default
spec:
  containers:
  - image: public.ecr.aws/eks-distro/kubernetes/pause:3.2
    name: inflate
    resources:
      requests:
        cpu: "1"
  nodeSelector:
    isolation: ellis

apiVersion: v1
kind: Node
metadata:
  annotations:
    node.alpha.kubernetes.io/ttl: "0"
  creationTimestamp: "2022-03-19T19:47:34Z"
  finalizers:
  - karpenter.sh/termination
  labels:
    isolation: ellis <------------------------------ Here it is!
    karpenter.sh/capacity-type: on-demand
    karpenter.sh/provisioner-name: default
    node.kubernetes.io/instance-type: t3a.micro
    topology.kubernetes.io/zone: us-west-2b
  name: ip-192-168-120-29.us-west-2.compute.internal

4. Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: link to issue
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2022-03-19T19:00:22Z

✅ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: dff9703

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/62392661df9ab20009933e06

ellistarn · 2022-03-19T19:12:41Z

pkg/apis/provisioning/v1alpha5/requirements.go

 		}
+		if existing, ok := r.requirements[requirement.Key]; ok {


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.

ellistarn · 2022-03-19T19:13:43Z

pkg/apis/provisioning/v1alpha5/requirements.go

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


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

ellistarn · 2022-03-19T19:16:39Z

pkg/apis/provisioning/v1alpha5/suite_test.go

 			A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists})
 			B := NewRequirements()
-			Expect(A.Compatible(B)).ToNot(Succeed())
+			Expect(A.Compatible(B)).To(Succeed())


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

ellistarn · 2022-03-19T19:17:53Z

pkg/apis/provisioning/v1alpha5/suite_test.go

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


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.

…tion logic

tzneal · 2022-03-20T00:32:59Z

pkg/apis/provisioning/v1alpha5/requirements.go

-			r.requirements[requirement.Key] = sets.NewSet()
+			values = sets.NewSet()
+		default:
+			values = sets.NewSet()


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

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

tzneal · 2022-03-20T00:41:40Z

pkg/utils/sets/sets.go

+	if s.Len() > 0 {
+		return v1.NodeSelectorOpIn
+	}
+	return v1.NodeSelectorOpDoesNotExist
 }


pkg/apis/provisioning/v1alpha5/requirements.go

felix-zhe-huang · 2022-03-22T00:21:13Z

pkg/apis/provisioning/v1alpha5/constraints.go

@@ -76,7 +76,7 @@ func (c *Constraints) GenerateLabels() map[string]string {
 			case v1.NodeSelectorOpIn:
 				labels[key] = c.Requirements.Get(key).Values().UnsortedList()[0]
 			case v1.NodeSelectorOpExists, v1.NodeSelectorOpNotIn:
-				labels[key] = rand.String(10)
+				labels[key] = rand.String(10) // At size of the universe odds, NotIn could conflict


Can we resolve the conflict? We use to do it with for(r.Get(key).has(label)){}.

Or better yet, why bother assigning a node label for NotIn? We should just ignore it and k8s will have no problem assigning the pod to the node.

Wow even better 🤯

felix-zhe-huang · 2022-03-22T00:29:26Z

I love this new requirement logics! So clean. I would like to walk through the unit tests again later according to these new logics to make sure that nothing is missed.

…ements.Compatible()

felix-zhe-huang

Love it! Ship it!

ellistarn force-pushed the requirements branch 2 times, most recently from 4cb053d to e4f5519 Compare March 19, 2022 19:10

ellistarn commented Mar 19, 2022

View reviewed changes

Resolved an issue with NotExists requirements and simplified intersec…

50ce3a8

…tion logic

ellistarn force-pushed the requirements branch from e4f5519 to 50ce3a8 Compare March 19, 2022 19:20

ellistarn changed the title ~~Resolved an issue with NotExists requirements and simplified intersection logic~~ Resolved two bugs with NodeAffinity and simplified intersection logic Mar 19, 2022

tzneal previously approved these changes Mar 20, 2022

View reviewed changes

ellistarn dismissed tzneal’s stale review via 1731409 March 20, 2022 18:27

tzneal previously approved these changes Mar 20, 2022

View reviewed changes

ellistarn dismissed tzneal’s stale review via ffbfd03 March 20, 2022 20:19

ellistarn changed the title ~~Resolved two bugs with NodeAffinity and simplified intersection logic~~ Resolved two bugs with NodeAffinity and simplified compatibility logic Mar 20, 2022

ellistarn mentioned this pull request Mar 21, 2022

fix bug with pod node affinity testing #1550

Merged

3 tasks

felix-zhe-huang reviewed Mar 21, 2022

View reviewed changes

pkg/apis/provisioning/v1alpha5/requirements.go Show resolved Hide resolved

felix-zhe-huang reviewed Mar 21, 2022

View reviewed changes

pkg/apis/provisioning/v1alpha5/requirements.go Outdated Show resolved Hide resolved

ellistarn force-pushed the requirements branch 3 times, most recently from 5262819 to 224dfd1 Compare March 21, 2022 22:33

felix-zhe-huang reviewed Mar 22, 2022

View reviewed changes

ellistarn force-pushed the requirements branch from acaefd2 to bedcf28 Compare March 22, 2022 01:22

Simplified validation logic and deferred correctness checks to requir…

dff9703

…ements.Compatible()

ellistarn force-pushed the requirements branch from bedcf28 to dff9703 Compare March 22, 2022 01:29

felix-zhe-huang approved these changes Mar 22, 2022

View reviewed changes

felix-zhe-huang merged commit d526a49 into aws:main Mar 22, 2022

ellistarn deleted the requirements branch March 22, 2022 02:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

ellistarn commented Mar 19, 2022 •

edited

Loading

netlify bot commented Mar 19, 2022 •

edited

Loading

ellistarn Mar 19, 2022 •

edited

Loading

ellistarn Mar 19, 2022 •

edited

Loading

ellistarn Mar 19, 2022

ellistarn Mar 19, 2022

tzneal Mar 20, 2022

ellistarn Mar 20, 2022

tzneal Mar 20, 2022

felix-zhe-huang Mar 22, 2022

felix-zhe-huang Mar 22, 2022

ellistarn Mar 22, 2022

felix-zhe-huang commented Mar 22, 2022 •

edited

Loading

felix-zhe-huang left a comment

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

Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

Resolved two bugs with NodeAffinity and simplified compatibility logic #1549

Conversation

ellistarn commented Mar 19, 2022 • edited Loading

Description of changes

Fixed: A should be compatible to B, <DoesNotExist, NotIn> operator

Fixed: A should be compatible to B, <Exists, Empty> operator

Cleanup: NullSet == EmptySet

Cleanup: Compatibility Checks

Cleanup: Requirement Validation

netlify bot commented Mar 19, 2022 • edited Loading

ellistarn Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

ellistarn Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix-zhe-huang commented Mar 22, 2022 • edited Loading

felix-zhe-huang left a comment

Choose a reason for hiding this comment

ellistarn commented Mar 19, 2022 •

edited

Loading

netlify bot commented Mar 19, 2022 •

edited

Loading

ellistarn Mar 19, 2022 •

edited

Loading

ellistarn Mar 19, 2022 •

edited

Loading

felix-zhe-huang commented Mar 22, 2022 •

edited

Loading