Allow enabling versioning on buckets with a GCP location constraint - #6266
Allow enabling versioning on buckets with a GCP location constraint#6266maeldonn wants to merge 3 commits into
Conversation
Hello maeldonn,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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6266 +/- ##
===================================================
+ Coverage 86.45% 86.50% +0.05%
===================================================
Files 212 212
Lines 14578 14577 -1
===================================================
+ Hits 12603 12610 +7
+ Misses 1975 1967 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
Arsenal now supports versioning on the GCP backend, so drop gcp from versioningNotImplBackends and stop naming it in the error messages. Issue: CLDSRV-984
860728b to
9c78d85
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
A delete marker has no body, but createAndStoreObject took its checksum from the request headers, which on a multi-object delete cover the request XML. On an external location the mismatch failed the put, and the rollback error then hit an unguarded result.versionId in multiObjectDelete, killing the worker with a 502. Skip the header checksum for delete markers, and guard the result so any later error is returned as a proper S3 error. Issue: CLDSRV-984
9c78d85 to
5749a75
Compare
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
S3 has a suspended versioning state, with nullversion ids. GCS versioning is only on or off
Suspended versioning is really just a name : in our implementation it differs (so in particular performance may not be the same), but functionally suspended is about the same as off?
i.e. is suspended not just a different name for no versioning (no versioning → versioning ←→ suspended versioning), so we could map both no & suspended versioning on GCP's disabled versioning?
Object versioning on the GCS bucket is a hard requirement: without it, the delete issued behind a delete marker destroys the data instead of archiving it
- not sure what you mean, there is no "archiving" in S3...
- if using GCP versioning is not practical, another option would be to "flatten" the versions: i.e. store each version side-by-side, as
<objKey>.<scalVersionId>
Block writes on a non-versioned bucket, instead of only reporting it in the healthcheck
If there a GCP "extension" to make the putObject/deleteObject conditional on versioning? Otherwise this may be very costly....
GCS lifecycle rules on noncurrent versions (isLive: false, daysSinceNoncurrentTime, numNewerVersions) can reap generations that S3 still serves as current
- Should be part of the documentation, and may be verified when creating location and/or enabling versioning (though none of these are foolproof, and we still risk a data loss)
- Or a reason to switch to "emulating" versions by flattening them....
GCS never promotes a noncurrent generation, so the backend state diverges from the S3 view after an undelete or a version delete
That is not really a problem for us, right: The issue is more about what the user would see in GCP's bucket?
All in all, I don't think we should merge this -and esp. in 9.4- but take the time to make a (short) design in Citadel : so we can align on the requirements and limits of the solution (i.e. do we want to keep the same "view" on GCP, or is this really just data storage, accessed only through the product ? what to do with GCP-side lifecycle rules ? ...)
| // A delete marker has no body: an x-amz-checksum-* header | ||
| // describes the request payload (e.g. a multi-object delete XML). | ||
| const headerChecksum = | ||
| isDeleteMarker || !areChecksumsEnabled() ? null : getChecksumDataFromHeaders(request.headers); |
There was a problem hiding this comment.
Is this not an issue for other data locations as well? Ie a separate bugfix for integrity check feature?
There was a problem hiding this comment.
no change here AFAICS, only removal of " or GCP" in the externalVersioningErrorMessage const?
| const missingVerIdInternalError = errorInstances.InternalError.customizeDescription( | ||
| 'Invalid state. Please ensure versioning is enabled ' + | ||
| 'in AWS for the location constraint and try again.' | ||
| 'Invalid state. Please ensure versioning is enabled ' + 'in AWS for the location constraint and try again.', |
There was a problem hiding this comment.
| 'Invalid state. Please ensure versioning is enabled ' + 'in AWS for the location constraint and try again.', | |
| 'Invalid state. Please ensure versioning is enabled in AWS for the location constraint and try again.', |
There was a problem hiding this comment.
same for a few below
There was a problem hiding this comment.
- the GPC removal should not go in 9.4, but 9.5
- I wonder if we should build the strings, instead of having the source of truth in 2 constants (one in arsenal + one in cloudserver) AND having manually duplicated that info in multiple error strings...
4 participants
Not handled yet
nullversion ids. GCS versioning is only on or offisLive: false,daysSinceNoncurrentTime,numNewerVersions) can reap generations that S3 still serves as current