Snapshot store #3643

AlexandruCihodaru · 2024-02-19T12:14:48Z

This PR is a first step in an attempt to fix #3324.
To be able to add S3 support and other storage back ends later I defined a trait that will contain all needed methods to handle created snapshots. I took a simplifying assumption that the snapshots will first be created in a temp local place and after the snapshotting is completed we will move the snapshot and checksum.
After S3 support is added, it should be straight forward to implement support for additional storage back ends, the only needed action being implementing the trait for the new type.

To keep the PR small and easy to review, currently it only handles create_snapshot,delete,list (collection/full/shard). If the direction is good others could be implemented in subsequent PRs without breaking compatibility.

All Submissions:

Contributions should target the dev branch. Did you create your branch from dev?
Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

Does your submission pass tests?
Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

AlexandruCihodaru · 2024-02-19T12:16:25Z

lib/storage/src/types.rs

@@ -53,7 +53,7 @@ pub struct StorageConfig {
    pub storage_path: String,
    #[serde(default = "default_snapshots_path")]
    #[validate(length(min = 1))]
-    pub snapshots_path: String,
+    pub snapshots_path: Option<String>,


This change is needed because in the future there could be either snapshot_path for local storage either some details for the storage back end. It would have been cleaner with an enum in here but I believe it would break backwards compatibility.

@generall an enum would mean to also have a custom deserialization implementation and everything, overall I felt that it was cleaner how I implemented it here, but for reference this is how it would look like AlexandruCihodaru@887d426

lib/collection/src/collection/mod.rs

AlexandruCihodaru · 2024-02-19T12:21:21Z

lib/collection/src/collection/mod.rs

            snapshots_path: snapshots_path.to_owned(),
+            snapshot_storage_manager: Self::create_snapshot_storage_manager(snapshots_path),
            channel_service,


I think that in the future snapshots_path could be remove so we will avoid info duplication, the path can be retrieved from the snapshot_storage_manager.

AlexandruCihodaru · 2024-02-19T13:24:22Z

lib/collection/src/collection/mod.rs

@@ -50,6 +51,7 @@ pub struct Collection {
    this_peer_id: PeerId,
    path: PathBuf,
    snapshots_path: PathBuf,
+    snapshot_storage_manager: Box<dyn SnapshotStorage>,


This could be static dispatch by making Collection generic over SnapshotStorage. We could switch to it if snapshotting happens often because it is cheaper.

lib/collection/src/collection/snapshots.rs

lib/collection/src/shards/transfer/snapshot.rs

lib/storage/src/content_manager/snapshots/mod.rs

AlexandruCihodaru · 2024-02-25T22:41:06Z

Hey @generall, I have addressed your comments. In a subsequent PR we should also move the get_snapshot_description and get_snapshot/checksum_path into the trait because we will have different implementations for the LocalFS and S3.

lib/collection/src/collection/mod.rs

* Define a trait with methods for managing snapshots after their creation. The goal of this trait is to allow implementation of snapshot management logic for different storage backends without major changes in the code. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com>

* Move the snapshot deletion logic into the implementation of SnapshotStorage for LocalFileSystemConfig. * Replace snapshot deletion logic with calls to the SnapshotStorage implementation. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com>

* Move the snapshot listing logic into the implementation of SnapshotStorage for LocalFileSystemConfig. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com>

* Move the snapshot creation logic into the implementation of SnapshotStorage for LocalFileSystemConfig. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com>

AlexandruCihodaru · 2024-02-28T16:03:29Z

Hey @generall I was wondering if the desired outcome is to use S3 like a "persistent" storage or to use it for any snapshot related actions. If we targeting the first option than I think it is possible to bring locally from S3 the snapshot dir before initializing the TableOfContent in here

qdrant/lib/storage/src/content_manager/toc/mod.rs

Line 94 in d0baa5e

    
           let snapshots_path = Path::new(&storage_config.snapshots_path.clone()).to_owned();

and I believe that there will not be a lot of additional changes after this PR.

…removed

generall · 2024-02-28T18:59:01Z

/tip $200

algora-pbc · 2024-02-28T18:59:04Z

👉 @generall: Navigate to your dashboard to proceed

algora-pbc · 2024-02-28T18:59:30Z

@AlexandruCihodaru: You just got a $200 tip! 👉 Complete your Algora onboarding to collect your payment.

algora-pbc · 2024-02-28T19:02:50Z

🎉🎈 @AlexandruCihodaru has been awarded $200! 🎈🎊

* [snapshots]: Define trait for snapshots managemet * Define a trait with methods for managing snapshots after their creation. The goal of this trait is to allow implementation of snapshot management logic for different storage backends without major changes in the code. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com> * [snapshots]: Move snapshot deletion logic * Move the snapshot deletion logic into the implementation of SnapshotStorage for LocalFileSystemConfig. * Replace snapshot deletion logic with calls to the SnapshotStorage implementation. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com> * [snapshots]: Move snapshot list logic * Move the snapshot listing logic into the implementation of SnapshotStorage for LocalFileSystemConfig. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com> * [snapshots]: Move snapshot creation logic * Move the snapshot creation logic into the implementation of SnapshotStorage for LocalFileSystemConfig. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com> * review fixes * fmt * refactor full storage snapshot * create directory on get_stored_file + make sure temp archive file is removed --------- Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com> Co-authored-by: generall <andrey@vasnetsov.com>

AlexandruCihodaru commented Feb 19, 2024

View reviewed changes

lib/collection/src/collection/mod.rs Outdated Show resolved Hide resolved

AlexandruCihodaru commented Feb 19, 2024

View reviewed changes

generall self-requested a review February 23, 2024 11:07

generall reviewed Feb 23, 2024

View reviewed changes

lib/collection/src/collection/snapshots.rs Outdated Show resolved Hide resolved

generall reviewed Feb 23, 2024

View reviewed changes

lib/collection/src/shards/transfer/snapshot.rs Outdated Show resolved Hide resolved

AlexandruCihodaru force-pushed the snapshot_store branch from b662a4f to 965a818 Compare February 25, 2024 22:37

AlexandruCihodaru commented Feb 25, 2024

View reviewed changes

lib/storage/src/content_manager/snapshots/mod.rs Outdated Show resolved Hide resolved

AlexandruCihodaru requested a review from generall February 27, 2024 08:15

generall reviewed Feb 27, 2024

View reviewed changes

lib/collection/src/collection/mod.rs Outdated Show resolved Hide resolved

AlexandruCihodaru added 4 commits February 27, 2024 19:31

[snapshots]: Move snapshot list logic

88db371

* Move the snapshot listing logic into the implementation of SnapshotStorage for LocalFileSystemConfig. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com>

[snapshots]: Move snapshot creation logic

48fc10c

* Move the snapshot creation logic into the implementation of SnapshotStorage for LocalFileSystemConfig. Signed-off-by: Alexandru Cihodaru <alexandru.cihodaru@gmail.com>

AlexandruCihodaru force-pushed the snapshot_store branch from 965a818 to 48fc10c Compare February 27, 2024 17:31

AlexandruCihodaru requested a review from generall February 27, 2024 17:32

generall added 4 commits February 28, 2024 18:11

review fixes

9810772

fmt

3c1dda9

refactor full storage snapshot

d588c3e

create directory on get_stored_file + make sure temp archive file is …

86f6d46

…removed

generall approved these changes Feb 28, 2024

View reviewed changes

algora-pbc bot added the 💰 Rewarded label Feb 28, 2024

generall merged commit 61fa4e7 into qdrant:dev Feb 28, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot store #3643

Snapshot store #3643

AlexandruCihodaru commented Feb 19, 2024 •

edited

AlexandruCihodaru Feb 19, 2024

generall Feb 23, 2024

AlexandruCihodaru Feb 26, 2024

AlexandruCihodaru Feb 19, 2024

AlexandruCihodaru Feb 19, 2024

AlexandruCihodaru commented Feb 25, 2024

AlexandruCihodaru commented Feb 28, 2024

generall commented Feb 28, 2024

algora-pbc bot commented Feb 28, 2024

algora-pbc bot commented Feb 28, 2024

algora-pbc bot commented Feb 28, 2024

Snapshot store #3643

Snapshot store #3643

Conversation

AlexandruCihodaru commented Feb 19, 2024 • edited

All Submissions:

New Feature Submissions:

Changes to Core Features:

AlexandruCihodaru Feb 19, 2024

Choose a reason for hiding this comment

generall Feb 23, 2024

Choose a reason for hiding this comment

AlexandruCihodaru Feb 26, 2024

Choose a reason for hiding this comment

AlexandruCihodaru Feb 19, 2024

Choose a reason for hiding this comment

AlexandruCihodaru Feb 19, 2024

Choose a reason for hiding this comment

AlexandruCihodaru commented Feb 25, 2024

AlexandruCihodaru commented Feb 28, 2024

generall commented Feb 28, 2024

algora-pbc bot commented Feb 28, 2024

algora-pbc bot commented Feb 28, 2024

algora-pbc bot commented Feb 28, 2024

AlexandruCihodaru commented Feb 19, 2024 •

edited