Gh 22150 #671
Gh 22150 #671
Conversation
|
Thank you for this, @s4kh! I've been out since Friday, so I'll take a look as soon as I can! Thanks for your patience <3 |
There was a problem hiding this comment.
Thank you for your work on this, @s4kh! Before doing a line-by-line review, we should discuss how to make this in batches by paginating over all the objects in the bucket. It's not completely rare that a bucket in a load-test deployment contains hundreds of thousands or even millions of files, so listing all of them and then deleting them in one go is not feasible. We can do this by setting the ContinuationToken in ListObjectsV2 param to the NextContinuationToken returned by the previous ListObjectsV2 output (there's more info on these in https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html and in https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/s3#Client.ListObjectsV2 (see also the docs for ListObjectsV2Input and ListObjectsV2Output)). With this approach, we can list, say, 1000 objects and delete them, list another 1000 objects and delete those, and so on.
Let me know if I can help with this!
|
@s4kh - have you tested this? |
Extracted the part and tested it. |
|
I won't be able to properly review this until after holidays (I'm back on January 8). Sorry for the delay on this! cc/@streamer45 in case someone else from the team can review this in the meantime. If not, I'm happy to do it when I'm back. |
|
Thanks @s4kh ! Will let @agarciamontoro take a look. |
There was a problem hiding this comment.
First of all: sorry for the incredible delay, all my bad.
I finally went through the code and tested it manually, and with some changes here and there, detailed in the comments below, it did work! It was quite slow, though (it copies 1000 files every 3-4 minutes, and the buckets may contain dozens of thousands of files) so we may want to parallelize this as much as possible. Given the long life of this PR, though, I'd be happy to merge it as soon as the requested changes are solved, and we can tackle the optimization later.
Thank you for all your work, @s4kh, and for your patience, and sorry again for the long wait.
|
@agarciamontoro addressed all the requested changes, if I have missed something feel free to request a change again. |
There was a problem hiding this comment.
Thank you, @s4kh! I left a couple more nits, but nothing major. I will test it again with the current code in the meantime.
|
Didn't report it here, but the test was successful with the latest changes, so once my requested changes are addressed, I'm good to give this the green light! |
|
@agarciamontoro addressed the requested changes and resolved the conflict. If I have missed something please feel free to request a change. |
|
Going to defer to Alejandro's review here. |
There was a problem hiding this comment.
Sorry for the late answer, @s4kh! I left a couple more comments and it should be good to go!
|
Hey, @s4kh, let me know if there is anything I can do to help with this! |
|
@agarciamontoro - could we just wrap up the remaining work ourselves? Something for the fixathon perhaps. |
|
@agarciamontoro @agnivade addressed the requested changes. |
|
Will let @agarciamontoro take a look. |
|
Thank you, @s4kh! Code looks good to me, I'm running a test deployment to double-check all is good. I'll get back to you when it's done :) |
| } | ||
|
|
||
| resetBucketCmds = []localCmd{deleteBucketCmd, prepopulateBucketCmd} | ||
| cfg, err := config.LoadDefaultConfig(context.Background(), config.WithSharedConfigProfile(t.Config().AWSProfile)) |
There was a problem hiding this comment.
@agarciamontoro in the future if we want to integrate with Github Actions this profile AWS access might not be eligible . We might need to perform a small refactor but it's simple
There was a problem hiding this comment.
Specifically we might need to use config.LoadDefaultConfig to ensure stability across envs
There was a problem hiding this comment.
@toninis, do you mean using another profile? We are using config.LoadDefaultConfig, just using the profile configured by the user in the deployer.json file.
|
Test finished successfully! It took around 30 minutes to empty the bucket and populate it with the copy of another 500MB bucket, but I'd say this is a good start and we can work from here. @agnivade, do you want to take an additional look or are you ok merging this already? |
|
Thank you for the infinite patience, @s4kh! |
Summary
Uses aws SDK v2 for resetting buckets for load test.
Ticket Link
mattermost/mattermost#22150