-
Notifications
You must be signed in to change notification settings - Fork 302
docs: Node Overlay RFC #2166
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
docs: Node Overlay RFC #2166
Conversation
Pull Request Test Coverage Report for Build 16333981357Details
💛 - Coveralls |
ce506d2
to
c155548
Compare
d3ef8d2
to
1987b1c
Compare
Thanks for tackling this problem with a new revision! |
fa43c6a
to
e2e038b
Compare
72592c5
to
2241bf1
Compare
Hello all, I wanted to provide a short consumer perspective on this proposed feature. Our organization has been in demand for several changes that would benefit from the proposed
We're still exploring where new nodepools would be necessary, but if these features were to be implemented directly into the NodePool API I fear it would near exponentially increase the number of node pools we'd need to maintain as Karpenter admins. I would welcome the ease of an overlay, especially one that would target specific instances through a familiar spec. I would accept this increased blast radius and put it on our teams to ensure proper testing is done in case poor changes are performed on an overlay. Just an opinion of one Karpenter admin of course. 👍 In short, it's fantastic that Karpenter can smartly chose the right instance for the demand, but we're starting to see some shortcomings when it comes to more complex logic. An optional API to implement complex logic on a subset of nodes in a given nodepool seems like a good potential path forward In transparency, we are also looking for the ability to dynamically adjust EBS throughput and IOPS depending on instance size, but I don't think NodeOverlay would apply here since this is part of the EC2NodeClass. |
Great feedback @Beardface123! Can you expand a little bit on why you'd imagine this would increase the number of NodePools? I initially thought this, but realized an alternative path where Your comment here seems to suggest that you would scope these overrides to a nodepool anyways:
It's a bit nuanced, so I'm very curious to your thinking on this. |
What I wanted to avoid is an implementation that will require NodePools to propagate out exponentially. Given an example of a general node pool:
Given the scenario, we might need to take a single NodePools, split it to two to support the
I didn't express myself correctly, sorry about that. To correct myself I should've said "An optional API to implement complex logic on a subset of nodes across specified NodePools" If I were to try to represent my opinions concisely in a few points now that I've re-read the threads and given it another round of thought:
Ultimately, the overlay concept in general is very appealing in either implementation, but I think the preference is to use a separate API object to define overlays and have that propagate to all nodepools that reference it. The alternative is a more cumbersome experience if there are many NodePools with diverse overlays. If manually set, I'd worry about user error. If templating overlays with Helm or other tools is done, well that's time my team needs to figure it out. Again, sorry for the misunderstanding up front. |
Out of curiosity where happens the Karpenter logic and the NodeOverlay in this schema ?
|
@gillg The only difference that NodeOverlay will provide is that Karpenter will now be aware of possible resource that can be present on the nodes once an Overlay is defined. Karpenter will not be aware of any custom scheduler |
61dd5ea
to
6303cde
Compare
50fb097
to
9a9454f
Compare
9a9454f
to
b592e8f
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: engedaam, jonathan-innis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold |
/lgtm |
/unhold |
Fixes
Description
This RFC is an updated Node Overlay RFC original opened a 6 month ago. This RFC plans to go over what is planned
How was this change tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.