A better approach to clearing DBs for crash test (#14254)

Summary:
Clearing DB dir for crash test is currently a hodgepodge of
1. Caller of db_crashtest.py maybe tries to clear the dir
2. db_crashtest.py tries to clear the dir in get_dbname() (but ignoring failure)
3. db_crashtest.py passes --destroy_db_initially to some db_stress calls as needed
4. db_crashtest.py tries to clear the dir between some db_stress calls
5. db_crashtest.py tries to clear the dir after everything is done and successful (no artifacts to investigate or save) (but ignoring failure)
6. Try to add more uniqueness to the directory from #14249

This change reverts or replaces 2, 4, 5, and 6 by doubling-down on (expanding) 3 and a small variant of it:

* crash_test.mk passes --destroy_db_initially=1 so that the first run of db_stress clears the db dir.
* After each db_stress invocation, db_crashtest.py resets destroy_db_initially=0 so that the next invocation reuses the same DB, except in cases where there is an incompatibility that requires a fresh DB (from cases 3 and 4 above).
* On success, uses new `db_stress --destroy_db_and_exit` option to clean up the DB dir without needing a custom cleanup_cmd (now ignored)

Note that although case 1 is likely obsolete, it is out of control of an open source PR.

Pull Request resolved: #14254

Test Plan: some manual runs

Reviewed By: xingbowang

Differential Revision: D91164731

Pulled By: pdillinger

fbshipit-source-id: 0a66c8c0e130c9eeacc55af411a18a09bc9debdf

Author: pdillinger

crash_test.mk

@@ -8,7 +8,7 @@ DB_STRESS_CMD?=./db_stress
 include common.mk
 
 CRASHTEST_MAKE=$(MAKE) -f crash_test.mk
-CRASHTEST_PY=$(PYTHON) -u tools/db_crashtest.py --stress_cmd=$(DB_STRESS_CMD) --cleanup_cmd='$(DB_CLEANUP_CMD)'
+CRASHTEST_PY=$(PYTHON) -u tools/db_crashtest.py --stress_cmd=$(DB_STRESS_CMD) --cleanup_cmd='$(DB_CLEANUP_CMD)' --destroy_db_initially=1
 
 .PHONY: crash_test crash_test_with_atomic_flush crash_test_with_txn \
 	crash_test_with_wc_txn crash_test_with_wp_txn crash_test_with_wup_txn \

db_stress_tool/db_stress_common.cc

@@ -877,5 +877,24 @@ Status DestroyUnverifiedSubdir(const std::string& dirname) {
   return s;
 }
 
+Status DbStressDestroyDb(const std::string& db_path) {
+  Status s;
+  Options options;
+  // NOTE: using db_stress_listener_env in order to see obsolete MANIFEST files
+  options.env = db_stress_listener_env;
+  // Remove DB files in a principled way to avoid issues
+  if (FLAGS_use_blob_db) {
+    s = blob_db::DestroyBlobDB(db_path, options, blob_db::BlobDBOptions());
+  } else {
+    s = DestroyDB(db_path, options);
+  }
+  if (!s.ok()) {
+    return s;
+  }
+  // Remove everything else recursively, only reporting success if able to
+  // delete everything
+  return DestroyDir(db_stress_listener_env, db_path);
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 #endif  // GFLAGS

db_stress_tool/db_stress_common.h

@@ -100,6 +100,7 @@ DECLARE_bool(enable_pipelined_write);
 DECLARE_bool(verify_before_write);
 DECLARE_bool(histogram);
 DECLARE_bool(destroy_db_initially);
+DECLARE_bool(destroy_db_and_exit);
 DECLARE_bool(verbose);
 DECLARE_bool(progress_reports);
 DECLARE_uint64(db_write_buffer_size);
@@ -820,5 +821,10 @@ Status SaveFilesInDirectory(const std::string& src_dirname,
                             const std::string& dst_dirname);
 Status DestroyUnverifiedSubdir(const std::string& dirname);
 Status InitUnverifiedSubdir(const std::string& dirname);
+
+// Destroy the DB at the given path under the env configured for db_stress.
+// Handles both regular DB and BlobDB, and cleans and removes the entire dir.
+Status DbStressDestroyDb(const std::string& db_path);
+
 }  // namespace ROCKSDB_NAMESPACE
 #endif  // GFLAGS

db_stress_tool/db_stress_gflags.cc

@@ -135,6 +135,10 @@ DEFINE_bool(histogram, false, "Print histogram of operation timings");
 DEFINE_bool(destroy_db_initially, true,
             "Destroys the database dir before start if this is true");
 
+DEFINE_bool(destroy_db_and_exit, false,
+            "Destroys the database dir and exits. Useful for cleanup without "
+            "running stress test. Other options are mostly ignored.");
+
 DEFINE_bool(verbose, false, "Verbose");
 
 DEFINE_bool(progress_reports, true,

db_stress_tool/db_stress_test_base.cc

@@ -77,22 +77,7 @@ StressTest::StressTest()
       secondary_db_(nullptr),
       is_db_stopped_(false) {
   if (FLAGS_destroy_db_initially) {
-    std::vector<std::string> files;
-    db_stress_env->GetChildren(FLAGS_db, &files);
-    for (unsigned int i = 0; i < files.size(); i++) {
-      if (Slice(files[i]).starts_with("heap-")) {
-        db_stress_env->DeleteFile(FLAGS_db + "/" + files[i]);
-      }
-    }
-
-    Options options;
-    options.env = db_stress_env;
-    // Remove files without preserving manfiest files
-    const Status s = !FLAGS_use_blob_db
-                         ? DestroyDB(FLAGS_db, options)
-                         : blob_db::DestroyBlobDB(FLAGS_db, options,
-                                                  blob_db::BlobDBOptions());
-
+    const Status s = DbStressDestroyDb(FLAGS_db);
     if (!s.ok()) {
       fprintf(stderr, "Cannot destroy original db: %s\n", s.ToString().c_str());
       exit(1);

db_stress_tool/db_stress_tool.cc

@@ -98,6 +98,19 @@ int db_stress_tool(int argc, char** argv) {
       raw_env, std::make_shared<DbStressFSWrapper>(raw_env->GetFileSystem()));
   db_stress_env = env_wrapper_guard.get();
 
+  // Handle --destroy_db_and_exit early, before other option validation
+  if (FLAGS_destroy_db_and_exit) {
+    s = DbStressDestroyDb(FLAGS_db);
+    if (s.ok()) {
+      fprintf(stdout, "Successfully destroyed db at %s\n", FLAGS_db.c_str());
+      return 0;
+    } else {
+      fprintf(stderr, "Failed to destroy db at %s: %s\n", FLAGS_db.c_str(),
+              s.ToString().c_str());
+      return 1;
+    }
+  }
+
   FLAGS_rep_factory = StringToRepFactory(FLAGS_memtablerep.c_str());
 
   // The number of background threads should be at least as much the

tools/db_crashtest.py

@@ -60,10 +60,10 @@ def early_argument_parsing_before_main():
     global per_iteration_random_seed_override
     per_iteration_random_seed_override = args.per_iteration_random_seed_override
     global is_remote_db
-    # Set is_remote_db if remain_args has a non-empty --env_uri= argument
+    # Set is_remote_db if remain_args has a non-empty --env_uri= or --fs_uri= argument
     for arg in remain_args:
         parts = arg.split("=", 1)
-        if parts[0] == "--env_uri" and len(parts) > 1 and parts[1]:
+        if parts[0] in ["--env_uri", "--fs_uri"] and len(parts) > 1 and parts[1]:
             is_remote_db = True
             break
 
@@ -454,32 +454,20 @@ def apply_random_seed_per_iteration():
 _DEBUG_LEVEL_ENV_VAR = "DEBUG_LEVEL"
 
 stress_cmd = "./db_stress"
-cleanup_cmd = None
 
 
 def is_release_mode():
     return os.environ.get(_DEBUG_LEVEL_ENV_VAR) == "0"
 
 
-# Generate a unique run ID for this script execution. This ensures each run
-# gets a unique database directory when TEST_TMPDIR is set, avoiding issues
-# with parameter changes (like use_put_entity_one_in) between runs.
-run_id = str(random.randint(0, 2**63))
-
-
 def get_dbname(test_name):
     test_dir_name = "rocksdb_crashtest_" + test_name
     test_tmpdir = os.environ.get(_TEST_DIR_ENV_VAR)
     if test_tmpdir is None or test_tmpdir == "":
         dbname = tempfile.mkdtemp(prefix=test_dir_name)
     else:
-        dbname = test_tmpdir + "/" + test_dir_name + "_" + run_id
+        dbname = test_tmpdir + "/" + test_dir_name
         if not is_remote_db:
-            shutil.rmtree(dbname, True)
-            if cleanup_cmd is not None:
-                print("Running DB cleanup command - %s\n" % cleanup_cmd)
-                # Ignore failure
-                os.system(cleanup_cmd)
             os.makedirs(dbname, exist_ok=True)
     return dbname
 
@@ -1387,13 +1375,18 @@ def print_output_and_exit_on_error(stdout, stderr, print_stderr_separately=False
 
 
 def cleanup_after_success(dbname):
-    if not is_remote_db:
-        shutil.rmtree(dbname, True)
-    if cleanup_cmd is not None:
-        print("Running DB cleanup command - %s\n" % cleanup_cmd)
-        ret = os.system(cleanup_cmd)
-        if ret != 0:
-            print("WARNING: DB cleanup returned error %d\n" % ret)
+    # Use db_stress --destroy_db_and_exit, which simplifies remote DB cleanup
+    cleanup_cmd_parts = [stress_cmd, "--destroy_db_and_exit=1", "--db=" + dbname]
+    # Pass through relevant arguments for remote DB access
+    for arg in remain_args:
+        parts = arg.split("=", 1)
+        if parts[0] in ["--env_uri", "--fs_uri"]:
+            cleanup_cmd_parts.append(arg)
+    print("Running DB cleanup command - %s\n" % " ".join(cleanup_cmd_parts))
+    ret = subprocess.call(cleanup_cmd_parts)
+    if ret != 0:
+        print("ERROR: DB cleanup returned error %d\n" % ret)
+        sys.exit(2)
 
 
 # This script runs and kills db_stress multiple times. It checks consistency
@@ -1421,6 +1414,10 @@ def blackbox_crash_main(args, unknown_args):
 
         hit_timeout, retcode, outs, errs = execute_cmd(cmd, cmd_params["interval"])
 
+        # Reset destroy_db_initially after each run (it may have been set by
+        # command line for first run only)
+        cmd_params["destroy_db_initially"] = 0
+
         if not hit_timeout:
             print("Exit Before Killing")
             print_output_and_exit_on_error(outs, errs, args.print_stderr_separately)
@@ -1563,7 +1560,7 @@ def whitebox_crash_main(args, unknown_args):
                 "`compaction_style` is changed in current run so `destroy_db_initially` is set to 1 as a short-term solution to avoid cycling through previous db of different compaction style."
                 + "\n"
             )
-            additional_opts["destroy_db_initially"] = 1
+            cmd_params["destroy_db_initially"] = 1
         prev_compaction_style = cur_compaction_style
 
         cmd = gen_cmd(
@@ -1588,6 +1585,11 @@ def whitebox_crash_main(args, unknown_args):
         hit_timeout, retncode, stdoutdata, stderrdata = execute_cmd(
             cmd, exit_time - time.time() + 900
         )
+
+        # Reset destroy_db_initially after each run (it may have been set by
+        # command line for first run, or set for various reasons for a step)
+        cmd_params["destroy_db_initially"] = 0
+
         msg = "check_mode={}, kill option={}, exitcode={}\n".format(
             check_mode, additional_opts["kill_random_test"], retncode
         )
@@ -1617,7 +1619,8 @@ def whitebox_crash_main(args, unknown_args):
         # First half of the duration, keep doing kill test. For the next half,
         # try different modes.
         if time.time() > half_time:
-            cleanup_after_success(dbname)
+            # Set next iteration to destroy DB (works for remote DB)
+            cmd_params["destroy_db_initially"] = 1
             if expected_values_dir is not None:
                 shutil.rmtree(expected_values_dir, True)
                 os.mkdir(expected_values_dir)
@@ -1633,7 +1636,6 @@ def whitebox_crash_main(args, unknown_args):
 
 def main():
     global stress_cmd
-    global cleanup_cmd
 
     parser = argparse.ArgumentParser(
         description="This script runs and kills \
@@ -1649,7 +1651,7 @@ def main():
     parser.add_argument("--test_multiops_txn", action="store_true")
     parser.add_argument("--stress_cmd")
     parser.add_argument("--test_tiered_storage", action="store_true")
-    parser.add_argument("--cleanup_cmd")
+    parser.add_argument("--cleanup_cmd")  # ignore old option for now
     parser.add_argument("--print_stderr_separately", action="store_true", default=False)
 
     all_params = dict(
@@ -1690,8 +1692,6 @@ def main():
 
     if args.stress_cmd:
         stress_cmd = args.stress_cmd
-    if args.cleanup_cmd:
-        cleanup_cmd = args.cleanup_cmd
     if args.test_type == "blackbox":
         blackbox_crash_main(args, unknown_args)
     if args.test_type == "whitebox":