Skip to content

CLDSRV-632 ✨ put metadata edge cases #5913

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

Open
wants to merge 15 commits into
base: development/9.1
Choose a base branch
from
Open

CLDSRV-632 ✨ put metadata edge cases #5913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d6fe9b0
✨ add the versioningPreprocessing call to copy object when needed
DarkIsDude Aug 25, 2025
b9b4a83
✅ re-enable test that were disable because of the issue
DarkIsDude Aug 25, 2025
0745a11
✅ update test logic to use putObjectMD onCall 1 only and manage local
DarkIsDude Aug 25, 2025
6eef57f
♻️ move code to async await to management concurrent delete
DarkIsDude Aug 25, 2025
d60fac1
✨ skip logic if versionId is defined (update a specific version)
DarkIsDude Aug 25, 2025
d6480ea
✨ retrieve the master object to copy it when needed only
DarkIsDude Aug 26, 2025
6d6fcb5
⬆️ bump arsenal version
DarkIsDude Sep 2, 2025
f391235
💚 fix unit tests
DarkIsDude Sep 2, 2025
52b06fd
✅ make sure master object to parse it
DarkIsDude Sep 2, 2025
2effafe
✨ add nullVersionId to master and current version object
DarkIsDude Sep 3, 2025
51a4101
🐛 fix case 2 by adding data to the new object
DarkIsDude Sep 3, 2025
68abc28
📌 pin arsenal to specific branch
DarkIsDude Sep 8, 2025
1c32c0b
♻️ don't fetch bucket data all the time
DarkIsDude Sep 23, 2025
65452c5
🚨 use 4 spaces indentation
DarkIsDude Oct 6, 2025
ad98e47
💚 try catch getObject as the function return an error if the object d…
DarkIsDude Oct 6, 2025
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
6 changes: 6 additions & 0 deletions lib/api/apiUtils/object/createAndStoreObject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
metadataStoreParams.contentMD5 = constants.emptyFileMd5;
return next(null, null, null);
}

// Handle mdOnlyHeader as a metadata only operation. If
// the object in question is actually 0 byte or has a body size
// then handle normally.
Expand All @@ -244,6 +245,7 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
return next(null, dataGetInfo, _md5);
}
}

return dataStore(objectKeyContext, cipherBundle, request, size,
streamingV4Params, backendInfo, log, next);
},
Expand Down Expand Up @@ -280,10 +282,12 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
const options = overwritingVersioning(objMD, metadataStoreParams);
return process.nextTick(() => next(null, options, infoArr));
}

if (!bucketMD.isVersioningEnabled() && objMD?.archive?.archiveInfo) {
// Ensure we trigger a "delete" event in the oplog for the previously archived object
metadataStoreParams.needOplogUpdate = 's3:ReplaceArchivedObject';
}

return versioningPreprocessing(bucketName, bucketMD,
metadataStoreParams.objectKey, objMD, log, (err, options) => {
if (err) {
Expand Down Expand Up @@ -316,9 +320,11 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
metadataStoreParams.versioning = options.versioning;
metadataStoreParams.isNull = options.isNull;
metadataStoreParams.deleteNullKey = options.deleteNullKey;

if (options.extraMD) {
Object.assign(metadataStoreParams, options.extraMD);
}

return _storeInMDandDeleteData(bucketName, infoArr,
cipherBundle, metadataStoreParams,
options.dataToDelete, log, requestMethod, next);
Expand Down
13 changes: 11 additions & 2 deletions lib/api/apiUtils/object/versioning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ function _storeNullVersionMD(bucketName, objKey, nullVersionId, objMD, log, cb)
log.debug('error from metadata storing null version as new version',
{ error: err });
}

cb(err);
});
}
Expand Down Expand Up @@ -252,6 +253,7 @@ function processVersioningState(mst, vstat, nullVersionCompatMode) {
}
return { options, nullVersionId };
}

if (mst.isNull && !mst.isNull2) {
// if master null version was put with an older
// Cloudserver (or in compat mode), there is a
Expand All @@ -265,6 +267,7 @@ function processVersioningState(mst, vstat, nullVersionCompatMode) {
}
return { options, nullVersionId };
}

