Add Crr Cascade capabilities to backbeat crr replication #2747
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,30 @@ | ||
| const async = require('async'); | ||
| const { S3Client, GetBucketReplicationCommand, GetObjectCommand } = require('@aws-sdk/client-s3'); | ||
|
|
||
| const { errors, jsutil, versioning } = require('arsenal'); | ||
| const { ObjectMDLocation } = require('arsenal').models; | ||
| const { ReplicationConfiguration } = require('arsenal').models; | ||
| const { | ||
| encode: encodeMicroVersionId, | ||
| decode: decodeMicroVersionId, | ||
| compare: compareMicroVersionId, | ||
| Ordering, | ||
| } = versioning.VersionID; | ||
|
|
||
| const ClientManager = require('../../../lib/clients/ClientManager'); | ||
| const BackbeatMetadataProxy = require('../../../lib/BackbeatMetadataProxy'); | ||
| const { | ||
| BackbeatRoutesClient, | ||
| PutDataCommand, | ||
| BatchDeleteCommand, | ||
| PutMetadataCommand, | ||
| GetMetadataCommand, | ||
| addContentLengthMiddleware, | ||
| attachReqUids, | ||
| attachExpectContinueMiddleware, | ||
| VersionIdCollisionException, | ||
| StaleMicroVersionIdException, | ||
| MicroVersionIdAlreadyStoredException, | ||
| } = require('@scality/cloudserverclient'); | ||
|
|
||
| const mapLimitWaitPendingIfError = require('../../../lib/util/mapLimitWaitPendingIfError'); | ||
|
|
@@ -452,11 +461,32 @@ class ReplicateObject extends BackbeatTask { | |
| const mpuConcLimit = this.repConfig.queueProcessor.mpuPartsConcurrency; | ||
| return mapLimitWaitPendingIfError(locations, mpuConcLimit, (part, done) => { | ||
| this._getAndPutPart(sourceEntry, destEntry, part, log, done); | ||
| }, (err, partResults) => { | ||
| let collisionResult; | ||
| const nonCollisionParts = []; | ||
| for (const result of (partResults || [])) { | ||
| if (!result) { | ||
| continue; | ||
| } | ||
| if (result.isCollision) { | ||
| collisionResult = collisionResult || result; | ||
| } else { | ||
| nonCollisionParts.push(result); | ||
| } | ||
| } | ||
| const hasPutDataConflict = collisionResult !== undefined; | ||
| if (err || hasPutDataConflict) { | ||
| // On error or conflict, drop all parts written | ||
| return this._deleteOrphans(destEntry, nonCollisionParts, log, () => { | ||
| if (hasPutDataConflict) { | ||
| return cb(null, [], { | ||
| microVersionId: collisionResult.destinationMicroVersionId, | ||
| }); | ||
| } | ||
| return cb(err); | ||
| }); | ||
| } | ||
| return cb(null, partResults, undefined); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -570,7 +600,9 @@ class ReplicateObject extends BackbeatTask { | |
| // destination bucket has to be versioning enabled. | ||
| VersioningRequired: true, | ||
| RequestUids: log.getSerializedUids(), | ||
| VersionId: sourceEntry.getEncodedVersionId(), | ||
| }); | ||
| attachExpectContinueMiddleware(putCommand, this.backbeatDest.config?.requestHandler); | ||
| addContentLengthMiddleware( | ||
| putCommand, | ||
| response.ContentLength, | ||
|
|
@@ -600,6 +632,17 @@ class ReplicateObject extends BackbeatTask { | |
| incomingMsg.destroy(); | ||
| } | ||
| } | ||
|
|
||
| if (err instanceof VersionIdCollisionException) { | ||
| log.info('cascade putData: data already at destination', { | ||
| method: 'ReplicateObject._getAndPutPartOnce', | ||
| entry: destEntry.getLogInfo(), | ||
| }); | ||
| return doneOnce(null, { | ||
| isCollision: true, | ||
| destinationMicroVersionId: err.microVersionId, | ||
| }); | ||
| } | ||
| // eslint-disable-next-line no-param-reassign | ||
| err.origin = 'target'; | ||
| log.error('an error occurred on putData to S3', | ||
|
|
@@ -657,9 +700,9 @@ class ReplicateObject extends BackbeatTask { | |
| accountId = _extractAccountIdFromRole(this.targetRole); | ||
| } | ||
|
|
||
| // 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'); | ||
|
Contributor
Author
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. |
||
| const mdBlob = entry.getSerialized(); | ||
| const command = new PutMetadataCommand({ | ||
| Bucket: entry.getBucket(), | ||
|
|
@@ -671,6 +714,8 @@ class ReplicateObject extends BackbeatTask { | |
| // destination bucket has to be versioning enabled. | ||
| VersioningRequired: true, | ||
| RequestUids: log.getSerializedUids(), | ||
| MicroVersionId: entry.getMicroVersionId() | ||
| ? encodeMicroVersionId(entry.getMicroVersionId()) : '', | ||
|
Comment on lines
+717
to
+718
Contributor
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?
Contributor
Author
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 writeStartTime = Date.now(); | ||
| return this.backbeatDest.send(command) | ||
|
|
@@ -681,7 +726,9 @@ class ReplicateObject extends BackbeatTask { | |
| .catch(err => { | ||
| // eslint-disable-next-line no-param-reassign | ||
| err.origin = 'target'; | ||
| if (err.ObjNotFound || err.name === 'ObjNotFound' || | ||
| err instanceof MicroVersionIdAlreadyStoredException || | ||
| err instanceof StaleMicroVersionIdException) { | ||
| return cbOnce(err); | ||
| } | ||
| log.error('an error occurred when putting metadata to S3', | ||
|
SylvainSenechal marked this conversation as resolved.
|
||
|
|
@@ -920,16 +967,27 @@ class ReplicateObject extends BackbeatTask { | |
| return this._getAndPutData(sourceEntry, destEntry, log, | ||
| next); | ||
| } | ||
| return next(null, [], undefined); | ||
| }, | ||
| // update location, replication status and put metadata in | ||
| // target bucket | ||
| (destLocations, conflict, next) => { | ||
| if (this._shouldSkipMetadata(sourceEntry.getMicroVersionId(), conflict, log)) { | ||
| log.info('skipping putMetadata: destination already has same or newer revision', { | ||
| entry: destEntry.getLogInfo(), | ||
| }); | ||
| return next(); | ||
| } | ||
| // METADATA tells cloudserver to UPDATE the existing metadata document | ||
| // (preserving the stored location). DATA,METADATA tells cloudserver to | ||
| // 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; | ||
| destEntry.setLocation(destLocations); | ||
| return this._putMetadata(destEntry, localMdOnly, log, err => { | ||
| if (err) { | ||
| return this._deleteOrphans(destEntry, destLocations, log, () => next(err)); | ||
| } | ||
| return next(); | ||
| }); | ||
|
|
@@ -938,30 +996,63 @@ class ReplicateObject extends BackbeatTask { | |
| err, sourceEntry, destEntry, kafkaEntry, log, done)); | ||
| } | ||
|
|
||
| // Returns true if putMetadata can be skipped because the destination already | ||
| // 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) { | ||
|
Contributor
Author
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 |
||
| if (!conflict) { | ||
| return false; | ||
| } | ||
| let destMvId = null; | ||
| if (conflict.microVersionId) { | ||
| const decoded = decodeMicroVersionId(conflict.microVersionId); | ||
| if (decoded instanceof Error) { | ||
| log.warn('could not decode microVersionId from putData 409, ' + | ||
| 'proceeding to putMetadata without skip optimisation', { | ||
| error: decoded.message, | ||
| }); | ||
| } else { | ||
| destMvId = decoded; | ||
| } | ||
| } | ||
| const comparison = compareMicroVersionId(sourceMicroVersionId, destMvId); | ||
| return destMvId !== null && | ||
|
Contributor
Author
There was a problem hiding this comment. May completely remove the null check if I end up update the compareMicroVersionId function. |
||
| (comparison === Ordering.OLDER || comparison === Ordering.EQUAL); | ||
| } | ||
|
|
||
| _processQueueEntryRetryFull(sourceEntry, destEntry, kafkaEntry, log, done) { | ||
| return async.waterfall([ | ||
| next => this._getAndPutData(sourceEntry, destEntry, log, next), | ||
| (destLocations, conflict, next) => { | ||
| if (this._shouldSkipMetadata(sourceEntry.getMicroVersionId(), conflict, log)) { | ||
| log.info('skipping putMetadata: destination already has same or newer revision', { | ||
| entry: destEntry.getLogInfo(), | ||
| }); | ||
| return next(); | ||
| } | ||
| const localMdOnly = conflict !== undefined; | ||
| destEntry.setLocation(destLocations); | ||
| return this._putMetadata(destEntry, localMdOnly, log, next); | ||
| }, | ||
| ], err => this._handleReplicationOutcome( | ||
| err, sourceEntry, destEntry, kafkaEntry, log, done)); | ||
| } | ||
|
|
||
| _handleReplicationOutcome(err, sourceEntry, destEntry, kafkaEntry, | ||
|
Contributor
There was a problem hiding this comment. This section has quite a bit of nested conditional logic and duplicate checks that make it hard to read and maintain. Could we flatten this using guard clauses (early returns) and abstract the err.XYZ || err.name === 'XYZ' checks into a helper function? It would drastically reduce the cognitive load of this function. Let me know if you want to pair on it!
Contributor
Author
There was a problem hiding this comment. I agree its trash code and the diff is hard to read with the last else I just changed it and tried something that i didn't want to do first but i think it's fine : for each condition, directly publish/return, without having to do it at the end of the function. I believe its quite readable this way
Contributor
Author
There was a problem hiding this comment. Could still use some refactor maybe, although all the erro don't have the same form, I think the diff is reasonnable here |
||
| log, done) { | ||
| if (err instanceof MicroVersionIdAlreadyStoredException || | ||
| err instanceof StaleMicroVersionIdException) { | ||
| log.info('replication completed: object already at destination', | ||
| { entry: sourceEntry.getLogInfo(), reason: err.name }); | ||
| this._publishReplicationStatus(sourceEntry, 'COMPLETED', { kafkaEntry, log }); | ||
| return done(null, { committable: false }); | ||
| } | ||
| if (!err) { | ||
| log.debug('replication succeeded for object, publishing ' + | ||
| 'replication status as COMPLETED', | ||
| { entry: sourceEntry.getLogInfo() }); | ||
| this._publishReplicationStatus(sourceEntry, 'COMPLETED', { kafkaEntry, log }); | ||
| return done(null, { committable: false }); | ||
| } | ||
| if (err.BadRole || err.name === 'BadRole' || | ||
|
|
@@ -991,8 +1082,7 @@ class ReplicateObject extends BackbeatTask { | |
| { entry: sourceEntry.getLogInfo() }); | ||
| return done(); | ||
| } | ||
| log.info('replication target object not found, retrying with full data write', | ||
| { entry: sourceEntry.getLogInfo() }); | ||
| // TODO: Is this the right place to capture retry metrics? | ||
| return this._processQueueEntryRetryFull( | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.