-
Notifications
You must be signed in to change notification settings - Fork 983
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.
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
our price ordering no longer provides the guarantee that made this work #1555
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
bestPackedPods = result.packed | ||
remainingPods = result.unpacked | ||
break | ||
// reached the max that we care about | ||
if len(bestInstances) == MaxInstanceTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we filter out instance types in the CP now, it might make sense to just move the MaxInstanceTypes to the CP or increase this value. It might make the bin-packer do a lot more work though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, build this on a branch and whole hog move towards a unified scheduler/binpacker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the next step, but right now it sometimes creates instance types that are too small. Was hoping to get this in as a patch before any release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we filter out instance types in the CP now, it might make sense to just move the MaxInstanceTypes to the CP or increase this value. It might make the bin-packer do a lot more work though...
I think it will with a converged scheduler/bin packer. I think scheduling costs will dominate any bin packing cost anyway.
53940b7
to
6453d73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Get an approval from @bwagner5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
1. Issue, if available:
N/A
2. Description of changes:
Remove packing optimization that doesn't work with arbitrary price sorting.
3. How was this change tested?
Unit tests & by hand 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.