Add CRR Cascaded capabilities#6179
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Review by Claude Code |
There was a problem hiding this comment.
all these worked locally, need to wait for the bumps now
c15fb2b to
7912480
Compare
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
|
7912480 to
973261a
Compare
|
c8acd08 to
51638dd
Compare
maeldonn
left a comment
There was a problem hiding this comment.
LGTM. Please bump package.json and create a new release. Could you just confirm that the location guard is working ?
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| const incomingVersionIdEncoded = request.headers['x-scal-version-id']; | ||
| const decoded = incomingVersionIdEncoded ? decode(incomingVersionIdEncoded) : null; | ||
| const incomingVersionIdDecoded = decoded instanceof Error ? null : decoded; | ||
| if (incomingVersionIdDecoded && objMd && objMd.versionId === incomingVersionIdDecoded) { |
There was a problem hiding this comment.
objMd.versionId === incomingVersionIdDecoded
- Can this condition actually fail?
- Is it really part of the condition?
AFAIU, versionId is used (or should be?) to find the existing objMD:
- so if not found, we would get objMD=null ;
- and if found, it should have the versionID...
- Unless there is a some logic here which gets the matching version if it exist, else the latest one : in that case objMd.versionId !== incomingVersionIdDecoded means we actually did not an object already
→ should clarify how we get there, and nice if we can drop this condition
(remember that with CRR with cannot work in non-versioned buckets -so no "overwrites"-, and that versions may be replicated out of order)
There was a problem hiding this comment.
I rewrote some part of the if statement into 2 stages, but your comment still stands.
I am not sure i understand it though, I think you are saying that if we were able to get objMd, then necessarily objMd will be equal to incomingVersionIdDecoded ?
- objMd is obtained through a middleware, from the provided bucket + the key, and it's the latest available version of that bucket
- the x-scal-micro-version-id is anything people using the client decide to put inside. I don't see a situation where we would skip checking objMd.version === incomingVersionId
There was a problem hiding this comment.
objMd is obtained through a middleware, from the provided bucket + the key, and it's the latest available version of that bucket
- if by middleware you mean the "router" in the same routeBackbeat file, indeed; but still this is something under our control and closely related to this code
- I thought this would return the specific
versionIdassociated with the result ; if it returns the latest, then the issue is much larger : we need to try and check if we have this specificversionId. We may be replicating the "previous" version, but the logic must apply in that case as well...
the x-scal-micro-version-id is anything people using the client decide to put inside. I don't see a situation where we would skip checking objMd.version === incomingVersionId
- this API cannot be used by "people", it is used only by scality software
- if the
x-scal-version-idis set to tell which version we are interested in (and should be retrieved). If there is no such revision, there is no conflict (and responsibility/bug of the caller if they did not provide the right versionId) ; if this revision is present, this is a conflict. We don't care if there is another revision of the object, we should only attempt to retrieve the ObjMD of the revision we are interested in.
There was a problem hiding this comment.
This is outdated (was discussed in meeting and addressed with code change)
| replicationInfo: getReplicationInfo(config, | ||
| objectKey, bucketMD, false, size, null, null, authInfo), | ||
| overheadField, | ||
| updateMicroVersionId: true, |
There was a problem hiding this comment.
- should "new" objects not keep having an empty microVersionId (to avoid wasting metadata space)?
and just consider "no micro version id" is the oldest micro versionId (the only thing it cannot compare to is another no micro version id - but that would not happen on new object, each get their own versionId) - in case of un-versioned buckets (i.e. only master objects), we may need to update the micro version id (because we have no version id) ; however there is no replication in that case, so no problem not to do it
|
|
||
| const incomingVersionIdEncoded = request.headers['x-scal-version-id']; | ||
| const decoded = incomingVersionIdEncoded ? decode(incomingVersionIdEncoded) : null; | ||
| const incomingVersionIdDecoded = decoded instanceof Error ? null : decoded; |
There was a problem hiding this comment.
should we not actually fail if the version id cannot be decoded? (this would be a bad request)
There was a problem hiding this comment.
yes actually i changed the single if into 2 ifs :
If the versionId is provided, but we can't decode it, it means something is wrong and we shouldn't continue anything
| request.resume(); | ||
| return _respondWithHeaders( | ||
| response, | ||
| { code: MicroVersionIdAlreadyStoredException.name, | ||
| message: 'incoming microVersionId already at destination' }, | ||
| {}, | ||
| log, | ||
| callback, | ||
| 409, | ||
| ); |
There was a problem hiding this comment.
the code is completely duplicated, the only difference is really name of exception and "text" of message.
as discussed in cloudserverclient, do we really need cloudserver to make that distinction? just returning the same error (micro version id conflict) AND the micro version id of the object stored allows the caller to make the distinction, if they need - without duplication.
| request.resume(); | ||
| return _respondWithHeaders( | ||
| response, | ||
| { code: MicroVersionIdAlreadyStoredException.name, | ||
| message: 'incoming microVersionId already at destination' }, | ||
| {}, | ||
| log, | ||
| callback, | ||
| 409, | ||
| ); |
There was a problem hiding this comment.
ahhh, I missed the extra type MicroVersionIdAlreadyStoredException, c.f. https://github.com/scality/cloudserverclient/pull/24/changes#r3496549728
we had the discussion and ended adding microVersionId in StaleMicroVersionIdException, but it is useless if we generate 2 kinds of errors...
→ IMHO we should drop MicroVersionIdAlreadyStoredException, which allows deduplicating here and simplifying backbeat?
otherwise we shoud still dedup, something like:
| request.resume(); | |
| return _respondWithHeaders( | |
| response, | |
| { code: MicroVersionIdAlreadyStoredException.name, | |
| message: 'incoming microVersionId already at destination' }, | |
| {}, | |
| log, | |
| callback, | |
| 409, | |
| ); | |
| if (cmp !=== Ordering.NEWER) { | |
| request.resume(); | |
| return _respondWithHeaderCrrConflict( | |
| response, log, callback, | |
| cmp === Ordering.EQUAL ? MicroVersionIdAlreadyStoredException.name : StaleMicroVersionIdException.name, | |
| 'incoming microVersionId already at destination', | |
| ); | |
| } |
51638dd to
afa7f2e
Compare
There was a problem hiding this comment.
@SylvainSenechal seems there are quite the a few open points, which don't look like they will converge quickly through review comments.
Can you prepare a small doc to list each of these topics (both on this PR and the backbeat one, likely the same but best be sure), with the different approaches and arguments in favor of each?
→ then we can go through them in a short meeting (30min should be enough?), settle the question and move forward
(note: some of these questions may be refined/addressed incrementally in followup PRs, so we could merge this PR already; and for others it may make sense to make the change first. But either way we need to settle the points ASAP -and implement the followup, if any- before we are ready to ship)
b721b8c to
da34813
Compare
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
putData correctly calls writeContinue(request, response) before consuming the body (line 466), but putMetadata doesn't call it before _getRequestPayload. The cloudserverclient upgrade from 1.0.7 to 1.0.9 adds @aws-sdk/middleware-expect-continue, so clients will now send Expect: 100-continue for metadata requests. Since the server registers a checkContinue listener (lib/server.js:296), Node.js won't auto-send 100 Continue — the client will stall until its expectContinueTimeout (default 1s) before sending the body, adding latency to every metadata replication.
| writeContinue(request, response); | |
There was a problem hiding this comment.
Mhhh, no ?
We introduce 100-continue for putData as it's data heavy
We don't particularly care about it for putMetada 🤔 Or else we will start using it everywhere
| } | ||
|
|
||
| const incomingVersionIdEncoded = request.headers['x-scal-version-id']; | ||
| if (incomingVersionIdEncoded) { |
There was a problem hiding this comment.
header may be present but empty, for null versions
| if (incomingVersionIdEncoded) { | |
| if (incomingVersionIdEncoded != undefined) { |
There was a problem hiding this comment.
Mhh, looking at the way this is sent from backbeat, we are very unlikely to send '' unless we hardcode '' in the client.
This is because in backbeat, we are getting the versionID from an Arsenal helper that will returns null when there is no version ID
Also, if we let '' go though this if, it will fail right after on the decode function as its an invalid version id.
We should still make the api here a bit more self documenting, how about this :
"We expect either no version ID, or a real version id that isnt an empty string" :
if (incomingVersionIdEncoded != null && incomingVersionIdEncoded !== '') ?
francoisferrand
left a comment
There was a problem hiding this comment.
the condition microVersionId update is not atomic → must somehow involve the mongo/metadata layer, i.e. perform an actual conditional metadata update on microVersionId
| const isSourceOlderThanDestination = (sourceMicroVersionId, destinationMicroVersionId) => | ||
| destinationMicroVersionId !== null && | ||
| (sourceMicroVersionId === null || sourceMicroVersionId > destinationMicroVersionId); |
There was a problem hiding this comment.
is this not handled already in arsenal's compareMicroVersionId function ?
There was a problem hiding this comment.
So, I ended up not using the Arsenal utility function. If found it was getting over complicated to use, as I tried to make some kind of generic Arsenal comparison function that deals with null values, and multiple cases, but here we already have more context and focus only on doing comparison that we care about.
Basically, I think if we wanted to use Arsenal utility, we would have to introduce Crr cascade business into arsenal.
And if we want to use the generic Arsenal function here, we will need to add logic on top of it to fit it for Crr Cascade
| function putMetadata(request, response, bucketInfo, objMd, log, callback) { | ||
| const { bucketName, objectKey } = request; | ||
|
|
||
| const encodedMicroVersionId = request.headers['x-scal-micro-version-id']; |
There was a problem hiding this comment.
when the header is set, maybe we should have a sanity check (later) to verify the metadata contains the same microVersionId as the header
There was a problem hiding this comment.
A check that the microVersionId in the header is the same as the microVersionId in the metadata body ? Yes why not, I added it as soon as the request payload is decoded
| // For metadata-only writes, the destination's data location must be | ||
| // preserved from the existing object. If there is no existing object | ||
| // and the source has data, there is nothing to copy the location from. | ||
| // Zero-byte objects have no data location, so this is safe to skip. |
There was a problem hiding this comment.
it is safe to skip, but also safe to keep as well : and the previous code was much simpler. If there is no need change it, best not to change it.
Especially semantically, headers['x-scal-replication-content'] === 'METADATA' means we expect to update the metadata of an object: so that object MUST exist. Removing the check could mask bugs (e.g. backbeat set METADATA instead of DATA+METADATA) or lead to re-creating the object/version (empty) after it was deleted.
→ keep the previous chunk, this should not be changed
There was a problem hiding this comment.
Ok I went back on this :
This modification should indeed not be there, and instead it requires some small changes on backbeat to deal with metadata only writes on zero sized object, but yeah there is no need to make that modification here
| // as cascade targets because they use the MultiBackend S3 path which | ||
| // bypasses the putData/putMetadata routes, so loop detection cannot fire | ||
| // on those destinations. | ||
| const BLOCKED_LOCATION_TYPES = ['location-scality-ring-s3-v1', 'location-scality-artesca-s3-v1']; |
There was a problem hiding this comment.
can we know the "source" (sender) of the request here, and map it to a location?
It would not solve the loop problem, but passing that information would allow skipping extra API call(s) in case of bidirectional CRR, and keep same "load" as the existing code.
→ please create followup, could be a nice optimization?
(kind of flimsy, but we could extract the "replication group" from versionId ; not sure how to map to locations though. Or checking if source IP is in the location's bootstrap list. Other maybe other ways...)
There was a problem hiding this comment.
Ah you want to have something like
a<->b, and when b receives the requests, it identifies the next replication is A, which is also where the current request came from, so it stops
I think from just 2 minutes of thinking we would need to pass a new header, that contains the location name. And here in backbeat compare the 2 locations names.
Anyways creating a follow-up ticket
There was a problem hiding this comment.
microVersionID may be specified as well when NOT doing replication, i.e. when headers['x-scal-replication-content'] is not set.
→ need to test both microVersionID semantics (i.e. conditional updates) AND the interaction with replication (when replicating, should cascade).
→ maybe more readable to split into 2 files putMetadata.js and putData.js tests matching the actual APIs, and cover both aspects (and possibly combinations)
There was a problem hiding this comment.
Yeah the file is big enough that a split makes sense, also some tests can be added
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
|
Removing myself as reviewer since alrdy covered by 2 people; and quite complex. Re-add me if needed. |
dc58e7d to
f54b101
Compare
f54b101 to
14aa3a0
Compare
|
Btw I will deal with prettier at the end |
ISSUE : CLDSRV-897
Crr cascaded design : https://github.com/scality/citadel/pull/349
Related PRs :
Arsenal : scality/Arsenal#2628
CloudserverClient : scality/cloudserverclient#24
Backbeat : scality/backbeat#2747
S3utils : scality/s3utils#395