diff --git a/source/common/common/block_memory_hash_set.h b/source/common/common/block_memory_hash_set.h index 1295bf51750c..6677e8a6d37a 100644 --- a/source/common/common/block_memory_hash_set.h +++ b/source/common/common/block_memory_hash_set.h @@ -216,8 +216,8 @@ template class BlockMemoryHashSet : public Logger::Loggableoptions.toString(), - control_->hash_signature); + return fmt::format("options={} hash={} size={}", control_->options.toString(), + control_->hash_signature, numBytes()); } private: diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 29ca31b3b531..e841c9183244 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -26,7 +26,7 @@ namespace Server { // Increment this whenever there is a shared memory / RPC change that will prevent a hot restart // from working. Operations code can then cope with this and do a full restart. -const uint64_t SharedMemory::VERSION = 9; +const uint64_t SharedMemory::VERSION = 10; static BlockMemoryHashSetOptions blockMemHashOptions(uint64_t max_stats) { BlockMemoryHashSetOptions hash_set_options; diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 5aa1997754f6..497dd31c2121 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -139,13 +139,6 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, // TODO(jmarantz): should we also multiply these to bound the total amount of memory? 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(), - !hot_restart_disabled_); - throw NoServingException(); - } log_level_ = default_log_level; for (size_t i = 0; i < ARRAY_SIZE(spdlog::level::level_names); i++) { @@ -196,5 +189,14 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, 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(); + + if (hot_restart_version_option.getValue()) { + Stats::RawStatData::configure(*this); + std::cerr << hot_restart_version_cb(max_stats.getValue(), + max_obj_name_len.getValue() + + Stats::RawStatData::maxStatSuffixLength(), + !hot_restart_disabled_); + throw NoServingException(); + } } } // namespace Envoy diff --git a/test/server/BUILD b/test/server/BUILD index d035d27990c3..5a7c7d9a26cf 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -103,6 +103,7 @@ envoy_cc_test( srcs = ["options_impl_test.cc"], deps = [ "//source/common/common:utility_lib", + "//source/common/stats:stats_lib", "//source/server:options_lib", "//test/test_common:utility_lib", ], diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 8fbddadb6bb0..bda4509117b8 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -59,7 +59,7 @@ TEST_F(HotRestartImplTest, versionString) { // Tests that the version-string will be consistent and SharedMemory::VERSION, // between multiple instantiations. std::string version; - uint64_t max_stats; + uint64_t max_stats, max_obj_name_length; // The mocking infrastructure requires a test setup and teardown every time we // want to re-instantiate HotRestartImpl. @@ -68,6 +68,7 @@ TEST_F(HotRestartImplTest, versionString) { version = hot_restart_->version(); EXPECT_TRUE(absl::StartsWith(version, fmt::format("{}.", SharedMemory::VERSION))) << version; max_stats = options_.maxStats(); // Save this so we can double it below. + max_obj_name_length = options_.maxObjNameLength(); TearDown(); } @@ -80,7 +81,15 @@ TEST_F(HotRestartImplTest, versionString) { { ON_CALL(options_, maxStats()).WillByDefault(Return(2 * max_stats)); setup(); - EXPECT_NE(version, hot_restart_->version()) << "Version changes when options change"; + EXPECT_NE(version, hot_restart_->version()) << "Version changes when max-stats change"; + TearDown(); + } + + { + ON_CALL(options_, maxObjNameLength()).WillByDefault(Return(2 * max_obj_name_length)); + setup(); + EXPECT_NE(version, hot_restart_->version()) + << "Version changes when max-obj-name-length changes"; // TearDown is called automatically at end of test. } } diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index 3327fce29431..26281847534a 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -6,6 +6,7 @@ #include "envoy/common/exception.h" #include "common/common/utility.h" +#include "common/stats/stats_impl.h" #include "server/options_impl.h" @@ -19,6 +20,8 @@ using testing::HasSubstr; namespace Envoy { +namespace { + // Do the ugly work of turning a std::string into a char** and create an OptionsImpl. Args are // separated by a single space: no fancy quoting or escaping. std::unique_ptr createOptionsImpl(const std::string& args) { @@ -27,12 +30,23 @@ std::unique_ptr createOptionsImpl(const std::string& args) { for (const std::string& s : words) { argv.push_back(s.c_str()); } - return std::unique_ptr(new OptionsImpl(argv.size(), const_cast(&argv[0]), - [](uint64_t, uint64_t, bool) { return "1"; }, - spdlog::level::warn)); + return std::make_unique( + argv.size(), argv.data(), [](uint64_t, uint64_t, bool) { return "1"; }, spdlog::level::warn); } +} // namespace + TEST(OptionsImplTest, HotRestartVersion) { + // There's an evil static local in + // Stats::RawStatsData::initializeAndGetMutableMaxObjNameLength, which causes + // problems when all test.cc files are linked together for coverage-testing. + // This resets the static to the default options-value of 60. Note; this is only + // needed in coverage tests. + { + auto options = createOptionsImpl("envoy"); + Stats::RawStatData::configureForTestsOnly(*options); + } + EXPECT_THROW_WITH_REGEX(createOptionsImpl("envoy --hot-restart-version"), NoServingException, "NoServingException"); }