feat: backwards compatible labelling per node per nodepool. #1185
Conversation
|
@valkenburg-prevue-ch Looks great! I will review ASAP. It's something that might help us too with the placement group issue we had. See comment by @mnencia here on his findings: #1161 (comment). And the original issue here #1157. If you can address this too, I would be super greatful! |
Thanks! I'll make some small modifications after @maggie44 already went through everything and proposed some improvements. |
valkenburg-prevue-ch
force-pushed
the
feat/individual-nodes-in-nodepools
branch
from
efeb1ee
to
cfadcea
Compare
|
@maggie44 @mnencia @aleksasiriski IMHO this PR is ready for review, I addressed the discussions (please check them, although I marked them as resolved). |
|
Oh by the way, added a section in the readme for migrating from count-based to map-based node pools, it's super simple in the end. |
|
Thank you. I'll review the PR ASAP. |
Actually, now that I read the placement group issue, I realize that this PR is broken when someone uses node keys > 10 or simply more than 10 nodes, as the placement group logic doesn't account for this sparse use of indices. I think I'll make a separate PR which fixes the placement group bug, but I'm struggling to find an implementation which is backwards compatible. |
|
@valkenburg-prevue-ch Good job with #1217 fixing the placement group logic. So this current PR is ready too? 🙏 |
Yes! I think both are ready. But if course to be tested live in a staging environment, I guess. |
|
@kube-hetzner/core Please review if you can, super important PR. I think it adds loads of possibility to the agent nodes setup without making the code overly complex. @maggie44 You also please, you are one of the main stakeholders here ;) I intend to merge and release tonight if no opposition comes up by then. |
There was a problem hiding this comment.
This is such a great change! Opens up a whole new realm of control, can't wait to get it on to all the clusters.
Thanks @mysticaltech for the ping.
|
@valkenburg-prevue-ch In light of the new review(s), will await your signal to merge this 🙏 |
And now you can remove / replace individual nodes in a nodepool, instead of having to set the count all the way to zero to make sure that the one sick node is removed. This is an example implementation of the discussion #1038 .
…el, in order to facilitate migrating from counts to nodes as a map.
Following suggestion from @maggie44 .
valkenburg-prevue-ch
force-pushed
the
feat/individual-nodes-in-nodepools
branch
from
742fa22
to
d1147b1
Compare
|
Rebased on master, with some added code for making the placement-group logic work well with the individual node configuration. Adopted comments from @maggie44 . New review, anyone? |
Awesome, will review ASAP 🙏 @kube-hetzner/core FYI, that's an important change, your input is always valued! |
There was a problem hiding this comment.
If this is well tested for backward compatibility, and I guess that it is, looks great! Super powerful multi-layered logic there 🫡
@mnencia If you can find the time to review this it would be great.
|
@kube-hetzner/core This is merging soon, if you want to review it's now or never! |
|
@valkenburg-prevue-ch @maggie44 Shipped in v2.13.0, thanks for the wonderful work people! |
And now you can remove / replace individual nodes in a nodepool, instead of having to set the count all the way to zero to make sure that the one sick node is removed.
This is an example implementation of the discussion #1038 .