Skip to content

Commit

Permalink
Fix quota inflights for cold objects
Browse files Browse the repository at this point in the history 
Deleting cold objects should not increment inflight, unless they are
restored.

Issue: CLDSRV-590
  • Loading branch information
@francoisferrand
francoisferrand committed Dec 8, 2024
1 parent 8faf9b3 commit 7149ef5
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 35 deletions.
31 changes: 21 additions & 10 deletions lib/api/apiUtils/quotas/quotaUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,38 @@ const QuotaService = require('../../../quotas/quotas');
* @return {number} processed content length
*/
function processBytesToWrite(apiMethod, bucket, versionId, contentLength, objMD, destObjMD = null) {
// Get the size of "hot" data. Object being restored are _considered_ hot, as we "reserve" the
// space for them as soon as restore is requested: therefore we must 'free' it immediately on
// delete as well.
const isCold = objMD => objMD.archive && !objMD.archive.restoreRequestedAt;
const getHotContentLength = objMD => {
if (isCold(objMD)) {
return 0;
}
return Number.parseInt(objMD['content-length'], 10);
};

let bytes = contentLength;
if (apiMethod === 'objectRestore') {
// object is being restored
bytes = Number.parseInt(objMD['content-length'], 10);
} else if (!bytes && objMD?.['content-length']) {
if (apiMethod === 'objectCopy' || apiMethod === 'objectPutCopyPart') {
if (!destObjMD || bucket.isVersioningEnabled()) {
// object is being copied
bytes = Number.parseInt(objMD['content-length'], 10);
} else if (!bucket.isVersioningEnabled()) {
// object is being copied and replaces the target
bytes = Number.parseInt(objMD['content-length'], 10) -
Number.parseInt(destObjMD['content-length'], 10);
// object is being copied, increases the storage...
bytes = Number.parseInt(objMD['content-length'], 10);

if (destObjMD && !bucket.isVersioningEnabled()) {
// but it also replaces the target, which decreases storage
bytes -= getHotContentLength(destObjMD);
}
} else if (!bucket.isVersioningEnabled() || bucket.isVersioningEnabled() && versionId) {
// object is being deleted
bytes = -Number.parseInt(objMD['content-length'], 10);
// object is being deleted (non versioned) or hard-deleted (versioned, as indicated by
// the `versionId` field)
bytes = -getHotContentLength(objMD);
}
} else if (bytes && objMD?.['content-length'] && !bucket.isVersioningEnabled()) {
// object is being replaced: store the diff, if the bucket is not versioned
bytes = bytes - Number.parseInt(objMD['content-length'], 10);
bytes = bytes - getHotContentLength(objMD);
}
return bytes || 0;
}
Expand Down
104 changes: 79 additions & 25 deletions tests/unit/api/apiUtils/quotas/quotaUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -595,32 +595,76 @@ describe('processBytesToWrite', () => {
objMD = null;
});

it('should return a negative number if the operation is a delete and bucket is not versioned', () => {
bucket.isVersioningEnabled.returns(false);
objMD = { 'content-length': 100 };

const bytes = processBytesToWrite('objectDelete', bucket, versionId, contentLength, objMD);

assert.strictEqual(bytes, -100);
});

it('should return 0 if the operation is a delete and bucket is versioned', () => {
bucket.isVersioningEnabled.returns(true);
objMD = { 'content-length': 100 };

const bytes = processBytesToWrite('objectDelete', bucket, versionId, contentLength, objMD);

assert.strictEqual(bytes, 0);
});

it('should return a negative number for a versioned bucket with a versionid deletion', () => {
bucket.isVersioningEnabled.returns(true);
objMD = { 'content-length': 100 };
versionId = 'versionId';

const bytes = processBytesToWrite('objectDelete', bucket, versionId, contentLength, objMD);
const hotObject = {
'content-length': 100,
dataStoreName: 'eu-west-1',
};
const coldObject = {
...hotObject,
dataStoreName: 'glacier',
archive: {
archiveInfo: '{archiveID,archiveVersion}'
},
};
const restoringObject = {
...coldObject,
archive: {
...coldObject.archive,
restoreRequestedAt: new Date(Date.now() - 2 * 3600 * 1000).toISOString(),
restoreRequestedDays: 1,
},
};
const restoredObject = {
...restoringObject,
dataStoreName: 'eu-west-1',
'x-amz-storage-class': 'glacier',
archive: {
...restoringObject.archive,
restoreCompletedAt: new Date(Date.now() - 3600 * 1000),
restoreWillExpireAt: new Date(Date.now() + 23 * 3600 * 1000),
},
};
const expiredObject = {
...restoredObject,
archive: {
...coldObject.archive,
restoreRequestedAt: new Date(Date.now() - 25 * 3600 * 1000 - 1000).toString(),
restoreRequestedDays: 1,
restoreCompletedAt: new Date(Date.now() - 24 * 3600 * 1000 - 1000),
restoreWillExpireAt: new Date(Date.now() - 1000),
},
};

assert.strictEqual(bytes, -100);
[
// non versionned case
['the content-length when deleting hot object', hotObject, false, undefined, -100],
['0 when deleting cold object', coldObject, false, undefined, 0],
['the content-length when deleting restoring object', restoringObject, false, undefined, -100],
['the content-length when deleting restored object', restoredObject, false, undefined, -100],
['the content-length when deleting expired object', expiredObject, false, undefined, -100],

// versionned case
['the content-length when deleting hot object version', hotObject, true, 'versionId', -100],
['0 when deleting cold versioned object version', coldObject, true, 'versionId', 0],
['the content-length when deleting restoring object version', restoringObject, true, 'versionId', -100],
['the content-length when deleting restored object version', restoredObject, true, 'versionId', -100],
['the content-length when deleting expired object version', expiredObject, true, 'versionId', -100],

// delete marker case
['0 when adding delete marker over hot object', hotObject, true, undefined, 0],
['0 when adding delete marker over cold object', coldObject, true, undefined, 0],
['0 when adding delete marker over restoring object', restoringObject, true, undefined, 0],
['0 when adding delete marker over restored object', restoredObject, true, undefined, 0],
['0 when adding delete marker over expired object', expiredObject, true, undefined, 0],
].forEach(([scenario, object, versionned, reqVersionId, expected]) => {
it(`should return ${scenario}`, () => {
bucket.isVersioningEnabled.returns(versionned);
objMD = object;

const bytes = processBytesToWrite('objectDelete', bucket, reqVersionId, 0, objMD);

assert.strictEqual(bytes, expected);
});
});

it('should return 0 for a delete operation if the object metadata is missing', () => {
Expand Down Expand Up @@ -699,6 +743,16 @@ describe('processBytesToWrite', () => {

assert.strictEqual(bytes, 100);
});

it('should not detect object replacement during copy object operation if the object is cold', () => {
bucket.isVersioningEnabled.returns(true);
objMD = { 'content-length': 100 };
const destObjMD = coldObject;

const bytes = processBytesToWrite('objectCopy', bucket, versionId, 0, objMD, destObjMD);

assert.strictEqual(bytes, 100);
});
});

describe('isMetricStale', () => {
Expand Down

0 comments on commit 7149ef5

Please sign in to comment.