From e0edef1c67f589d701b212c518ece342f951c52a 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 # Conflicts: # HISTORY.md --- 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 728a1533573..718d6386850 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.22.1 (2021-06-25) ### Bug Fixes * `GetLiveFilesMetaData()` now populates the `temperature`, `oldest_ancester_time`, and `file_creation_time` fields of its `LiveFileMetaData` results when the information is available. Previously these fields always contained zero indicating unknown. diff --git a/db/db_test_util.h b/db/db_test_util.h index 0086ca82106..4c117a8e1a1 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 1ff08c1adef..d32645a4231 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 59e0ebac658..b9e8ed55a6b 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) {