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

Do more cleanup on delete #180

Closed
wants to merge 4 commits into from

Conversation

benbridts
Copy link

Description of changes:
This change will:

  • cleanup files from the storage bucket when scenario's are deleted
  • delete result files after a configurable amount of days (default: 10 years)
  • delete non-current versions in s3 after a configurable amount of days (default: 1 year)

This seemed to use as a good compromise between things that are easy to delete (the test scripts) and things that would need an s3 list-objects (results, where we use a lifecycle instead)

I also included a script to look for resources for test that were already deleted

Checklist

  • [] 👋 I have run the unit tests, and all unit tests have passed.
  • ⚠️ This pull request might incur a breaking change.

Unit tests: They passed when run on our fork, but currently fail. Unit tests on the main branch also fail for me, so it seems unrelated to this change. I can rebase this branch after the main branch is fixed.

Breaking change: Debatable, it is a change in behaviour, but I don't think anyone would be relying on files sticking around for that long

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bogdankatishev
Copy link

Ping, any update on this? :) @kamyarz-aws

@kamyarz-aws
Copy link
Member

  • I dont see the value in orphaned-assets scripts. I dont think we need to add it to this CR.
  • I am not very sure what are you trying to achieve with lifecycle rule
  • From your description "delete non-current versions in s3 after a configurable amount of days (default: 1 year) "I am not sure what you mean by this
  • I see some values in deleting items from S3 buckets after deleting a test/scenario. However this should be on demand, but I couldnt find anything in your code that adjusts the GUI for that matter.

My suggestions
Please, Remove the orphaned-assets scripts, provide proper explanation for lifecycle rule or remove it. Provide some explanation "delete non-current versions in s3 after a configurable amount of days (default: 1 year) " . Adjust the GUI so on delete of a scenario as part of the pop up users agree that residual assets in S3 buckets are removed.

@benbridts
Copy link
Author

benbridts commented Jul 26, 2024

I see some values in deleting items from S3 buckets after deleting a test/scenario. However this should be on demand, but I couldnt find anything in your code that adjusts the GUI for that matter.

Why do we want this to be different from how other data is treated? We remove the config, history, and (cloudwatch) dashboards without any extra confirmations

I dont see the value in orphaned-assets scripts. I dont think we need to add it to this CR.

It does these two things, but I don't mind removing it from this PR:

  • It helps with debugging (and was very helpful creating this PR)
  • It shows which results are still around, without any test refering the them (which might be useful for manual cleanup instead of relying on a lifecycle rule)

I am not very sure what are you trying to achieve with lifecycle rule

When you delete a test, there are two kinds of files left over in S3 (that nothing refers to anymore): the test scenario's (and assets) and the test results. Test scenario's are easy to remove and have a bounded number of API calls that are needed to do that. Result files are unbounded. So to avoid an unknown runtime (and a bunch of list + delete calls), I decided to leave results in S3 when scenario's are deleted and instead delete any result older than 10 years.

From your description "delete non-current versions in s3 after a configurable amount of days (default: 1 year) "I am not sure what you mean by this

Since versioning is enabled, deleting (or overwriting) files from S3 does not actually completely removes them. I added a lifecycle rule that removes versions that are not the current version after a year (we have backups for 35 days in dynamodb so this is a bunch longer).

My suggestions

Please, Remove the orphaned-assets scripts

That shouldn't be a problem

provide proper explanation for lifecycle rule or remove it.

I can copy part of the explanation, if we agree that the approach here is acceptable

Provide some explanation "delete non-current versions in s3 after a configurable amount of days (default: 1 year) " .

I can copy part of the explanation above

Adjust the GUI so on delete of a scenario as part of the pop up users agree that residual assets in S3 buckets are removed.

Can you look at my remark about this being different first?

@kamyarz-aws kamyarz-aws self-assigned this Aug 8, 2024
@kamyarz-aws
Copy link
Member

Hi, I went through the CR again, and we brought it up internally. I understand that you might not see value in your case in keeping certain assets in the s3 buckets. But specifically for the logs we think many clients may want to keep them in their buckets for longer period.

We decided to hold this features until we collect more signals from our clients to come up with feature description and solution suitable for more clients. For now we think individual clients can manage their assets on their accord until we understand more clients needs and apply certain changes to resolve this.

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