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

Clean up orphaned instances and security groups (HMS-3632) #4513

Merged

Conversation

schuellerf
Copy link
Contributor

This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as
    • submit a PR for the READMEs listed here
    • submit a PR for the osbuild.org website repository if this PR changed any behavior not covered by the automatically updated READMEs

Implement removal of orphaned items as described in https://issues.redhat.com/browse/HMS-3632
removal of launch templates is not yet implemented (that's why it's a draft for now)

@schuellerf schuellerf requested a review from croissanne December 3, 2024 17:01
@schuellerf schuellerf force-pushed the HMS-3632-clean-up-orphaned-instances branch 4 times, most recently from 63f34e1 to 3103a97 Compare December 4, 2024 11:39
@schuellerf schuellerf marked this pull request as ready for review December 4, 2024 11:39
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

In general we don't put the jira ticket in the commit message itself, just the PR title.

edit: woops clicked review too soon, still reviewing.

internal/cloud/awscloud/maintenance.go Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

thanks, looks good overall

Could you reword this commit message though?

Usually the parent does not exist when also the secure instance is
older than 2h, but this an extra safeguard that secure instances
without a valid parent instance does not make sense to keep.

And then the jira tickets tend to just go in the PR descriptions.

cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
@schuellerf
Copy link
Contributor Author

And then the jira tickets tend to just go in the PR descriptions.

I think they should be part of the commit message
https://osbuild.org/docs/developer-guide/general/workflow
but I'll move it to the "git commit subject" that's more readable for sure

@schuellerf schuellerf force-pushed the HMS-3632-clean-up-orphaned-instances branch 2 times, most recently from ba65a81 to 7684e9e Compare December 5, 2024 16:09
@schuellerf schuellerf requested a review from croissanne December 5, 2024 16:48
@schuellerf schuellerf force-pushed the HMS-3632-clean-up-orphaned-instances branch 2 times, most recently from b1456a2 to 94492ab Compare December 6, 2024 11:16
@schuellerf schuellerf enabled auto-merge (rebase) December 6, 2024 11:18
Support running the maintenance locally with a valid
`~/aws/credentials` file. HMS-3632
@schuellerf schuellerf force-pushed the HMS-3632-clean-up-orphaned-instances branch from 94492ab to fa02bac Compare December 9, 2024 09:10
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this - quick drive-by comments/suggestions (purely on the tactical level :)

cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
cmd/osbuild-service-maintenance/aws.go Outdated Show resolved Hide resolved
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

other than @mvo5's comments lgtm :)

@schuellerf schuellerf force-pushed the HMS-3632-clean-up-orphaned-instances branch 2 times, most recently from f4d001a to ca3c904 Compare December 9, 2024 12:33
Add a safeguard to ensure secure instances without valid
parent instances are terminated, as they are unnecessary to retain.
Typically, the parent does not exist if the secure instance is
older than 2 hours, but this check provides additional validation.
HMS-3632
Security groups of instances that are terminated should be removed.
HMS-3632
Launch templates of instances that are terminated should be removed.
HMS-3632
Passing an empty list to `TerminateInstances` causes an
error message, which is not necessary, as there is
nothing to terminate.
Collect and merge errors, instead of
nesting errors.
In this case we want to continue execution if only one
cleanup fails.
@schuellerf schuellerf force-pushed the HMS-3632-clean-up-orphaned-instances branch from ca3c904 to cae703a Compare December 9, 2024 12:35
@schuellerf schuellerf requested review from croissanne and mvo5 December 9, 2024 12:36
@schuellerf schuellerf disabled auto-merge December 10, 2024 10:37
@schuellerf schuellerf enabled auto-merge (rebase) December 10, 2024 10:38
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

LGTM, ty!

@schuellerf schuellerf merged commit 153bcad into osbuild:main Dec 10, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants