fix bug with pod node affinity testing #1550
Conversation
|
✅ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 79d31a5 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6237cc842bd3c10008b0cfa0 |
tzneal
force-pushed
the
refactor-and-fix-node-affinity-bug
branch
from
32c4b9d to
c5b8096
Compare
tzneal
force-pushed
the
refactor-and-fix-node-affinity-bug
branch
from
c5b8096 to
c04fd70
Compare
| @@ -80,14 +58,14 @@ func (n Node) Compatible(pod *v1.Pod) error { | |||
| return err | |||
| } | |||
|
|
|||
| tightened := n.Constraints.Requirements.Add(podRequirements.Requirements...) | |||
There was a problem hiding this comment.
This feels like we're tripping over ourselves a bit. This is a tight loop that scales linearly with pods. We're already adding these requirements together inside n.Constraints.Requirements.Compatible(podRequirements), and then we're doing it again here. Effectively, we have two areas of the code base responsible for the same thing (e.g. checking if a pod can schedule to a node), but done at different levels of abstraction.
As you've rightly done, we don't want to push this logic into the API package to create a dependency between v1alpha5 -> cloudprovider. Instead, if we could move the compatibility logic up to this layer, I think a lot of this impedance mismatch goes away. Obviously, we're blocked on this right now due to #1549, but it might be worth exploring once that's merged.
I'm happy to merge this PR as it is currently, but would love to remove this rough edge for simplicity/performance.
There was a problem hiding this comment.
I was debating if the initial compatibility check was required when I wrote this and ended up with the conclusion that I wasn't certain that our Add() was written to handle possibly incompatible requirements, so it felt safer to keep it in.
There was a problem hiding this comment.
Just verified that the outer compatibility check handles the stuff that's not instance related (e.g. hostname requirements to implement topology spread).
There was a problem hiding this comment.
Not currently. You'd need to smoosh em together :D
tzneal
force-pushed
the
refactor-and-fix-node-affinity-bug
branch
from
c04fd70 to
79d31a5
Compare
1. Issue, if available:
N/A
2. Description of changes:
Adds a test and fixes a bug related to not checking pod node affinity.
3. How was this change tested?
Unit test + on EKS
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.