Snapshot storage on S3 via SnapshotManager abstraction #3430

mogery · 2024-01-20T00:52:45Z

This PR adds a new package, snapshot_manager, which houses the SnapshotManager struct. The SnapshotManager abstracts (SnapshotFile) the filesystem-dependent parts of snapshots away from the rest of the codebase, which allows us to write two "backends" for snapshot storage: local and S3.

The local backend is 99% ported over from the existing code. Most of the trouble is with adjusting the existing code to use this new interface.

The S3 backend is built with the rust-s3 crate, which I've found to be the most reliable. It's battle-tested by having a million all-time downloads, it supports non-AWS S3-compatible file storage services, etc.

This is a biiiig PR which changes a considerable amount of code structure. Let me know if you like it or not. I'll be doing a larger sweep of all my changes to catch structure mistakes once the todo's finished.

To-do

Finish porting local backend and adjusting code for compatibility (won't build without this, don't try)
Add S3 backend
Write tests?
- Does this need explicit testing? S3-specific testing would be good, but I'm not sure where we could get a mock for that.
Update docs with config changes

Checklist

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?

mogery · 2024-01-21T23:09:03Z

alright, everything is working on local! i have confidence i can build this now. onto S3

mogery · 2024-01-22T02:30:52Z

*mad scientist laughter*

mogery · 2024-01-22T03:24:40Z

I'm opening this as ready for review, but take it more as "request for comments". Everything seems to work after messing around a bit, and all the tests run, but I haven't tested yet for any accidental breaking API or backwards compatibility issues.

Concerns/notes to self:

how does this behave on Windows? PathBuf will use \ as a delimiter, and S3 might not like that
i'm fairly sure i'm failing to parse S3's Last-Modified header when converting a HEAD request to a SnapshotDescription

generall · 2024-01-22T08:56:21Z

Hey @mogery, thanks for the PR!
It is indeed huge and would take significant time to review all together.
I would appreciate if you could split this PR into several independent ones, it would significantly speed up the review.

mogery · 2024-01-22T09:04:38Z

I would appreciate if you could split this PR into several independent ones, it would significantly speed up the review.

Hey, it's sort of hard to split, since it's an architectural change, and I can't really break it down without breaking build. I could break it down into two: one that just switches everything to use SnapshotManager, and another one which adds S3 support to it, but I'm not sure how much that would help.

mogery · 2024-01-22T09:15:37Z

Gonna take a look at the failing tests soon and fix 'em up.

… for S3 upload

mogery · 2024-01-23T21:56:24Z

sorry, sent that message right before realizing that i was running the tests in the wrong directory...

anyways, i've fixed everything up now. all tests run for me on local both with the local backend and with S3. hopefully this time it all passes on your side too

mogery · 2024-01-23T21:57:30Z

i've also verified that nothing is present in the local snapshot folder (at idle, temp files are still created now and then) when using S3

mogery · 2024-01-25T13:30:21Z

I'm very confused. All the integration tests run successfully on my local machine. Here's a log.

Exit code is reliably 0 for me.

EDIT: seems like i'm running the wrong test actually...

mogery · 2024-01-25T20:34:09Z

Another round of fixes done. Ran even more tests on local, everything is coming back green.

…shot

… exist

mogery · 2024-01-25T22:12:58Z

Heh, I didn't see that that one failed, because I couldn't see the exit code. Anyways, should be fixed. Thanks for bearing with me.

mogery · 2024-01-25T22:13:49Z

I'll also be running the whole test suite while mounted to S3 tomorrow.

mogery · 2024-02-01T09:03:32Z

hi there @generall, any updates on reviewing this?

generall · 2024-02-02T08:40:24Z

hey @mogery, review of that big pr requires about 4 hours uninterrupted timeframe. If possible, could you please split it into smaller pieces?

mogery · 2024-02-04T10:22:29Z

@generall Closing this PR and splitting it into two.

mogery · 2024-02-04T12:01:13Z

Superseded by #3520 and [TBD]

