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 2 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}
❌ 13 Tests Failed:
View the full list of 13 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
860728b to
9c78d85
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 ? ...)
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
5749a75 to
58e07b2
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| error: errors.NotImplemented, | ||
| }); | ||
| const error = | ||
| errorInstances.NotImplemented.customizeDescription(externalVersioningErrorMessage); |
There was a problem hiding this comment.
_checkBackendVersioningImplemented can now reject a GCP location (when supportsVersioning is false/undefined) at the second check. Before this PR, GCP was caught by the versioningNotImplBackends lookup and never reached the supportsVersioning path. Now it can, and this error message says "Azure" — confusing for a user operating on a GCP bucket.
Consider making the message generic when the rejection comes from the supportsVersioning check (e.g. return the backend type from _checkBackendVersioningImplemented and build the message accordingly), or use a backend-agnostic message like "Versioning is not supported for this location constraint."
Arsenal now supports versioning on the GCP backend, so drop gcp from versioningNotImplBackends and stop naming it in the error messages. Issue: CLDSRV-984
58e07b2 to
39b4b41
Compare
|
|
||
| const externalVersioningErrorMessage = 'We do not currently support putting ' + | ||
| 'a versioned object to a location-constraint of type Azure or GCP.'; | ||
| const { locationVersioningErrorMessage } = require('../../../constants'); |
There was a problem hiding this comment.
require() should be grouped with the other imports at the top of the file (lines 1–10), not placed after constant declarations.
| const { locationVersioningErrorMessage } = require('../../../constants'); | |
| const { cleanup, DummyRequestLogger, makeAuthInfo } = require('../helpers'); | |
| const metadata = require('../../../lib/metadata/wrapper'); | |
| const { locationVersioningErrorMessage } = require('../../../constants'); |
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