From 60f5a22cffea7dcd1c367c6491d4d9584fdf5121 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Wed, 4 Aug 2021 17:24:06 -0700 Subject: [PATCH] Do not attempt to rename non-existent info log (#8622) Summary: Previously we attempted to rename "LOG" to "LOG.old.*" without checking its existence first. "LOG" had no reason to exist in a new DB. Errors in renaming a non-existent "LOG" were swallowed via `PermitUncheckedError()` so things worked. However the storage service's error monitoring was detecting all these benign rename failures. So it is better to fix it. Also with this PR we can now distinguish rename failure for other reasons and return them. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8622 Test Plan: new unit test Reviewed By: akankshamahajan15 Differential Revision: D30115189 Pulled By: ajkr fbshipit-source-id: e2f337ffb2bd171be0203172abc8e16e7809b170 --- HISTORY.md | 4 +++ db/db_test_util.h | 13 +++++++++ logging/auto_roll_logger.cc | 19 +++++++++----- logging/auto_roll_logger_test.cc | 45 ++++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 3bdcd3d8a..dc35ec9f1 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,4 +1,8 @@ # Rocksdb Change Log +## Unreleased +### Bug Fixes +* Removed a call to `RenameFile()` on a non-existent info log file ("LOG") when opening a new DB. Such a call was guaranteed to fail though did not impact applications since we swallowed the error. Now we also stopped swallowing errors in renaming "LOG" file. + ## 6.23.2 (2021-08-04) ### Bug Fixes * Fixed a race related to the destruction of `ColumnFamilyData` objects. The earlier logic unlocked the DB mutex before destroying the thread-local `SuperVersion` pointers, which could result in a process crash if another thread managed to get a reference to the `ColumnFamilyData` object. diff --git a/db/db_test_util.h b/db/db_test_util.h index 0086ca821..4c117a8e1 100644 --- a/db/db_test_util.h +++ b/db/db_test_util.h @@ -675,6 +675,14 @@ class SpecialEnv : public EnvWrapper { } } + Status RenameFile(const std::string& src, const std::string& dest) override { + rename_count_.fetch_add(1); + if (rename_error_.load(std::memory_order_acquire)) { + return Status::NotSupported("Simulated `RenameFile()` error."); + } + return target()->RenameFile(src, dest); + } + // Something to return when mocking current time const int64_t maybe_starting_time_; @@ -702,6 +710,9 @@ class SpecialEnv : public EnvWrapper { // Force write to log files to fail while this pointer is non-nullptr std::atomic log_write_error_; + // Force `RenameFile()` to fail while this pointer is non-nullptr + std::atomic rename_error_{false}; + // Slow down every log write, in micro-seconds. std::atomic log_write_slowdown_; @@ -745,6 +756,8 @@ class SpecialEnv : public EnvWrapper { std::atomic delete_count_; + std::atomic rename_count_{0}; + std::atomic is_wal_sync_thread_safe_{true}; std::atomic compaction_readahead_size_{}; diff --git a/logging/auto_roll_logger.cc b/logging/auto_roll_logger.cc index 1ff08c1ad..d32645a42 100644 --- a/logging/auto_roll_logger.cc +++ b/logging/auto_roll_logger.cc @@ -296,12 +296,19 @@ Status CreateLoggerFromOptions(const std::string& dbname, } #endif // !ROCKSDB_LITE // Open a log file in the same directory as the db - env->RenameFile( - fname, OldInfoLogFileName(dbname, clock->NowMicros(), db_absolute_path, - options.db_log_dir)) - .PermitUncheckedError(); - s = env->NewLogger(fname, logger); - if (logger->get() != nullptr) { + s = env->FileExists(fname); + if (s.ok()) { + s = env->RenameFile( + fname, OldInfoLogFileName(dbname, clock->NowMicros(), db_absolute_path, + options.db_log_dir)); + } else if (s.IsNotFound()) { + // "LOG" is not required to exist since this could be a new DB. + s = Status::OK(); + } + if (s.ok()) { + s = env->NewLogger(fname, logger); + } + if (s.ok() && logger->get() != nullptr) { (*logger)->SetInfoLogLevel(options.info_log_level); } return s; diff --git a/logging/auto_roll_logger_test.cc b/logging/auto_roll_logger_test.cc index 59e0ebac6..b9e8ed55a 100644 --- a/logging/auto_roll_logger_test.cc +++ b/logging/auto_roll_logger_test.cc @@ -19,6 +19,7 @@ #include #include +#include "db/db_test_util.h" #include "logging/logging.h" #include "port/port.h" #include "rocksdb/db.h" @@ -686,6 +687,50 @@ TEST_F(AutoRollLoggerTest, FileCreateFailure) { ASSERT_NOK(CreateLoggerFromOptions("", options, &logger)); ASSERT_TRUE(!logger); } + +TEST_F(AutoRollLoggerTest, RenameOnlyWhenExists) { + InitTestDb(); + SpecialEnv env(Env::Default()); + Options options; + options.env = &env; + + // Originally no LOG exists. Should not see a rename. + { + std::shared_ptr logger; + ASSERT_OK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_EQ(0, env.rename_count_); + } + + // Now a LOG exists. Create a new one should see a rename. + { + std::shared_ptr logger; + ASSERT_OK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_EQ(1, env.rename_count_); + } +} + +TEST_F(AutoRollLoggerTest, RenameError) { + InitTestDb(); + SpecialEnv env(Env::Default()); + env.rename_error_ = true; + Options options; + options.env = &env; + + // Originally no LOG exists. Should not be impacted by rename error. + { + std::shared_ptr logger; + ASSERT_OK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_TRUE(logger != nullptr); + } + + // Now a LOG exists. Rename error should cause failure. + { + std::shared_ptr logger; + ASSERT_NOK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_TRUE(logger == nullptr); + } +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {