database/rocksdb
1
0
Fork 0
mirror of https://github.com/facebook/rocksdb.git synced 2026-10-23 08:09:02 +03:00
Code Activity

Make db_stress actually tolerate transient remote IO errors (#15170)

Browse Source 
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15170

`warm_storage_tsan_liveness_crash_test` fails with

```
error : Get() returns IO error: Failed to read 1032 bytes from file
ws://ws.zdb.ldc0zdbtest1/.../000093.sst from offset 5461, message
[0 code=DEADLINE_EXCEEDED isRetryable=1 ... error=WSE_BLOCK_READ_TIMEOUT
classification=TRANSIENT_ERROR ...] for key: 0000...78 (30280).
Verification failed :(
```

The Warm Storage error is explicitly `classification=TRANSIENT_ERROR isRetryable=1`, and the DB artifact confirms there is no RocksDB problem: `000093.sst` is intact (37760 bytes, downloaded successfully after the run) and the DB `LOG` reaches `Shutdown complete` with zero IO-error or corruption entries. This is exactly the class of failure `--tolerate_non_injected_io_errors_for_remote_dbs` was added for (D112136438), but that mechanism never actually took effect. Two independent defects:

**1. The triggering non-default option: `tolerate_non_injected_io_errors_for_remote_dbs=0` on a remote DB.**
`db_crashtest.py` hard-coded the parameter to `0` in `default_params` and only ever *force-disabled* it (for local DBs), leaving each remote job to opt in on its own command line. `rocks/sandcastle/test-driver.sh`'s `warm_storage_*crash*` branch never passed it, so every Warm Storage crash-test job ran with the flag off and a single transient read timeout failed the run. Fix: the parameter now defaults to `1` whenever `--env_uri` / `--fs_uri` names a remote backend, so no remote job has to remember to opt in. A caller can still pass `0` explicitly, and `finalize_and_sanitize()` still force-disables it for local DBs so genuine local IO errors (e.g. io_uring failures) are never masked.

**2. `injected_error_count == 0` made the flag a no-op in `no_batched_ops_stress.cc`.**
All five read-path guards were
`injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)`.
`IsErrorInjectedAndRetryable()` covers *non-injected* remote IO errors, which by definition have no injected fault to correlate with — and the `liveness` profile forces every `*_fault_one_in` to `0`, pinning `injected_error_count` to `0` for the whole run. So the left operand short-circuited to true and the error was reported as a verification failure regardless of the flag. Fix: extract `StressTest::IsTolerableNonInjectedRemoteIOError()` and add `StressTest::IsRetryableOperationError(injected_error_count, s)`, which gates the injected-error branch on `injected_error_count > 0` but lets a tolerated non-injected remote IO error through unconditionally. The five call sites now use the helper.

With the tolerate flag off (all local runs), the new guard reduces algebraically to the old expression, so local behaviour is unchanged. The genuine consistency-check gate at `no_batched_ops_stress.cc:1146` (`do_consistency_check && injected_error_count == 0`) is intentionally left alone.

Fixes T286575055.

[Session trajectory link](https://www.internalfb.com/intern/devai/devmate/inspector/?id=9646c050-6791-4ac3-9abb-3ee110dcb54e)

Reviewed By: pdillinger

Differential Revision: D117886823

fbshipit-source-id: d7591472db8ee6e03c9d9f018480409b6d802838
This commit is contained in:
Jay Huh
2026-09-03 10:43:54 -07:00
committed by meta-codesync[bot]
parent 897162a6bc
commit bec9dcec7c
5 changed files with 73 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
db_stress_tool/db_stress_test_base.cc
View File

@@ -454,15 +454,29 @@ Status VerifyCheckpointColumnFamilies(
} // namespace
bool StressTest::IsTolerableNonInjectedRemoteIOError(const Status& error_s) {
assert(!error_s.ok());
return FLAGS_tolerate_non_injected_io_errors_for_remote_dbs &&
(!FLAGS_env_uri.empty() || !FLAGS_fs_uri.empty()) &&
error_s.IsIOError() &&
!status_to_io_status(Status(error_s)).GetDataLoss();
}
bool StressTest::IsErrorInjectedAndRetryable(const Status& error_s) {
assert(!error_s.ok());
const IOStatus io_s = status_to_io_status(Status(error_s));
return !io_s.GetDataLoss() &&
((error_s.getState() &&
FaultInjectionTestFS::IsInjectedError(error_s)) ||
(FLAGS_tolerate_non_injected_io_errors_for_remote_dbs &&
(!FLAGS_env_uri.empty() || !FLAGS_fs_uri.empty()) &&
error_s.IsIOError()));
const bool injected_and_retryable =
error_s.getState() && FaultInjectionTestFS::IsInjectedError(error_s) &&
!status_to_io_status(Status(error_s)).GetDataLoss();
return injected_and_retryable || IsTolerableNonInjectedRemoteIOError(error_s);
}
bool StressTest::IsRetryableOperationError(int injected_error_count,
const Status& error_s) {
assert(!error_s.ok());
if (IsTolerableNonInjectedRemoteIOError(error_s)) {
return true;
}
return injected_error_count > 0 && IsErrorInjectedAndRetryable(error_s);
}
bool StressTest::LazyEntityReadEnabled() const {

22
db_stress_tool/db_stress_test_base.h
View File

@@ -43,6 +43,28 @@ class StressTest {
// header, so they are not visible here.
static bool IsErrorInjectedAndRetryable(const Status& error_s);
// Returns true if `error_s` is a non-injected, non-data-loss IO error that
// --tolerate_non_injected_io_errors_for_remote_dbs asks us to tolerate while
// a remote backend (--env_uri / --fs_uri) is in use. Infrastructure behind a
// remote backend can fail a read transiently with no RocksDB bug involved.
//
// Defined out-of-line in db_stress_test_base.cc for the same reason as
// IsErrorInjectedAndRetryable.
static bool IsTolerableNonInjectedRemoteIOError(const Status& error_s);
// Returns true if `error_s`, surfaced by an operation during which
// `injected_error_count` faults were injected, is retryable rather than a
// verification failure.
//
// An injected-error status is only trusted when this operation actually
// injected a fault, because otherwise the status did not come from fault
// injection. A tolerated non-injected remote IO error has no injected fault
// to correlate with, so `injected_error_count` must not gate it -- gating it
// would make --tolerate_non_injected_io_errors_for_remote_dbs a no-op
// whenever fault injection is disabled.
static bool IsRetryableOperationError(int injected_error_count,
const Status& error_s);
static bool IsTolerableCompactionFailure(const Status& s) {
// TOOD (hx235): allow an exact list of tolerable failures under stress
// test

25
db_stress_tool/no_batched_ops_stress.cc
View File

@@ -852,7 +852,7 @@ class NonBatchedOpsStressTest : public StressTest {
key.ToString(true).c_str(), rand_keys[0]);
}
}
} else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
} else if (!IsRetryableOperationError(injected_error_count, s)) {
thread->shared->SetVerificationFailure();
fprintf(stderr, "error : Get() returns %s for key: %s (%" PRIi64 ").\n",
s.ToString().c_str(), key.ToString(true).c_str(), rand_keys[0]);
@@ -1069,8 +1069,10 @@ class NonBatchedOpsStressTest : public StressTest {
ThreadStatus::OperationType::OP_MULTIGET);
}
if (!tmp_s.ok() && !tmp_s.IsNotFound()) {
fprintf(stderr, "Get error: %s\n", s.ToString().c_str());
is_consistent = false;
if (!IsTolerableNonInjectedRemoteIOError(tmp_s)) {
fprintf(stderr, "Get error: %s\n", tmp_s.ToString().c_str());
is_consistent = false;
}
} else if (!s.ok() && tmp_s.ok()) {
fprintf(stderr,
"MultiGet(%d) returned different results with key %s. "
@@ -1126,7 +1128,7 @@ class NonBatchedOpsStressTest : public StressTest {
} else if (s.IsMergeInProgress() && use_txn) {
// With txn this is sometimes expected.
thread->stats.AddGets(1, 1);
} else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
} else if (!IsRetryableOperationError(injected_error_count, s)) {
fprintf(stderr, "MultiGet error: %s\n", s.ToString().c_str());
thread->stats.AddErrors(1);
shared->SetVerificationFailure();
@@ -1328,7 +1330,7 @@ class NonBatchedOpsStressTest : public StressTest {
StringToHex(key_str).c_str(), rand_keys[0]);
}
}
} else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
} else if (!IsRetryableOperationError(injected_error_count, s)) {
fprintf(stderr,
"error : GetEntity() returns %s for key: %s (%" PRIi64 ").\n",
s.ToString().c_str(), StringToHex(key_str).c_str(), rand_keys[0]);
@@ -1495,9 +1497,11 @@ class NonBatchedOpsStressTest : public StressTest {
ran_cmp_get_entity = true;
if (!cmp_s.ok() && !cmp_s.IsNotFound()) {
fprintf(stderr, "GetEntity error: %s\n",
cmp_s.ToString().c_str());
is_consistent = false;
if (!IsTolerableNonInjectedRemoteIOError(cmp_s)) {
fprintf(stderr, "GetEntity error: %s\n",
cmp_s.ToString().c_str());
is_consistent = false;
}
} else if (cmp_s.IsNotFound()) {
if (s.ok()) {
fprintf(
@@ -1616,8 +1620,7 @@ class NonBatchedOpsStressTest : public StressTest {
thread->stats.AddGets(1, 1);
} else if (s.IsNotFound()) {
thread->stats.AddGets(1, 0);
} else if (injected_error_count == 0 ||
!IsErrorInjectedAndRetryable(s)) {
} else if (!IsRetryableOperationError(injected_error_count, s)) {
fprintf(stderr, "MultiGetEntity error: %s\n", s.ToString().c_str());
thread->stats.AddErrors(1);
thread->shared->SetVerificationFailure();
@@ -1933,7 +1936,7 @@ class NonBatchedOpsStressTest : public StressTest {
if (s.ok()) {
thread->stats.AddPrefixes(1, count);
} else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
} else if (!IsRetryableOperationError(injected_error_count, s)) {
fprintf(stderr,
"TestPrefixScan error: %s with ReadOptions::iterate_upper_bound: "
"%s, prefix_same_as_start: %s \n",

6
tools/db_crashtest.py
View File

@@ -274,7 +274,11 @@ default_params = {
"flush_one_in": lambda: random.choice([1000, 1000000]),
"manual_wal_flush_one_in": lambda: random.choice([0, 1000]),
"sync_wal_one_in": 0,
"tolerate_non_injected_io_errors_for_remote_dbs": 0,
# Remote backends can surface transient infrastructure IO errors that are
# not RocksDB bugs, so default to tolerating them whenever a remote
# --env_uri / --fs_uri is in use. A caller can still pass 0 explicitly, and
# finalize_and_sanitize() force-disables it for local DBs.
"tolerate_non_injected_io_errors_for_remote_dbs": lambda: 1 if is_remote_db else 0,
"file_checksum_impl": lambda: random.choice(["none", "crc32c", "xxh64", "big"]),
"get_live_files_apis_one_in": lambda: random.choice([10000, 1000000]),
"checkpoint_atomic_flush": lambda: random.choice([0, 1]),

11
tools/db_crashtest_test.py
View File

@@ -489,6 +489,17 @@ class DBCrashTestTest(unittest.TestCase):
self.assertEqual(10, finalized["clear_column_family_one_in"])
self.assertEqual(0, finalized["tolerate_non_injected_io_errors_for_remote_dbs"])
def test_finalize_defaults_tolerate_non_injected_io_errors_for_remote_db(self):
db_crashtest = self.load_db_crashtest()
db_crashtest.is_remote_db = True
params = self.build_params(db_crashtest.default_params)
finalized = db_crashtest.finalize_and_sanitize(params)
# Remote backends can return transient infrastructure IO errors that
# are not RocksDB bugs, so remote runs tolerate them by default.
self.assertEqual(1, finalized["tolerate_non_injected_io_errors_for_remote_dbs"])
def test_finalize_tolerates_non_injected_io_errors_for_remote_db(self):
db_crashtest = self.load_db_crashtest()
db_crashtest.is_remote_db = True