envoyproxy · htuch · Mar 2, 2018 · Feb 26, 2018 · Feb 27, 2018 · Feb 28, 2018
diff --git a/source/exe/main.cc b/source/exe/main.cc
@@ -10,19 +10,13 @@
  * after setting up command line options.
  */
 int main(int argc, char** argv) {
-#ifdef ENVOY_HOT_RESTART
-  constexpr bool enable_hot_restart = true;
-#else
-  constexpr bool enable_hot_restart = false;
-#endif
-
   std::unique_ptr<Envoy::MainCommon> main_common;
 
   // Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE
   // as needed. Whatever code in the initialization path that fails is expected to log an error
   // message so the user can diagnose.
   try {
-    main_common = std::make_unique<Envoy::MainCommon>(argc, argv, enable_hot_restart);
+    main_common = std::make_unique<Envoy::MainCommon>(argc, argv);
   } catch (const Envoy::NoServingException& e) {
     return EXIT_SUCCESS;
   } catch (const Envoy::MalformedArgvException& e) {

diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc
@@ -37,19 +37,19 @@ Runtime::LoaderPtr ProdComponentFactory::createRuntime(Server::Instance& server,
   return Server::InstanceUtil::createRuntime(server, config);
 }
 
-MainCommonBase::MainCommonBase(OptionsImpl& options, bool hot_restart) : options_(options) {
+MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) {
   ares_library_init(ARES_LIB_INIT_ALL);
   Event::Libevent::Global::initialize();
   RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors());
 
   switch (options_.mode()) {
   case Server::Mode::Serve: {
 #ifdef ENVOY_HOT_RESTART
-    if (hot_restart) {
+    if (!options.hotRestartDisabled()) {
       restarter_.reset(new Server::HotRestartImpl(options_));
     }
 #endif
-    if (!hot_restart) {
+    if (restarter_.get() == nullptr) {
       restarter_.reset(new Server::HotRestartNopImpl());
     }
 
@@ -88,26 +88,21 @@ bool MainCommonBase::run() {
   NOT_REACHED;
 }
 
-MainCommon::MainCommon(int argc, char** argv, bool hot_restart)
-    : options_(computeOptions(argc, argv, hot_restart)), base_(*options_, hot_restart) {}
-
-std::unique_ptr<OptionsImpl> MainCommon::computeOptions(int argc, char** argv, bool hot_restart) {
-  OptionsImpl::HotRestartVersionCb hot_restart_version_cb = [](uint64_t, uint64_t) {
-    return "disabled";
-  };
+MainCommon::MainCommon(int argc, char** argv)
+    : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info), base_(options_) {}
 
+std::string MainCommon::hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len,
+                                          bool hot_restart_enabled) {
 #ifdef ENVOY_HOT_RESTART
-  if (hot_restart) {
-    // Enabled by default, except on OS X. Control with "bazel --define=hot_restart=disabled"
-    hot_restart_version_cb = [](uint64_t max_num_stats, uint64_t max_stat_name_len) {
-      return Server::HotRestartImpl::hotRestartVersion(max_num_stats, max_stat_name_len);
-    };
+  if (hot_restart_enabled) {
+    return Server::HotRestartImpl::hotRestartVersion(max_num_stats, max_stat_name_len);
   }
 #else
-  // Hot-restart should not be specified if the support is not compiled in.
-  RELEASE_ASSERT(!hot_restart);
+  UNREFERENCED_PARAMETER(hot_restart_enabled);
+  UNREFERENCED_PARAMETER(max_num_stats);
+  UNREFERENCED_PARAMETER(max_stat_name_len);
 #endif
-  return std::make_unique<OptionsImpl>(argc, argv, hot_restart_version_cb, spdlog::level::info);
+  return "disabled";
 }
 
 // Legacy implementation of main_common.
@@ -116,11 +111,7 @@ std::unique_ptr<OptionsImpl> MainCommon::computeOptions(int argc, char** argv, b
 // and MainCommon can be merged. The current theory is that only Google calls this.
 int main_common(OptionsImpl& options) {
   try {
-#if ENVOY_HOT_RESTART
-    MainCommonBase main_common(options, true);
-#else
-    MainCommonBase main_common(options, false);
-#endif
+    MainCommonBase main_common(options);
     return main_common.run() ? EXIT_SUCCESS : EXIT_FAILURE;
   } catch (EnvoyException& e) {
     return EXIT_FAILURE;

diff --git a/source/exe/main_common.h b/source/exe/main_common.h
@@ -22,7 +22,7 @@ class ProdComponentFactory : public Server::ComponentFactory {
 
 class MainCommonBase {
 public:
-  MainCommonBase(OptionsImpl& options, bool hot_restart);
+  MainCommonBase(OptionsImpl& options);
   ~MainCommonBase();
 
   bool run();
@@ -39,18 +39,18 @@ class MainCommonBase {
 
 class MainCommon {
 public:
-  MainCommon(int argc, char** argv, bool hot_restart);
+  MainCommon(int argc, char** argv);
   bool run() { return base_.run(); }
 
-private:
-  static std::unique_ptr<Envoy::OptionsImpl> computeOptions(int argc, char** argv,
-                                                            bool hot_restart);
+  static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len,
+                                       bool hot_restart_enabled);
 
+private:
 #ifdef ENVOY_HANDLE_SIGNALS
   Envoy::SignalAction handle_sigs;
 #endif
 
-  std::unique_ptr<Envoy::OptionsImpl> options_;
+  Envoy::OptionsImpl options_;
   MainCommonBase base_;
 };
 

diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc
@@ -120,10 +120,12 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r
     throw MalformedArgvException(message);
   }
 
+  hot_restart_disabled_ = disable_hot_restart.getValue();
   if (hot_restart_version_option.getValue()) {
     std::cerr << hot_restart_version_cb(max_stats.getValue(),
                                         max_obj_name_len.getValue() +
-                                            Stats::RawStatData::maxStatSuffixLength());
+                                            Stats::RawStatData::maxStatSuffixLength(),
+                                        !hot_restart_disabled_);
     throw NoServingException();
   }
 
@@ -171,6 +173,5 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r
   parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue());
   max_stats_ = max_stats.getValue();
   max_obj_name_length_ = max_obj_name_len.getValue();
-  hot_restart_disabled_ = disable_hot_restart.getValue();
 }
 } // namespace Envoy
diff --git a/source/server/options_impl.h b/source/server/options_impl.h
@@ -15,7 +15,10 @@ namespace Envoy {
  */
 class OptionsImpl : public Server::Options {
 public:
-  typedef std::function<std::string(uint64_t, uint64_t)> HotRestartVersionCb;
+  /**
+   * Parameters are max_num_stats, max_stat_name_len, hot_restart_enabled
+   */
+  typedef std::function<std::string(uint64_t, uint64_t, bool)> HotRestartVersionCb;
 
   /**
    * @throw NoServingException if Envoy has already done everything specified by the argv (e.g.

diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc
@@ -7,71 +7,106 @@
 // TODO(issues/2649): This test needs to be parameterized on IP versions.
 #ifndef ENVOY_CONFIG_COVERAGE
 
+#include <unistd.h>
+
 #include "exe/main_common.h"
 
 #include "server/options_impl.h"
 
 #include "test/test_common/environment.h"
+#include "test/test_common/utility.h"
 
 #include "gtest/gtest.h"
 
 #ifdef ENVOY_HANDLE_SIGNALS
 #include "exe/signal_action.h"
 #endif
 
-#ifdef ENVOY_HOT_RESTART
-#include "server/hot_restart_impl.h"
-#endif
-
 namespace Envoy {
 
-TEST(MainCommon, ConstructDestruct) {
+/**
+ * Captures common functions needed for invoking MainCommon. Generates a
+ * unique --base-id setting based on the pid and a random number. Maintains
+ * an argv array that is terminated with nullptr. Identifies the config
+ * file relative to $TEST_RUNDIR.
+ *
+ * TODO(jmarantz): Make these tests work with ipv6. See
+ * https://github.com/envoyproxy/envoy/issues/2649
+ */
+class MainCommonTest : public testing::Test {
+public:
+  MainCommonTest()
+      : config_file_(Envoy::TestEnvironment::getCheckedEnvVar("TEST_RUNDIR") +
+                     "/test/config/integration/google_com_proxy_port_0.v2.yaml"),
+        random_string_(fmt::format("{}", static_cast<uint32_t>(getpid()))),
+        argv_({"envoy-static", "--base-id", random_string_.c_str(), nullptr}) {}
+
+  char** argv() { return const_cast<char**>(&argv_[0]); }
+  int argc() { return argv_.size() - 1; }
+
+  void addConfig() {
+    addArg("-c");
+    addArg(config_file_.c_str());
+  }
+
+  // Adds an argument, assuring that argv remains null-terminated.
+  void addArg(const char* arg) {
+    ASSERT(!argv_.empty());
+    const size_t last = argv_.size() - 1;
+    ASSERT(argv_[last] == nullptr); // invariant established in ctor, maintained below.
+    argv_[last] = arg;              // guaranteed non-empty
+    argv_.push_back(nullptr);
+  }
+
+  std::string config_file_;
+  std::string random_string_;
+  std::vector<const char*> argv_;
+};
+
+// Exercise the codepath to instantiate MainCommon and destruct it, with hot restart.
+TEST_F(MainCommonTest, ConstructDestructHotRestartEnabled) {
+  if (!Envoy::TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion::v4)) {
+    return;
+  }
+  addConfig();
+  VERBOSE_EXPECT_NO_THROW(MainCommon main_common(argc(), argv()));
+}
+
+// Exercise the codepath to instantiate MainCommon and destruct it, without hot restart.
+TEST_F(MainCommonTest, ConstructDestructHotRestartDisabled) {
   if (!Envoy::TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion::v4)) {
     return;
   }
-  std::string config_file = Envoy::TestEnvironment::getCheckedEnvVar("TEST_RUNDIR") +
-                            "/test/config/integration/google_com_proxy_port_0.v2.yaml";
-  const char* argv[] = {"envoy-static", "-c", config_file.c_str(), nullptr};
-  MainCommon main_common(3, const_cast<char**>(argv), false);
+  addConfig();
+  addArg("--disable-hot-restart");
+  VERBOSE_EXPECT_NO_THROW(MainCommon main_common(argc(), argv()));
 }
 
-TEST(MainCommon, LegacyMain) {
+// Ensurees that existing users of main_common() can link.
+TEST_F(MainCommonTest, LegacyMain) {
   if (!Envoy::TestEnvironment::shouldRunTestForIpVersion(Network::Address::IpVersion::v4)) {
     return;
   }
-  // Testing the legacy path is difficult because if we give it a valid config, it will
-  // never exit. So just give it an empty config and let it fail.
-  int argc = 1;
-  const char* argv[] = {"envoy_static", nullptr};
 
 #ifdef ENVOY_HANDLE_SIGNALS
   // Enabled by default. Control with "bazel --define=signal_trace=disabled"
   Envoy::SignalAction handle_sigs;
 #endif
 
-#ifdef ENVOY_HOT_RESTART
-  // Enabled by default, except on OS X. Control with "bazel --define=hot_restart=disabled"
-  const Envoy::OptionsImpl::HotRestartVersionCb hot_restart_version_cb =
-      [](uint64_t max_num_stats, uint64_t max_stat_name_len) {
-        return Envoy::Server::HotRestartImpl::hotRestartVersion(max_num_stats, max_stat_name_len);
-      };
-#else
-  const Envoy::OptionsImpl::HotRestartVersionCb hot_restart_version_cb = [](uint64_t, uint64_t) {
-    return "disabled";
-  };
-#endif
-
   std::unique_ptr<Envoy::OptionsImpl> options;
   int return_code = -1;
   try {
-    options = std::make_unique<Envoy::OptionsImpl>(argc, const_cast<char**>(argv),
-                                                   hot_restart_version_cb, spdlog::level::info);
+    options = std::make_unique<Envoy::OptionsImpl>(argc(), argv(), &MainCommon::hotRestartVersion,
+                                                   spdlog::level::info);
   } catch (const Envoy::NoServingException& e) {
     return_code = EXIT_SUCCESS;
   } catch (const Envoy::MalformedArgvException& e) {
     return_code = EXIT_FAILURE;
   }
   if (return_code == -1) {
+    // Note that Envoy::main_common() will run an event loop if properly configured, which
+    // would hang the test. This is why we don't supply a config file in this testcase;
+    // we just want to make sure we wake up this code in a test.
     return_code = Envoy::main_common(*options);
   }
   EXPECT_EQ(EXIT_FAILURE, return_code);

diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh
@@ -247,4 +247,8 @@ fi
 enableHeapCheck
 set -e
 
+start_test disabling hot_restart by command line.
+CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --disable-hot-restart 2>&1)
+check [ "disabled" = "${CLI_HOT_RESTART_VERSION}" ]
+
 echo "PASS"
diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc
@@ -27,7 +27,7 @@ std::unique_ptr<OptionsImpl> createOptionsImpl(const std::string& args) {
     argv.push_back(s.c_str());
   }
   return std::unique_ptr<OptionsImpl>(new OptionsImpl(argv.size(), const_cast<char**>(&argv[0]),
-                                                      [](uint64_t, uint64_t) { return "1"; },
+                                                      [](uint64_t, uint64_t, bool) { return "1"; },
                                                       spdlog::level::warn));
 }
 

diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc
@@ -87,8 +87,8 @@ std::vector<Network::Address::IpVersion> TestEnvironment::getIpVersionsForTest()
 }
 
 Server::Options& TestEnvironment::getOptions() {
-  static OptionsImpl* options =
-      new OptionsImpl(argc_, argv_, [](uint64_t, uint64_t) { return "1"; }, spdlog::level::err);
+  static OptionsImpl* options = new OptionsImpl(
+      argc_, argv_, [](uint64_t, uint64_t, bool) { return "1"; }, spdlog::level::err);
   return *options;
 }