From a685a701ca97411834059e688f4fa9813ca49890 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 | 1 + db/db_test_util.h | 13 +++++++++ logging/auto_roll_logger.cc | 19 +++++++++----- logging/auto_roll_logger_test.cc | 45 ++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 7f65f86f6..f696d9d8f 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,6 +3,7 @@ ### Bug Fixes * If the primary's CURRENT file is missing or inaccessible, the secondary instance should not hang repeatedly trying to switch to a new MANIFEST. It should instead return the error code encountered while accessing the file. * 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. +* 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. ### New Features * Made the EventListener extend the Customizable class. diff --git a/db/db_test_util.h b/db/db_test_util.h index f2f56e890..689af0788 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) {