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. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review by Claude Code |
There was a problem hiding this comment.
all these worked locally, need to wait for the bumps now
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
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 |
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
7912480 to
973261a
Compare
|
| // 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.
Could it be tested on a real lab ?
There was a problem hiding this comment.
should be honestly, i think i need to double check this together with the design and the code
| replicationInfo: getReplicationInfo(config, | ||
| objectKey, bucketMD, false, size, null, null, authInfo), | ||
| overheadField, | ||
| updateMicroVersionId: true, |
There was a problem hiding this comment.
yes, actually this is important, I added this after making my tests in codespaces :
Without this :
You put an object : object isn't assigned an micro version Id
You setup the whole replication, backbeat picks up that source object, it doesn't have any microversion id so cloudserver just doesn't try to process it as a potential cascade replication
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
There was a problem hiding this comment.
- Similar thread here: https://github.com/scality/citadel/pull/349#discussion_r3137680994
Should be done in objectPut instead:7a63acf(#6178)
There was a problem hiding this comment.
We discussed this a bit with Maël.
I think we could try to not do it all the time, and do it only for putObject (but not even sure).
But we were wondering what happens if we don't write microVersionId on putObject : how do we compare two objects that don't have any micro version ID, it may be possible on the first replication to consider no micro version id => source is newer, but then we must write the microVersionID anyways to block loop and make later write work.
Also in the design we said microVersionId would necessarily be used for this feature only but more generally to be able to track and compare update time, I'm very worried we're gonna have issues later if we start writing microVersionID only on certain conditiion. Actually the code in arsenal, backbeat and cloudserver is already filled with quite a lot of defensive and smelly if (microVersionID) because we never bothered adding this from the start
There was a problem hiding this comment.
we were wondering what happens if we don't write microVersionId on putObject : how do we compare two objects that don't have any micro version ID
no issue there:
- there are 2 "independent" putObject on 2 different sites : they will have different versionId, so no need to compare microVersionId
- "within" a specific version (versionID), the empty/missing versionID is inequivoquality the oldest revision of metadata (the one from the original putObject). Any followup updates to that versionID need to bump the micro version id
This case may be a "lack" of microVersionID in the metadata (field not set, for metadata size efficiency), but it is really just a sentinel value for the oldest microVersionID; and it compares exactly the same:
- "empty" versionID on both side → already up to date (equality)
- replicate "empty" versoinID while the target already have a versionID → stale versionID (older)
- replicate versionID while the target has an empty versionID → proceed with replication (newer)
we start writing microVersionID only on certain conditiion.
this case will be present anyway: all objects before the upgrade will not have versionID, so we must be able to manage this state: we cannot assume it will be present.
(and crrExistingObject may add microVersionID, but I'd rather not be protected by this alone: with DR/... there may be other cases coming, best be prepared)
There was a problem hiding this comment.
discussed live and code is updated :
microVersionId is only used for metadata diff checks now
header microVersionId absent => no crr cascade
header microVersionId present but empty string '' => replicate without asking question
header microVersionId present with a value => do comparison
| request.resume(); | ||
| return _respondWithHeaders( | ||
| response, | ||
| { code: MicroVersionIdAlreadyStoredException.name, | ||
| message: 'incoming microVersionId already at destination' }, | ||
| {}, | ||
| log, | ||
| callback, | ||
| 409, | ||
| ); |
There was a problem hiding this comment.
could you remove duplication to simplify here ?
There was a problem hiding this comment.
Ok I did something, you take a look but honestly it's really not that great 🤔
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.
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', | |
| ); | |
| } |
There was a problem hiding this comment.
Sending back the microVersionID is indeed not super useful, I added it because it was suggested it could become useful or nice to have the info for the caller. I'm not really gonna focus on this to much, i can remove it or keep it, imo I think it's fine that an api returns it.
But for the double error, well tbh i don't really mind either as I just wanna put an end to this feature, but for me it's better to have backbeat check the errors like this :
Than having it just receive a microVersion id, then does another loop if microversion == null, if microversion < > xxx, then we don't do exactly the same thing (cbOnce(cascadeLoopDetected) vs cbOnce(cascadeDataStale)).
In the screenshot, we directly have errors that we can nicely catch and that's pretty much it.
I just see calling the client like calling any functions, and if the function I call return an error, I want the error to be as detailed as possible and not redo the checks that was already done, because anyways cloudserver has to do those comparisons here.
There was a problem hiding this comment.
I will recheck the suggested simpification after we settle the discussion, not fully sure it should be done though because compare can return 4 values
older
younger
equal
not comparable
so doing this cmp !=== Ordering.NEWER
still leaves room for NOT_COMPARABLE, I think it's safer to assert on what we want that exclude only value
There was a problem hiding this comment.
Sending back the microVersionID is indeed not super useful, I added it because it was suggested it could become useful or nice to have the info for the caller
not sure where it was lost in the discussion, but the suggestion was really to add microVersionID in the result so we could have a single error : i.e. reduce the logic to minimal in cloudserver, and keep the most in backbeat.
(btw, the code you mention also has similar comment about duplication: my feeling -to be confirmed- is that this duplication at every layer is due at least in part to the qualification happening too early, and would be avoided by propagating a single error [wiht microVersionId] accross all layers, and differentiating the cases only at the end of the chain)
There was a problem hiding this comment.
I reviewed everything, but I don't really see any code duplication on putMetadata, backbeat does a putMetada, receive the error which directly goes to "handleReplicationOutcome", without needing to do any comparison again. Looks good to me, the functions called sends an error that's as precise as possible
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
3c698dc to
c8acd08
Compare
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
c8acd08 to
51638dd
Compare
maeldonn
left a comment
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', | |
| ); | |
| } |
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
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)
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
6 times, most recently
from
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
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. |
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
da34813 to
dc58e7d
Compare
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
dc58e7d to
f54b101
Compare
| const olderMvId = makeMicroVersionId(); | ||
| const newerMvId = makeMicroVersionId(); | ||
|
|
||
| await backbeatClient.send(new PutMetadataCommand({ |
There was a problem hiding this comment.
Both PutMetadataCommand calls in this test (lines 338 and 344) are missing ReplicationContent. Without x-scal-replication-content header, the cascade block in routeBackbeat.js:841 (const isReplicationWrite = !!headers['x-scal-replication-content']) won't fire. The body's replicationInfo.status ('REPLICA' from buildMetadataBody) is stored as-is, so the assertion at line 356 expecting 'PENDING' will fail.
Same issue applies to the test at line 315 ("should clear replicationInfo when no CRR rules match") which uses the putMetadata() helper — documented at line 68 as "without x-scal-replication-content" — but asserts status: '', a value only set when the cascade block fires and finds no matching rules.
| await backbeatClient.send(new PutMetadataCommand({ | |
| await backbeatClient.send(new PutMetadataCommand({ | |
| Bucket: TEST_BUCKET_CRR, | |
| Key: key, | |
| MicroVersionId: olderMvId.encoded, | |
| ReplicationContent: 'METADATA', | |
| Body: buildMetadataBody({ microVersionId: olderMvId.raw }), | |
| })); |
6 participants
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