// backward-compat: keep a reference to the existing null
// versioned key
if (mst.nullVersionId) {
Expand Down Expand Up @@ -295,6 +298,7 @@ function getMasterState(objMD) {
if (!objMD) {
return {};
}

const mst = {
exists: true,
versionId: objMD.versionId,
Expand All @@ -304,10 +308,12 @@ function getMasterState(objMD) {
nullVersionId: objMD.nullVersionId,
nullUploadId: objMD.nullUploadId,
};

if (objMD.location) {
mst.objLocation = Array.isArray(objMD.location) ?
objMD.location : [objMD.location];
}

return mst;
}
/** versioningPreprocessing - return versioning information for S3 to handle
Expand All @@ -329,19 +335,22 @@ function versioningPreprocessing(bucketName, bucketMD, objectKey, objMD,
log, callback) {
const mst = getMasterState(objMD);
const vCfg = bucketMD.getVersioningConfiguration();
// bucket is not versioning configured

if (!vCfg) {
const options = { dataToDelete: mst.objLocation };
return process.nextTick(callback, null, options);
}
// bucket is versioning configured

const { options, nullVersionId, delOptions } =
processVersioningState(mst, vCfg.Status, config.nullVersionCompatMode);

return async.series([
function storeNullVersionMD(next) {
if (!nullVersionId) {
return process.nextTick(next);
}

options.nullVersionId = nullVersionId;
return _storeNullVersionMD(bucketName, objectKey, nullVersionId, objMD, log, next);
},
function prepareNullVersionDeletion(next) {
Expand Down
55 changes: 49 additions & 6 deletions lib/routes/routeBackbeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ const { listLifecycleOrphanDeleteMarkers } = require('../api/backbeat/listLifecy
const { objectDeleteInternal } = require('../api/objectDelete');
const quotaUtils = require('../api/apiUtils/quotas/quotaUtils');
const { handleAuthorizationResults } = require('../api/api');
const { versioningPreprocessing }
= require('../api/apiUtils/object/versioning');
const {promisify} = require('util');

const versioningPreprocessingPromised = promisify(versioningPreprocessing);
metadata.getObjectMDPromised = promisify(metadata.getObjectMD);
metadata.getBucketAndObjectMDPromised = promisify(metadata.getBucketAndObjectMD);

const { CURRENT_TYPE, NON_CURRENT_TYPE, ORPHAN_DM_TYPE } = constants.lifecycleListing;

Expand Down Expand Up @@ -508,23 +515,22 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
if (err) {
return callback(err);
}

let omVal;

try {
omVal = JSON.parse(payload);
} catch {
// FIXME: add error type MalformedJSON
return callback(errors.MalformedPOSTRequest);
}

const { headers, bucketName, objectKey } = request;
// check if it's metadata only operation

if (headers['x-scal-replication-content'] === 'METADATA') {
if (!objMd) {
// if the target does not exist, return an error to
// backbeat, who will have to retry the operation as a
// complete replication
return callback(errors.ObjNotFound);
}
// use original data locations and encryption info

[
'location',
'x-amz-server-side-encryption',
Expand Down Expand Up @@ -611,6 +617,12 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
// To prevent this, the versionId field is only included in options when it is defined.
if (versionId !== undefined) {
options.versionId = versionId;
omVal.versionId = versionId;
Copy link
Contributor

Choose a reason for hiding this comment

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

the semantics of putObject in metadata layer (either backends, i.e. metadata or mongodbClientInterface) are not clear or precisely defined : does setting this not interract (in some weird or unacceptable way?) the behavior of the versionId option?


if (isNull) {
omVal.isNull = isNull;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there maybe null2 field to handle as well ?
(somehow required for metadata backend, with the latest listing)

}

// In the MongoDB metadata backend, setting the versionId option leads to the creation
// or update of the version object, the master object is only updated if its versionId
// is the same as the version. This can lead to inconsistencies when replicating objects
Expand Down Expand Up @@ -641,6 +653,7 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
if (!request.query?.accountId) {
return next();
}

return getCanonicalIdsByAccountId(request.query.accountId, log, (err, res) => {
if (err) {
return next(err);
Expand All @@ -650,6 +663,36 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
return next();
});
},
async () => {
if (versioning && !objMd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

when does this !objMd case happen? Before this function is called, we already call standardMetadataValidateBucketAndObj (line 1655), which should return the object already...
(and looking at versioningPreprocessing, it seems that i knows how to handle objMd=nil as well as the case where object is/isn't the master?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@francoisferrand when you do a PUT on PutMD and the object does not exists. This case can happens when it's an empty object

Copy link
Contributor

Choose a reason for hiding this comment

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

still not clear why we need the conditional, and not (more) systematically call versioningPreprocessing.
in particular, this function is responsible to handle version-suspended case (i.e. delete previous version). Or is this case objMD == nil a way to detect the insertion case (i.e. create a new version) instead of the udpate case (update -internal- metadata of an existing version, e.g. for replication status or transition)?

may help to have a comment explaining what use-case this "branch" handles.

This case can happens when it's an empty object

do you mean "empty" as in "no data"? I don't understand how it would/should affect the behavior here :-/

let masterMD;

try {
masterMD = await metadata.getObjectMDPromised(bucketName, objectKey, {}, log);
} catch (err) {
if (err.is?.NoSuchKey) {
log.debug('no master found for versioned object', {
method: 'putMetadata',
bucketName,
objectKey,
});
} else {
throw err;
}
}

if (!masterMD) {
return;
}

const versioningPreprocessingResult =
await versioningPreprocessingPromised(bucketName, bucketInfo, objectKey, masterMD, log);

if (versioningPreprocessingResult?.nullVersionId) {
omVal.nullVersionId = versioningPreprocessingResult.nullVersionId;
}
Comment on lines +691 to +693
Copy link
Contributor

Choose a reason for hiding this comment

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

in other uses of versioningPreprocessing (in putObject / copyObject / putMpu) we set multiple (and different) fields:

			metadataStoreParams.versionId = options.versionId;
            metadataStoreParams.versioning = options.versioning;
            metadataStoreParams.isNull = options.isNull;
            metadataStoreParams.deleteNullKey = options.deleteNullKey;
            if (options.extraMD) {
                Object.assign(metadataStoreParams, options.extraMD);
            }

this gets translated (in metadataStoreObject) to this:

        if (versioning) {
            options.versioning = versioning;
        }
        if (versionId || versionId === '') {
            options.versionId = versionId;
        }

        if (deleteNullKey) {
            options.deleteNullKey = deleteNullKey;
        }

        const { isNull, nullVersionId, nullUploadId, isDeleteMarker } = params;
        md.setIsNull(isNull)
            .setNullVersionId(nullVersionId)
            .setNullUploadId(nullUploadId)
            .setIsDeleteMarker(isDeleteMarker);
        if (versionId && versionId !== 'null') {
            md.setVersionId(versionId);
        }
        if (isNull && !config.nullVersionCompatMode) {
            md.setIsNull2(true);
        }

→ so it seems many more fields may be set, to cover all cases?

}
},
next => {
log.trace('putting object version', {
objectKey: request.objectKey, omVal, options });
Expand Down
Loading
Loading