mogery added 5 commits January 20, 2024 01:29

SnapshotManager PoC pt. 1

38042f3

lint

dd78670

clippy

8a0ff3d

lint

f2ab637

Re-add Proto-Struct From trait for SnapshotDescription

b1b0e91

mogery force-pushed the mog/s3 branch from e47c903 to b1b0e91 Compare January 20, 2024 03:01

mogery added 6 commits January 21, 2024 21:17

SnapshotManager PoC pt. 2

e774977

SnapshotManager PoC pt. 3

3ea6eb5

lint

9fc98b5

clean up comments and imports

ae8a581

Revert tempfile into dev-dependencies

f02ac2a

full local implementation 🎉

a75dc70

S3 backend, pt 1.

cfa0744

mogery added 3 commits January 22, 2024 04:03

various S3-related fixes

676d743

fix missing await

74c15d7

add config hints for S3

e22cda4

mogery marked this pull request as ready for review January 22, 2024 03:24

algora-pbc bot mentioned this pull request Jan 22, 2024

Snapshot storage on S3 #3324

Closed

algora-pbc bot added the 🙋 Bounty claim label Jan 22, 2024

mogery mentioned this pull request Jan 22, 2024

Add S3 config into example qdrant/landing_page#543

Merged

mogery added 4 commits January 22, 2024 10:35

fix: create dir in do_save_uploaded_snapshot

7fe8ae6

Merge branch 'dev' into mog/s3

f2ba23e

fix: create temp snapshot dir when generating shard snapshot

07bce68

fix: enable temporary collection snapshot creation for full snapshots

997bb8c

mogery added 3 commits January 23, 2024 22:35

chore: lint

685f547

fix: parse S3 SnapshotDescription created_at with proper function

fad0d82

fix: increase test_snapshot_operations_non_wait timeout to allow time…

8f9aea9

… for S3 upload

chore: fix clippy

c39b3da

mogery added 4 commits January 25, 2024 19:35

fix(snapshots): re-add snapshot existence check

755beca

fix(s3): force rustls

ca6d202

Merge branch 'dev' into mog/s3

52389de

fix: merge weirdness

d2ea8e5

mogery added 2 commits January 25, 2024 22:06

fix(common/snapshots): allow too many arguments on recover_shard_snap…

7ce6c5f

…shot

fix(snapshot_manager): return proper error when snapshot_path doesn't…

9cbf231

… exist

Merge branch 'dev' into mog/s3

afe1f3a

mogery closed this Feb 4, 2024

mogery mentioned this pull request Feb 4, 2024

feat: add and port to SnapshotManager #3520

Closed

9 tasks

generall mentioned this pull request Apr 24, 2024

Implement S3 snapshot manager #4109

Open

Snapshot storage on S3 via SnapshotManager abstraction #3430

Snapshot storage on S3 via SnapshotManager abstraction #3430

Uh oh!

Conversation

mogery commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To-do

Checklist

All Submissions:

New Feature Submissions:

Changes to Core Features:

Uh oh!

mogery commented Jan 21, 2024

Uh oh!

mogery commented Jan 22, 2024

Uh oh!

mogery commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

generall commented Jan 22, 2024

Uh oh!

mogery commented Jan 22, 2024

Uh oh!

mogery commented Jan 22, 2024

Uh oh!

mogery commented Jan 23, 2024

Uh oh!

mogery commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mogery commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mogery commented Jan 25, 2024

Uh oh!

mogery commented Jan 25, 2024

Uh oh!

mogery commented Jan 25, 2024

Uh oh!

mogery commented Feb 1, 2024

Uh oh!

generall commented Feb 2, 2024

Uh oh!

mogery commented Feb 4, 2024

Uh oh!

mogery commented Feb 4, 2024

Uh oh!

Uh oh!

mogery commented Jan 20, 2024 •

edited

Loading

mogery commented Jan 22, 2024 •

edited

Loading

mogery commented Jan 23, 2024 •

edited

Loading

mogery commented Jan 25, 2024 •

edited

Loading