Repository: kudu Updated Branches: refs/heads/master 8598bd947 -> d47ce5dc2
EMC: Don't reuse data dir for log dir Separate the data and log directories in ExternalMiniCluster. Prior to this patch, the ExternalMiniCluster often used a shared directory for the data dir and the log dir. That's not typical of a production cluster, so this is primarily a test environment change. The reason for making this change is so that we have the option of placing files in the log directory before initializing the FsManager. If we put any files into the log dir before initializing the FsManager for the first time, which happens in nearly every integration test, the FsManager will refuse to start up while returning the following error: Already present: FSManager root is not empty: /path/to/data/root In a breakpad-related change following this one, we default the minidump path to a subdirectory of the log directory. Since breakpad installs signal handlers designed to collect a form of coredump, and we naturally initialize breakpad very early in the daemon startup process in order to collect information on any possible crash, we create the minidump destination directories before initializing the FsManager. Without separating the log directory from the data directory, creation of these subdirectories in the data directory before initializing the FsManager results in the error described above. The simplest solution to this problem is to separate the data and log directories in our integration testing environment. Change-Id: Iba23c429989df524da51eb012a491766df06e955 Reviewed-on: http://gerrit.cloudera.org:8080/5619 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/d47ce5dc Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/d47ce5dc Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/d47ce5dc Branch: refs/heads/master Commit: d47ce5dc28b561f9a8d72bcb5133a8e1b30a0d1e Parents: 8598bd9 Author: Mike Percy <[email protected]> Authored: Tue Jan 3 19:59:29 2017 -0800 Committer: Adar Dembo <[email protected]> Committed: Thu Jan 19 22:17:39 2017 +0000 ---------------------------------------------------------------------- .../integration-tests/external_mini_cluster.cc | 130 ++++++++----------- .../integration-tests/external_mini_cluster.h | 64 ++++----- src/kudu/integration-tests/log-rolling-itest.cc | 2 +- .../integration-tests/master_migration-itest.cc | 1 + 4 files changed, 81 insertions(+), 116 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/d47ce5dc/src/kudu/integration-tests/external_mini_cluster.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/external_mini_cluster.cc b/src/kudu/integration-tests/external_mini_cluster.cc index ec613b9..fb3f645 100644 --- a/src/kudu/integration-tests/external_mini_cluster.cc +++ b/src/kudu/integration-tests/external_mini_cluster.cc @@ -40,6 +40,7 @@ #include "kudu/util/async_util.h" #include "kudu/util/curl_util.h" #include "kudu/util/env.h" +#include "kudu/util/env_util.h" #include "kudu/util/fault_injection.h" #include "kudu/util/jsonreader.h" #include "kudu/util/metrics.h" @@ -229,14 +230,12 @@ string ExternalMiniCluster::GetBinaryPath(const string& binary) const { string ExternalMiniCluster::GetDataPath(const string& daemon_id) const { CHECK(!data_root_.empty()); - return JoinPathSegments(data_root_, daemon_id); + return JoinPathSegments(JoinPathSegments(data_root_, daemon_id), "data"); } -boost::optional<string> ExternalMiniCluster::GetLogPath(const string& daemon_id) const { - if (opts_.logtostderr) { - return boost::none; - } - return GetDataPath(daemon_id) + "-logs"; +string ExternalMiniCluster::GetLogPath(const string& daemon_id) const { + CHECK(!data_root_.empty()); + return JoinPathSegments(JoinPathSegments(data_root_, daemon_id), "logs"); } namespace { @@ -254,12 +253,14 @@ vector<string> SubstituteInFlags(const vector<string>& orig_flags, Status ExternalMiniCluster::StartSingleMaster() { string daemon_id = "master-0"; - scoped_refptr<ExternalMaster> master = - new ExternalMaster(messenger_, - GetBinaryPath(kMasterBinaryName), - GetDataPath(daemon_id), - GetLogPath(daemon_id), - SubstituteInFlags(opts_.extra_master_flags, 0)); + + ExternalDaemonOptions opts(opts_.logtostderr); + opts.messenger = messenger_; + opts.exe = GetBinaryPath(kMasterBinaryName); + opts.data_dir = GetDataPath(daemon_id); + opts.log_dir = GetLogPath(daemon_id); + opts.extra_flags = SubstituteInFlags(opts_.extra_master_flags, 0); + scoped_refptr<ExternalMaster> master = new ExternalMaster(opts); if (opts_.enable_kerberos) { RETURN_NOT_OK_PREPEND(master->EnableKerberos(kdc_.get(), Substitute("$0", kLoopbackIpAddr)), "could not enable Kerberos"); @@ -290,13 +291,15 @@ Status ExternalMiniCluster::StartDistributedMasters() { // Start the masters. for (int i = 0; i < num_masters; i++) { string daemon_id = Substitute("master-$0", i); - scoped_refptr<ExternalMaster> peer = - new ExternalMaster(messenger_, - exe, - GetDataPath(daemon_id), - GetLogPath(daemon_id), - peer_addrs[i], - SubstituteInFlags(flags, i)); + + ExternalDaemonOptions opts(opts_.logtostderr); + opts.messenger = messenger_; + opts.exe = exe; + opts.data_dir = GetDataPath(daemon_id); + opts.log_dir = GetLogPath(daemon_id); + opts.extra_flags = SubstituteInFlags(flags, i); + + scoped_refptr<ExternalMaster> peer = new ExternalMaster(opts, peer_addrs[i]); if (opts_.enable_kerberos) { RETURN_NOT_OK_PREPEND(peer->EnableKerberos(kdc_.get(), Substitute("$0", kLoopbackIpAddr)), "could not enable Kerberos"); @@ -335,14 +338,16 @@ Status ExternalMiniCluster::AddTabletServer() { master_hostports.push_back(DCHECK_NOTNULL(master(i))->bound_rpc_hostport()); } string bind_host = GetBindIpForTabletServer(idx); + + ExternalDaemonOptions opts(opts_.logtostderr); + opts.messenger = messenger_; + opts.exe = GetBinaryPath(kTabletServerBinaryName); + opts.data_dir = GetDataPath(daemon_id); + opts.log_dir = GetLogPath(daemon_id); + opts.extra_flags = SubstituteInFlags(opts_.extra_tserver_flags, idx); + scoped_refptr<ExternalTabletServer> ts = - new ExternalTabletServer(messenger_, - GetBinaryPath(kTabletServerBinaryName), - GetDataPath(daemon_id), - GetLogPath(daemon_id), - bind_host, - master_hostports, - SubstituteInFlags(opts_.extra_tserver_flags, idx)); + new ExternalTabletServer(opts, bind_host, master_hostports); if (opts_.enable_kerberos) { RETURN_NOT_OK_PREPEND(ts->EnableKerberos(kdc_.get(), bind_host), "could not enable Kerberos"); @@ -588,16 +593,13 @@ Status ExternalMiniCluster::SetFlag(ExternalDaemon* daemon, // ExternalDaemon //------------------------------------------------------------ -ExternalDaemon::ExternalDaemon(std::shared_ptr<rpc::Messenger> messenger, - string exe, - string data_dir, - boost::optional<string> log_dir, - vector<string> extra_flags) - : messenger_(std::move(messenger)), - data_dir_(std::move(data_dir)), - log_dir_(std::move(log_dir)), - exe_(std::move(exe)), - extra_flags_(std::move(extra_flags)) {} +ExternalDaemon::ExternalDaemon(ExternalDaemonOptions opts) + : messenger_(std::move(opts.messenger)), + data_dir_(std::move(opts.data_dir)), + log_dir_(std::move(opts.log_dir)), + logtostderr_(opts.logtostderr), + exe_(std::move(opts.exe)), + extra_flags_(std::move(opts.extra_flags)) {} ExternalDaemon::~ExternalDaemon() { } @@ -634,21 +636,17 @@ Status ExternalDaemon::StartProcess(const vector<string>& user_flags) { // Enable metrics logging. argv.push_back("--metrics_log_interval_ms=1000"); - if (log_dir_) { - argv.push_back("--log_dir=" + *log_dir_); - Env* env = Env::Default(); - if (!env->FileExists(*log_dir_)) { - RETURN_NOT_OK(env->CreateDir(*log_dir_)); - } - } else { + if (logtostderr_) { // Ensure that logging goes to the test output and doesn't get buffered. argv.push_back("--logtostderr"); argv.push_back("--logbuflevel=-1"); - // Even though we are logging to stderr, metrics logs end up being written - // based on -log_dir. So, we have to set that too. - argv.push_back("--log_dir=" + data_dir_); } + // Even if we are logging to stderr, metrics logs and minidumps end up being + // written based on -log_dir. So, we have to set that too. + argv.push_back("--log_dir=" + log_dir_); + RETURN_NOT_OK(env_util::CreateDirsRecursively(Env::Default(), log_dir_)); + // Tell the server to dump its port information so we can pick it up. string info_path = JoinPathSegments(data_dir_, "info.pb"); argv.push_back("--server_dump_info_path=" + info_path); @@ -989,30 +987,14 @@ ScopedResumeExternalDaemon::~ScopedResumeExternalDaemon() { // ExternalMaster //------------------------------------------------------------ -ExternalMaster::ExternalMaster(std::shared_ptr<rpc::Messenger> messenger, - string exe, - string data_dir, - boost::optional<string> log_dir, - vector<string> extra_flags) - : ExternalDaemon(std::move(messenger), - std::move(exe), - std::move(data_dir), - std::move(log_dir), - std::move(extra_flags)) { +ExternalMaster::ExternalMaster(ExternalDaemonOptions opts) + : ExternalDaemon(std::move(opts)) { set_rpc_bind_address(Substitute("$0:0", kLoopbackIpAddr)); } -ExternalMaster::ExternalMaster(std::shared_ptr<rpc::Messenger> messenger, - string exe, - string data_dir, - boost::optional<string> log_dir, - string rpc_bind_address, - std::vector<string> extra_flags) - : ExternalDaemon(std::move(messenger), - std::move(exe), - std::move(data_dir), - std::move(log_dir), - std::move(extra_flags)) { +ExternalMaster::ExternalMaster(ExternalDaemonOptions opts, + string rpc_bind_address) + : ExternalDaemon(std::move(opts)) { set_rpc_bind_address(std::move(rpc_bind_address)); } @@ -1092,18 +1074,10 @@ Status ExternalMaster::WaitForCatalogManager() { // ExternalTabletServer //------------------------------------------------------------ -ExternalTabletServer::ExternalTabletServer(std::shared_ptr<rpc::Messenger> messenger, - string exe, - string data_dir, - boost::optional<string> log_dir, +ExternalTabletServer::ExternalTabletServer(ExternalDaemonOptions opts, string bind_host, - vector<HostPort> master_addrs, - vector<string> extra_flags) - : ExternalDaemon(std::move(messenger), - std::move(exe), - std::move(data_dir), - std::move(log_dir), - std::move(extra_flags)), + vector<HostPort> master_addrs) + : ExternalDaemon(std::move(opts)), master_addrs_(HostPort::ToCommaSeparatedString(master_addrs)) { set_rpc_bind_address(std::move(bind_host)); } http://git-wip-us.apache.org/repos/asf/kudu/blob/d47ce5dc/src/kudu/integration-tests/external_mini_cluster.h ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/external_mini_cluster.h b/src/kudu/integration-tests/external_mini_cluster.h index 287d57d..801a436 100644 --- a/src/kudu/integration-tests/external_mini_cluster.h +++ b/src/kudu/integration-tests/external_mini_cluster.h @@ -24,8 +24,6 @@ #include <sys/types.h> #include <vector> -#include <boost/optional.hpp> - #include "kudu/client/client.h" #include "kudu/gutil/gscoped_ptr.h" #include "kudu/gutil/macros.h" @@ -294,11 +292,9 @@ class ExternalMiniCluster : public MiniClusterBase { // standard Kudu test directory otherwise. std::string GetDataPath(const std::string& daemon_id) const; - // Returns the path where 'daemon_id' is expected to store its logs, or none - // if it will log to stderr. Based on ExternalMiniClusterOptions.logtostderr - // and ExternalMiniClusterOptions.data_root, or on the standard Kudu test - // directory otherwise. - boost::optional<std::string> GetLogPath(const std::string& daemon_id) const; + // Returns the path where 'daemon_id' is expected to store its logs, or other + // files that reside in the log dir. + std::string GetLogPath(const std::string& daemon_id) const; private: FRIEND_TEST(MasterFailoverTest, TestKillAnyMaster); @@ -326,13 +322,22 @@ class ExternalMiniCluster : public MiniClusterBase { DISALLOW_COPY_AND_ASSIGN(ExternalMiniCluster); }; +struct ExternalDaemonOptions { + explicit ExternalDaemonOptions(bool logtostderr) + : logtostderr(logtostderr) { + } + + bool logtostderr; + std::shared_ptr<rpc::Messenger> messenger; + std::string exe; + std::string data_dir; + std::string log_dir; + std::vector<std::string> extra_flags; +}; + class ExternalDaemon : public RefCountedThreadSafe<ExternalDaemon> { public: - ExternalDaemon(std::shared_ptr<rpc::Messenger> messenger, - std::string exe, - std::string data_dir, - boost::optional<std::string> log_dir, - std::vector<std::string> extra_flags); + explicit ExternalDaemon(ExternalDaemonOptions opts); HostPort bound_rpc_hostport() const; Sockaddr bound_rpc_addr() const; @@ -390,11 +395,8 @@ class ExternalDaemon : public RefCountedThreadSafe<ExternalDaemon> { const std::string& data_dir() const { return data_dir_; } - // Returns the log dir of the external daemon, or none if the daemon is - // configured to log to stderr. - const boost::optional<std::string>& log_dir() const { - return log_dir_; - } + // Returns the log dir of the external daemon. + const std::string& log_dir() const { return log_dir_; } // Return a pointer to the flags used for this server on restart. // Modifying these flags will only take effect on the next restart. @@ -450,7 +452,8 @@ class ExternalDaemon : public RefCountedThreadSafe<ExternalDaemon> { const std::shared_ptr<rpc::Messenger> messenger_; const std::string data_dir_; - const boost::optional<std::string> log_dir_; + const std::string log_dir_; + const bool logtostderr_; std::string exe_; std::vector<std::string> extra_flags_; std::map<std::string, std::string> extra_env_; @@ -486,21 +489,12 @@ class ScopedResumeExternalDaemon { DISALLOW_COPY_AND_ASSIGN(ScopedResumeExternalDaemon); }; - class ExternalMaster : public ExternalDaemon { public: - ExternalMaster(std::shared_ptr<rpc::Messenger> messenger, - std::string exe, - std::string data_dir, - boost::optional<std::string> log_dir, - std::vector<std::string> extra_flags); - - ExternalMaster(std::shared_ptr<rpc::Messenger> messenger, - std::string exe, - std::string data_dir, - boost::optional<std::string> log_dir, - std::string rpc_bind_address, - std::vector<std::string> extra_flags); + explicit ExternalMaster(ExternalDaemonOptions opts); + + ExternalMaster(ExternalDaemonOptions opts, + std::string rpc_bind_address); Status Start(); @@ -519,13 +513,9 @@ class ExternalMaster : public ExternalDaemon { class ExternalTabletServer : public ExternalDaemon { public: - ExternalTabletServer(std::shared_ptr<rpc::Messenger> messenger, - std::string exe, - std::string data_dir, - boost::optional<std::string> log_dir, + ExternalTabletServer(ExternalDaemonOptions opts, std::string bind_host, - std::vector<HostPort> master_addrs, - std::vector<std::string> extra_flags); + std::vector<HostPort> master_addrs); Status Start(); http://git-wip-us.apache.org/repos/asf/kudu/blob/d47ce5dc/src/kudu/integration-tests/log-rolling-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/log-rolling-itest.cc b/src/kudu/integration-tests/log-rolling-itest.cc index 73eb5f5..10d59ee 100644 --- a/src/kudu/integration-tests/log-rolling-itest.cc +++ b/src/kudu/integration-tests/log-rolling-itest.cc @@ -62,7 +62,7 @@ TEST(LogRollingITest, TestLogCleanupOnStartup) { for (int i = 1; i <= 10; i++) { AssertEventually([&] () { - ASSERT_EQ(std::min(3, i), CountInfoLogs(*cluster.master()->log_dir())); + ASSERT_EQ(std::min(3, i), CountInfoLogs(cluster.master()->log_dir())); }); cluster.master()->Shutdown(); ASSERT_OK(cluster.master()->Restart()); http://git-wip-us.apache.org/repos/asf/kudu/blob/d47ce5dc/src/kudu/integration-tests/master_migration-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/master_migration-itest.cc b/src/kudu/integration-tests/master_migration-itest.cc index 20336f9..84b6a9e 100644 --- a/src/kudu/integration-tests/master_migration-itest.cc +++ b/src/kudu/integration-tests/master_migration-itest.cc @@ -110,6 +110,7 @@ TEST_F(MasterMigrationTest, TestEndToEndMigration) { // Format a filesystem tree for each of the new masters and get the uuids. for (int i = 1; i < kMasterRpcPorts.size(); i++) { string data_root = cluster_->GetDataPath(Substitute("master-$0", i)); + ASSERT_OK(env_->CreateDir(DirName(data_root))); { vector<string> args = { kBinPath,
