Allow for in-place restores. #1094

mattl-netflix · 2024-05-05T17:16:55Z

6369b6a 2024-05-02 | Cease stopping Cassandra in the restore process and reveal the ability to restore to a staging area and then import or move the data depending on whether Cassandra is running.
4ece8d3 2024-05-01 | Move getBackupDirectories to BackupRestoreUtil and change its signature to cease requiring an IConfiguration but rather just take the data directory path as a String.
6b3d45a 2024-04-10 | Cease wiping the data directory in advance of restore. It is counter-intuitive and will be empty in practice in most cases anyway.
f10289f 2024-04-10 | Consolidate restore status API.
4d6d07f 2024-03-09 | Remove backup racs as that configuration is not used in practice and is not relevant to whether a restore should be attempted.
aa3dffe 2024-03-09 | Remove redundant vertical whitespace.
8c23800 2024-03-09 | Remove redundant logging.
646973d 2024-03-09 | Remove all non-javadoc comments that don't provide warnings to future maintainers.
8d18f51 2024-03-09 | Remove waitForCompletion parameter to download method which is always false in practice. Remove final modifier on static method. Make static property an instance variable instead. Use parameterized constructor call in place of two lines.
ce6c7c1 2024-03-09 | Remove unused code from our restore tooling.

jrwest · 2024-10-29T19:18:04Z

priam/src/main/java/com/netflix/priam/backup/AbstractBackupPath.java

-                String restoreFileName =
-                        PATH_JOINER.join(dataDir, keyspace, columnFamily, indexDir, fileName);
-                return_ = new File(restoreFileName);
+                return_ =


While we are in here, consider renaming this variable, its not idiomatic java

jrwest · 2024-10-29T19:28:31Z

priam/src/main/java/com/netflix/priam/backup/BackupRestoreUtil.java

