Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MigratableKVStore trait #3481

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Dec 11, 2024

Closes #3387

We add a MigratableKVStore trait (suggestion for better naming are welcome) that describes methods required to write generalized KVStore-to-KVStore migration logic. In particular, it adds afn list_all_keys method that exhaustively lists all keys stored, which then can be used by a utility to transfer all of them to the new store before deleting the old state.

We implement this new trait for FilesystemStore.

TODOs (draft until then):

  • Add simple migration utility function.
  • Add test code for FilesystemStore to FilesystemStore migration.

Should be ready for review.

@tnull tnull requested a review from G8XSU December 11, 2024 15:59
@tnull tnull marked this pull request as draft December 11, 2024 15:59
@tnull tnull added this to the 0.1 milestone Dec 11, 2024
.. which will be used for generic `KVStore`-to-`KVStore` migration
logic.
@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from 98239df to b75d953 Compare December 12, 2024 12:35
@tnull tnull changed the title Add KVStoreMigrator trait Add MigratableKVStore trait Dec 12, 2024
@tnull tnull marked this pull request as ready for review December 12, 2024 14:45
@tnull
Copy link
Contributor Author

tnull commented Dec 12, 2024

Added a migration util method and a test for FilesystemStore-to-FilesystemStore migration. Should be ready for review now.

@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from 877a636 to bbcd31c Compare December 12, 2024 18:05
@tnull
Copy link
Contributor Author

tnull commented Dec 12, 2024

Addressed pending comments, let me know if I can squash fixups.

@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from bbcd31c to adcbd91 Compare December 12, 2024 18:33
@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from adcbd91 to f391296 Compare December 12, 2024 21:13
Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, mod nits.

@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from f391296 to ecdacd3 Compare December 13, 2024 08:46
@tnull
Copy link
Contributor Author

tnull commented Dec 13, 2024

Addressed pending feedback.

@TheBlueMatt
Copy link
Collaborator

Feel free to squash, IMO.

@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from ecdacd3 to 3804d4c Compare December 16, 2024 08:17
@tnull
Copy link
Contributor Author

tnull commented Dec 16, 2024

Feel free to squash, IMO.

Squashed fixups without further changes.

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 54.48718% with 71 lines in your changes missing coverage. Please review.

Project coverage is 91.28%. Comparing base (fe1cf69) to head (3804d4c).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
lightning-persister/src/fs_store.rs 41.88% 51 Missing and 17 partials ⚠️
lightning/src/util/persist.rs 66.66% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3481      +/-   ##
==========================================
+ Coverage   89.70%   91.28%   +1.58%     
==========================================
  Files         130      130              
  Lines      107422   118991   +11569     
  Branches   107422   118991   +11569     
==========================================
+ Hits        96362   108622   +12260     
+ Misses       8669     8136     -533     
+ Partials     2391     2233     -158     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

G8XSU
G8XSU previously approved these changes Dec 16, 2024
Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

We implement the new interface on `FilesystemStore`, in particular
`list_all_keys`.
.. allowing to migrate data from one store to another.
@tnull tnull force-pushed the 2024-12-add-kvstore-migration-ext branch from 3804d4c to b0af39f Compare December 16, 2024 19:08
@tnull
Copy link
Contributor Author

tnull commented Dec 16, 2024

Force-pushed with fixups:

