-
Notifications
You must be signed in to change notification settings - Fork 249
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.
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
base: development/9.1
Are you sure you want to change the base?
CLDSRV-632 ✨ put metadata edge cases #5913
Conversation
Hello darkisdude,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
4e1523f
to
300aaef
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
❌ 33 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 32 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
5589b9c
to
59ee1fa
Compare
}); | ||
}, | ||
async () => { | ||
if (versioning && !objMd) { |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :-/
lib/routes/routeBackbeat.js
Outdated
async () => { | ||
if (versioning && !objMd) { | ||
const masterObjectAndBucket = | ||
await metadata.getBucketAndObjectMDPromised(bucketName, objectKey, {}, log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is 2 extra I/Os: to get the bucket then get the object.
We already retrieved that in standardMetadataValidateBucketAndObj
, so
- we definitely already have the bucket -> no point retrieving it again
- if we try to get the same object, we will get the same result i guess; if not did we actually need the first one? (i.e. could we fix that call instead of making another one here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I try to get the master / null version, so not the one that we want (objMD is undefined so we didn't find it). For the bucket I'll remove this extra call 🙏
lib/routes/routeBackbeat.js
Outdated
const masterObjectAndBucket = | ||
await metadata.getBucketAndObjectMDPromised(bucketName, objectKey, {}, log); | ||
const masterObject = JSON.parse(masterObjectAndBucket.obj); | ||
await versioningPreprocessingPromised(bucketName, bucketInfo, objectKey, masterObject, log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putMetadata can be used in 2 ways:
- in CRR, it is most of the time used to "create" (or replace) object: it similar to a regular putObject, but dealing only with metadata (CRR replicates data and metadata separately)
--> there it may need to just replace the document (non-versionned case), create a new one (no previous object or versionned case), or create a new one + delete previous one (version-suspended case) - in backbeat, it is used to "update" the metadata of an existing object: and it should never create a new document/revision
- (in CRR, there may also be "real" metadata updates on the source, which get replicated the way: so the call from CRR may also be used to udpate the MD of an existing object, as backbeat does..... what may help though it that CRR can only ever happen in version-enabled buckets)
Looking at the code in putObject, we mostly handle the first case, though we have a corner case "putObjectVersion", which is more related to the second case (it is used to restore cold objects, and thus will write the data + simply update the metadata of existing document).
In that case, we actually must not (and do not 😁) call versioningPreprocessingPromised
: so it seems we should have something similar here, and not call versioningPreprocessingPromised
in the "update metadata" case...
Maybe that is the purpose of the !objMd
condition, but it is a bit hard to see...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For larger context : I believe that we will eventually need to be able to differentiate "updates" from "puts", as there are some subbtle difference in handle race conditions/corner cases.
In particular, we may need to do this distinction to fix https://scality.atlassian.net/browse/ARTESCA-8449 (backbeat updates metadata by doing getMetadata...putMetadata, we multiple such sequences may happen in parallel). To fix this (which rarely happen), the idea is to introduce some some sort to counter
and perform update with a condition on that counter: but it must be done only when the semantic is that of updating existing MD, not when the update is actually the replacement of a version.
Not sure it is really needed for this specific fix, but best to know this in case your change may go in similar direction.
.then(() => object))) | ||
) | ||
); | ||
const listedObjects = await this.s3.listObjectVersions(param).promise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how many objects are (typically) talking about in these tests?
could this affect the duration of the tests?
(or can we fix the backend so we can continue to delete in parallel?)
0b5a099
to
9c143cb
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
b5c2131
to
da6bfb6
Compare
14f2308
to
e0ba601
Compare
timezone difference Issue: CLDSRV-632
Issue: CLDSRV-632
e0ba601
to
1b518c1
Compare
1b518c1
to
dfc7d0d
Compare
6acde44
to
2547220
Compare
2547220
to
f54f702
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somehow I am not sure this covers all cases - and not confident everything is fully tests: there are tests, but logic is quite intricate with the different versioning cases + different listings/... and all the flags: isNull, isNull2, ...
→ would be worth reviewing the tests to see if we miss some cases (e.g. all the combinations of versioning disabled/enabled/suspended, creating vs updating an object, with previous version/nullVersion/no previous version...). May help to look at putObject tests for inspiration on the test case?
→ since we have multiple backend, and the separation is sometimes not clear (or not clearly documented), we should make sure this test is run both with mongo and metadata backends: is it the case?
omVal.versionId = versionId; | ||
|
||
if (isNull) { | ||
omVal.isNull = isNull; |
There was a problem hiding this comment.
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)
// To prevent this, the versionId field is only included in options when it is defined. | ||
if (versionId !== undefined) { | ||
options.versionId = versionId; | ||
omVal.versionId = versionId; |
There was a problem hiding this comment.
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?
}); | ||
}, | ||
async () => { | ||
if (versioning && !objMd) { |
There was a problem hiding this comment.
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 :-/
if (versioningPreprocessingResult?.nullVersionId) { | ||
omVal.nullVersionId = versioningPreprocessingResult.nullVersionId; | ||
} |
There was a problem hiding this comment.
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?
Description
Reactivate some tests that were disable because some edge case were not covered.
Motivation and context
Manage 2 use case under the put metadata route:
Related issues
https://scality.atlassian.net/browse/CLDSRV-632
scality/Arsenal#2490