Handle BucketAlreadyExists races and bump arsenal for MPU afterEach fix#6222
Handle BucketAlreadyExists races and bump arsenal for MPU afterEach fix#6222benzekrimaha wants to merge 3 commits into
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
ab63d85 to
2cae9bc
Compare
benzekrimaha
changed the title
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_pull_requests, create_integration_branches |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 4 files with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6222 +/- ##
===================================================
+ Coverage 84.93% 85.03% +0.09%
===================================================
Files 206 206
Lines 13366 13372 +6
===================================================
+ Hits 11353 11371 +18
+ Misses 2013 2001 -12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2cae9bc to
6178ad8
Compare
benzekrimaha
changed the title
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
b8c7b85 to
19a2bd2
Compare
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
nit: this is exactly the kind of PR where prettier is really adding noise... 5-10 lines of "real" change, but prettier fully rewrote 2 large files...
| } else { | ||
| log.trace('created bucket in metadata'); | ||
| } | ||
| return addToUsersBucket(canonicalID, bucketName, bucket, log, err => { |
There was a problem hiding this comment.
should not be be done if bucket already exists, to avoid race and/or incorrect content?
There was a problem hiding this comment.
I also don't understand why bucket already exists error does not cause the function to return early with an error ?
More generally i don't understand why this wasn't catched before in the err ?
I would've expect current behavior to be :
CreateBucket(bucketName) => return err because collision in the db on bucket name, then return callback(err)
I think I'm missing some context information about : what trigger this bug precisely, cuz the pr talks about MPU but here we are fixing a function called freshStartCreateBucket, is this not called all the time to create any bucket ?
There was a problem hiding this comment.
On BucketAlreadyExists we now only run addToUsersBucket and skip removeTransientOrDeletedLabel / updateBucket with our local BucketInfo, so we don't overwrite metadata the concurrent winner already wrote. The winner path still runs full cleanUpBucket when we actually created the metastore entry. No extra metastore read.
There was a problem hiding this comment.
ARSN-613 changes metadata.createBucket() to return BucketAlreadyExists on a metastore duplicate instead of silent success. Without handling it we surface InternalError. Previously cloudserver always continued because there was no err to catch.
The MPU flake is in getMPUBucket (InitiateMultipartUpload / ListMultipartUploads). freshStartCreateBucket is the normal CreateBucket path when two requests both see NoBucketYet and race on metadata.createBucket — same contract change.
We don't return BucketAlreadyExists to the S3 client here: the parallel check already decided this is our bucket being created. On duplicate we only ensure the users-bucket listing; we skip updateBucket with our local object.
There was a problem hiding this comment.
On
BucketAlreadyExistswe now only runaddToUsersBucket
why do we still need to run addToUsersBucket ? Should be done by the "successful" bucket creation?
| return metadata.createBucket(MPUBucketName, mpuBucket, log, | ||
| err => { | ||
| if (err?.is?.BucketAlreadyExists) { | ||
| return metadata.getBucket(MPUBucketName, log, |
There was a problem hiding this comment.
is this function getMPUBucket() called a lot (it is not an s3 api, not sure if it is called everywhere in mpu code, or just in a few places) : if that is the case, maybe we should plan another improvement to let createBucket return the bucket data as well it case it already exists.
...or maybe we could just return the (existing) mpuBucket variable? for actual buckets, there may be more in the BucketInfo -tags, lifecycle rules...-, but likely not the case for mpu's BucketInfo ?
There was a problem hiding this comment.
getMPUBucket is only called from initiateMultipartUpload and listMultipartUploads. On BucketAlreadyExists we now return the local mpuBucket , MPU shadow BucketInfo is minimal (no tags/lifecycle), so that should be enough and avoids an extra getBucket. Longer term, Arsenal could return existing bucket data on duplicate so all callers benefit.
There was a problem hiding this comment.
Arsenal could return existing bucket data on duplicate
that would not be createBucket anymore (and the semantics of "error, it already exists but here is the data" is not very nice to map) ; and not just an arsenal / mongoclientinterface : it would change the whole "metadata" interface, and require consistent changes in MetadataWrapper and all implementation (in-mem, mongo, metadata).
since "BucketInfo is minimal [...] should be enough", no reason to go there I think.
(another change could be the opposite actually: let getMPUBucket only retrieve what is there, use it for read -e.g. in listMultipartUploads, handling the error appropriately- and call createBucket if/when needed -e.g. in initiateMultipartUpload, handling the already created error. But likely some impact still, and certainly out of scope of this ticket)
| * upload bucket or error if any | ||
| * @return {undefined} | ||
| */ | ||
| getMPUBucket(destinationBucket, bucketName, log, cb) { |
There was a problem hiding this comment.
Mhh. I find this very weird :
It's called
GetMpuBucket
It currently does :
GetBucket, if not found, create it.
So ok this is like a "get or create if not found".
But then with the new code changes, this function becomes
GetBucket, if not found, create it. When creating it, if it already exists, getBucket
But if the bucket was not found in the first place, why would we get an "alreadyExists" error when creating it 🤔 ? Race condition ? But then, this is infinite, because in the last getBucket you can also get a notExist error 🤔
There was a problem hiding this comment.
Two concurrent MPU calls can both get NoSuchBucket, one wins createBucket, the other gets BucketAlreadyExists. We return the local mpuBucket we built for the create attempt — same owner/displayName as the winner's shadow bucket, no second read. Not a loop: BucketAlreadyExists means the metastore entry exists; we don't refetch.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
922f83d to
e9a6dbe
Compare
e9a6dbe to
84a0d65
Compare
|
Removing self as there are already 3 other reviewers, more involved in the issue. Feel free to add me again if needed. |
| log.trace('created bucket in metadata'); | ||
| // Ensure users-bucket listing only; do not updateBucket with our | ||
| // local BucketInfo when a concurrent request won createBucket. | ||
| return addToUsersBucket(canonicalID, bucketName, bucket, log, callback); |
There was a problem hiding this comment.
I don't have much knowledge of this, but isn't it problematic to run addToUsersBucket when the bucket already exists ? or is it idempotent or something ? But even then, isn't addToUsersBucket already called when the bucket was created ?
| return removeTransientOrDeletedLabel(bucket, log, callback); | ||
| }); | ||
| log.trace('created bucket in metadata'); | ||
| return cleanUpBucket(bucket, canonicalID, log, callback); |
There was a problem hiding this comment.
I don't really know this functions but yeah it's hard to understand what's happening here, why are we dropping removeTransientOrDeletedLabel and using cleanUpBucket ?
There was a problem hiding this comment.
I think the pr is fine but some changes are complicated to understand (#6222 (comment)), also the linter with some commits not squashed yet is very hard to read
| log.debug('error from metadata', { error: err }); | ||
| return callback(err); | ||
| } | ||
| if (err?.is?.BucketAlreadyExists) { |
There was a problem hiding this comment.
seems weird to do this ONLY when BucketAlreadyExists...
- this was always done previously, especially on success
- in case of error, some other process created the bucket, and should handle addToUsersBucket ?
| if (err?.is?.BucketAlreadyExists) { | |
| if (!err?.is?.BucketAlreadyExists) { |
There was a problem hiding this comment.
Updated in f9b8e38: addToUsersBucket and transient cleanup run only after a successful createBucket; BucketAlreadyExists now returns success without side effects, leaving cleanup to the winning request. I did not apply the literal suggestion because inverting this condition would skip the successful cleanup path and let the race error fall through.
Treat a concurrent creator as the owner of normal bucket cleanup and return the local minimal MPU bucket without another metadata read. Issue: CLDSRV-919
Keep the Prettier migration isolated from the functional race fix so reviewers can inspect the behavior independently. Issue: CLDSRV-919
84a0d65 to
a5ee5bb
Compare
Use Arsenal 506014399741d3fd011c25288fc38c985337babe for paired testing until ARSN-613 is merged and published in a release tag. Issue: CLDSRV-919
a5ee5bb to
63926ab
Compare
7 participants
Context
ARSN-613 changes MongoDB
createBucket()to returnBucketAlreadyExistswhen another request wins a concurrent metastore upsert.Cloudserver must handle this explicit result instead of surfacing an
InternalError.This fixes an intermittent MPU test-cleanup failure where concurrent
ListMultipartUploadscalls race while creating the hidden MPU shadow bucket.Paired Arsenal change:
Arsenal #2672.
Arsenal dependency
Cloudserver is temporarily pinned to Arsenal commit:
506014399741d3fd011c25288fc38c985337babeBefore release, this SHA will be replaced with the Arsenal release tag containing
ARSN-613.
Issue: CLDSRV-919