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 21 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
21 commits
Select commit Hold shift + click to select a range
e59e6ac
✨ add the versioningPreprocessing call to copy object when needed
DarkIsDude Aug 25, 2025
f6c75df
✅ re-enable test that were disable because of the issue
DarkIsDude Aug 25, 2025
fffb8a5
✅ update test logic to use putObjectMD onCall 1 only and manage local
DarkIsDude Aug 25, 2025
b4f9b60
♻️ move code to async await to management concurrent delete
DarkIsDude Aug 25, 2025
40a2ad1
✨ skip logic if versionId is defined (update a specific version)
DarkIsDude Aug 25, 2025
c3e9a00
✨ retrieve the master object to copy it when needed only
DarkIsDude Aug 26, 2025
214cb41
⬆️ bump arsenal version
DarkIsDude Sep 2, 2025
277c22c
💚 fix unit tests
DarkIsDude Sep 2, 2025
ceb98aa
✅ make sure master object to parse it
DarkIsDude Sep 2, 2025
d6accef
✨ add nullVersionId to master and current version object
DarkIsDude Sep 3, 2025
618f29d
🐛 fix case 2 by adding data to the new object
DarkIsDude Sep 3, 2025
f9e0f16
📌 pin arsenal to specific branch
DarkIsDude Sep 8, 2025
f2524f9
♻️ don't fetch bucket data all the time
DarkIsDude Sep 23, 2025
6d8ef87
🚨 use 4 spaces indentation
DarkIsDude Oct 6, 2025
304215f
💚 try catch getObject as the function return an error if the object d…
DarkIsDude Oct 6, 2025
fb1e2b7
✨ manage isNull2 fields of versioningPreprocessing
DarkIsDude Oct 8, 2025
8e47a3c
✨ manage more fields from versioningPreprocessing
DarkIsDude Oct 9, 2025
504cde3
✅ add a new test for the versioningPreprocess
DarkIsDude Oct 10, 2025
affeae3
🧪 update the test to don't fail if the seconds / minutes / ... is dif…
DarkIsDude Oct 10, 2025
e384352
🚨 remove unused parameter and unused import
DarkIsDude Oct 10, 2025
3d7d86a
✏️ fix some typo on comment and indentation
DarkIsDude Oct 17, 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
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably be automated soon with some prettier solutions
I started this but it was on hold as we wanted to finish the unification first #5877

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
68 changes: 62 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
Comment on lines -522 to -527
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments do not seem useless at all, they should be kept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one for location and encryption don't add any information ? Reading the code is enough to understand that we reuse the objMd ?

For the other one, the comment don't help to understand the code neither, he just describe what happens later. IMO explaining what happens in other component just add noise. If we do that everywhere, we'll have comment everywhere. A comment should be here to help to understand the code, some weird behaviour or exception ?


[
'location',
'x-amz-server-side-encryption',
Expand Down Expand Up @@ -611,6 +617,16 @@ 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I checked, this seems not be the case. But maybe I'm missing something here 😬. Maybe the second review will have more insight

Copy link
Contributor

Choose a reason for hiding this comment

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

did you perform such second review? (you can resolve this comment once you confirmed)


if (isNull) {
if (!nullVersionCompatMode) {
omVal.isNull2 = true;
}

omVal.isNull = isNull;
}

// 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 +657,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 +667,45 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
return next();
});
},
async () => {
// If we create a new version of an object (so objMd is null),
// we should make sure that the masterVersion is versioned.
// If an object already exists, we just want to update the metadata
// of the existing object and not create a new one
if (versioning && !objMd) {
let masterMD;

try {
masterMD = await metadata.getObjectMDPromised(bucketName, objectKey, {}, log);
Copy link
Contributor

Choose a reason for hiding this comment

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

this introduces an extra metadata IO in case of insert (first one was in standardMetadataValidateBucketAndObj, which returned -or not- the version document) : this extra I/O can be skipped if we know the expected semantics of the putMetadata call (i.e. is it an insertion → can directly lookup the "master" in standardMetadataValidateBucketAndObj ; or an update → must lookup the version) -OR- update standardMetadataValidateBucketAndObj (and everything below) to be able to return both the version and/or master...

➡ maybe acceptable to keep the extra I/O for now, but we should create a ticket for tracking this, or make a note in https://scality.atlassian.net/browse/ARTESCA-8449

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note, if we want to get both the master and the version, then this is just moving the problem, as we will still need two queries. Today we try to get the master version only for objects like delete markers, but if we provide a versionID and it's not there, then we just return NoSuchVersion. Unless we kind of batch or use operators like $in, this will probably lead to more I/Os in any cases. So, important that if we go with this approach, we do not affect all API calls outside of backbeat routes

The best would be to know in advance

} catch (err) {
if (err.is?.NoSuchKey) {
log.debug('no master found for versioned object', {
method: 'putMetadata',
bucketName,
objectKey,
});
} else {
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to throw, the current stack is not using promises yet, so we tend to use more errors and calling the callbacks with it. I fear that a throw here will make the process crash, as we do not try/catch our code?

Suggested change
throw err;
return next(err);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can throw safely. We are in an async process. Async is able to switch from 'async/await' to 'callback'. Async will transform this throw in a callback one 🙏

import async from "async";

function main() {
  async.series(
    [
      function (callback) {
        setTimeout(function () {
          console.log("Task 1");
          callback(null, "one");
        }, 200);
      },
      async function () {
        await new Promise((resolve) => {
          setTimeout(function () {
            console.log("Task 1.5");
            resolve();
          }, 150);
        });

        throw new Error("Error in Task 1.5");
      },
    ],
    function (err, results) {
      console.error(err);
      console.log(results);
    },
  );
}

main();

But I'm open minded as you mention that the current stack don't use promises yet. I would like to start to change that and move to a more modern stack by starting async/await syntax, in parallel of the SDK migration that introduce a lot async/await. Let me know

}
}

if (!masterMD) {
return;
}

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

if (versioningPreprocessingResult?.nullVersionId) {
omVal.nullVersionId = versioningPreprocessingResult.nullVersionId;
options.deleteNullKey = versioningPreprocessingResult.deleteNullKey;

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