Enforce microVersionId conflicts at the metadata write level - #6235
Conversation
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.4 #6235 +/- ##
===================================================
+ Coverage 86.38% 86.39% +0.01%
===================================================
Files 212 212
Lines 14567 14574 +7
===================================================
+ Hits 12583 12591 +8
+ Misses 1984 1983 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
maeldonn
changed the title
b85feb8 to
ff93206
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/arsenal#8.5.6", | ||
| "arsenal": "git+https://github.com/scality/arsenal#fa9abec316dc0a48b9511eec5b41f7b9a626d5f4", |
There was a problem hiding this comment.
reminder to point to the released arsenal tag containing ARSN-619 before merging (/after_pull_request on scality/Arsenal#2678 if needed).
|
|
||
| // atomic counterpart of the pre-checks above ($gt cannot match a missing microVersionId) | ||
| if (incomingMicroVersionId !== null && objMd?.microVersionId && metadata.supportsConditionalPutObjectMD?.()) { | ||
| options.conditions = { microVersionId: { $gt: incomingMicroVersionId } }; |
There was a problem hiding this comment.
could the condition also cover the missing-field case, e.g. $or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: … } }]? otherwise this window stays unprotected , if translateConditions cannot express it, worth mentionning in the comment.
There was a problem hiding this comment.
Yeah, known limitation, I will rediscuss with Maël next week, but yeah we would need to update arsenal with some more logic to deal with $exist, $or which may be more ocmplex
9b5d259 to
c43d014
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
af5972a to
3ca7d7e
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/arsenal#8.5.12", | ||
| "arsenal": "git+https://github.com/scality/arsenal#6675ce3895cb779b0d07756a373762c82ff5e335", |
22062b7 to
bfcf42b
Compare
delthas
left a comment
delthas
left a comment
There was a problem hiding this comment.
nit: Can you squash your 2 last commits into the main Mael commit? So that we get proper atomic commits in history.
18e0975 to
b2ae192
Compare
b2ae192 to
886ca0b
Compare
|
will rerun prettier later |
|
@francoisferrand its approved but you can retake a look if you want, most of the logic is happening in arsenal anyways |
| @@ -791,25 +801,21 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) { | |||
| omVal.replicationInfo.isNFS = !omVal.replicationInfo.isReplica; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: could we not have kept options here?
less changes, more locality... and either way we need to "share" a variable from the main function to the continuation callback, so moving does not really help...
const options = {
overheadField: constants.overheadField,
};
// Atomic counterpart of the JS pre-checks above, performed at database level.
if (incomingMicroVersionId !== null) {
options.conditions = {
$or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: incomingMicroVersionId } }],
};
}
There was a problem hiding this comment.
its a bit weird to have re add an isolated
if (incomingMicroVersionId !== null)
later in the code when we already have it at the top of the function 🤔
There was a problem hiding this comment.
to me feels to me much weirder (and harder to read) to spread the construction of the options...
incomingMicroVersionId is kind of like a parameter, so no problem using it throughout the function
The JS pre-checks in putMetadata are not atomic: two concurrent writes can both pass them and let a stale revision overwrite a newer one. Pass a conditional put to the metadata backend so the write itself rejects stale microVersionIds with PreconditionFailed, mapped to the same CRR conflict response as the pre-checks. Issue: CLDSRV-952
886ca0b to
6d3a2d4
Compare
Issue: CLDSRV-952
6d3a2d4 to
7a82ba5
Compare
|
/bypass_author_approval |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
This pull request does not target the following hotfix branch(es) so they
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: bypass_author_approval |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-952. Goodbye maeldonn. |
6 participants
The JS pre-checks in putMetadata are not atomic: two concurrent CRR writes can
both pass them and let a stale revision overwrite a newer one. This attaches a
conditional put ($gt on microVersionId) so the write itself rejects stale
revisions with PreconditionFailed, mapped to the same conflict response as the
pre-checks. Enabled only on backends supporting conditional puts (mongodb).
Issue: CLDSRV-952