Make backups openable as read-only DBs #8142

pdillinger · 2021-04-01T22:26:48Z

Summary: A current limitation of backups is that you don't know the
exact database state of when the backup was taken. With this new
feature, you can at least inspect the backup's DB state without
restoring it by opening it as a read-only DB.

Rather than add something like OpenAsReadOnlyDB to the BackupEngine API,
which would inhibit opening stackable DB implementations read-only
(if/when their APIs support it), we instead provide a DB name and Env
that can be used to open as a read-only DB.

Possible follow-up work:

Add a version of GetBackupInfo for a single backup.
Let CreateNewBackup return the BackupID of the newly-created backup.

Implementation details:

Refactored ChrootFileSystem to split off new base class RemapFileSystem,
which allows more general remapping of files. We use this base class to
implement BackupEngineImpl::RemapSharedFileSystem.

To minimize API impact, I decided to just add these fields name_for_open
and env_for_open to those set by GetBackupInfo when
include_file_details=true. Creating the RemapSharedFileSystem adds a bit
to the memory consumption, perhaps unnecessarily in some cases, but this
has been mitigated by (a) only initialize the RemapSharedFileSystem
lazily when GetBackupInfo with include_file_details=true is called, and
(b) using the existing shared_ptr<FileInfo> objects to hold most of the
mapping data.

To enhance API safety, RemapSharedFileSystem is wrapped by new
ReadOnlyFileSystem which rejects any attempts to write. This uncovered a
couple of places in which DB::OpenForReadOnly would write to the
filesystem, so I fixed these. Added a release note because this affects
logging.

Additional minor refactoring in backupable_db.cc to support the new
functionality.

Test Plan: new test (run with ASAN and UBSAN), added to stress test and
ran it for a while with amplified backup_one_in

Summary: A current limitation of backups is that you don't know the exact database state of when the backup was taken. With this new feature, you can at least inspect the backup's DB state without restoring it by opening it as a read-only DB. Rather than add something like OpenAsReadOnlyDB to the BackupEngine API, which would inhibit opening stackable DB implementations read-only (if/when their APIs support it), we instead provide a DB name and Env that can be used to open as a read-only DB. Possible follow-up work: * Add a version of GetBackupInfo for a single backup. * Let CreateNewBackup return the BackupID of the newly-created backup. Implementation details: Refactored ChrootFileSystem to split off new base class RemapFileSystem, which allows more general remapping of files. We use this base class to implement BackupEngineImpl::RemapSharedFileSystem. To minimize API impact, I decided to just add these fields name_for_open and env_for_open to those set by GetBackupInfo when include_file_details=true. Creating the RemapSharedFileSystem adds a bit to the memory consumption, perhaps unnecessarily in some cases, but this has been mitigated by (a) only initialize the RemapSharedFileSystem lazily when GetBackupInfo with include_file_details=true is called, and (b) using the existing shared_ptr<FileInfo> objects to hold most of the mapping data. To enhance API safety, RemapSharedFileSystem is wrapped by new ReadOnlyFileSystem which rejects any attempts to write. This uncovered a couple of places in which DB::OpenForReadOnly would write to the filesystem, so I fixed these. Added a release note because this affects logging. Additional minor refactoring in backupable_db.cc to support the new functionality. Test Plan: new test (run with ASAN and UBSAN), added to stress test and ran it for a while with amplified backup_one_in

facebook-github-bot · 2021-04-02T15:09:47Z

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mrambacher · 2021-04-02T15:45:42Z

db/db_impl/db_impl_files.cc

+      if (read_only) {
+        db_id_ = env_->GenerateUniqueId();
+        return Status::OK();


This seems wrong to me. The documentation says/implies that if I open the same DB twice, the DB ID should be the same. In this case, opening the same DB read-only will return different IDs, which seems wrong.

Also, if db_id_ is not empty, the code still call SetIdentityFile, which will attempt to write a file to the Env...

I don't know the history on why, but backups do not save the IDENTITY file. If you restore to a fresh dir, you get a new db ID, which is what this replicates.

mrambacher · 2021-04-02T15:56:42Z

env/fs_remap.h

+                           IODebugContext* dbg) override;
+};
+
+// An abstract FileSystem wrapper that only allows read-only operation.


This is not abstract is it? One could create an instance of it.

Thoughtless copy-paste error

mrambacher · 2021-04-02T15:57:25Z

env/fs_remap.h

