-
Notifications
You must be signed in to change notification settings - Fork 985
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
dynamically retrieve instance types #287
Conversation
7e62b95
to
ebd26a2
Compare
nc := instanceTypeInfoToNodeCapacity(*instanceTypeInfo) | ||
kubeletOverhead := binpacking.CalculateKubeletOverhead(nc.total) | ||
if ok := nc.reserve(resources.Merge(constraints.Overhead, kubeletOverhead)); !ok { | ||
zap.S().Errorf("Failed to reserve kubelet overhead for node capacity type %v", nc.instanceType) |
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.
Do we want to error here? Is debug better? IIUC, this only errors if the theoretical node is full, which might happen multiple times in simulation.
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.
I'd probably not consider it an error even - although it should never even happen, if there were some really extreme instance type in the future where perhaps it did trigger, it means the instance type cannot handle kubelet overhead at all, so we shouldn't consider using it anyway - maybe make this a log info ("excluding ...")?
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.
I think that makes sense for info level. I just brought this in from a rebase, so not sure if there was a reason for it being error.
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.
We're really judicious about Info level -- we can talk about the ideology here, maybe debug?
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.
I don't have a super strong opinion, so I can go either way. I think Jacob is right that it shouldn't ever happen the way the kubelet overhead is being calculated right now since it's a scaled a percentage. But I suppose if there was a weird instance type that was released, it could spam everyone using karpenter if it's at info level, so I'm fine with debug.
…ynamic instance type retrieval
ebd26a2
to
f147d6b
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.
Nice work. More or less LGTM, a couple nits.
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.
| | |
)_) )_) )_)
)___))___))___)\
)____)____)_____)\\
_____|____|____|____\\\__
---------\ /---------
^^^^^ ^^^^^^^^^^^^^^^^^^^^^
^^^^ ^^^^ ^^^ ^^
^^^^ ^^^
Issue #, if available:
#286
Description of changes:
ec2.DescribeInstanceTypes
and transforms to nodeCapacity information.go mod tidy
which updated some go.sum stuffBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.