Add cleanup function to remove all the objects from a bucket after tests #835
Conversation
nitisht
force-pushed
the
cleanup-tests
branch
from
d2ac531
to
6a62f16
Compare
functional_tests.go
Outdated
| // Remove the newly created bucket. | ||
| if err = c.RemoveBucket(bucketName + ".withperiod"); err != nil { | ||
| failureLog(function, args, startTime, "", "Remove bucket failed", err).Fatal() | ||
| // cleanup |
There was a problem hiding this comment.
Add more descriptive comment rather than place holder like cleanup
There was a problem hiding this comment.
Agreed. Updated the comments
functional_tests.go
Outdated
| @@ -1613,7 +1608,7 @@ func testPutObjectWithContext() { | |||
| args := map[string]interface{}{ | |||
| "bucketName": "", | |||
| "objectName": "", | |||
| "opts": "minio.PutObjectOptions{ContentType:objectContentType}", | |||
There was a problem hiding this comment.
why this change? shouldn't we add this?
nitisht
force-pushed
the
cleanup-tests
branch
from
6a62f16
to
4a77076
Compare
functional_tests.go
Outdated
| buckets, err := c.ListBuckets() | ||
|
|
||
| for _, bucket := range buckets { | ||
| for obj := range c.ListObjects(bucket.Name, "", true, doneCh) { |
There was a problem hiding this comment.
you can put each ListObjects() on a go-routine since they are independent buckets and clean them up.
nitisht
force-pushed
the
cleanup-tests
branch
4 times, most recently
from
3059a7c
to
5229da4
Compare
functional_tests.go
Outdated
| } | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
this function will end up cleaning more than the buckets created by this minio-go functional test. Shouldn't you be checking whether the bucket-prefix matches minio-go and then delete?
There was a problem hiding this comment.
Yes it will delete everything on the server. Let me add the bucket prefix check
functional_tests.go
Outdated
| // Indicate to our routine to exit cleanly upon return. | ||
| defer close(doneCh) | ||
|
|
||
| buckets, err = c.ListBuckets() |
There was a problem hiding this comment.
Return on error right here..
functional_tests.go
Outdated
| err = c.RemoveObject(bucketName, object.Key) | ||
| } | ||
| } else { | ||
| err = object.Err |
There was a problem hiding this comment.
Do not set the err from outside scope in a go-routine, collect all errors.. in a errs []error and then store them based on the buckets index. like this
for i, bucket := range buckets {
go func(i int, bucketName string) {
....
errs[i] = err
}(i, bucket)
}There was a problem hiding this comment.
Also since there is no wait group, this won't delete all buckets all the time. Since the program might exit if this function returns after all go-routines have spawned.
Use wait group to limit yourself and use wg.Wait() to wait for cleaning up of all buckets. Once done loop through
for _, err := range errs {
if err != nil {
return err
}
}
nitisht
force-pushed
the
cleanup-tests
branch
11 times, most recently
from
e486703
to
057573d
Compare
nitisht
changed the title
|
Updated the PR with a simpler approach to cleanup, since previous approach failed if there are multiple instances of Please take a look @harshavardhana @poornas |
| return err | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
what if there are incomplete upload parts? that would need cleanup as well.
There was a problem hiding this comment.
can you explain a bit more @poornas
There was a problem hiding this comment.
if there are incomplete uploads to a bucket, RemoveBucket would fail. For tests using large uploads, shouldn't the cleanup also remove incomplete uploads if any?
There was a problem hiding this comment.
RemoveBucket works fine on all gateways even with incomplete uploads, that is the S3 behavior. But it is a good change since it will cater for B2 gateway backend as well. Which requires explicit removal of incomplete uploads.
functional_tests.go
Outdated
| for objPartInfo := range c.ListIncompleteUploads(bucketName, "", true, doneCh) { | ||
| if objPartInfo.Err != nil { | ||
| return objPartInfo.Err | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block
functional_tests.go
Outdated
| for objCh := range c.ListObjectsV2(bucketName, "", true, doneCh) { | ||
| if objCh.Err != nil { | ||
| return objCh.Err | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block
Is this true that it partially fixes? @nitisht |
Let me change it to fixes, initially there were stale buckets left by minio-js as well but that is not seen anymore. |
Fixes : minio/mint#165
and improves logging by adding args in few test cases.