Skip to content

Commit

Permalink
minor cleanups to db_crashtest.py (#10654)
Browse files Browse the repository at this point in the history
Summary:
Expanded `all_params` to include all parameters crash test may set. Previously, `atomic_flush` was not included in `all_params` and thus was not visible to `finalize_and_sanitize()`. The consequence was manual crash test runs could provide unsafe combinations of parameters to `db_stress`. For example, running `db_crashtest.py` with `-atomic_flush=0` could cause `db_stress` to run with `-atomic_flush=0 -disable_wal=1`, which is known to produce inconsistencies across column families.

While expanding `all_params`, I found we cannot have an entry in it for both `db_stress` and `db_crashtest.py`. So I renamed `enable_tiered_storage` to `test_tiered_storage` for `db_crashtest.py`, which appears more conventional anyways.

Pull Request resolved: facebook/rocksdb#10654

Reviewed By: hx235

Differential Revision: D39369349

Pulled By: ajkr

fbshipit-source-id: 31d9010c760c868b20d5e9bd78ba75c8ff3ce348
  • Loading branch information
ajkr authored and facebook-github-bot committed Sep 9, 2022
1 parent 0148c49 commit 4100eb3
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 6 deletions.
4 changes: 2 additions & 2 deletions crash_test.mk
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ blackbox_crash_test_with_multiops_wp_txn: $(DB_STRESS_CMD)
$(CRASHTEST_PY) --test_multiops_txn --write_policy write_prepared blackbox $(CRASH_TEST_EXT_ARGS)

blackbox_crash_test_with_tiered_storage: $(DB_STRESS_CMD)
$(CRASHTEST_PY) --enable_tiered_storage blackbox $(CRASH_TEST_EXT_ARGS)
$(CRASHTEST_PY) --test_tiered_storage blackbox $(CRASH_TEST_EXT_ARGS)

ifeq ($(CRASH_TEST_KILL_ODD),)
CRASH_TEST_KILL_ODD=888887
Expand All @@ -103,5 +103,5 @@ whitebox_crash_test_with_ts: $(DB_STRESS_CMD)
$(CRASH_TEST_KILL_ODD) $(CRASH_TEST_EXT_ARGS)

whitebox_crash_test_with_tiered_storage: $(DB_STRESS_CMD)
$(CRASHTEST_PY) --enable_tiered_storage whitebox --random_kill_odd \
$(CRASHTEST_PY) --test_tiered_storage whitebox --random_kill_odd \
$(CRASH_TEST_KILL_ODD) $(CRASH_TEST_EXT_ARGS)
13 changes: 9 additions & 4 deletions tools/db_crashtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ def gen_cmd_params(args):
params.update(multiops_wc_txn_params)
elif args.write_policy == 'write_prepared':
params.update(multiops_wp_txn_params)
if args.enable_tiered_storage:
if args.test_tiered_storage:
params.update(tiered_params)

# Best-effort recovery and BlobDB are currently incompatible. Test BE recovery
Expand All @@ -616,7 +616,8 @@ def gen_cmd(params, unknown_params):
if k not in set(['test_type', 'simple', 'duration', 'interval',
'random_kill_odd', 'cf_consistency', 'txn',
'test_best_efforts_recovery', 'enable_ts',
'test_multiops_txn', 'write_policy', 'stress_cmd'])
'test_multiops_txn', 'write_policy', 'stress_cmd',
'test_tiered_storage'])
and v is not None] + unknown_params
return cmd

Expand Down Expand Up @@ -840,7 +841,7 @@ def main():
parser.add_argument("--test_multiops_txn", action='store_true')
parser.add_argument("--write_policy", choices=["write_committed", "write_prepared"])
parser.add_argument("--stress_cmd")
parser.add_argument("--enable_tiered_storage", action='store_true')
parser.add_argument("--test_tiered_storage", action='store_true')

all_params = dict(list(default_params.items())
+ list(blackbox_default_params.items())
Expand All @@ -852,7 +853,11 @@ def main():
+ list(ts_params.items())
+ list(multiops_txn_default_params.items())
+ list(multiops_wc_txn_params.items())
+ list(multiops_wp_txn_params.items()))
+ list(multiops_wp_txn_params.items())
+ list(best_efforts_recovery_params.items())
+ list(cf_consistency_params.items())
+ list(tiered_params.items())
+ list(txn_params.items()))

for k, v in all_params.items():
parser.add_argument("--" + k, type=type(v() if callable(v) else v))
Expand Down

0 comments on commit 4100eb3

Please sign in to comment.