-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Delete Launch templates on deletion of EC2NodeClass #5453
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
Pull Request Test Coverage Report for Build 7629633106
💛 - Coveralls |
NIce work 🎉 A high level thing, since each cluster can have multiple EC2NodeClasses we really only want to delete the LaunchTemplates which are 'owned' by that NodeClass. We just need to keep in mind there's not a 1-1 mapping between launch templates, some EC2NodeClasses could share launch templates and some could have multiple. |
241fa45
to
0bb14d5
Compare
Thank you. Also, that makes sense. I have updated the LT deletion in such a way that it will check for a tag that corresponds to the nodeclass it is associated with. Also the tag would be added to LT on creation now. The hashed LT name now also contains clusterName and nodeClassName. |
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 🎉
0bb14d5
to
0c86a4b
Compare
Do we have an E2E test to validate all of the launch templates are gone when we delete the EC2NodeClass? Take a look at https://github.com/aws/karpenter-provider-aws/blob/main/test/suites/integration/instance_profile_test.go#L47 for a similar style of test |
No, there's no E2E test to validate that. I will add it. I was thinking if it should be done in a separate PR? |
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.
/karpenter snapshot
Snapshot successfully published to
|
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.
/karpenter snapshot
Snapshot successfully published to
|
9244e17
to
21359e1
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.
/karpenter snapshot
Snapshot successfully published to
|
21359e1
to
2e894f7
Compare
7877d8c
to
2557965
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.
/karpenter snapshot
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 🚀
Snapshot successfully published to
|
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.
/karpenter snapshot
Snapshot successfully published to
|
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 🚀
Fixes #995
Description
Added a method to delete the launch templates on deletion of EC2NodeClass. Added tests for the same.
How was this change tested?
Deployed the changes on my local EKS cluster. Steps followed -
Performing the nodeclass object deletion will trigger a flow that deletes the launch templates.
Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.