+class ReadOnlyFileSystem : public FileSystemWrapper {
+  static inline IOStatus FailReadOnly() {
+    return IOStatus::IOError("Attempted write to ReadOnlyFileSystem");
+  }


Since this is not really part of the fs_remap, can we move it to its own header file?

mrambacher · 2021-04-02T16:03:06Z

env/fs_remap.h

+// guarantees.
+class ReadOnlyFileSystem : public FileSystemWrapper {
+  static inline IOStatus FailReadOnly() {
+    return IOStatus::IOError("Attempted write to ReadOnlyFileSystem");


Why an IOError and not NotSupported? Seems like an IOError could lead to situations where the operation might be retried when it will always fail.

I attempted to replicate the same error you would get by trying to write to a read-only filesystem. I believe this construction sets retryable to false.

mrambacher · 2021-04-02T16:06:42Z

include/rocksdb/utilities/backupable_db.h

+  // This Env might or might not be shared with other backups. To work
+  // around DBOptions::env being a raw pointer, this is a shared_ptr so
+  // that keeping either this BackupInfo, the BackupEngine, or a copy of
+  // this shared_ptr alive is sufficient to keep the Env alive for use by
+  // a read-only DB.
+  std::shared_ptr<Env> env_for_open;


This seems very dangerous to me. If env_for_open is set to Env::Default (for example), then won't the default env be deleted once this is out of scope?

Cn this be a FileSystem instead of an Env? Are there operations in the Env that you require that are not in the FS?

env_for_open is a field, so for an API user to reset it to some raw ptr they would have to use .reset() on a raw ptr they don't control the lifetime of. What's the new hazard here? Also this is an "output" struct for our API. Why would an API user be writing to these fields at all?

Cn this be a FileSystem instead of an Env?

Then the caller has unnecessary complexity of creating their own temporary Env. What's the benefit?

mrambacher · 2021-04-02T16:08:17Z

utilities/backupable/backupable_db.cc

+const std::string kMetaDirSlash = kMetaDirName + "/";
+const std::string kSharedDirSlash = kSharedDirName + "/";
+const std::string kSharedChecksumDirSlash = kSharedChecksumDirName + "/";
+


Can/Should these be static const?

They're in an anonymous namespace. I didn't think it was important to put them in the class.

I was not suggesting in a class, just static to guarantee scoping and creation constraints.

mrambacher · 2021-04-02T16:11:42Z

utilities/backupable/backupable_db.cc

+  static inline std::string WithoutTrailingSlash(const std::string& path) {
+    if (path.empty() || path.back() != '/') {
+      return path;
+    } else {
+      return path.substr(path.size() - 1);
+    }
+  }
+
+  static inline std::string WithTrailingSlash(const std::string& path) {
+    if (path.empty() || path.back() != '/') {
+      return path + '/';
+    } else {
+      return path;
+    }
+  }


Would NormalizePath meet the needs here?

It does not remove trailing /

ajkr

LGTM! Glad validation can now be done natively without full restore. Seems good to push users through the DB open functions.

ajkr · 2021-04-05T23:11:39Z

db_stress_tool/db_stress_test_base.cc

@@ -1404,7 +1405,8 @@ Status StressTest::TestBackupRestore(
      }
    }
  }
-  if (s.ok()) {
+  if (s.ok() && !inplace_not_restore) {
+    // Purge early if restoring


Maybe comment that we do early purge in restore mode so we can cover reading from a restored+purged DB? It's probably obvious to a lot of people but I had to stop and think...

ajkr · 2021-04-05T23:50:11Z

env/fs_remap.h

+class ReadOnlyFileSystem : public FileSystemWrapper {
+  static inline IOStatus FailReadOnly() {
+    return IOStatus::IOError("Attempted write to ReadOnlyFileSystem");
+  }


facebook-github-bot · 2021-04-06T19:44:07Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-04-06T19:46:32Z

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-04-06T21:38:09Z

@pdillinger merged this pull request in 879357f.

Summary: Forgot to re-test crash test after adding read-only filesystem enforcement to facebook#8142. The problem is ReadOnlyFileSystem would reject CreateDirIfMissing whenever DBOptions::create_if_missing=true. The fix that is better for users is to allow CreateDirIfMissing in ReadOnlyFileSystem if the directory exists, so that they don't cause a failure on using create_if_missing with opening backups as read-only DBs. Also fixed a couple of lints. Test Plan: local blackbox_crash_test with amplified backup_one_in

Summary: Forgot to re-test crash test after adding read-only filesystem enforcement to #8142. The problem is ReadOnlyFileSystem would reject CreateDirIfMissing whenever DBOptions::create_if_missing=true. The fix that is better for users is to allow CreateDirIfMissing in ReadOnlyFileSystem if the directory exists, so that they don't cause a failure on using create_if_missing with opening backups as read-only DBs. Added this option test to the unit test (in addition to being in the crash test). Also fixed a couple of lints. And some better messaging from 'make format' so that when you run it with uncommitted changes, it's clear that it's only checking the uncommitted changes. Pull Request resolved: #8161 Test Plan: local blackbox_crash_test with amplified backup_one_in Reviewed By: ajkr Differential Revision: D27614409 Pulled By: pdillinger fbshipit-source-id: 63ccb626c7e34c200d61c6bca2a8f60da9015179

Summary: A current limitation of backups is that you don't know the exact database state of when the backup was taken. With this new feature, you can at least inspect the backup's DB state without restoring it by opening it as a read-only DB. Rather than add something like OpenAsReadOnlyDB to the BackupEngine API, which would inhibit opening stackable DB implementations read-only (if/when their APIs support it), we instead provide a DB name and Env that can be used to open as a read-only DB. Possible follow-up work: * Add a version of GetBackupInfo for a single backup. * Let CreateNewBackup return the BackupID of the newly-created backup. Implementation details: Refactored ChrootFileSystem to split off new base class RemapFileSystem, which allows more general remapping of files. We use this base class to implement BackupEngineImpl::RemapSharedFileSystem. To minimize API impact, I decided to just add these fields `name_for_open` and `env_for_open` to those set by GetBackupInfo when include_file_details=true. Creating the RemapSharedFileSystem adds a bit to the memory consumption, perhaps unnecessarily in some cases, but this has been mitigated by (a) only initialize the RemapSharedFileSystem lazily when GetBackupInfo with include_file_details=true is called, and (b) using the existing `shared_ptr<FileInfo>` objects to hold most of the mapping data. To enhance API safety, RemapSharedFileSystem is wrapped by new ReadOnlyFileSystem which rejects any attempts to write. This uncovered a couple of places in which DB::OpenForReadOnly would write to the filesystem, so I fixed these. Added a release note because this affects logging. Additional minor refactoring in backupable_db.cc to support the new functionality. Pull Request resolved: facebook/rocksdb#8142 Test Plan: new test (run with ASAN and UBSAN), added to stress test and ran it for a while with amplified backup_one_in Reviewed By: ajkr Differential Revision: D27535408 Pulled By: pdillinger fbshipit-source-id: 04666d310aa0261ef6b2385c43ca793ce1dfd148 Signed-off-by: Changlong Chen <levisonchen@live.cn>

Summary: Forgot to re-test crash test after adding read-only filesystem enforcement to facebook/rocksdb#8142. The problem is ReadOnlyFileSystem would reject CreateDirIfMissing whenever DBOptions::create_if_missing=true. The fix that is better for users is to allow CreateDirIfMissing in ReadOnlyFileSystem if the directory exists, so that they don't cause a failure on using create_if_missing with opening backups as read-only DBs. Added this option test to the unit test (in addition to being in the crash test). Also fixed a couple of lints. And some better messaging from 'make format' so that when you run it with uncommitted changes, it's clear that it's only checking the uncommitted changes. Pull Request resolved: facebook/rocksdb#8161 Test Plan: local blackbox_crash_test with amplified backup_one_in Reviewed By: ajkr Differential Revision: D27614409 Pulled By: pdillinger fbshipit-source-id: 63ccb626c7e34c200d61c6bca2a8f60da9015179 Signed-off-by: Changlong Chen <levisonchen@live.cn>

Summary: A current limitation of backups is that you don't know the exact database state of when the backup was taken. With this new feature, you can at least inspect the backup's DB state without restoring it by opening it as a read-only DB. Rather than add something like OpenAsReadOnlyDB to the BackupEngine API, which would inhibit opening stackable DB implementations read-only (if/when their APIs support it), we instead provide a DB name and Env that can be used to open as a read-only DB. Possible follow-up work: * Add a version of GetBackupInfo for a single backup. * Let CreateNewBackup return the BackupID of the newly-created backup. Implementation details: Refactored ChrootFileSystem to split off new base class RemapFileSystem, which allows more general remapping of files. We use this base class to implement BackupEngineImpl::RemapSharedFileSystem. To minimize API impact, I decided to just add these fields `name_for_open` and `env_for_open` to those set by GetBackupInfo when include_file_details=true. Creating the RemapSharedFileSystem adds a bit to the memory consumption, perhaps unnecessarily in some cases, but this has been mitigated by (a) only initialize the RemapSharedFileSystem lazily when GetBackupInfo with include_file_details=true is called, and (b) using the existing `shared_ptr<FileInfo>` objects to hold most of the mapping data. To enhance API safety, RemapSharedFileSystem is wrapped by new ReadOnlyFileSystem which rejects any attempts to write. This uncovered a couple of places in which DB::OpenForReadOnly would write to the filesystem, so I fixed these. Added a release note because this affects logging. Additional minor refactoring in backupable_db.cc to support the new functionality. Pull Request resolved: facebook/rocksdb#8142 Test Plan: new test (run with ASAN and UBSAN), added to stress test and ran it for a while with amplified backup_one_in Reviewed By: ajkr Differential Revision: D27535408 Pulled By: pdillinger fbshipit-source-id: 04666d310aa0261ef6b2385c43ca793ce1dfd148 Signed-off-by: Changlong Chen <levisonchen@live.cn>

Summary: Forgot to re-test crash test after adding read-only filesystem enforcement to facebook/rocksdb#8142. The problem is ReadOnlyFileSystem would reject CreateDirIfMissing whenever DBOptions::create_if_missing=true. The fix that is better for users is to allow CreateDirIfMissing in ReadOnlyFileSystem if the directory exists, so that they don't cause a failure on using create_if_missing with opening backups as read-only DBs. Added this option test to the unit test (in addition to being in the crash test). Also fixed a couple of lints. And some better messaging from 'make format' so that when you run it with uncommitted changes, it's clear that it's only checking the uncommitted changes. Pull Request resolved: facebook/rocksdb#8161 Test Plan: local blackbox_crash_test with amplified backup_one_in Reviewed By: ajkr Differential Revision: D27614409 Pulled By: pdillinger fbshipit-source-id: 63ccb626c7e34c200d61c6bca2a8f60da9015179 Signed-off-by: Changlong Chen <levisonchen@live.cn>

pdillinger requested a review from ajkr April 1, 2021 22:26

facebook-github-bot added the CLA Signed label Apr 1, 2021

pdillinger added 2 commits April 1, 2021 16:06

oops

5f6221e

Fix ASC

7555667

mrambacher reviewed Apr 2, 2021

View reviewed changes

ajkr approved these changes Apr 6, 2021

View reviewed changes

ajkr mentioned this pull request Apr 6, 2021

Why LOG.old.* is continuously increased for Read Only Mode. #7966

Closed

pdillinger added 2 commits April 6, 2021 11:31

Merge remote-tracking branch 'origin/master' into open_backup_as_db

adb1511

Improvements from feedback

3324c92

facebook-github-bot closed this in 879357f Apr 6, 2021

facebook-github-bot added the Merged label Apr 6, 2021

pdillinger mentioned this pull request Apr 7, 2021

Fix crash test with backup as read-only DB #8161

Closed

ajkr mentioned this pull request Apr 13, 2021

Add internal compaction API for Secondary instance #8171

Closed

jay-zhuang mentioned this pull request Apr 15, 2021

Secondary Instance should pass read_only=true #8193

Closed

pdillinger mentioned this pull request Apr 19, 2021

Is there any way to determine the exact last write/sequence number in the rocksdb's backup #7712

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make backups openable as read-only DBs #8142

Make backups openable as read-only DBs #8142

pdillinger commented Apr 1, 2021 •

edited

Loading

facebook-github-bot commented Apr 2, 2021

mrambacher Apr 2, 2021

pdillinger Apr 2, 2021

mrambacher Apr 2, 2021

pdillinger Apr 2, 2021

mrambacher Apr 2, 2021

ajkr Apr 5, 2021

mrambacher Apr 2, 2021

pdillinger Apr 2, 2021

mrambacher Apr 2, 2021

pdillinger Apr 2, 2021

mrambacher Apr 2, 2021

pdillinger Apr 2, 2021

mrambacher Apr 2, 2021

mrambacher Apr 2, 2021

pdillinger Apr 2, 2021

ajkr left a comment •

edited

Loading

ajkr Apr 5, 2021

ajkr Apr 5, 2021

facebook-github-bot commented Apr 6, 2021

facebook-github-bot commented Apr 6, 2021

facebook-github-bot commented Apr 6, 2021

Make backups openable as read-only DBs #8142

Make backups openable as read-only DBs #8142

Conversation

pdillinger commented Apr 1, 2021 • edited Loading

facebook-github-bot commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 6, 2021

facebook-github-bot commented Apr 6, 2021

facebook-github-bot commented Apr 6, 2021

pdillinger commented Apr 1, 2021 •

edited

Loading

ajkr left a comment •

edited

Loading