Disallow creating empty-prefix repositories on object stores - #2245
Conversation
There was a problem hiding this comment.
What happens if someone creates a repo at the root of the bucket manually (e.g. by bulk-copying objects over from another location)? Presumably that is treated the same as any other pre-existing "rooted" store, because there is no on-disk difference, and no dependency on library versions.
|
|
||
| - Expiration now records the full ancestry of pruned transaction logs in a new optional `pruned_ancestor_tx_logs` field on `SnapshotInfo`, so `diff`, `amend`, `rebase`, and `inspect` produce correct results after expiration ([#2184](https://github.com/earth-mover/icechunk/pull/2184)). | ||
| - The native S3 backend used to write every object under a leading slash (`/chunks/...`) when `prefix` was empty, which made repositories unreadable by external tools and could cause garbage collection to silently orphan objects. New empty-prefix repositories now use clean keys, and pre-existing "rooted" repositories are detected and handled automatically ([#2239](https://github.com/earth-mover/icechunk/issues/2239)). | ||
| - The native S3 backend used to write every object under a leading slash (`/chunks/...`) when `prefix` was empty, which made repositories unreadable by external tools and could cause garbage collection to silently orphan objects. Pre-existing "rooted" repositories are detected and handled automatically. Creating new repositories at an empty prefix on an object store is no longer allowed. Existing empty-prefix repositories can still be opened and updated ([#2239](https://github.com/earth-mover/icechunk/issues/2239)). |
There was a problem hiding this comment.
which made repositories unreadable by external tools and could cause garbage collection to silently orphan objects
So does this PR fix that for pre-existing "rooted" repositories?
There was a problem hiding this comment.
no. sadly doing that would require moving all the /chunks objects. Also its a bit confusing so heads up that this is a PR merging into the other PR, not main
There was a problem hiding this comment.
yeah, forgot to mention this needs to go on top of the previous PR to make sense
|
|
||
| The `legacy_rooted_keys` parameter overrides that detection: | ||
| Creating a repository at an empty `prefix` (`None` or `""`) — i.e. at the | ||
| bucket root — is not supported on object stores in modern Icechunk versions |
There was a problem hiding this comment.
| bucket root — is not supported on object stores in modern Icechunk versions | |
| bucket root — is not supported on object stores since Icechunk v2.1.0 |
|
|
||
| When opening, the `legacy_rooted_keys` parameter overrides that detection: | ||
|
|
||
| - `None` (default) — auto-detect the layout by probing storage. Use this |
There was a problem hiding this comment.
So presumably (I haven't looked at the implementation yet) in order to probe storage you make two concurrent requests for the two possible keys for the repo_info object.
There was a problem hiding this comment.
yes, we probe well-known objects in the v1 and v2 version formats, lazily on first access to the storage.
ianhi
left a comment
ianhi
left a comment
There was a problem hiding this comment.
seems good. especially for merging into the other pr
| #[error("repositories can only be created in clean prefixes")] | ||
| ParentDirectoryNotClean, | ||
| #[error( | ||
| "creating a repository at an empty prefix (the object store bucket root) is not supported in modern Icechunk versions; use a non-empty prefix. Existing empty-prefix repositories can still be opened and updated." |
There was a problem hiding this comment.
Cant make an inline suggestion here for some reason. But another spot for a specific version.
|
|
||
| - Expiration now records the full ancestry of pruned transaction logs in a new optional `pruned_ancestor_tx_logs` field on `SnapshotInfo`, so `diff`, `amend`, `rebase`, and `inspect` produce correct results after expiration ([#2184](https://github.com/earth-mover/icechunk/pull/2184)). | ||
| - The native S3 backend used to write every object under a leading slash (`/chunks/...`) when `prefix` was empty, which made repositories unreadable by external tools and could cause garbage collection to silently orphan objects. New empty-prefix repositories now use clean keys, and pre-existing "rooted" repositories are detected and handled automatically ([#2239](https://github.com/earth-mover/icechunk/issues/2239)). | ||
| - The native S3 backend used to write every object under a leading slash (`/chunks/...`) when `prefix` was empty, which made repositories unreadable by external tools and could cause garbage collection to silently orphan objects. Pre-existing "rooted" repositories are detected and handled automatically. Creating new repositories at an empty prefix on an object store is no longer allowed. Existing empty-prefix repositories can still be opened and updated ([#2239](https://github.com/earth-mover/icechunk/issues/2239)). |
There was a problem hiding this comment.
no. sadly doing that would require moving all the /chunks objects. Also its a bit confusing so heads up that this is a PR merging into the other PR, not main
|
|
||
| /// Creating a repository at an empty prefix (the bucket/container root) is refused on | ||
| /// every cloud object-store backend — native S3 and the `object_store`-based S3, | ||
| /// Azure, and GCS — but allowed on in-memory and local-filesystem storage, and via |
There was a problem hiding this comment.
Should we just be consistent everywhere?
There was a problem hiding this comment.
no, because having to pass a prefix for memory storage would be a pain without any reason, what does the prefix even mean for in memory (and a breaking change). For filesystem, i think it makes sense to support empty prefix, and it doesn't have the difficulties we have for testing in the case of object stores.
f59b418 to
5fbfb16
Compare
4 participants
No description provided.