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.

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

🐛 AWSMachinePool: Prune old Launch Template versions #2525

Merged
merged 2 commits into from Jun 22, 2021
Merged

🐛 AWSMachinePool: Prune old Launch Template versions #2525

merged 2 commits into from Jun 22, 2021

Conversation

dlipovetsky
Copy link
Contributor

@dlipovetsky dlipovetsky commented Jun 19, 2021 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Remove one old version before creating a new version.

Fixes #2368

Special notes for your reviewer:
I'm not sure what sort of unit test we want to see.

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

AWSMachinePool controller removes one old LaunchTemplate version before creating a new version, preventing the number of versions from growing without bound, and reaching the maximum limit.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Jun 19, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 19, 2021
@dlipovetsky dlipovetsky force-pushed the dlipovetsky/prune-launchtemplateversion branch from 0c89d6b to 8a94a14 Compare June 19, 2021 21:54
🐛 AWSMachinePool: Prune old Launch Template versions
a1bba0a 
Remove one old version before creating a new version.
@dlipovetsky dlipovetsky force-pushed the dlipovetsky/prune-launchtemplateversion branch from 8a94a14 to a1bba0a Compare June 19, 2021 21:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 19, 2021
@dlipovetsky dlipovetsky changed the title fix (MachinePool): Prune old Launch Template versions 🐛 AWSMachinePool: Prune old Launch Template versions Jun 19, 2021
@dlipovetsky
Copy link
Contributor Author

Once this is approved, I'd like to backport to release-0.6

@randomvariable
Copy link
Member

Once this is approved, I'd like to backport to release-0.6

sgtm

@randomvariable
Copy link
Member

/priority important/soon

@k8s-ci-robot
Copy link
Contributor

@randomvariable: The label(s) priority/important/soon cannot be applied, because the repository doesn't have them.

In response to this:

/priority important/soon

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@randomvariable randomvariable added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 21, 2021
sedefsavas
sedefsavas reviewed Jun 21, 2021
View reviewed changes
exp/controllers/awsmachinepool_controller.go
@@ -455,6 +455,11 @@ func (r *AWSMachinePoolReconciler) reconcileLaunchTemplate(machinePoolScope *sco
// userdata, OR we've discovered a new AMI ID.
if needsUpdate || tagsChanged || *imageID != *launchTemplate.AMI.ID || launchTemplateUserDataHash != bootstrapDataHash {
machinePoolScope.Info("creating new version for launch template", "existing", launchTemplate, "incoming", machinePoolScope.AWSMachinePool.Spec.AWSLaunchTemplate)
// There is a limit to the number of Launch Template Versions.
// We ensure that the number of versions does not grow without bound by following a simple rule: For each version we create, we delete one old version.
if err := ec2svc.PruneLaunchTemplateVersions(machinePoolScope.AWSMachinePool.Status.LaunchTemplateID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to create one before pruning? In case creation fails we don't want to delete the previous one.
We create a new one, it is tagged as latest, so the previous can be deleted.

CreateLaunchTemplateVersion returns the version created, how about directly trying to delete the previous version? Assuming the numbering is strictly increasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to create one before pruning?

If we create one before removing one, then we may end up never removing any. Whether or not we return an error when we fail to remove one, we'll create more than we'll remove, and therefore eventually reach the limit.

In case creation fails we don't want to delete the previous one.
We create a new one, it is tagged as latest, so the previous can be deleted.

I took care to never remove the version that is currently "in use." Please see the PruneLaunchTemplateVersions godoc, and let me know if anything is unclear. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've revised the comment at this line. I hope it's more clear now.

@sedefsavas
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit 6297c7d into kubernetes-sigs:main Jun 22, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.7.0 milestone Jun 22, 2021
@dlipovetsky
Copy link
Contributor Author

Thanks for reviewing, @sedefsavas!

@dlipovetsky dlipovetsky mentioned this pull request Jun 22, 2021
Merged
4 tasks
@sedefsavas sedefsavas mentioned this pull request Apr 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sedefsavas sedefsavas sedefsavas left review comments

@dthorsen dthorsen Awaiting requested review from dthorsen

@randomvariable randomvariable Awaiting requested review from randomvariable

Assignees

@sedefsavas sedefsavas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging this pull request may close these issues.

AWSMachinePool controller should clean up unused Launch Template Versions
4 participants
@dlipovetsky @randomvariable @k8s-ci-robot @sedefsavas