Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Propagation of ManagedNodeGroup Tags to their corresponding AutoScalingGroups #5002

Merged
merged 14 commits into from
Apr 25, 2022
Merged

Add Propagation of ManagedNodeGroup Tags to their corresponding AutoScalingGroups #5002

merged 14 commits into from
Apr 25, 2022

Conversation

SlevinWasAlreadyTaken
Copy link
Contributor

@SlevinWasAlreadyTaken SlevinWasAlreadyTaken commented Mar 25, 2022 •
edited
Loading

Description

Resume

I have added Propagation of ManagedNodeGroup (MNG) Tags to their AutoScalingGroups (ASG). This should resolve the issue #1571

  • The changes ensure that AutoScalingGroups Tags are the same as their corresponding ManagedNodeGroup after it has been automatically created by AWS.
  • All tags are copied from the ManagedNodeGroup to the AutoScalingGroup. If the tags already exists, it is overridden.
  • The propagation is enabled by default (as it is for Unmanaged NodeGroup) and it can be disabled using the DisableASGTagPropagation boolean in the configuration.

How it has been done

I tried to find a good place to do it, and added it as an additional task on managed group creation. I switched the Parallel task boolean to false since it requires to be Sequential: we can't propagate the MNG tags to the ASGs without having the MNG created (since the ASGs are created automatically after the MNG creation).

ℹ️ This is my first contribution, thanks for your understanding if I've missed something =)

Checklist

  • Added tests that cover your change (if possible)
    - ⚠️ As it is my first contribution, I would prefer a first review before of course, implementing them
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes

@afirth
Copy link

afirth commented Mar 25, 2022

Nice one @SlevinWasAlreadyTaken !
I agree, it makes sense to hold off on automatically generating these tags for taints and labels until #4853 is resolved

@afirth
Copy link

afirth commented Mar 25, 2022

@aclevername @Himangini will be interested I guess 💯

@afirth afirth mentioned this pull request Mar 29, 2022
Closed
@SlevinWasAlreadyTaken SlevinWasAlreadyTaken force-pushed the 2022-03-24-propagate-managed-ng-tags-to-asg branch from 9065b74 to 2fe3c88 Compare March 30, 2022 12:45
@SlevinWasAlreadyTaken
Copy link
Contributor Author

SlevinWasAlreadyTaken commented Mar 30, 2022 •
edited
Loading

@aclevername @Himangini I have rebased this PR considering this fresh changes in the way we configure the propagation of tags.

@Himangini Himangini requested a review from a team March 30, 2022 13:55
@Himangini
Copy link
Collaborator

@aclevername @Himangini I have rebased this PR considering this fresh changes in the way we configure the propagation of tags.

Thanks, @SlevinWasAlreadyTaken. We are a bit swamped atm but I have tagged the team for reviews, we'll review and get back to you.

Skarlso
Skarlso suggested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple changes required, otherwise this is on a good track! :) Thank you!

pkg/apis/eksctl.io/v1alpha5/assets/schema.json Show resolved Hide resolved
pkg/cfn/manager/create_tasks.go Outdated Show resolved Hide resolved
pkg/cfn/manager/nodegroup.go Outdated Show resolved Hide resolved
pkg/cfn/manager/nodegroup.go Outdated Show resolved Hide resolved
pkg/cfn/manager/tasks_test.go Outdated Show resolved Hide resolved
userdocs/src/usage/eks-managed-nodes.md Show resolved Hide resolved
afirth
afirth reviewed Apr 4, 2022
View reviewed changes
pkg/cfn/manager/nodegroup.go Outdated Show resolved Hide resolved
userdocs/src/usage/autoscaling.md Outdated Show resolved Hide resolved
userdocs/src/usage/eks-managed-nodes.md Show resolved Hide resolved
@Skarlso
Copy link
Contributor

Skarlso commented Apr 5, 2022

@SlevinWasAlreadyTaken hi. You need to resolve the merge so I can run the tests. :) thanks!

@SlevinWasAlreadyTaken SlevinWasAlreadyTaken force-pushed the 2022-03-24-propagate-managed-ng-tags-to-asg branch from 6939017 to cf1f7be Compare April 5, 2022 11:40
@SlevinWasAlreadyTaken
Copy link
Contributor Author

@SlevinWasAlreadyTaken hi. You need to resolve the merge so I can run the tests. :) thanks!

I just rebased!

@SlevinWasAlreadyTaken SlevinWasAlreadyTaken force-pushed the 2022-03-24-propagate-managed-ng-tags-to-asg branch from cf1f7be to 65303d8 Compare April 5, 2022 12:11
@Skarlso
Copy link
Contributor

Skarlso commented Apr 5, 2022

@SlevinWasAlreadyTaken There is something wrong with the merge?

Check failure on line 284 in pkg/cfn/manager/api.go

GitHub Actions
/ Unit tests
pkg/cfn/manager/api.go#L284
not enough arguments in call to c.asgAPI.CreateOrUpdateTags

