Add Crr Cascade capabilities to backbeat crr replication#2747
Add Crr Cascade capabilities to backbeat crr replication#2747SylvainSenechal wants to merge 1 commit into
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}
|
There was a problem hiding this comment.
I think we can functional tests instead of just these,
But waiting for Arsenal/cloudserver to be merged, as it will be easier to make these tests (functional tests in backbeat rely on an image of cloudserver)
There was a problem hiding this comment.
keeping unit test is good, functional test should just be an addition?
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2747 +/- ##
===================================================
- Coverage 75.42% 75.36% -0.07%
===================================================
Files 201 201
Lines 13868 13903 +35
===================================================
+ Hits 10460 10478 +18
- Misses 3398 3415 +17
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
|
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
4c64ed6 to
3237f9e
Compare
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
handling of MPU is not correct : each "replicant" must write their own object fully, there is no situation where 2 "sources" each replicate part of the data (nor a way for these to identify the data they have written). Each source create the metadata document with the parts they uploaded, so they MUST successfully upload all parts data and MUST not have a conflict on any part.
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: |
| if (err instanceof MicroVersionIdAlreadyStoredException) { | ||
| log.info('replication completed via cascade loop: ' + | ||
| 'object already at destination with the same revision', | ||
| { entry: sourceEntry.getLogInfo() }); | ||
| this._publishReplicationStatus(sourceEntry, 'COMPLETED', { kafkaEntry, log }); | ||
| return done(null, { committable: false }); | ||
| } | ||
| if (err instanceof StaleMicroVersionIdException) { | ||
| log.info('replication completed: destination already holds ' + | ||
| 'this version with a newer revision', | ||
| { entry: sourceEntry.getLogInfo() }); | ||
| this._publishReplicationStatus(sourceEntry, 'COMPLETED', { kafkaEntry, log }); | ||
| return done(null, { committable: false }); | ||
| } |
There was a problem hiding this comment.
there is a risk of creating orphans here, please create a followup:
- new object was created → try to replicate DATA+META
- putDataParts succeeds (no conflict)
- before we could put metadata, another site was able to putData (same as us, not metadata → impossible to detect conflict) and putMetadata
- putMetadata will thus fail with MicroVersionIdAlreadyStoredException/StaleMicroVersionIdException
→ in that case, data created in 2. must be deleted, i.e. call _deleteOrphans. _deleteOrphans MUST NOT be called if this was a META-only update / if we did not call putDataParts
There was a problem hiding this comment.
Yeah I see, created this : https://scality.atlassian.net/browse/BB-802
Looking from afar i think it shouldn't be too hard to handle, I will address it when we merge this pr otherwise this one will never be merged
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
5c4ed70 to
7d2eda2
Compare
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
67f10a1 to
7afe54b
Compare
| return cb(null, destLocations); | ||
| // When no new data locations were written, force metadata-only mode (noNewDataLocations) | ||
| // so cloudserver preserves the destination's existing location field instead of overwriting it | ||
| const noNewDataLocations = destLocations.length === 0; |
There was a problem hiding this comment.
AFAIU this still does not implement what was agreed in the MPU thread: if some parts return partAlreadyAtDest and some were written by us, destLocations is a partial list, noNewDataLocations is false, and we proceed to putMetadata with only the parts we wrote → corrupted location metadata at destination. A conflict on any part means another replicant already wrote the metadata: we must delete all the parts we uploaded and downgrade to a metadata-only update. i.e. the check should be destLocations.length !== partResults.length, not === 0
There was a problem hiding this comment.
Yes sorry I had not addressed comments yet when you did the review I was on cloudserver, it's updated now
| MicroVersionId: entry.getMicroVersionId() | ||
| ? encodeMicroVersionId(entry.getMicroVersionId()) : '', |
There was a problem hiding this comment.
the empty-string convention is one of the open topics on scality/cloudserver#6179. Is the shape settled now?
There was a problem hiding this comment.
yep this is very intentional, using undefined would cause the sdk to not even send the header and would break the cascade replication. A bit fragile if you ask me but this is the current design
| const { errors, jsutil, versioning } = require('arsenal'); | ||
| const { ObjectMDLocation } = require('arsenal').models; | ||
| const { ReplicationConfiguration } = require('arsenal').models; | ||
| const { encode: encodeMicroVersionId, decode } = versioning.VersionID; |
There was a problem hiding this comment.
nit: rename both or neither; decode unqualified next to encodeMicroVersionId reads as if they operate on different things
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
7afe54b to
acde116
Compare
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
acde116 to
50b6765
Compare
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
2 times, most recently
from
38d87c8 to
29d498b
Compare
| if (err instanceof MicroVersionIdAlreadyStoredException || | ||
| err instanceof StaleMicroVersionIdException) { | ||
| return cbOnce(err); | ||
| } |
There was a problem hiding this comment.
do we need the condition here? Can't we just log the error (like all other errors) and pass it up in these case as well?
(the main difference is that we would get a ERROR log instead of INFO... and log here instead of in the continuation callback)
There was a problem hiding this comment.
I think its nicer to keep, these arent unexpected errors, and they are logged already in the replication outcome.
I moved them with the if (err.ObjNotFound) above though, as its a similar situation : expected error
| entry: destEntry.getLogInfo(), | ||
| error: destMvIdRaw.message, | ||
| }); | ||
| return doneOnce(destMvIdRaw); |
There was a problem hiding this comment.
should still be a collision : even if we can't parse the microVersionID, we are still in the case "data already at destination", so we shoud still try to write the metadata anyway ?
(the only difference is that we can't really make the optimization of skipping the metadata updated if the microVersionId is already up to date on target)
There was a problem hiding this comment.
Yes, I updated the code, this will be a situation where we still do a putMetadata.
I also moved the decoding to another place, more centralized
| // Another replicant already stored the object data | ||
| // Compare microVersionIds to decide what to do with metadata: | ||
| // - source is newer : proceed in metadata-only mode | ||
| // - same or older : skip putMetadata | ||
| const destMvId = collisionResult.destMvIdRaw; | ||
| const srcMvId = sourceEntry.getMicroVersionId() || null; | ||
| const isLoop = srcMvId === destMvId; | ||
| const isStale = destMvId !== null && | ||
| (srcMvId === null || srcMvId > destMvId); |
There was a problem hiding this comment.
this is where compareMicroVersionId shines, please use the arsenal function:
- it makes the intent explicit (just comparing micro version id)
- the function should handle the corner cases itself ("null" micro version id...)
- we don't actually need to differentiate loop vs stale here
It would look something like this, i.e. we skip if we receive the destination micro version id (field present and no parse error) AND the micro version id is newer or equal at destination
const skipPutMetadata = destMvId !== undefined && compareMicroVersoinId(sourceEntry.getMicroVersionId(), destMvId) <= 0;
return cb(null, [], true, skipPutMetadata);
| if (err instanceof VersionIdCollisionException) { | ||
| const destMvIdRaw = err.microVersionId | ||
| ? decodeMicroVersionId(err.microVersionId) : null; | ||
| if (destMvIdRaw instanceof Error) { |
There was a problem hiding this comment.
nit: I wonder if it is best to do the decoding here, or just pass the field "verbatim" in the part and let _getAndPutData() decode
- it would avoid useless decoding of every parts' microVersionId
- it would increase locally (
- but the code here still needs to make sure if forwards both the microVersionId "value" AND the information that the field was indeed provided
| // update location, replication status and put metadata in | ||
| // target bucket | ||
| (destLocations, next) => { | ||
| (destLocations, noNewDataLocations, skipMetadata, next) => { |
There was a problem hiding this comment.
these 2 parameters are not independent, there are actually only 3 cases:
- no conflict (i.e. as before)
- conflict with metadata already up to date
- conflict but we have newer metadata which need to be replicated
not sure how best to handle this, maybe an object ("conflict") with the microVersionId : so we also improve locality by performing the microVersionId comparison (optimization) here?
→ undefined in the usual case (no conflict), don't even need the false like line 972
→ { microVersionId: ... } (possibly with the "conflict" field you already have) otherwise ; skip if compareMicroVersionId() <= 0
There was a problem hiding this comment.
I updated it with a conflict object and move some logic around into a helper function as we still need it in 2 different places
| destEntry.setLocation(location); | ||
| this._putMetadata(destEntry, false, log, next); | ||
| (destLocations, noNewDataLocations, skipMetadata, next) => { | ||
| if (skipMetadata) { |
There was a problem hiding this comment.
this block is duplicated and not trivial (it handles putData conflict resolution) : should be written just once... does it makes sense do do this in _putMetadata, by adding it another parameter for putData conflict ?
There was a problem hiding this comment.
I'm gonna refactor this but I prefer extracting a helper functoin that putting it in putMetadata.
It looks very weird to call a function with an object that's used at the top of the function so that the function itself decides whether it should conitnue or not
| const replicationContent = (mdOnly ? 'METADATA' : undefined); | ||
| // Send x-scal-replication-content so cloudserver know the putMetadata api | ||
| // is used in the context of a replication | ||
| const replicationContent = (mdOnly ? 'METADATA' : 'DATA,METADATA'); |
There was a problem hiding this comment.
This is required as otherwise, in non mdOnly situation, there is no replicationContent header, and cloudserver has no way no know we are calling putMetadata in the context of a replication.
Anyways i think sending data, metadata for non mdOnly should've been added earlier.
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
e214df0 to
b4a44d3
Compare
| // holds this revision or a newer one. Returns false when there is no conflict, | ||
| // when the destination microVersionId is absent or can't be parsed (proceed | ||
| // conservatively), or when the source holds a newer revision. | ||
| _shouldSkipMetadata(sourceMicroVersionId, conflict, log) { |
There was a problem hiding this comment.
Discuss : Could be moved into putMetada so that we don't have to call this from 2 places, and instead inline it.
But imo it's a weird pattern to add an extra param to a function, then call that function and use that extra param at the top to decide to potentially not run this function
There was a problem hiding this comment.
either are fine with me.
- while I agree on the param (especially the
skip(bool skip)anti-pattern), in this case it seems fine to me : the parameter is not just an indication to skip, but really an input allowing to make the decision ("details of the conflict"). - in my mind, putting it is
putMetadatamakes it a further from the anti-pattern : to me it is not skipping the metadata write, but just raising the abstraction level. PutMetadata's goal is to ensure the metadata is in the system, and it would gain the ability to reach the goal without actually making a write (kind of magic!) - similar in a way to how a getter could either read from the actual DB or quickly return the data from a cache.
(but either way may be better to keep _shouldSkipMetadata separate -just decide if it is called from within _putMetadata or before it- so each path can be demonstrated and tested easily)
SylvainSenechal
force-pushed
the
improvement/BB-767
branch
from
b4a44d3 to
8580536
Compare
|
The code is getting smaller after each review 😆 |
| } | ||
| } | ||
| const comparison = compareMicroVersionId(sourceMicroVersionId, destMvId); | ||
| return destMvId !== null && |
There was a problem hiding this comment.
May completely remove the null check if I end up update the compareMicroVersionId function.
Still reviewing cloudserver, haven't done it/decided yet
| continue; | ||
| } | ||
| if (result.isCollision) { | ||
| collisionResult = collisionResult || result; |
There was a problem hiding this comment.
nit:
| collisionResult = collisionResult || result; | |
| collisionResult ||= result; |
| return this._deleteOrphans(destEntry, destLocations, log, () => cb(err)); | ||
| }, (err, partResults) => { | ||
| let collisionResult; | ||
| const nonCollisionParts = []; |
There was a problem hiding this comment.
nit:
| const nonCollisionParts = []; | |
| const parts = []; |
| }); | ||
| return doneOnce(null, { | ||
| isCollision: true, | ||
| destinationMicroVersionId: err.microVersionId, |
There was a problem hiding this comment.
I would keep the name simple here : the destination prefix adds verbosity, but does not improve readability IMHO
| return cb(null, [], { | ||
| microVersionId: collisionResult.destinationMicroVersionId, | ||
| }); |
There was a problem hiding this comment.
nit: could even return the collisionResult directly
| return cb(null, [], { | |
| microVersionId: collisionResult.destinationMicroVersionId, | |
| }); | |
| return cb(null, [], collisionResult); |
| // CREATE a new document. Use METADATA when: | ||
| // - this is a metadata-only replication entry (mdOnly), or | ||
| // - data was already written to the destination from another path (conflict). | ||
| const localMdOnly = mdOnly || conflict !== undefined; |
There was a problem hiding this comment.
nit: would work the same if conflict is null ?
| const localMdOnly = mdOnly || conflict !== undefined; | |
| const localMdOnly = mdOnly || !!conflict; |
| const localMdOnly = mdOnly || conflict !== undefined; | ||
| destEntry.setLocation(destLocations); | ||
| this._putMetadata(destEntry, mdOnly, log, err => { | ||
| return this._putMetadata(destEntry, localMdOnly, log, err => { |
There was a problem hiding this comment.
local (in variable name) is a technicality, which makes the variable name weird.
maybe the cleaner way is just keep things simple:
| return this._putMetadata(destEntry, localMdOnly, log, err => { | |
| return this._putMetadata(destEntry, mdOnly || !!conflict, log, err => { |
| // holds this revision or a newer one. Returns false when there is no conflict, | ||
| // when the destination microVersionId is absent or can't be parsed (proceed | ||
| // conservatively), or when the source holds a newer revision. | ||
| _shouldSkipMetadata(sourceMicroVersionId, conflict, log) { |
There was a problem hiding this comment.
either are fine with me.
- while I agree on the param (especially the
skip(bool skip)anti-pattern), in this case it seems fine to me : the parameter is not just an indication to skip, but really an input allowing to make the decision ("details of the conflict"). - in my mind, putting it is
putMetadatamakes it a further from the anti-pattern : to me it is not skipping the metadata write, but just raising the abstraction level. PutMetadata's goal is to ensure the metadata is in the system, and it would gain the ability to reach the goal without actually making a write (kind of magic!) - similar in a way to how a getter could either read from the actual DB or quickly return the data from a cache.
(but either way may be better to keep _shouldSkipMetadata separate -just decide if it is called from within _putMetadata or before it- so each path can be demonstrated and tested easily)
| const localMdOnly = conflict !== undefined; | ||
| destEntry.setLocation(destLocations); | ||
| return this._putMetadata(destEntry, localMdOnly, log, next); |
There was a problem hiding this comment.
nit: same as above
| const localMdOnly = conflict !== undefined; | |
| destEntry.setLocation(destLocations); | |
| return this._putMetadata(destEntry, localMdOnly, log, next); | |
| destEntry.setLocation(destLocations); | |
| return this._putMetadata(destEntry, !!conflict, log, next); |
| log, done) { | ||
| if (err instanceof MicroVersionIdAlreadyStoredException || | ||
| err instanceof StaleMicroVersionIdException) { | ||
| log.info('replication completed: object already at destination', |
There was a problem hiding this comment.
nit: "object" may be ambiguous here, it is not about the object (~data) but really about the metadata revision ?
| log.info('replication completed: object already at destination', | |
| log.info('replication completed: metadata already at destination', |
or
| log.info('replication completed: object already at destination', | |
| log.info('replication completed: metadata revision already at destination', |
or skip the ambiguity (but maybe not clear)
| log.info('replication completed: object already at destination', | |
| log.info('replication completed: already at destination', |
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
should add tests to verify _shouldSkipMetadata behavior directly in the different cases
- no conflict
- conflict with "invalid" microVersionId
- conflict with missing microVersionId
- conflict with newer/equal/older microVersionId
- conflict with null microVersionId (either in conflict or in destination)
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
maeldonn
left a comment
maeldonn
left a comment
There was a problem hiding this comment.
Some minor issues to fix
There was a problem hiding this comment.
Please avoid callbacks on new isolated functions.
| const { ObjectMDLocation } = require('arsenal').models; | ||
| const { ReplicationConfiguration } = require('arsenal').models; |
| // sends extra header x-scal-replication-content to the target | ||
| // if it's a metadata operation only | ||
| const replicationContent = (mdOnly ? 'METADATA' : undefined); | ||
| // Send x-scal-replication-content so cloudserver know the putMetadata api |
There was a problem hiding this comment.
| // Send x-scal-replication-content so cloudserver know the putMetadata api | |
| // Send x-scal-replication-content so cloudserver knows the putMetadata api |
benzekrimaha
left a comment
benzekrimaha
left a comment
There was a problem hiding this comment.
Nothing more to add after mael and françois's comments. Approving
5 participants
Issue: BB-767
Related PRs :
Arsenal : scality/Arsenal#2628
Cloudserver : scality/cloudserver#6179
CloudserverClient : scality/cloudserverclient#24
S3utils : scality/s3utils#395