From a4f9c62e2aac4319ac726ab540ec40e8cb5c6b0a Mon Sep 17 00:00:00 2001 From: Francois Ferrand Date: Mon, 24 Aug 2026 10:48:03 +0200 Subject: [PATCH 1/3] Do not garbage-collect the from-location when it is an isCRR location Data sitting on an isCRR location is production data owned by a remote site: we may read it, but deleting it is never ours to do. The copy engine reuses the lifecycle transition pipeline, which garbage-collects the from-location once the new one is merged into the metadata - against an isCRR source that would wipe the remote production copy. Unlike a transition, a localization merge must therefore leave the from-location alone. Keying this on the location type rather than making it a clean-room special case also gives us replay safety for duplicate copy actions: the second merge supersedes the first and collects its copy, which is only safe because the first never touched the remote source. Issue: BB-813 --- conf/locationConfig.json | 7 ++ .../tasks/LifecycleUpdateTransitionTask.js | 18 ++++- lib/util/locations.js | 52 ++++++++++++++ tests/unit/lib/util/locations.spec.js | 67 +++++++++++++++++++ .../lifecycle/CircuitBreakerGroup.spec.js | 10 +++ .../LifecycleUpdateTransitionTask.spec.js | 43 ++++++++++++ 6 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 lib/util/locations.js create mode 100644 tests/unit/lib/util/locations.spec.js diff --git a/conf/locationConfig.json b/conf/locationConfig.json index 8ba3dc334c..dceb31ddac 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/lifecycle/tasks/LifecycleUpdateTransitionTask.js b/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js index 4f57c33c7f..52801e5820 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 { filterOutCRRLocations, getCRRLocationNames } = require('../../../lib/util/locations'); /** @typedef { import('../objectProcessor/LifecycleObjectProcessor.js') } LifecycleObjectProcessor */ class LifecycleUpdateTransitionTask extends BackbeatTask { @@ -112,6 +113,21 @@ 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. + const locationsToGC = filterOutCRRLocations(locations); + if (locationsToGC.length !== locations.length) { + log.info('skipping garbage collection of data on CRR location', { + method: 'LifecycleUpdateTransitionTask._garbageCollectLocation', + bucket, + objectKey: key, + versionId: version, + dataStoreNames: getCRRLocationNames(locations), + }); + } + if (locationsToGC.length === 0) { + return process.nextTick(done); + } const gcEntry = ActionQueueEntry.create('deleteData') .addContext({ origin: 'lifecycle', @@ -126,7 +142,7 @@ class LifecycleUpdateTransitionTask extends BackbeatTask { .setAttribute('serviceName', 'lifecycle-transition') .setAttribute('target.accountId', accountId) .setAttribute('target.owner', owner) - .setAttribute('target.locations', locations); + .setAttribute('target.locations', locationsToGC); this.gcProducer.publishActionEntry(gcEntry); return process.nextTick(done); } diff --git a/lib/util/locations.js b/lib/util/locations.js new file mode 100644 index 0000000000..fb09e6eb14 --- /dev/null +++ b/lib/util/locations.js @@ -0,0 +1,52 @@ +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(dataStoreName && locationsConfig[dataStoreName] && + locationsConfig[dataStoreName].isCRR); +} + +/** + * Remove from a list of location parts those living on a CRR location, + * i.e. those which must never be garbage-collected. + * + * @param {Object[]} locations - array of location parts + * @return {Object[]} the location parts which are safe to delete + */ +function filterOutCRRLocations(locations) { + if (!Array.isArray(locations)) { + return []; + } + return locations.filter(location => !isCRRLocation(location && location.dataStoreName)); +} + +/** + * List the distinct CRR location names found in a list of location parts, + * for logging purposes. + * + * @param {Object[]} locations - array of location parts + * @return {String[]} distinct CRR location names + */ +function getCRRLocationNames(locations) { + if (!Array.isArray(locations)) { + return []; + } + const names = locations + .map(location => location && location.dataStoreName) + .filter(dataStoreName => isCRRLocation(dataStoreName)); + return [...new Set(names)]; +} + +module.exports = { + isCRRLocation, + filterOutCRRLocations, + getCRRLocationNames, +}; diff --git a/tests/unit/lib/util/locations.spec.js b/tests/unit/lib/util/locations.spec.js new file mode 100644 index 0000000000..c86e0fe355 --- /dev/null +++ b/tests/unit/lib/util/locations.spec.js @@ -0,0 +1,67 @@ +const assert = require('assert'); + +const { + isCRRLocation, + filterOutCRRLocations, + getCRRLocationNames, +} = require('../../../../lib/util/locations'); + +const crrPart = { + key: 'crrKey', + size: 10, + start: 0, + dataStoreName: 'location-crr-source', + dataStoreType: 'scality', +}; +const localPart = { + key: 'localKey', + size: 10, + start: 0, + dataStoreName: 'us-east-1', + dataStoreType: 'file', +}; + +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); + }); + }); + + describe('filterOutCRRLocations', () => { + it('should drop the parts living on a CRR location', () => { + assert.deepStrictEqual( + filterOutCRRLocations([localPart, crrPart]), [localPart]); + }); + + it('should keep all parts when none is on a CRR location', () => { + assert.deepStrictEqual( + filterOutCRRLocations([localPart]), [localPart]); + }); + + it('should return an empty array when locations is not an array', () => { + assert.deepStrictEqual(filterOutCRRLocations(undefined), []); + }); + }); + + describe('getCRRLocationNames', () => { + it('should list the distinct CRR location names', () => { + assert.deepStrictEqual( + getCRRLocationNames([localPart, crrPart, crrPart]), + ['location-crr-source']); + }); + + it('should return an empty array when there is no CRR location', () => { + assert.deepStrictEqual(getCRRLocationNames([localPart]), []); + }); + }); +}); diff --git a/tests/unit/lifecycle/CircuitBreakerGroup.spec.js b/tests/unit/lifecycle/CircuitBreakerGroup.spec.js index 56e825d455..c8c8737b6f 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 61748e96e3..d05a1abd6b 100644 --- a/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js +++ b/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js @@ -148,6 +148,49 @@ 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 only GC the parts which are not 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); + const receivedGcEntry = gcProducer.getReceivedEntry(); + assert.deepStrictEqual( + receivedGcEntry.getAttribute('target.locations'), oldLocation); + 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 => { From c5a5e88d72e4e87488476edaa18a87129bbb1fb3 Mon Sep 17 00:00:00 2001 From: Francois Ferrand Date: Mon, 24 Aug 2026 10:48:10 +0200 Subject: [PATCH 2/3] Skip isCRR location parts in the GC service Same no-GC rule as in the copy engine, applied where data actually gets deleted: whatever published the deleteData action, parts living on an isCRR location are remote production data and get skipped rather than deleted. That covers every publisher, notably restored-object expiration, where expiring a clean-room object that was never localized would otherwise delete the production copy. Reaching the GC service with such a location means something upstream is wrong, so it warns, but deleting is never the right answer: the entry is still completed and the offset committed, and no completion metric is emitted for a delete that did not happen. Issue: BB-818 --- extensions/gc/tasks/GarbageCollectorTask.js | 25 ++++- tests/unit/gc/GarbageCollectorTask.spec.js | 109 ++++++++++++++++++++ 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/extensions/gc/tasks/GarbageCollectorTask.js b/extensions/gc/tasks/GarbageCollectorTask.js index dae27c5807..812691165e 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 { filterOutCRRLocations, getCRRLocationNames } = require('../../../lib/util/locations'); /** @typedef { import('../GarbageCollector.js') } GarbageCollector */ class GarbageCollectorTask extends BackbeatTask { @@ -142,8 +143,26 @@ 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. + const locationsToDelete = filterOutCRRLocations(locations); + if (locationsToDelete.length !== (locations || []).length) { + log.warn('refusing to delete data on CRR location', { + method: 'GarbageCollectorTask._executeDeleteDataOnce', + bucket: entry.getAttribute('source.bucket'), + objectKey: entry.getAttribute('source.objectKey'), + dataStoreNames: getCRRLocationNames(locations), + ruleType, + ...entry.getLogInfo(), + }); + } + if (locationsToDelete.length === 0) { + entry.setEnd(null); + log.info('action execution ended, nothing to delete', entry.getLogInfo()); + return process.nextTick(done); + } const params = { - Locations: locations.map(location => ({ + Locations: locationsToDelete.map(location => ({ key: location.key, dataStoreName: location.dataStoreName, size: location.size, @@ -159,7 +178,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); @@ -184,7 +203,7 @@ class GarbageCollectorTask extends BackbeatTask { } GarbageCollectorMetrics.onGcCompleted(log, ruleType, - locations[0]?.dataStoreName, Date.now() - entry.getAttribute('timestamp')); + locationsToDelete[0]?.dataStoreName, Date.now() - entry.getAttribute('timestamp')); return done(); }); } diff --git a/tests/unit/gc/GarbageCollectorTask.spec.js b/tests/unit/gc/GarbageCollectorTask.spec.js index 3cd4d7fdbc..36a8ae4133 100644 --- a/tests/unit/gc/GarbageCollectorTask.spec.js +++ b/tests/unit/gc/GarbageCollectorTask.spec.js @@ -387,4 +387,113 @@ 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.deepStrictEqual( + log.warn.firstCall.args[1].dataStoreNames, + ['location-crr-source']); + assert.strictEqual(entry.getStatus(), 'success'); + batchDeleteDataSpy.restore(); + onGcCompletedSpy.restore(); + done(); + }); + }); + + it('should only delete the parts which are not on a CRR location', done => { + const entry = createDeleteDataEntry([crrLocation, 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, 1); + 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(); + }); + }); + }); }); From fc650018e64b6542a7468352154da066a6cf6389 Mon Sep 17 00:00:00 2001 From: Francois Ferrand Date: Wed, 26 Aug 2026 09:37:44 +0200 Subject: [PATCH 3/3] Simplify the isCRR check to a single location test All parts of an object version live on the same location, so filtering the location list part by part, and handling a mix of CRR and local parts, was solving a problem that cannot happen. Reduce the helper to a plain isCRRLocation() lookup and make both call sites skip the whole action when the data being collected sits on such a location. Keyed on the dataStoreName of the parts rather than the object storage class: on the transition rollback paths we collect the freshly written local copy while the source storage class still points at the old, possibly remote, location. Issue: BB-813 --- extensions/gc/tasks/GarbageCollectorTask.js | 21 +++----- .../tasks/LifecycleUpdateTransitionTask.js | 13 ++--- lib/util/locations.js | 36 +------------- tests/unit/gc/GarbageCollectorTask.spec.js | 17 +++---- tests/unit/lib/util/locations.spec.js | 49 +------------------ .../LifecycleUpdateTransitionTask.spec.js | 6 +-- 6 files changed, 24 insertions(+), 118 deletions(-) diff --git a/extensions/gc/tasks/GarbageCollectorTask.js b/extensions/gc/tasks/GarbageCollectorTask.js index 812691165e..82ca27a16d 100644 --- a/extensions/gc/tasks/GarbageCollectorTask.js +++ b/extensions/gc/tasks/GarbageCollectorTask.js @@ -5,7 +5,7 @@ const { ObjectMD } = require('arsenal').models; const BackbeatTask = require('../../../lib/tasks/BackbeatTask'); const { BatchDeleteCommand } = require('@scality/cloudserverclient'); const { GarbageCollectorMetrics } = require('../GarbageCollectorMetrics'); -const { filterOutCRRLocations, getCRRLocationNames } = require('../../../lib/util/locations'); +const { isCRRLocation } = require('../../../lib/util/locations'); /** @typedef { import('../GarbageCollector.js') } GarbageCollector */ class GarbageCollectorTask extends BackbeatTask { @@ -145,24 +145,17 @@ class GarbageCollectorTask extends BackbeatTask { 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. - const locationsToDelete = filterOutCRRLocations(locations); - if (locationsToDelete.length !== (locations || []).length) { - log.warn('refusing to delete data on CRR location', { + if (locations.some(location => isCRRLocation(location.dataStoreName))) { + log.warn('refusing to delete data on a CRR location', Object.assign({ method: 'GarbageCollectorTask._executeDeleteDataOnce', - bucket: entry.getAttribute('source.bucket'), - objectKey: entry.getAttribute('source.objectKey'), - dataStoreNames: getCRRLocationNames(locations), + dataStoreName: locations[0]?.dataStoreName, ruleType, - ...entry.getLogInfo(), - }); - } - if (locationsToDelete.length === 0) { + }, entry.getLogInfo())); entry.setEnd(null); - log.info('action execution ended, nothing to delete', entry.getLogInfo()); return process.nextTick(done); } const params = { - Locations: locationsToDelete.map(location => ({ + Locations: locations.map(location => ({ key: location.key, dataStoreName: location.dataStoreName, size: location.size, @@ -203,7 +196,7 @@ class GarbageCollectorTask extends BackbeatTask { } GarbageCollectorMetrics.onGcCompleted(log, ruleType, - locationsToDelete[0]?.dataStoreName, Date.now() - entry.getAttribute('timestamp')); + locations[0]?.dataStoreName, Date.now() - entry.getAttribute('timestamp')); return done(); }); } diff --git a/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js b/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js index 52801e5820..54d0503043 100644 --- a/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js +++ b/extensions/lifecycle/tasks/LifecycleUpdateTransitionTask.js @@ -6,7 +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 { filterOutCRRLocations, getCRRLocationNames } = require('../../../lib/util/locations'); +const { isCRRLocation } = require('../../../lib/util/locations'); /** @typedef { import('../objectProcessor/LifecycleObjectProcessor.js') } LifecycleObjectProcessor */ class LifecycleUpdateTransitionTask extends BackbeatTask { @@ -115,17 +115,14 @@ class LifecycleUpdateTransitionTask extends BackbeatTask { 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. - const locationsToGC = filterOutCRRLocations(locations); - if (locationsToGC.length !== locations.length) { - log.info('skipping garbage collection of data on CRR location', { + 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, - dataStoreNames: getCRRLocationNames(locations), + dataStoreName: locations[0]?.dataStoreName, }); - } - if (locationsToGC.length === 0) { return process.nextTick(done); } const gcEntry = ActionQueueEntry.create('deleteData') @@ -142,7 +139,7 @@ class LifecycleUpdateTransitionTask extends BackbeatTask { .setAttribute('serviceName', 'lifecycle-transition') .setAttribute('target.accountId', accountId) .setAttribute('target.owner', owner) - .setAttribute('target.locations', locationsToGC); + .setAttribute('target.locations', locations); this.gcProducer.publishActionEntry(gcEntry); return process.nextTick(done); } diff --git a/lib/util/locations.js b/lib/util/locations.js index fb09e6eb14..bb7c114e21 100644 --- a/lib/util/locations.js +++ b/lib/util/locations.js @@ -10,43 +10,9 @@ const locationsConfig = require('../../conf/locationConfig.json') || {}; * @return {Boolean} true if the location is a CRR (remote) location */ function isCRRLocation(dataStoreName) { - return Boolean(dataStoreName && locationsConfig[dataStoreName] && - locationsConfig[dataStoreName].isCRR); -} - -/** - * Remove from a list of location parts those living on a CRR location, - * i.e. those which must never be garbage-collected. - * - * @param {Object[]} locations - array of location parts - * @return {Object[]} the location parts which are safe to delete - */ -function filterOutCRRLocations(locations) { - if (!Array.isArray(locations)) { - return []; - } - return locations.filter(location => !isCRRLocation(location && location.dataStoreName)); -} - -/** - * List the distinct CRR location names found in a list of location parts, - * for logging purposes. - * - * @param {Object[]} locations - array of location parts - * @return {String[]} distinct CRR location names - */ -function getCRRLocationNames(locations) { - if (!Array.isArray(locations)) { - return []; - } - const names = locations - .map(location => location && location.dataStoreName) - .filter(dataStoreName => isCRRLocation(dataStoreName)); - return [...new Set(names)]; + return Boolean(locationsConfig[dataStoreName]?.isCRR); } module.exports = { isCRRLocation, - filterOutCRRLocations, - getCRRLocationNames, }; diff --git a/tests/unit/gc/GarbageCollectorTask.spec.js b/tests/unit/gc/GarbageCollectorTask.spec.js index 36a8ae4133..8a5477e1c3 100644 --- a/tests/unit/gc/GarbageCollectorTask.spec.js +++ b/tests/unit/gc/GarbageCollectorTask.spec.js @@ -453,9 +453,9 @@ describe('GarbageCollectorTask', () => { assert.strictEqual(backbeatClient.times.batchDeleteResponse, 0); assert.strictEqual(onGcCompletedSpy.callCount, 0); assert.strictEqual(log.warn.callCount, 1); - assert.deepStrictEqual( - log.warn.firstCall.args[1].dataStoreNames, - ['location-crr-source']); + assert.strictEqual( + log.warn.firstCall.args[1].dataStoreName, + 'location-crr-source'); assert.strictEqual(entry.getStatus(), 'success'); batchDeleteDataSpy.restore(); onGcCompletedSpy.restore(); @@ -463,17 +463,16 @@ describe('GarbageCollectorTask', () => { }); }); - it('should only delete the parts which are not on a CRR location', done => { - const entry = createDeleteDataEntry([crrLocation, regularLocation]); + 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, 1); - assert.deepStrictEqual( - batchDeleteDataSpy.firstCall.args[0].Locations, - [regularLocation]); + assert.strictEqual(batchDeleteDataSpy.callCount, 0); assert.strictEqual(log.warn.callCount, 1); + assert.strictEqual(entry.getStatus(), 'success'); batchDeleteDataSpy.restore(); done(); }); diff --git a/tests/unit/lib/util/locations.spec.js b/tests/unit/lib/util/locations.spec.js index c86e0fe355..00aef10101 100644 --- a/tests/unit/lib/util/locations.spec.js +++ b/tests/unit/lib/util/locations.spec.js @@ -1,25 +1,6 @@ const assert = require('assert'); -const { - isCRRLocation, - filterOutCRRLocations, - getCRRLocationNames, -} = require('../../../../lib/util/locations'); - -const crrPart = { - key: 'crrKey', - size: 10, - start: 0, - dataStoreName: 'location-crr-source', - dataStoreType: 'scality', -}; -const localPart = { - key: 'localKey', - size: 10, - start: 0, - dataStoreName: 'us-east-1', - dataStoreType: 'file', -}; +const { isCRRLocation } = require('../../../../lib/util/locations'); describe('locations util', () => { describe('isCRRLocation', () => { @@ -36,32 +17,4 @@ describe('locations util', () => { assert.strictEqual(isCRRLocation(undefined), false); }); }); - - describe('filterOutCRRLocations', () => { - it('should drop the parts living on a CRR location', () => { - assert.deepStrictEqual( - filterOutCRRLocations([localPart, crrPart]), [localPart]); - }); - - it('should keep all parts when none is on a CRR location', () => { - assert.deepStrictEqual( - filterOutCRRLocations([localPart]), [localPart]); - }); - - it('should return an empty array when locations is not an array', () => { - assert.deepStrictEqual(filterOutCRRLocations(undefined), []); - }); - }); - - describe('getCRRLocationNames', () => { - it('should list the distinct CRR location names', () => { - assert.deepStrictEqual( - getCRRLocationNames([localPart, crrPart, crrPart]), - ['location-crr-source']); - }); - - it('should return an empty array when there is no CRR location', () => { - assert.deepStrictEqual(getCRRLocationNames([localPart]), []); - }); - }); }); diff --git a/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js b/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js index d05a1abd6b..04cc2a1502 100644 --- a/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js +++ b/tests/unit/lifecycle/LifecycleUpdateTransitionTask.spec.js @@ -162,15 +162,13 @@ describe('LifecycleUpdateTransitionTask', () => { }); }); - it('should only GC the parts which are not on a CRR location', 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); - const receivedGcEntry = gcProducer.getReceivedEntry(); - assert.deepStrictEqual( - receivedGcEntry.getAttribute('target.locations'), oldLocation); + assert.strictEqual(gcProducer.getReceivedEntry(), null); done(); }); });