@SlevinWasAlreadyTaken
Copy link
Contributor Author

SlevinWasAlreadyTaken commented Apr 5, 2022 •
edited
Loading

@Skarlso Indeed. The switch to aws-sdk-go-v2 went wrong with the rebase.
Should be fixed with d1255db & 057ea8b

@SlevinWasAlreadyTaken SlevinWasAlreadyTaken force-pushed the 2022-03-24-propagate-managed-ng-tags-to-asg branch 2 times, most recently from 19e6b0b to 8016bf7 Compare April 11, 2022 10:09
@Skarlso Skarlso self-requested a review April 12, 2022 12:37
Skarlso
Skarlso approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after conflict and comment resolution.

It needs another set of eyes from the Team. :)

nikimanoledaki
nikimanoledaki approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@nikimanoledaki nikimanoledaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you for addressing the change requests. LGTM once the merge conflicts are addressed 😄 🎉

@SlevinWasAlreadyTaken SlevinWasAlreadyTaken force-pushed the 2022-03-24-propagate-managed-ng-tags-to-asg branch 2 times, most recently from ec62b7f to d4e85b6 Compare April 22, 2022 08:26
@SlevinWasAlreadyTaken
Added Propagation of ManagedNodeGroup Tags to their AutoScalingGroups
c82003f 
Changes	to ensure that AutoScalingGroups Tags are the same as their
 ManagedNodeGroup.

All tags are copied from the ManagedNodeGroup to the AutoScalingGroup.
If the tags already exists, it is overridden.

This is the default behaviour (as it is for Unmanaged NodeGroup) and
can be enabled using propagateASGTags boolean configuration.

Issue eksctl-io#1571
@SlevinWasAlreadyTaken
Update generated files
58f19c7
@SlevinWasAlreadyTaken
fix existing unit tests
c88a114
@SlevinWasAlreadyTaken
update documentation
09d727d
@SlevinWasAlreadyTaken
Update generated files
b35296c
@SlevinWasAlreadyTaken
consider new way of configuring tags propagation after the rebase
43c24c8
@SlevinWasAlreadyTaken
re-add disableASGTagPropagation doc for managed nodegroup
712fa96
@SlevinWasAlreadyTaken
consider tags limits, move logic to a more appropriated place, re-add…
5a3d4ce 
… test for managernodegroup
@SlevinWasAlreadyTaken
fix usage of aws-sdk-go-v2, improve task parallelisation for managedn…
ed4061b 
…odegroup creation
@SlevinWasAlreadyTaken
fix test linting
fbd93d8
@SlevinWasAlreadyTaken
add unit tests
bea86ab
@SlevinWasAlreadyTaken
improve unit test structure for PropagateManagedNodeGroupTagsToASG, r…
505f05b 
…ename astypes package to asTypes
@SlevinWasAlreadyTaken
use const for auto-scaling-group string
27ee551
@SlevinWasAlreadyTaken
improve some code structure
be607ce
@SlevinWasAlreadyTaken SlevinWasAlreadyTaken force-pushed the 2022-03-24-propagate-managed-ng-tags-to-asg branch from d4e85b6 to be607ce Compare April 22, 2022 13:00
@SlevinWasAlreadyTaken
Copy link
Contributor Author

@nikimanoledaki 👋 conflicts addressed :)

@Skarlso
Copy link
Contributor

Skarlso commented Apr 25, 2022

@SlevinWasAlreadyTaken thank you for your contribution and sticking to this long process. :) Well done! :)

@Skarlso Skarlso merged commit 0284bc3 into eksctl-io:main Apr 25, 2022
@SlevinWasAlreadyTaken SlevinWasAlreadyTaken mentioned this pull request Apr 25, 2022
Open
Skarlso added a commit that referenced this pull request Apr 25, 2022
@Skarlso
Revert "Added Propagation of ManagedNodeGroup Tags to their correspon…
f5a541f 
…ding AutoScalingGroups (#5002)"

This reverts commit 0284bc3.
@Skarlso Skarlso mentioned this pull request Apr 25, 2022
Closed
@nikimanoledaki nikimanoledaki changed the title Added Propagation of ManagedNodeGroup Tags to their corresponding AutoScalingGroups Add Propagation of ManagedNodeGroup Tags to their corresponding AutoScalingGroups Apr 25, 2022
This was referenced Apr 28, 2022
Closed
Closed
@hspencer77 hspencer77 mentioned this pull request Jul 8, 2022
Closed
7 tasks
@rwilson-release rwilson-release mentioned this pull request Feb 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skarlso Skarlso Skarlso approved these changes

@afirth afirth afirth left review comments

@Himangini Himangini Himangini approved these changes

@cPu1 cPu1 Awaiting requested review from cPu1

@nikimanoledaki nikimanoledaki Awaiting requested review from nikimanoledaki

Assignees
No one assigned
Labels
kind/improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SlevinWasAlreadyTaken @afirth @Himangini @Skarlso @nikimanoledaki