+            String dataDirectory, String monitoringFolder) throws IOException {
+        ImmutableSet.Builder<Path> backupPaths = ImmutableSet.builder();
+        Path dataPath = Paths.get(dataDirectory);
+        if (Files.exists(dataPath) && Files.isDirectory(dataPath))


The File.exists call is redundant here: https://docs.oracle.com/javase/6/docs/api/java/io/File.html#isDirectory()

jrwest · 2024-10-29T19:32:30Z

priam/src/main/java/com/netflix/priam/backup/BackupRestoreUtil.java

+                            Files.newDirectoryStream(keyspaceDirPath, Files::isDirectory)) {
+                        for (Path columnfamilyDirPath : keyspaceStream) {
+                            Path backupDirPath =
+                                    Paths.get(columnfamilyDirPath.toString(), monitoringFolder);


I believe you can use columnFamilyDirPath.resolve(monitoringFolder) here instead of converting back and forth between string and path. Reference: https://docs.oracle.com/javase/8/docs/api/index.html?java/nio/file/Path.html

jrwest · 2024-10-29T19:32:43Z

priam/src/main/java/com/netflix/priam/backup/BackupRestoreUtil.java

+                        for (Path columnfamilyDirPath : keyspaceStream) {
+                            Path backupDirPath =
+                                    Paths.get(columnfamilyDirPath.toString(), monitoringFolder);
+                            if (Files.exists(backupDirPath) && Files.isDirectory(backupDirPath)) {


Same comment here that the File.exists call is redundant.

jrwest · 2024-10-29T19:33:50Z

priam/src/main/java/com/netflix/priam/backupv2/BackupVerificationTask.java

-        if (instanceState.getRestoreStatus() != null
-                && instanceState.getRestoreStatus().getStatus() != null
-                && instanceState.getRestoreStatus().getStatus() == Status.STARTED) {
+        if (instanceState.isRestoring()) {


I like this cleanup 🙌

jrwest · 2024-10-29T19:46:07Z

priam/src/main/java/com/netflix/priam/connection/CassandraOperations.java

+                failedImports.addAll(importData(keyspace, table, tableDir.toString()));
+            }
+        } else {
+            recursiveMove(Paths.get(srcDir), Paths.get(configuration.getDataFileLocation()));


Since the move is non-atomic I can see a weird edge case where we start Cassandra while the move is happening (and we have like 10,000+ files). Should we have some sort of "fence" that tells the subsystem that starts Cassandra it needs to wait while this happens?

Yes we should. I will need to address this in a follow-up, however. This is analogous to Cassandra getting started accidentally before the full download is complete the in the model we use today. Which hasn't happened yet, knock on wood. It might requires some rethinking about consolidating Cassandra's remediation policy in our environment. That is, the between Priam and systemd.

jrwest · 2024-10-29T19:48:26Z

priam/src/main/java/com/netflix/priam/connection/CassandraOperations.java

+                if (Files.isRegularFile(path)) {
+                    Files.move(path, destination.resolve(path.getFileName()));
+                } else if (Files.isDirectory(path)) {
+                    recursiveMove(path, destination.resolve(path.getFileName()));


If am reading this right, if the source path we are currently iterating over is a directory we recurse by moving the files in that directory into [destination/directory name]. Cant we just move the entire directory instead of recursing then?

My goal here was for the the operation to be strictly additive. That is, if there are already files present I want to ensure that for them to get overwritten that the operator does something explicit.

Note that to that end I have changed the policy regarding whether to replace files if they are already present in a follow-up commit.

jrwest · 2024-10-29T19:48:50Z

priam/src/main/java/com/netflix/priam/connection/CassandraOperations.java

+                } else if (Files.isDirectory(path)) {
+                    recursiveMove(path, destination.resolve(path.getFileName()));
+                } else {
+                    throw new IOException("Failed determining type of inode is " + path);


Do we need this error or just skip/log? Likely an edge case but do we really want to fail if thats the case?

In the new model using walkFileTree() we always continue.

jrwest · 2024-10-29T19:54:52Z

priam/src/main/java/com/netflix/priam/restore/AbstractRestore.java

-
-            // Cleanup local data
-            File dataDir = new File(config.getDataFileLocation());
-            if (dataDir.exists() && dataDir.isDirectory()) FileUtils.cleanDirectory(dataDir);


Question about this: I know we don't need to stop cass anymore because we use import but I was surprised to see we no longer clean the directory. Is this for the case where we want to restore a subset of the data but keep whats there? Do we still need the option to clean the directory for some restores?

The common cases for restores are:

Restores into empty clusters (e.g., red-black)

Restored into live clusters (e.g., emergencies)

In neither case do we want any data to be deleted. I would prefer that in outlier cases where that is required that there is a separate explicit job or command that the operator needs to run to best manage expectations. It is scary today that we could configure Priam such that a restart deletes all of the data.

jrwest · 2024-10-29T19:55:48Z

priam/src/main/java/com/netflix/priam/restore/AbstractRestore.java

-                        "config.doesCassandraStartManually() is set to True, hence Cassandra needs to be started manually ...");
+            List<String> failedImports = cassOps.importAll(config.getRestoreDataLocation());
+            if (!failedImports.isEmpty()) {
+                instanceState.endRestore(Status.FAILED, LocalDateTime.now());


do we want to use the list of failed imports somewhere to log what failed and be able to resume? instead of starting over from the beginning?

Also if we only need the boolean why return the list? I would rather us use the list but if we are not going to I mean.

This is a good point. In case we don't specify --copy-data and Cassandra's behavior is to remove the origin files regardless of successful import, we want a log of what failed so we can at least get a sense of the scope of what is missing. I will log the failures.

…hs when fetching incrementals. The iterators are always fully materialized. Also, remove the now-redundant method from BackupRestoreUtil that merely wrapped the MetaProxy call.

… false in practice. Remove final modifier on static method. Make static property an instance variable instead. Use parameterized constructor call in place of two lines.

… maintainers.

…is not relevant to whether a restore should be attempted.

…intuitive and will be empty in practice in most cases anyway.

…re to cease requiring an IConfiguration but rather just take the data directory path as a String.

…y to restore to a staging area and then import or move the data depending on whether Cassandra is running.

…offline restore. Remove unused code.

mattl-netflix force-pushed the feature/in_place_restore_4 branch from 6369b6a to 9557275 Compare May 5, 2024 20:11

jrwest reviewed Oct 29, 2024

View reviewed changes

mattl-netflix added 18 commits March 17, 2025 10:38

Change IMetaProxy API to return an ImmutableList of AbstractBackupPat…

aa4dcc1

…hs when fetching incrementals. The iterators are always fully materialized. Also, remove the now-redundant method from BackupRestoreUtil that merely wrapped the MetaProxy call.

Include incremental secondary index files in restore.

f9de9a7

Remove unused code from our restore tooling.

a5a6bb0

Remove waitForCompletion parameter to download method which is always…

6b19184

… false in practice. Remove final modifier on static method. Make static property an instance variable instead. Use parameterized constructor call in place of two lines.

Remove all non-javadoc comments that don't provide warnings to future…

71756d1

… maintainers.

Remove redundant logging.

50b41cb

Remove redundant vertical whitespace.

515eb2f

Remove backup racs as that configuration is not used in practice and …

948485a

…is not relevant to whether a restore should be attempted.

Consolidate restore status API.

78aca40

Cease wiping the data directory in advance of restore. It is counter-…

78dae29

…intuitive and will be empty in practice in most cases anyway.

Move getBackupDirectories to BackupRestoreUtil and change its signatu…

a7c0975

…re to cease requiring an IConfiguration but rather just take the data directory path as a String.

Cease stopping Cassandra in the restore process and reveal the abilit…

71c045b

…y to restore to a staging area and then import or move the data depending on whether Cassandra is running.

Rename file variable to be idomatic Java.

6b1027a

Remove redundant File.exists() calls.

728645b

Use Path.resolve rather than switching between Path and String.

67a1c0a

Move to canonical method of moving one directory to another in Java.

9c3be36

Log failed imports.

a1b7e8d

Change policy to not replace existing files by default when doing an …

e1ebaa5

…offline restore. Remove unused code.

mattl-netflix force-pushed the feature/in_place_restore_4 branch from 9557275 to e1ebaa5 Compare March 17, 2025 17:38

Allow for in-place restores. #1094

Are you sure you want to change the base?

Allow for in-place restores. #1094

Uh oh!

Conversation

mattl-netflix commented May 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants