search

aws-solutions / distributed-load-testing-on-aws

Distributed Load Testing on AWS
https://aws.amazon.com/solutions/implementations/distributed-load-testing-on-aws/
Other
336 stars 121 forks source link

Do more cleanup on delete #180

Open benbridts opened 6 months ago

benbridts commented 6 months ago

Description of changes: This change will:

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

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 commented 2 months ago

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

kamyarz-aws commented 2 months ago

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 commented 2 months ago

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:

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?