2024-12-add-kvstore-migration-ext ~/workspace/rust-lightning> git diff-tree -U2 3804d4c3d b0af39faa
diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs
index a1d13c06b..487e1a7ac 100644
--- a/lightning-persister/src/fs_store.rs
+++ b/lightning-persister/src/fs_store.rs
@@ -348,6 +348,9 @@ fn dir_entry_is_key(p: &Path) -> Result<bool, lightning::io::Error> {

        let metadata = p.metadata().map_err(|e| {
-               let msg =
-                       format!("Failed to list keys at path {:?}: {}", p.to_str().map(PrintableString), e);
+               let msg = format!(
+                       "Failed to list keys at path {}: {}",
+                       PrintableString(p.to_str().unwrap_or_default()),
+                       e
+               );
                lightning::io::Error::new(lightning::io::ErrorKind::Other, msg)
        })?;
@@ -362,10 +365,10 @@ fn dir_entry_is_key(p: &Path) -> Result<bool, lightning::io::Error> {
                debug_assert!(
                        false,
-                       "Failed to list keys at path {:?}: file couldn't be accessed.",
-                       p.to_str().map(PrintableString)
+                       "Failed to list keys at path {}: file couldn't be accessed.",
+                       PrintableString(p.to_str().unwrap_or_default())
                );
                let msg = format!(
-                       "Failed to list keys at path {:?}: file couldn't be accessed.",
-                       p.to_str().map(PrintableString)
+                       "Failed to list keys at path {}: file couldn't be accessed.",
+                       PrintableString(p.to_str().unwrap_or_default())
                );
                return Err(lightning::io::Error::new(lightning::io::ErrorKind::Other, msg));
@@ -384,10 +387,10 @@ fn get_key_from_dir_entry(p: &Path, base_path: &Path) -> Result<String, lightnin
                                        debug_assert!(
                                                false,
-                                               "Failed to list keys of path {:?}: file path is not valid key",
-                                               p.to_str().map(PrintableString)
+                                               "Failed to list keys of path {}: file path is not valid key",
+                                               PrintableString(p.to_str().unwrap_or_default())
                                        );
                                        let msg = format!(
-                                               "Failed to list keys of path {:?}: file path is not valid key",
-                                               p.to_str().map(PrintableString)
+                                               "Failed to list keys of path {}: file path is not valid key",
+                                               PrintableString(p.to_str().unwrap_or_default())
                                        );
                                        return Err(lightning::io::Error::new(lightning::io::ErrorKind::Other, msg));
@@ -396,10 +399,10 @@ fn get_key_from_dir_entry(p: &Path, base_path: &Path) -> Result<String, lightnin
                                debug_assert!(
                                        false,
-                                       "Failed to list keys of path {:?}: file path is not valid UTF-8",
-                                       p
+                                       "Failed to list keys of path {}: file path is not valid UTF-8",
+                                       PrintableString(p.to_str().unwrap_or_default())
                                );
                                let msg = format!(
-                                       "Failed to list keys of path {:?}: file path is not valid UTF-8",
-                                       p.to_str().map(PrintableString)
+                                       "Failed to list keys of path {}: file path is not valid UTF-8",
+                                       PrintableString(p.to_str().unwrap_or_default())
                                );
                                return Err(lightning::io::Error::new(lightning::io::ErrorKind::Other, msg));
@@ -409,10 +412,13 @@ fn get_key_from_dir_entry(p: &Path, base_path: &Path) -> Result<String, lightnin
                        debug_assert!(
                                false,
-                               "Failed to list keys of path {:?}: {}",
-                               p.to_str().map(PrintableString),
+                               "Failed to list keys of path {}: {}",
+                               PrintableString(p.to_str().unwrap_or_default()),
+                               e
+                       );
+                       let msg = format!(
+                               "Failed to list keys of path {}: {}",
+                               PrintableString(p.to_str().unwrap_or_default()),
                                e
                        );
-                       let msg =
-                               format!("Failed to list keys of path {:?}: {}", p.to_str().map(PrintableString), e);
                        return Err(lightning::io::Error::new(lightning::io::ErrorKind::Other, msg));
                },
@@ -467,10 +473,10 @@ impl MigratableKVStore for FilesystemStore {
                                                debug_assert!(
                                                        false,
-                                                       "Failed to list keys of path {:?}: only two levels of namespaces are supported",
-                                                       tertiary_path.to_str()
-                                                       );
+                                                       "Failed to list keys of path {}: only two levels of namespaces are supported",
+                                                       PrintableString(tertiary_path.to_str().unwrap_or_default())
+                                               );
                                                let msg = format!(
-                                                       "Failed to list keys of path {:?}: only two levels of namespaces are supported",
-                                                       tertiary_path.to_str()
+                                                       "Failed to list keys of path {}: only two levels of namespaces are supported",
+                                                       PrintableString(tertiary_path.to_str().unwrap_or_default())
                                                );
                                                return Err(lightning::io::Error::new(

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can land this and fix up the test later, or feel free to fix it now.

@TheBlueMatt TheBlueMatt merged commit dcc531e into lightningdevkit:main Dec 17, 2024
17 of 19 checks passed
@TheBlueMatt TheBlueMatt mentioned this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KVStore: Add the capability to list namespaces
3 participants