diff --git a/conf/locationConfig.json b/conf/locationConfig.json index 8ba3dc334..dceb31dda 100644 --- a/conf/locationConfig.json +++ b/conf/locationConfig.json @@ -42,5 +42,12 @@ "legacyAwsBehavior": false, "isCold": true, "details": {} + }, + "location-crr-source": { + "type": "scality", + "objectId": "location-crr-source", + "legacyAwsBehavior": false, + "isCRR": true, + "details": {} } } diff --git a/extensions/gc/tasks/GarbageCollectorTask.js b/extensions/gc/tasks/GarbageCollectorTask.js index dae27c580..82ca27a16 100644 --- a/extensions/gc/tasks/GarbageCollectorTask.js +++ b/extensions/gc/tasks/GarbageCollectorTask.js @@ -5,6 +5,7 @@ const { ObjectMD } = require('arsenal').models; const BackbeatTask = require('../../../lib/tasks/BackbeatTask'); const { BatchDeleteCommand } = require('@scality/cloudserverclient'); const { GarbageCollectorMetrics } = require('../GarbageCollectorMetrics'); +const { isCRRLocation } = require('../../../lib/util/locations'); /** @typedef { import('../GarbageCollector.js') } GarbageCollector */ class GarbageCollectorTask extends BackbeatTask { @@ -142,6 +143,17 @@ class GarbageCollectorTask extends BackbeatTask { _executeDeleteDataOnce(entry, log, done) { const { locations } = entry.getAttribute('target'); const ruleType = entry.getContextAttribute('ruleType'); + // Last line of defense: whoever published this entry, data on a CRR + // location belongs to the remote site and must never be deleted. + if (locations.some(location => isCRRLocation(location.dataStoreName))) { + log.warn('refusing to delete data on a CRR location', Object.assign({ + method: 'GarbageCollectorTask._executeDeleteDataOnce', + dataStoreName: locations[0]?.dataStoreName, + ruleType, + }, entry.getLogInfo())); + entry.setEnd(null); + return process.nextTick(done); + } const params = { Locations: locations.map(location => ({ key: location.key, @@ -159,7 +171,7 @@ class GarbageCollectorTask extends BackbeatTask { }), }; - this._batchDeleteData(params, entry, log, err => { + return this._batchDeleteData(params, entry, log, err => { // ruleType can be either `transition` or `restore` (for restore-expiration) GarbageCollectorMetrics.onS3Request(log, 'batchdelete', ruleType, err); entry.setEnd(err); diff --git a/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js b/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js index 4f57c33c7..54d050304 100644 --- a/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js +++ b/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js @@ -6,6 +6,7 @@ const BackbeatTask = require('../../../lib/tasks/BackbeatTask'); const ActionQueueEntry = require('../../../lib/models/ActionQueueEntry'); const ObjectMD = require('arsenal').models.ObjectMD; const { LifecycleMetrics } = require('../LifecycleMetrics'); +const { isCRRLocation } = require('../../../lib/util/locations'); /** @typedef { import('../objectProcessor/LifecycleObjectProcessor.js') } LifecycleObjectProcessor */ class LifecycleUpdateTransitionTask extends BackbeatTask { @@ -112,6 +113,18 @@ class LifecycleUpdateTransitionTask extends BackbeatTask { _garbageCollectLocation(entry, locations, log, done) { const { bucket, key, version, eTag, accountId, owner } = this.getTargetAttribute(entry); + // Data stored on a CRR location belongs to the remote site: the copy we + // just made is an extra local copy, the source must be left untouched. + if (locations.some(location => isCRRLocation(location.dataStoreName))) { + log.info('skipping garbage collection of data on a CRR location', { + method: 'LifecycleUpdateTransitionTask._garbageCollectLocation', + bucket, + objectKey: key, + versionId: version, + dataStoreName: locations[0]?.dataStoreName, + }); + return process.nextTick(done); + } const gcEntry = ActionQueueEntry.create('deleteData') .addContext({ origin: 'lifecycle', diff --git a/lib/util/locations.js b/lib/util/locations.js new file mode 100644 index 000000000..bb7c114e2 --- /dev/null +++ b/lib/util/locations.js @@ -0,0 +1,18 @@ +const locationsConfig = require('../../conf/locationConfig.json') || {}; + +/** + * Tell whether a location holds data owned by a remote site. + * + * Data stored on such a location is remote production data: we may read it + * (e.g. to copy it locally), but we must never delete it. + * + * @param {String} dataStoreName - location name + * @return {Boolean} true if the location is a CRR (remote) location + */ +function isCRRLocation(dataStoreName) { + return Boolean(locationsConfig[dataStoreName]?.isCRR); +} + +module.exports = { + isCRRLocation, +}; diff --git a/tests/unit/gc/GarbageCollectorTask.spec.js b/tests/unit/gc/GarbageCollectorTask.spec.js index 3cd4d7fdb..8a5477e1c 100644 --- a/tests/unit/gc/GarbageCollectorTask.spec.js +++ b/tests/unit/gc/GarbageCollectorTask.spec.js @@ -387,4 +387,112 @@ describe('GarbageCollectorTask', () => { }); }); + describe('with CRR locations', () => { + let log; + + function createDeleteDataEntry(locations) { + return ActionQueueEntry.create('deleteData') + .addContext({ + origin: 'lifecycle', + ruleType: 'transition', + bucketName: bucket, + objectKey: key, + versionId: version, + }) + .setAttribute('serviceName', 'lifecycle-transition') + .setAttribute('source', { + bucket, + objectKey: key, + storageClass: 'sourceStorageClass', + }) + .setAttribute('target', { + bucket, + key: version, + version: key, + accountId, + owner, + locations, + }); + } + + const crrLocation = { + key: 'crrKey', + dataStoreName: 'location-crr-source', + size: 10, + dataStoreVersionId: 'crrVersionId', + }; + const regularLocation = { + key: 'locationKey', + dataStoreName: 'us-east-1', + size: 20, + dataStoreVersionId: 'dataStoreVersionId', + }; + + beforeEach(() => { + log = { + info: sinon.spy(), + warn: sinon.spy(), + debug: sinon.spy(), + error: sinon.spy(), + getSerializedUids: () => 'uids', + }; + log.end = () => log; + gcTask.logger = { newRequestLogger: () => log }; + backbeatClient.batchDeleteResponse = { error: null, res: null }; + }); + + it('should not delete anything and warn when all locations are on a ' + + 'CRR location', done => { + const entry = createDeleteDataEntry([crrLocation]); + const batchDeleteDataSpy = sinon.spy(gcTask, '_batchDeleteData'); + const onGcCompletedSpy = sinon.spy(GarbageCollectorMetrics, 'onGcCompleted'); + + gcTask.processActionEntry(entry, err => { + assert.ifError(err); + assert.strictEqual(batchDeleteDataSpy.callCount, 0); + assert.strictEqual(backbeatClient.times.batchDeleteResponse, 0); + assert.strictEqual(onGcCompletedSpy.callCount, 0); + assert.strictEqual(log.warn.callCount, 1); + assert.strictEqual( + log.warn.firstCall.args[1].dataStoreName, + 'location-crr-source'); + assert.strictEqual(entry.getStatus(), 'success'); + batchDeleteDataSpy.restore(); + onGcCompletedSpy.restore(); + done(); + }); + }); + + it('should not delete anything when any location is on a CRR ' + + 'location', done => { + const entry = createDeleteDataEntry([regularLocation, crrLocation]); + const batchDeleteDataSpy = sinon.spy(gcTask, '_batchDeleteData'); + + gcTask.processActionEntry(entry, err => { + assert.ifError(err); + assert.strictEqual(batchDeleteDataSpy.callCount, 0); + assert.strictEqual(log.warn.callCount, 1); + assert.strictEqual(entry.getStatus(), 'success'); + batchDeleteDataSpy.restore(); + done(); + }); + }); + + it('should delete all locations and not warn when none is on a CRR ' + + 'location', done => { + const entry = createDeleteDataEntry([regularLocation]); + const batchDeleteDataSpy = sinon.spy(gcTask, '_batchDeleteData'); + + gcTask.processActionEntry(entry, err => { + assert.ifError(err); + assert.strictEqual(batchDeleteDataSpy.callCount, 1); + assert.deepStrictEqual( + batchDeleteDataSpy.firstCall.args[0].Locations, + [regularLocation]); + assert.strictEqual(log.warn.callCount, 0); + batchDeleteDataSpy.restore(); + done(); + }); + }); + }); }); diff --git a/tests/unit/lib/util/locations.spec.js b/tests/unit/lib/util/locations.spec.js new file mode 100644 index 000000000..00aef1010 --- /dev/null +++ b/tests/unit/lib/util/locations.spec.js @@ -0,0 +1,20 @@ +const assert = require('assert'); + +const { isCRRLocation } = require('../../../../lib/util/locations'); + +describe('locations util', () => { + describe('isCRRLocation', () => { + it('should return true for a location flagged isCRR', () => { + assert.strictEqual(isCRRLocation('location-crr-source'), true); + }); + + it('should return false for a regular location', () => { + assert.strictEqual(isCRRLocation('us-east-1'), false); + }); + + it('should return false for an unknown or missing location', () => { + assert.strictEqual(isCRRLocation('does-not-exist'), false); + assert.strictEqual(isCRRLocation(undefined), false); + }); + }); +}); diff --git a/tests/unit/lifecycle/CircuitBreakerGroup.spec.js b/tests/unit/lifecycle/CircuitBreakerGroup.spec.js index 56e825d45..c8c8737b6 100644 --- a/tests/unit/lifecycle/CircuitBreakerGroup.spec.js +++ b/tests/unit/lifecycle/CircuitBreakerGroup.spec.js @@ -436,6 +436,11 @@ describe('extractBucketProcessorCircuitBreakerConfigs', () => { '${location}', 'location-dmf-v1', ), + formatProbeConfig( + topicSpecificLocationTemplateProbe, + '${location}', + 'location-crr-source', + ), ], }, global: [], @@ -493,6 +498,11 @@ describe('extractBucketProcessorCircuitBreakerConfigs', () => { '${location}', 'location-dmf-v1', ), + formatProbeConfig( + topicSpecificLocationTemplateProbe, + '${location}', + 'location-crr-source', + ), ], }, global: [], diff --git a/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js b/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js index 61748e96e..04cc2a150 100644 --- a/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js +++ b/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js @@ -148,6 +148,47 @@ describe('LifecycleUpdateTransitionTask', () => { }); }); + it('should update metadata but not GC the from-location when it is a CRR ' + + 'location', done => { + const crrLocation = [Object.assign({}, oldLocation[0], + { dataStoreName: 'location-crr-source' })]; + mdObj.setLocation(crrLocation); + task.processActionEntry(actionEntry, err => { + assert.ifError(err); + const receivedMd = backbeatMetadataProxyClient.getReceivedMd(); + assert.deepStrictEqual(receivedMd.location, newLocation); + assert.strictEqual(gcProducer.getReceivedEntry(), null); + done(); + }); + }); + + it('should not GC anything when any part is on a CRR location', done => { + const crrPart = Object.assign({}, oldLocation[0], + { key: 'crrKey', dataStoreName: 'location-crr-source' }); + mdObj.setLocation([crrPart, ...oldLocation]); + task.processActionEntry(actionEntry, err => { + assert.ifError(err); + assert.strictEqual(gcProducer.getReceivedEntry(), null); + done(); + }); + }); + + it('should still GC the new location on rollback even if the ' + + 'from-location is a CRR location', done => { + mdObj.setLocation([Object.assign({}, oldLocation[0], + { dataStoreName: 'location-crr-source' })]); + actionEntry.setAttribute('target.eTag', + '"6713e7cf89b6b16d5abf11d1fabac587"'); + task.processActionEntry(actionEntry, err => { + assert.ifError(err); + assert.strictEqual(backbeatMetadataProxyClient.getReceivedMd(), null); + const receivedGcEntry = gcProducer.getReceivedEntry(); + assert.deepStrictEqual( + receivedGcEntry.getAttribute('target.locations'), newLocation); + done(); + }); + }); + it('should reset transition-in-progress flag when transition fails', done => { actionEntry.setError(errors.InternalError); task.processActionEntry(actionEntry, err => {