Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/cldsrv 514 handling of metadata storage errors #5547

Open
wants to merge 3 commits into
base: archive/8.7
Choose a base branch
from
+180 −12
Open

Bugfix/cldsrv 514 handling of metadata storage errors #5547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b2e453
CLDSRV-514: clean orphans in storage if metadata stepo fails
williamlardier Mar 6, 2024
5301589
CLDSRV-514: test orphan deletion
williamlardier Mar 6, 2024
e215e4a
CLDSRV-514: add logs when potential oprhans are created
williamlardier Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions lib/api/apiUtils/object/createAndStoreObject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
/* eslint-disable camelcase */
const dontSkipBackend = externalBackends;
/* eslint-enable camelcase */
let dataGetInfoArr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you make this variable global, you need to remove it from the variables which trickle through the waterfall... (e.g. infoArr).... and it should problably be renamed, since it is not really "dataGet" anymore

let needsToCleanStorage = false;

const requestLogger =
logger.newRequestLoggerFromSerializedUids(log.getSerializedUids());
Expand Down Expand Up @@ -250,7 +252,7 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
const prefixedDataStoreETag = dataStoreETag
? `1:${dataStoreETag}`
: `1:${calculatedHash}`;
const dataGetInfoArr = [{ key, size, start: 0, dataStoreName,
dataGetInfoArr = [{ key, size, start: 0, dataStoreName,
dataStoreType, dataStoreETag: prefixedDataStoreETag,
dataStoreVersionId }];
if (cipherBundle) {
Expand Down Expand Up @@ -294,11 +296,30 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
if (options.extraMD) {
Object.assign(metadataStoreParams, options.extraMD);
}
return _storeInMDandDeleteData(bucketName, infoArr,
return _storeInMDandDeleteData(bucketName, dataGetInfoArr,
cipherBundle, metadataStoreParams,
options.dataToDelete, requestLogger, requestMethod, next);
options.dataToDelete, requestLogger, requestMethod, (err, data) => {
if (err) {
needsToCleanStorage = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of introducing 2 variables, it seems we simply need to batchDelete here : since this is the only case where cleanup needs to happen...

}
return next(err, data);
});
},
], callback);
], (err, result) => {
if (needsToCleanStorage && dataGetInfoArr) {
return data.batchDelete(dataGetInfoArr, requestMethod, null,
requestLogger, _err => {
if (_err) {
log.warn('potential orphan in storage', {
error: _err,
objects: dataGetInfoArr,
});
}
return callback(err, result);
});
}
return callback(err, result);
});
}

