Protect existing files in `FaultInjectionTest{Env,FS}::ReopenWritableFile()` #8995

ajkr · 2021-10-08T02:29:45Z

FaultInjectionTest{Env,FS}::ReopenWritableFile() functions were accidentally deleting WALs from previous db_stress runs causing verification to fail. They were operating under the assumption that ReopenWritableFile() would delete any existing file. It was a reasonable assumption considering the {Env,FileSystem}::ReopenWritableFile() documentation stated that would happen. The only problem was neither the implementations we offer nor the "real" clients in RocksDB code followed that contract. So, this PR updates the contract as well as fixing the fault injection client usage.

The fault injection change exposed that ExternalSSTFileBasicTest.SyncFailure was relying on a fault injection Env dropping unsynced data written by a regular Env. I changed that test to make its SstFileWriter use fault injection Env, and also implemented LinkFile() in fault injection so the unsynced data is tracked under the new name.

Test Plan:

Verified it fixes the following failure:

$ ./db_stress --clear_column_family_one_in=0 --column_families=1 --db=/dev/shm/rocksdb_crashtest_whitebox --delpercent=5 --expected_values_dir=/dev/shm/rocksdb_crashtest_expected --iterpercent=0 --key_len_percent_dist=1,30,69 --max_key=100000 --max_key_len=3 --nooverwritepercent=1 --ops_per_thread=1000 --prefixpercent=0 --readpercent=60 --reopen=0 --target_file_size_base=1048576 --test_batches_snapshots=0 --write_buffer_size=1048576 --writepercent=35 --value_size_mult=33 -threads=1
...
$ ./db_stress --avoid_flush_during_recovery=1 --clear_column_family_one_in=0 --column_families=1 --db=/dev/shm/rocksdb_crashtest_whitebox --delpercent=5 --destroy_db_initially=0 --expected_values_dir=/dev/shm/rocksdb_crashtest_expected --iterpercent=10 --key_len_percent_dist=1,30,69 --max_bytes_for_level_base=4194304 --max_key=100000 --max_key_len=3 --nooverwritepercent=1 --open_files=-1 --open_metadata_write_fault_one_in=8 --open_write_fault_one_in=16 --ops_per_thread=1000 --prefix_size=-1 --prefixpercent=0 --readpercent=50 --sync=1 --target_file_size_base=1048576 --test_batches_snapshots=0 --write_buffer_size=1048576 --writepercent=35 --value_size_mult=33 -threads=1
...
Verification failed for column family 0 key 000000000000001300000000000000857878787878 (1143): Value not found: NotFound:
Crash-recovery verification failed :(
...

make check -j48

facebook-github-bot · 2021-10-08T02:36:08Z

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

facebook-github-bot · 2021-10-10T21:38:16Z

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

facebook-github-bot · 2021-10-10T22:00:45Z

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

ltamasi

Thanks for all the fixes @ajkr ! LGTM, I just have a couple of questions

ltamasi · 2021-10-11T18:18:14Z

tools/db_crashtest.py

+    # Setting `nooverwritepercent > 0` is only possible because we do not vary
+    # the random seed, so the same keys are chosen by every run for disallowing
+    # overwrites.


I think it would be nice to mention this in the db_stress help as well

Yes, not noticing this took me on a long detour.

ltamasi · 2021-10-11T18:32:44Z

utilities/fault_injection_env.cc

+      } else if (!exists) {
+        // It was created by this `Env` just now.
+        should_track = true;
+        open_files_.insert(fname);


I'm wondering if we would want to add the file to open_files_ even if should_track is false? (The same question applies to FaultInjectionTestFS.)

I don't know, when we don't use TestWritableFile wrapper, open_files_ won't be maintained:

rocksdb/utilities/fault_injection_env.cc

Line 401 in 2e09a54

open_files_.erase(state.filename_);

From the perspective of this Env it doesn't need to know an unmanaged file is open, I guess? Maybe we should rename it open_managed_files_.

ltamasi · 2021-10-11T18:45:05Z

utilities/fault_injection_fs.cc

+  // We preserve contents of overwritten files up to a size threshold.
+  // We could keep previous file in another name, but we need to worry about
+  // garbage collect the those files. We do it if it is needed later.
+  // We ignore I/O errors here for simplicity.
+  std::string previous_contents = kNewFileNoOverwrite;
+  if (target()->FileExists(t, IOOptions(), nullptr).ok()) {
+    uint64_t file_size;
+    if (target()->GetFileSize(t, IOOptions(), &file_size, nullptr).ok() &&
+        file_size < 1024) {
+      ReadFileToString(target(), t, &previous_contents).PermitUncheckedError();
+    }
+  }


Just curious, do we force-create the link when there is already a file with the new name? (If my understanding is correct, that's the scenario when we would potentially need the previous contents.)

Good point, seems our LinkFile() doesn't actually overwrite. I will just use kNewFileNoOverwrite instead. We could also carry over the value of dir_to_new_files_since_last_sync_[sdn.first][sdn.second] but I find that hard to think about.

ajkr

Thanks for the review!

ajkr · 2021-10-11T19:00:57Z

utilities/fault_injection_fs.cc

+  // We preserve contents of overwritten files up to a size threshold.
+  // We could keep previous file in another name, but we need to worry about
+  // garbage collect the those files. We do it if it is needed later.
+  // We ignore I/O errors here for simplicity.
+  std::string previous_contents = kNewFileNoOverwrite;
+  if (target()->FileExists(t, IOOptions(), nullptr).ok()) {
+    uint64_t file_size;
+    if (target()->GetFileSize(t, IOOptions(), &file_size, nullptr).ok() &&
+        file_size < 1024) {
+      ReadFileToString(target(), t, &previous_contents).PermitUncheckedError();
+    }
+  }


Good point, seems our LinkFile() doesn't actually overwrite. I will just use kNewFileNoOverwrite instead. We could also carry over the value of dir_to_new_files_since_last_sync_[sdn.first][sdn.second] but I find that hard to think about.

ajkr · 2021-10-11T19:03:16Z

tools/db_crashtest.py

+    # Setting `nooverwritepercent > 0` is only possible because we do not vary
+    # the random seed, so the same keys are chosen by every run for disallowing
+    # overwrites.


Yes, not noticing this took me on a long detour.

ajkr · 2021-10-11T19:10:23Z

utilities/fault_injection_env.cc

+      } else if (!exists) {
+        // It was created by this `Env` just now.
+        should_track = true;
+        open_files_.insert(fname);


I don't know, when we don't use TestWritableFile wrapper, open_files_ won't be maintained:

rocksdb/utilities/fault_injection_env.cc

Line 401 in 2e09a54

open_files_.erase(state.filename_);

From the perspective of this Env it doesn't need to know an unmanaged file is open, I guess? Maybe we should rename it open_managed_files_.

facebook-github-bot · 2021-10-11T19:46:53Z

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

facebook-github-bot · 2021-10-11T19:48:00Z

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

…File()` (facebook#8995) Summary: `FaultInjectionTest{Env,FS}::ReopenWritableFile()` functions were accidentally deleting WALs from previous `db_stress` runs causing verification to fail. They were operating under the assumption that `ReopenWritableFile()` would delete any existing file. It was a reasonable assumption considering the `{Env,FileSystem}::ReopenWritableFile()` documentation stated that would happen. The only problem was neither the implementations we offer nor the "real" clients in RocksDB code followed that contract. So, this PR updates the contract as well as fixing the fault injection client usage. The fault injection change exposed that `ExternalSSTFileBasicTest.SyncFailure` was relying on a fault injection `Env` dropping unsynced data written by a regular `Env`. I changed that test to make its `SstFileWriter` use fault injection `Env`, and also implemented `LinkFile()` in fault injection so the unsynced data is tracked under the new name. Pull Request resolved: facebook#8995 Test Plan: - Verified it fixes the following failure: ``` $ ./db_stress --clear_column_family_one_in=0 --column_families=1 --db=/dev/shm/rocksdb_crashtest_whitebox --delpercent=5 --expected_values_dir=/dev/shm/rocksdb_crashtest_expected --iterpercent=0 --key_len_percent_dist=1,30,69 --max_key=100000 --max_key_len=3 --nooverwritepercent=1 --ops_per_thread=1000 --prefixpercent=0 --readpercent=60 --reopen=0 --target_file_size_base=1048576 --test_batches_snapshots=0 --write_buffer_size=1048576 --writepercent=35 --value_size_mult=33 -threads=1 ... $ ./db_stress --avoid_flush_during_recovery=1 --clear_column_family_one_in=0 --column_families=1 --db=/dev/shm/rocksdb_crashtest_whitebox --delpercent=5 --destroy_db_initially=0 --expected_values_dir=/dev/shm/rocksdb_crashtest_expected --iterpercent=10 --key_len_percent_dist=1,30,69 --max_bytes_for_level_base=4194304 --max_key=100000 --max_key_len=3 --nooverwritepercent=1 --open_files=-1 --open_metadata_write_fault_one_in=8 --open_write_fault_one_in=16 --ops_per_thread=1000 --prefix_size=-1 --prefixpercent=0 --readpercent=50 --sync=1 --target_file_size_base=1048576 --test_batches_snapshots=0 --write_buffer_size=1048576 --writepercent=35 --value_size_mult=33 -threads=1 ... Verification failed for column family 0 key 000000000000001300000000000000857878787878 (1143): Value not found: NotFound: Crash-recovery verification failed :( ... ``` - `make check -j48` Reviewed By: ltamasi Differential Revision: D31495388 Pulled By: ajkr fbshipit-source-id: 7886ccb6a07cb8b78ad7b6c1c341ccf40bb68385

…File()` (#8995) Summary: `FaultInjectionTest{Env,FS}::ReopenWritableFile()` functions were accidentally deleting WALs from previous `db_stress` runs causing verification to fail. They were operating under the assumption that `ReopenWritableFile()` would delete any existing file. It was a reasonable assumption considering the `{Env,FileSystem}::ReopenWritableFile()` documentation stated that would happen. The only problem was neither the implementations we offer nor the "real" clients in RocksDB code followed that contract. So, this PR updates the contract as well as fixing the fault injection client usage. The fault injection change exposed that `ExternalSSTFileBasicTest.SyncFailure` was relying on a fault injection `Env` dropping unsynced data written by a regular `Env`. I changed that test to make its `SstFileWriter` use fault injection `Env`, and also implemented `LinkFile()` in fault injection so the unsynced data is tracked under the new name. Pull Request resolved: facebook/rocksdb#8995 Test Plan: - Verified it fixes the following failure: ``` $ ./db_stress --clear_column_family_one_in=0 --column_families=1 --db=/dev/shm/rocksdb_crashtest_whitebox --delpercent=5 --expected_values_dir=/dev/shm/rocksdb_crashtest_expected --iterpercent=0 --key_len_percent_dist=1,30,69 --max_key=100000 --max_key_len=3 --nooverwritepercent=1 --ops_per_thread=1000 --prefixpercent=0 --readpercent=60 --reopen=0 --target_file_size_base=1048576 --test_batches_snapshots=0 --write_buffer_size=1048576 --writepercent=35 --value_size_mult=33 -threads=1 ... $ ./db_stress --avoid_flush_during_recovery=1 --clear_column_family_one_in=0 --column_families=1 --db=/dev/shm/rocksdb_crashtest_whitebox --delpercent=5 --destroy_db_initially=0 --expected_values_dir=/dev/shm/rocksdb_crashtest_expected --iterpercent=10 --key_len_percent_dist=1,30,69 --max_bytes_for_level_base=4194304 --max_key=100000 --max_key_len=3 --nooverwritepercent=1 --open_files=-1 --open_metadata_write_fault_one_in=8 --open_write_fault_one_in=16 --ops_per_thread=1000 --prefix_size=-1 --prefixpercent=0 --readpercent=50 --sync=1 --target_file_size_base=1048576 --test_batches_snapshots=0 --write_buffer_size=1048576 --writepercent=35 --value_size_mult=33 -threads=1 ... Verification failed for column family 0 key 000000000000001300000000000000857878787878 (1143): Value not found: NotFound: Crash-recovery verification failed :( ... ``` - `make check -j48` Reviewed By: ltamasi Differential Revision: D31495388 Pulled By: ajkr fbshipit-source-id: 7886ccb6a07cb8b78ad7b6c1c341ccf40bb68385

ajkr added 6 commits October 7, 2021 18:44

update doc

8b21ced

update HISTORY.md

a1f44a9

nooverwritepercent comment

16fcbda

protect existing files from FaultInjectionTestFS::ReopenWritableFile

d86e560

protect existing files from FaultInjectionTestEnv::ReopenWritableFile

dd7b8e3

make format

18cc8d5

ajkr added 3 commits October 10, 2021 11:40

fix ExternalSSTFileBasicTest.SyncFailure; update FaultInjectionTestEnv

45da39c

update FaultInjectionTestFS

300c57e

make format

970d891

ajkr changed the title ~~Protect existing WALs in FaultInjectionTest{Env,FS}::ReopenWritableFile()~~ Protect existing files in FaultInjectionTest{Env,FS}::ReopenWritableFile() Oct 10, 2021

ajkr requested a review from ltamasi October 11, 2021 17:44

ltamasi approved these changes Oct 11, 2021

View reviewed changes

ajkr commented Oct 11, 2021

View reviewed changes

ajkr added 3 commits October 11, 2021 12:33

change saved original target contents in FaultInjectionTestFS::LinkFile

f447882

rename open_files_ -> open_managed_files_

4694238

update db_stress help

4708579

facebook-github-bot closed this in a282eff Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect existing files in `FaultInjectionTest{Env,FS}::ReopenWritableFile()` #8995

Protect existing files in `FaultInjectionTest{Env,FS}::ReopenWritableFile()` #8995

ajkr commented Oct 8, 2021 •

edited

Loading

facebook-github-bot commented Oct 8, 2021

facebook-github-bot commented Oct 10, 2021

facebook-github-bot commented Oct 10, 2021

ltamasi left a comment

ltamasi Oct 11, 2021

ajkr Oct 11, 2021

ltamasi Oct 11, 2021

ajkr Oct 11, 2021

ltamasi Oct 11, 2021

ajkr Oct 11, 2021

ajkr left a comment

ajkr Oct 11, 2021

ajkr Oct 11, 2021

ajkr Oct 11, 2021

facebook-github-bot commented Oct 11, 2021

facebook-github-bot commented Oct 11, 2021

Protect existing files in FaultInjectionTest{Env,FS}::ReopenWritableFile() #8995

Protect existing files in FaultInjectionTest{Env,FS}::ReopenWritableFile() #8995

Conversation

ajkr commented Oct 8, 2021 • edited Loading

facebook-github-bot commented Oct 8, 2021

facebook-github-bot commented Oct 10, 2021

facebook-github-bot commented Oct 10, 2021

ltamasi left a 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

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

facebook-github-bot commented Oct 11, 2021

facebook-github-bot commented Oct 11, 2021

Protect existing files in `FaultInjectionTest{Env,FS}::ReopenWritableFile()` #8995

Protect existing files in `FaultInjectionTest{Env,FS}::ReopenWritableFile()` #8995

ajkr commented Oct 8, 2021 •

edited

Loading