Fix ManagedNodeGroup taints being wrongly set in userdata #1441
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Does the PR have any schema changes?Looking good! No breaking changes found. |
flostadler
force-pushed
the
flostadler/bottlerocket-mng-fix
branch
from
18fbcaa to
d0e7a0d
Compare
rquitales
reviewed
Oct 14, 2024
| @@ -449,7 +454,8 @@ function createBottlerocketUserData( | |||
| if (args.taints) { | |||
| const taints = {}; | |||
| const records = Object.entries(args.taints).map(([key, taint]) => { | |||
| return { [key]: `${taint.value}:${taint.effect}` }; | |||
| // empty taint values are represented as an empty string, see https://github.com/bottlerocket-os/bottlerocket/pull/1406 | |||
| return { [key]: `${taint.value ?? ""}:${taint.effect}` }; | |||
rquitales
approved these changes
Oct 14, 2024
t0yv0
reviewed
Oct 14, 2024
| coredns = eks.Addon( | ||
| f"{project_name}-cluster3-coredns", | ||
| cluster=cluster3, | ||
| addon_name="coredns", | ||
| addon_version="v1.11.1-eksbuild.9", | ||
| addon_version=coredns_version.version, |
There was a problem hiding this comment.
But great! I was fixing this again as port of ops, your solution is more robust.
t0yv0
approved these changes
Oct 14, 2024
flostadler
added a commit
that referenced
this pull request
Oct 17, 2024
The taints for the `ManagedNodeGroup` component were being wrongly calculated when using custom userdata. That was the case because the EKS service uses different capitalization for the taint effect enum than the kubernetes API(e.g. `NO_SCHEDULE` vs `NoSchedule`). When building the custom userdata we need to map the EKS style enums to kubernetes style enums, otherwise it doesn't work. Fixing this also revealed that taint values being absent aren't correctly handled either. The change fixes that as well.
|
This PR has been shipped in release v3.0.0-beta.2. |
flostadler
added a commit
that referenced
this pull request
Oct 17, 2024
The taints for the `ManagedNodeGroup` component were being wrongly calculated when using custom userdata. That was the case because the EKS service uses different capitalization for the taint effect enum than the kubernetes API(e.g. `NO_SCHEDULE` vs `NoSchedule`). When building the custom userdata we need to map the EKS style enums to kubernetes style enums, otherwise it doesn't work. Fixing this also revealed that taint values being absent aren't correctly handled either. The change fixes that as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The taints for the
ManagedNodeGroupcomponent were being wrongly calculated when using custom userdata.That was the case because the EKS service uses different capitalization for the taint effect enum than the kubernetes API(e.g.
NO_SCHEDULEvsNoSchedule).When building the custom userdata we need to map the EKS style enums to kubernetes style enums, otherwise it doesn't work.
Fixing this also revealed that taint values being absent aren't correctly handled either. The change fixes that as well.
While this was found during the v3 beta testing (see), it's a bug on v2 as well. The bug fix is targeting v3 anyways because the node group and user data logic was heavily rewritten during the development of v3. Back porting that to v2 would be too big of an effort given that v3 is right around the corner.