module.exports = createAndStoreObject;
12 changes: 10 additions & 2 deletions lib/api/multiObjectDelete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ function getObjMetadataAndDelete(authInfo, canonicalID, request,
objMD, authInfo, canonicalID, null, request,
deleteInfo.newDeleteMarker, null, overheadField, log,
's3:ObjectRemoved:DeleteMarkerCreated', (err, result) =>
callback(err, objMD, deleteInfo, result.versionId));
callback(err, objMD, deleteInfo, result?.versionId));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: this code is not strictly related to this PR, but when the function returns an error, result is null so we crash here.

},
], (err, objMD, deleteInfo, versionId) => {
if (err === skipError) {
Expand Down Expand Up @@ -428,7 +428,15 @@ function getObjMetadataAndDelete(authInfo, canonicalID, request,
}

return async.each(chunks, (chunk, done) => data.batchDelete(chunk, null, null,
logger.newRequestLoggerFromSerializedUids(log.getSerializedUids()), done),
logger.newRequestLoggerFromSerializedUids(log.getSerializedUids()), err => {
if (err) {
log.warn('potential orphan in storage', {
error: err,
objects: chunk,
});
}
return done();
}),
err => {
if (err) {
log.error('error deleting objects from data backend', { error: err });
Expand Down
18 changes: 18 additions & 0 deletions lib/api/objectPutCopyPart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
partNumber: paddedPartNumber,
uploadId,
};
let needsToCleanStorage = false;
let partLocations;

return async.waterfall([
function checkDestAuth(next) {
Expand Down Expand Up @@ -279,6 +281,7 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
}
return next(error, destBucketMD);
}
partLocations = locations;
return next(null, destBucketMD, locations, eTag,
copyObjectSize, sourceVerId, serverSideEncryption,
lastModified, splitter);
Expand Down Expand Up @@ -331,6 +334,7 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
if (err) {
log.debug('error storing new metadata',
{ error: err, method: 'storeNewPartMetadata' });
needsToCleanStorage = true;
Copy link
Contributor

@francoisferrand francoisferrand Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only case, might as well cleanup here: avoiding the global variables and keeping the existing waterfall logic...

return next(err);
}
return next(null, locations, oldLocations, destBucketMD, totalHash,
Expand Down Expand Up @@ -411,6 +415,20 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
{ error: err });
monitoring.promMetrics('PUT', destBucketName, err.code,
'putObjectCopyPart');
if (needsToCleanStorage && partLocations) {
const delLog = logger.newRequestLoggerFromSerializedUids(
log.getSerializedUids());
return data.batchDelete(partLocations, request.method, null, delLog,
_err => {
if (_err) {
log.warn('potential orphan in storage', {
error: _err,
objects: partLocations,
});
}
return callback(err, null, corsHeaders);
});
}
return callback(err, null, corsHeaders);
}
const xml = [
Expand Down
22 changes: 20 additions & 2 deletions lib/api/objectPutPart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
const mpuBucketName = `${constants.mpuBucketPrefix}${bucketName}`;
const { objectKey } = request;
const originalIdentityAuthzResults = request.actionImplicitDenies;
let needsToCleanStorage = false;
let partLocations;
let objectLocationConstraint;

return async.waterfall([
// Get the destination bucket.
Expand Down Expand Up @@ -195,7 +198,7 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
return next(errors.AccessDenied, destinationBucket);
}

const objectLocationConstraint =
objectLocationConstraint =
res.controllingLocationConstraint;
return next(null, destinationBucket,
objectLocationConstraint,
Expand Down Expand Up @@ -306,7 +309,7 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
prevObjectSize, oldLocations, objectLocationConstraint, splitter, next) => {
// Use an array to be consistent with objectPutCopyPart where there
// could be multiple locations.
const partLocations = [dataGetInfo];
partLocations = [dataGetInfo];
if (cipherBundle) {
const { algorithm, masterKeyId, cryptoScheme,
cipheredDataKey } = cipherBundle;
Expand All @@ -333,6 +336,7 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
error: err,
method: 'objectPutPart::metadata.putObjectMD',
});
needsToCleanStorage = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, best to cleanup here

return next(err, destinationBucket);
}
return next(null, partLocations, oldLocations, objectLocationConstraint,
Expand Down Expand Up @@ -413,6 +417,20 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
});
monitoring.promMetrics('PUT', bucketName, err.code,
'putObjectPart');
if (needsToCleanStorage && partLocations) {
const delLog = logger.newRequestLoggerFromSerializedUids(
log.getSerializedUids());
return data.batchDelete(partLocations, request.method, objectLocationConstraint, delLog,
_err => {
if (_err) {
log.warn('potential orphan in storage', {
error: _err,
objects: partLocations,
});
}
return cb(err, null, corsHeaders);
});
}
return cb(err, null, corsHeaders);
}
pushMetric('uploadPart', log, {
Expand Down
16 changes: 14 additions & 2 deletions lib/services.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,12 +363,24 @@ const services = {
}

if (!Array.isArray(objectMD.location)) {
data.delete(objectMD.location, deleteLog);
return cb(null, res);
return data.delete(objectMD.location, deleteLog, err => {
if (err) {
log.warn('potential orphan in storage', {
object: objectMD.location,
error: err,
});
return cb(err);
}
return cb(null, res);
});
Comment on lines +366 to +375
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: this code was not looking for any error when deleting data from storage. In case of error, that would create both orphans and invisible error codes for the user. Let me know if that should not be changed due to some non-documented reason(s).

}

return data.batchDelete(objectMD.location, null, null, deleteLog, err => {
if (err) {
log.warn('potential orphan in storage', {
objects: objectMD.location,
error: err,
});
return cb(err);
}
return cb(null, res);
Expand Down
49 changes: 49 additions & 0 deletions tests/unit/api/multipartUpload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ const expectedRetentionConfig = {
const expectedLegalHold = {
Status: ['ON'],
};
const originalPutObjectMD = metadataswitch.putObjectMD;

function _createPutPartRequest(uploadId, partNumber, partBody) {
const md5Hash = crypto.createHash('md5').update(partBody);
Expand Down Expand Up @@ -148,6 +149,10 @@ describe('Multipart Upload API', () => {
cleanup();
});

afterEach(() => {
metadataswitch.putObjectMD = originalPutObjectMD;
});

it('mpuBucketPrefix should be a defined constant', () => {
assert(constants.mpuBucketPrefix,
'Expected mpuBucketPrefix to be defined');
Expand Down Expand Up @@ -269,6 +274,50 @@ describe('Multipart Upload API', () => {
});
});

it('should not create orphans in storage when uplading a part with a failed metadata update', done => {
async.waterfall([
next => bucketPut(authInfo, bucketPutRequest, log, next),
(corsHeaders, next) => initiateMultipartUpload(authInfo,
initiateRequest, log, next),
(result, corsHeaders, next) => {
const mpuKeys = metadata.keyMaps.get(mpuBucket);
assert.strictEqual(mpuKeys.size, 1);
assert(mpuKeys.keys().next().value
.startsWith(`overview${splitter}${objectKey}`));
parseString(result, next);
},
],
(err, json) => {
// Need to build request in here since do not have uploadId
// until here
assert.ifError(err);
const testUploadId = json.InitiateMultipartUploadResult.UploadId[0];
const md5Hash = crypto.createHash('md5');
const bufferBody = Buffer.from(postBody);
md5Hash.update(bufferBody);
const calculatedHash = md5Hash.digest('hex');
const partRequest = new DummyRequest({
bucketName,
objectKey,
namespace,
url: `/${objectKey}?partNumber=1&uploadId=${testUploadId}`,
headers: { host: `${bucketName}.s3.amazonaws.com` },
query: {
partNumber: '1',
uploadId: testUploadId,
},
calculatedHash,
}, postBody);
sinon.stub(metadataswitch, 'putObjectMD').callsArgWith(5, errors.InternalError);
objectPutPart(authInfo, partRequest, undefined, log, err => {
assert(err.is.InternalError);
assert.strictEqual(ds.filter(obj => obj.keyContext.objectKey === objectKey).length, 0);
done();
});
});
});


it('should upload a part even if the client sent a base 64 ETag ' +
'(and the stored ETag in metadata should be hex)', done => {
async.waterfall([
Expand Down
20 changes: 19 additions & 1 deletion tests/unit/api/objectCopyPart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const assert = require('assert');
const async = require('async');
const sinon = require('sinon');
const { parseString } = require('xml2js');
const { storage } = require('arsenal');
const { storage, errors } = require('arsenal');
const { bucketPut } = require('../../../lib/api/bucketPut');
const objectPut = require('../../../lib/api/objectPut');
const objectPutCopyPart = require('../../../lib/api/objectPutCopyPart');
Expand All @@ -14,6 +14,7 @@ const DummyRequest = require('../DummyRequest');
const { cleanup, DummyRequestLogger, makeAuthInfo, versioningTestUtils }
= require('../helpers');

const { ds } = storage.data.inMemory.datastore;
const log = new DummyRequestLogger();
const canonicalID = 'accessKey1';
const authInfo = makeAuthInfo(canonicalID);
Expand Down Expand Up @@ -137,4 +138,21 @@ describe('objectCopyPart', () => {
done();
});
});

it('should not create orphans in storage when copying a part with a failed metadata update', done => {
const testObjectCopyRequest = _createObjectCopyPartRequest(destBucketName, uploadId);
sinon.restore();
sinon.stub(metadataswitch, 'putObjectMD').callsArgWith(5, errors.InternalError);
const storedPartsBefore = ds.filter(obj => obj.keyContext.objectKey === objectKey
&& obj.keyContext.uploadId === uploadId).length;

objectPutCopyPart(authInfo, testObjectCopyRequest, sourceBucketName, objectKey, undefined, log, err => {
assert(err.is.InternalError);
// ensure the number of stored parts is the same
const storedPartsAfter = ds.filter(obj => obj.keyContext.objectKey === objectKey
&& obj.keyContext.uploadId === uploadId).length;
assert.strictEqual(storedPartsBefore, storedPartsAfter);
done();
});
});
});
26 changes: 25 additions & 1 deletion tests/unit/api/objectPut.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const assert = require('assert');
const async = require('async');
const moment = require('moment');
const { s3middleware, storage } = require('arsenal');
const { s3middleware, storage, errors } = require('arsenal');
const sinon = require('sinon');

const { bucketPut } = require('../../../lib/api/bucketPut');
Expand All @@ -18,6 +18,7 @@ const DummyRequest = require('../DummyRequest');
const { maximumAllowedUploadSize } = require('../../../constants');
const mpuUtils = require('../utils/mpuUtils');
const { lastModifiedHeader } = require('../../../constants');
const services = require('../../../lib/services');

const { ds } = storage.data.inMemory.datastore;

Expand Down Expand Up @@ -48,9 +49,12 @@ const testPutBucketRequestLock = new DummyRequest({
});

const originalputObjectMD = metadata.putObjectMD;
const originalMetadataStoreObject = services.metadataStoreObject;
const objectName = 'objectName';
const objectNameFailure = 'objectNameFailure';

let testPutObjectRequest;
let testPutObjectRequestFailure;
const enableVersioningRequest =
versioningTestUtils.createBucketPutVersioningReq(bucketName, 'Enabled');
const suspendVersioningRequest =
Expand Down Expand Up @@ -112,11 +116,19 @@ describe('objectPut API', () => {
headers: { host: `${bucketName}.s3.amazonaws.com` },
url: '/',
}, postBody);
testPutObjectRequestFailure = new DummyRequest({
bucketName,
namespace,
objectKey: objectNameFailure,
headers: { host: `${bucketName}.s3.amazonaws.com` },
url: '/',
}, postBody);
});

afterEach(() => {
sinon.restore();
metadata.putObjectMD = originalputObjectMD;
services.metadataStoreObject = originalMetadataStoreObject;
});

it('should return an error if the bucket does not exist', done => {
Expand Down Expand Up @@ -529,6 +541,18 @@ describe('objectPut API', () => {
});
});

it('should not leave orphans in data when the metadata storage step fails', done => {
sinon.stub(services, 'metadataStoreObject').callsArgWith(4, errors.InternalError);

bucketPut(authInfo, testPutBucketRequest, log, () => {
objectPut(authInfo, testPutObjectRequestFailure, undefined, log, err => {
assert(err.is.InternalError);
assert.strictEqual(ds.filter(obj => obj.keyContext.objectKey === objectNameFailure).length, 0);
done();
});
});
});

it('should not leave orphans in data when overwriting an multipart upload object', done => {
bucketPut(authInfo, testPutBucketRequest, log, () => {
mpuUtils.createMPU(namespace, bucketName, objectName, log,
Expand Down
Loading