diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 1d6698e72c958..dbd14cff1de97 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -17,6 +17,7 @@ envoy_cc_library( "//include/envoy/server:options_interface", "//include/envoy/stats:stats_interface", "//source/common/common:assert_lib", + "//source/common/common:block_memory_hash_set_lib", "//source/common/common:hash_lib", "//source/common/common:perf_annotation_lib", "//source/common/common:utility_lib", diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 03b88d6b625d1..df03d373b43bf 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -35,6 +35,15 @@ bool regexStartsWithDot(absl::string_view regex) { } // namespace +BlockMemoryHashSetOptions blockMemHashOptions(uint64_t max_stats) { + BlockMemoryHashSetOptions hash_set_options; + hash_set_options.capacity = max_stats; + + // https://stackoverflow.com/questions/3980117/hash-table-why-size-should-be-prime + hash_set_options.num_slots = Primes::findPrimeLargerThan(hash_set_options.capacity / 2); + return hash_set_options; +} + size_t RawStatData::size() { // Normally the compiler would do this, but because name_ is a flexible-array-length // element, the compiler can't. RawStatData is put into an array in HotRestartImpl, so @@ -149,6 +158,40 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector return false; } +RawStatData* BlockRawStatDataAllocator::alloc(const std::string& name) { + std::unique_lock lock(stat_lock_); + + absl::string_view key = name; + if (key.size() > Stats::RawStatData::maxNameLength()) { + key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); + } + auto value_created = stats_set_->insert(key); + Stats::RawStatData* data = value_created.first; + if (data == nullptr) { + return nullptr; + } + // For new entries (value-created.second==true), BlockMemoryHashSet calls Value::initialize() + // automatically, but on recycled entries (value-created.second==false) we need to bump the + // ref-count. + if (!value_created.second) { + ++data->ref_count_; + } + return data; +} + +void BlockRawStatDataAllocator::free(RawStatData& data) { + // We must hold the lock since the reference decrement can race with an initialize above. + std::unique_lock lock(stat_lock_); + + ASSERT(data.ref_count_ > 0); + if (--data.ref_count_ > 0) { + return; + } + bool key_removed = stats_set_->remove(data.key()); + ASSERT(key_removed); + memset(&data, 0, Stats::RawStatData::size()); +} + RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { // This must be zero-initialized RawStatData* data = static_cast(::calloc(RawStatData::size(), 1)); @@ -156,6 +199,12 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { return data; } +void HeapRawStatDataAllocator::free(RawStatData& data) { + // This allocator does not ever have concurrent access to the raw data. + ASSERT(data.ref_count_ == 1); + ::free(&data); +} + TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v2::StatsConfig& config) : TagProducerImpl() { // To check name conflict. @@ -255,12 +304,6 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon return names; } -void HeapRawStatDataAllocator::free(RawStatData& data) { - // This allocator does not ever have concurrent access to the raw data. - ASSERT(data.ref_count_ == 1); - ::free(&data); -} - void RawStatData::initialize(absl::string_view key) { ASSERT(!initialized()); ASSERT(key.size() <= maxNameLength()); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index f900a46c98e3b..c2d1bfbbd2b96 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -17,6 +17,7 @@ #include "envoy/stats/stats.h" #include "common/common/assert.h" +#include "common/common/block_memory_hash_set.h" #include "common/common/hash.h" #include "common/common/utility.h" #include "common/protobuf/protobuf.h" @@ -26,6 +27,10 @@ namespace Envoy { namespace Stats { +typedef BlockMemoryHashSet RawStatDataSet; + +BlockMemoryHashSetOptions blockMemHashOptions(uint64_t max_stats); + class TagExtractorImpl : public TagExtractor { public: /** @@ -370,12 +375,43 @@ class HistogramImpl : public Histogram, public MetricImpl { }; /** - * Implementation of RawStatDataAllocator that just allocates a new structure in memory and returns - * it. + * Implementation of RawStatDataAllocator which accepts a block of (potentially shared) memory. + */ +class BlockRawStatDataAllocator : public RawStatDataAllocator { +public: + /** + * Creates a RawStatDataAllocator with an underlying MemoryHashMap to track stat names. Optionally + * works with shared memory, as in HotRestartImpl. Thread-safe but requires a lock as argument. + * @param options initialization parameters for BlockMemoryHashSet + * @param init true if the shared-memory should be initialized on construction. See the + * BlockMemoryHashSet constructor. + * @param memory the memory buffer for the set data. + * @param stat_lock a lock for thread safety. + * @return BlockRawStatDataAllocator newly constructed BlockRawStatDataAllocator. + */ + BlockRawStatDataAllocator(const BlockMemoryHashSetOptions& options, bool init, uint8_t* memory, + Thread::BasicLockable& stat_lock) + : stat_lock_(stat_lock) { + stats_set_ = std::make_unique(options, init, memory); + } + + RawStatData* alloc(const std::string& name); + void free(RawStatData& data); + std::string version() { return stats_set_->version(); } + +private: + std::unique_ptr stats_set_; + // We must hold the stat lock when attaching to an existing memory segment + // because it might be actively written to while we sanityCheck it. + Thread::BasicLockable& stat_lock_; +}; + +/** + * Heap-based implementation of RawStatDataAllocator that just allocates a new structure in memory + * and returns it. */ class HeapRawStatDataAllocator : public RawStatDataAllocator { public: - // RawStatDataAllocator RawStatData* alloc(const std::string& name) override; void free(RawStatData& data) override; }; diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 0fb9c7fa228bb..5c49ba2f41fee 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -53,7 +53,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { } #endif if (restarter_.get() == nullptr) { - restarter_.reset(new Server::HotRestartNopImpl()); + restarter_.reset(new Server::HotRestartNopImpl(options_)); } tls_.reset(new ThreadLocal::InstanceImpl); @@ -69,7 +69,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { break; } case Server::Mode::Validate: - restarter_.reset(new Server::HotRestartNopImpl()); + restarter_.reset(new Server::HotRestartNopImpl(options_)); Logger::Registry::initialize(options_.logLevel(), options_.logFormat(), restarter_->logLock()); break; } diff --git a/source/extensions/filters/http/dynamo/dynamo_utility.cc b/source/extensions/filters/http/dynamo/dynamo_utility.cc index ceaa4085aafa1..09cdca1c69dd9 100644 --- a/source/extensions/filters/http/dynamo/dynamo_utility.cc +++ b/source/extensions/filters/http/dynamo/dynamo_utility.cc @@ -9,6 +9,8 @@ namespace Extensions { namespace HttpFilters { namespace Dynamo { +uint64_t Utility::exampleMaxNameLength() { return 127; } + std::string Utility::buildPartitionStatString(const std::string& stat_prefix, const std::string& table_name, const std::string& operation, @@ -19,7 +21,7 @@ std::string Utility::buildPartitionStatString(const std::string& stat_prefix, partition_id.substr(partition_id.size() - 7, partition_id.size())); // Calculate how many characters are available for the table prefix. - size_t remaining_size = Stats::RawStatData::maxNameLength() - stats_partition_postfix.size(); + size_t remaining_size = Utility::exampleMaxNameLength() - stats_partition_postfix.size(); std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); // Truncate the table prefix if the current string is too large. diff --git a/source/extensions/filters/http/dynamo/dynamo_utility.h b/source/extensions/filters/http/dynamo/dynamo_utility.h index 87b4bd0bc119f..7a8ed89569b5b 100644 --- a/source/extensions/filters/http/dynamo/dynamo_utility.h +++ b/source/extensions/filters/http/dynamo/dynamo_utility.h @@ -9,6 +9,15 @@ namespace Dynamo { class Utility { public: + // returns example max name length, 127. This number is easily mutable in the + // outside world, and depending on it makes this test fragile. Thus, the examples + // in dynamo_utility_test are hard-coded to make sure + // `locations-sandbox-partition-test-iad-mytest-really-long-name` + // becomes + // `locations-sandbox-partition-test-iad-mytest-rea` + + static uint64_t exampleMaxNameLength(); + /** * Creates the partition id stats string. * The stats format is diff --git a/source/server/BUILD b/source/server/BUILD index 52225fe97677d..7929a5c3e5b96 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -128,6 +128,7 @@ envoy_cc_library( hdrs = ["hot_restart_nop_impl.h"], deps = [ "//include/envoy/server:hot_restart_interface", + "//source/server:hot_restart_lib", ], ) diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 6e81f7de1ab77..3ca8093cd2e49 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -17,6 +17,7 @@ #include "common/common/fmt.h" #include "common/common/utility.h" #include "common/network/utility.h" +#include "common/stats/stats_impl.h" #include "absl/strings/string_view.h" @@ -27,15 +28,6 @@ namespace Server { // from working. Operations code can then cope with this and do a full restart. const uint64_t SharedMemory::VERSION = 9; -static BlockMemoryHashSetOptions blockMemHashOptions(uint64_t max_stats) { - BlockMemoryHashSetOptions hash_set_options; - hash_set_options.capacity = max_stats; - - // https://stackoverflow.com/questions/3980117/hash-table-why-size-should-be-prime - hash_set_options.num_slots = Primes::findPrimeLargerThan(hash_set_options.capacity / 2); - return hash_set_options; -} - SharedMemory& SharedMemory::initialize(uint32_t stats_set_size, Options& options) { Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); @@ -84,8 +76,8 @@ SharedMemory& SharedMemory::initialize(uint32_t stats_set_size, Options& options } // Stats::RawStatData must be naturally aligned for atomics to work properly. - RELEASE_ASSERT((reinterpret_cast(shmem->stats_set_data_) % alignof(RawStatDataSet)) == - 0); + RELEASE_ASSERT( + (reinterpret_cast(shmem->stats_set_data_) % alignof(Stats::RawStatDataSet)) == 0); // Here we catch the case where a new Envoy starts up when the current Envoy has not yet fully // initialized. The startup logic is quite complicated, and it's not worth trying to handle this @@ -114,16 +106,14 @@ std::string SharedMemory::version(size_t max_num_stats, size_t max_stat_name_len } HotRestartImpl::HotRestartImpl(Options& options) - : options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), - shmem_(SharedMemory::initialize(RawStatDataSet::numBytes(stats_set_options_), options)), + : options_(options), stats_set_options_(Stats::blockMemHashOptions(options.maxStats())), + shmem_( + SharedMemory::initialize(Stats::RawStatDataSet::numBytes(stats_set_options_), options)), log_lock_(shmem_.log_lock_), access_log_lock_(shmem_.access_log_lock_), stat_lock_(shmem_.stat_lock_), init_lock_(shmem_.init_lock_) { { - // We must hold the stat lock when attaching to an existing memory segment - // because it might be actively written to while we sanityCheck it. - std::unique_lock lock(stat_lock_); - stats_set_.reset(new RawStatDataSet(stats_set_options_, options.restartEpoch() == 0, - shmem_.stats_set_data_)); + stats_allocator_ = std::make_unique( + stats_set_options_, options.restartEpoch() == 0, shmem_.stats_set_data_, stat_lock_); } my_domain_socket_ = bindDomainSocket(options.restartEpoch()); child_address_ = createDomainSocketAddress((options.restartEpoch() + 1)); @@ -138,39 +128,6 @@ HotRestartImpl::HotRestartImpl(Options& options) RELEASE_ASSERT(rc != -1); } -Stats::RawStatData* HotRestartImpl::alloc(const std::string& name) { - // Try to find the existing slot in shared memory, otherwise allocate a new one. - std::unique_lock lock(stat_lock_); - absl::string_view key = name; - if (key.size() > Stats::RawStatData::maxNameLength()) { - key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); - } - auto value_created = stats_set_->insert(key); - Stats::RawStatData* data = value_created.first; - if (data == nullptr) { - return nullptr; - } - // For new entries (value-created.second==true), BlockMemoryHashSet calls Value::initialize() - // automatically, but on recycled entries (value-created.second==false) we need to bump the - // ref-count. - if (!value_created.second) { - ++data->ref_count_; - } - return data; -} - -void HotRestartImpl::free(Stats::RawStatData& data) { - // We must hold the lock since the reference decrement can race with an initialize above. - std::unique_lock lock(stat_lock_); - ASSERT(data.ref_count_ > 0); - if (--data.ref_count_ > 0) { - return; - } - bool key_removed = stats_set_->remove(data.key()); - ASSERT(key_removed); - memset(&data, 0, Stats::RawStatData::size()); -} - int HotRestartImpl::bindDomainSocket(uint64_t id) { Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); // This actually creates the socket and binds it. We use the socket in datagram mode so we can @@ -472,23 +429,25 @@ void HotRestartImpl::terminateParent() { void HotRestartImpl::shutdown() { socket_event_.reset(); } std::string HotRestartImpl::version() { - return versionHelper(shmem_.maxStats(), Stats::RawStatData::maxNameLength(), *stats_set_); + return versionHelper(shmem_.maxStats(), Stats::RawStatData::maxNameLength(), + stats_allocator_->version()); } // Called from envoy --hot-restart-version -- needs to instantiate a RawStatDataSet so it // can generate the version string. std::string HotRestartImpl::hotRestartVersion(size_t max_num_stats, size_t max_stat_name_len) { - const BlockMemoryHashSetOptions options = blockMemHashOptions(max_num_stats); - const size_t bytes = RawStatDataSet::numBytes(options); + const BlockMemoryHashSetOptions options = Stats::blockMemHashOptions(max_num_stats); + const size_t bytes = Stats::RawStatDataSet::numBytes(options); std::unique_ptr mem_buffer_for_dry_run_(new uint8_t[bytes]); - RawStatDataSet stats_set(options, true /* init */, mem_buffer_for_dry_run_.get()); + Stats::RawStatDataSet stats_set(options, true /* init */, mem_buffer_for_dry_run_.get()); + auto stats_set_version = stats_set.version(); - return versionHelper(max_num_stats, max_stat_name_len, stats_set); + return versionHelper(max_num_stats, max_stat_name_len, stats_set_version); } std::string HotRestartImpl::versionHelper(size_t max_num_stats, size_t max_stat_name_len, - RawStatDataSet& stats_set) { - return SharedMemory::version(max_num_stats, max_stat_name_len) + "." + stats_set.version(); + std::string stats_set_version) { + return SharedMemory::version(max_num_stats, max_stat_name_len) + "." + stats_set_version; } } // namespace Server diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index eaa57dff75f54..7e1a4035b9829 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -18,8 +18,6 @@ namespace Envoy { namespace Server { -typedef BlockMemoryHashSet RawStatDataSet; - /** * Shared memory segment. This structure is laid directly into shared memory and is used amongst * all running envoy processes. @@ -113,9 +111,7 @@ class ProcessSharedMutex : public Thread::BasicLockable { /** * Implementation of HotRestart built for Linux. */ -class HotRestartImpl : public HotRestart, - public Stats::RawStatDataAllocator, - Logger::Loggable { +class HotRestartImpl : public HotRestart, Logger::Loggable { public: HotRestartImpl(Options& options); @@ -130,7 +126,7 @@ class HotRestartImpl : public HotRestart, std::string version() override; Thread::BasicLockable& logLock() override { return log_lock_; } Thread::BasicLockable& accessLogLock() override { return access_log_lock_; } - Stats::RawStatDataAllocator& statsAllocator() override { return *this; } + Stats::RawStatDataAllocator& statsAllocator() override { return *stats_allocator_; } /** * envoy --hot_restart_version doesn't initialize Envoy, but computes the version string @@ -138,10 +134,6 @@ class HotRestartImpl : public HotRestart, */ static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len); - // RawStatDataAllocator - Stats::RawStatData* alloc(const std::string& name) override; - void free(Stats::RawStatData& data) override; - private: enum class RpcMessageType { DrainListenersRequest = 1, @@ -204,12 +196,12 @@ class HotRestartImpl : public HotRestart, RpcBase* receiveRpc(bool block); void sendMessage(sockaddr_un& address, RpcBase& rpc); static std::string versionHelper(uint64_t max_num_stats, uint64_t max_stat_name_len, - RawStatDataSet& stats_set); + std::string stats_set_version); Options& options_; BlockMemoryHashSetOptions stats_set_options_; SharedMemory& shmem_; - std::unique_ptr stats_set_; + std::unique_ptr stats_allocator_; ProcessSharedMutex log_lock_; ProcessSharedMutex access_log_lock_; ProcessSharedMutex stat_lock_; diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index 73568945fdb93..d3b86bcdf01fd 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -5,6 +5,7 @@ #include "envoy/server/hot_restart.h" #include "common/common/thread.h" +#include "common/stats/stats_impl.h" namespace Envoy { namespace Server { @@ -14,7 +15,14 @@ namespace Server { */ class HotRestartNopImpl : public Server::HotRestart { public: - HotRestartNopImpl() {} + HotRestartNopImpl(Options& options) + : stats_set_options_(Stats::blockMemHashOptions(options.maxStats())) { + uint32_t num_bytes = BlockMemoryHashSet::numBytes(stats_set_options_); + memory_.reset(new uint8_t[num_bytes]); + memset(memory_.get(), 0, num_bytes); + stats_allocator_ = std::make_unique( + stats_set_options_, true, memory_.get(), stat_lock_); + } // Server::HotRestart void drainParentListeners() override {} @@ -27,12 +35,15 @@ class HotRestartNopImpl : public Server::HotRestart { std::string version() override { return "disabled"; } Thread::BasicLockable& logLock() override { return log_lock_; } Thread::BasicLockable& accessLogLock() override { return access_log_lock_; } - Stats::RawStatDataAllocator& statsAllocator() override { return stats_allocator_; } + Stats::RawStatDataAllocator& statsAllocator() override { return *stats_allocator_; } private: + BlockMemoryHashSetOptions stats_set_options_; + std::unique_ptr memory_; Thread::MutexBasicLockable log_lock_; + Thread::MutexBasicLockable stat_lock_; Thread::MutexBasicLockable access_log_lock_; - Stats::HeapRawStatDataAllocator stats_allocator_; + std::unique_ptr stats_allocator_; }; } // namespace Server diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index bbd3600d5526e..887a0123263fb 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -13,11 +13,6 @@ #include "spdlog/spdlog.h" #include "tclap/CmdLine.h" -// Can be overridden at compile time -#ifndef ENVOY_DEFAULT_MAX_STATS -#define ENVOY_DEFAULT_MAX_STATS 16384 -#endif - // Can be overridden at compile time // See comment in common/stat/stat_impl.h for rationale behind // this constant. diff --git a/source/server/options_impl.h b/source/server/options_impl.h index 3c8b63decafbe..0d86bbfcf2020 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -9,6 +9,11 @@ #include "spdlog/spdlog.h" +// Can be overridden at compile time +#ifndef ENVOY_DEFAULT_MAX_STATS +#define ENVOY_DEFAULT_MAX_STATS 16384 +#endif + namespace Envoy { /** * Implementation of Server::Options. diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index ce7cdf1052a18..de14e74f5488c 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -13,6 +13,8 @@ envoy_cc_test( srcs = ["stats_impl_test.cc"], deps = [ "//source/common/stats:stats_lib", + "//source/server:options_lib", + "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", "@envoy_api//envoy/config/metrics/v2:stats_cc", ], @@ -22,8 +24,10 @@ envoy_cc_test( name = "thread_local_store_test", srcs = ["thread_local_store_test.cc"], deps = [ + "//source/common/stats:stats_lib", "//source/common/stats:thread_local_store_lib", "//test/mocks/event:event_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/stats:stats_mocks", "//test/mocks/thread_local:thread_local_mocks", "//test/test_common:utility_lib", diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index 217e4569c6de2..8612bbc557daf 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -8,10 +8,15 @@ #include "common/config/well_known_names.h" #include "common/stats/stats_impl.h" +#include "server/options_impl.h" + +#include "test/mocks/server/mocks.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" +using testing::Return; + namespace Envoy { namespace Stats { @@ -468,5 +473,156 @@ TEST(TagProducerTest, CheckConstructor) { "No regex specified for tag specifier and no default regex for name: 'test_extractor'"); } +class BlockRawStatDataAllocatorTest : public testing::Test { +public: + void SetUp() override { + Stats::RawStatData::configureForTestsOnly(options_); + BlockMemoryHashSetOptions memory_hash_set_options_ = + Stats::blockMemHashOptions(options_.maxStats()); + uint32_t num_bytes = BlockMemoryHashSet::numBytes(memory_hash_set_options_); + memory_.reset(new uint8_t[num_bytes]); + memset(memory_.get(), 0, num_bytes); + allocator_ = std::make_unique(memory_hash_set_options_, true, + memory_.get(), stat_lock_); + } + + void TearDown() override { + // Configure it back so that later tests don't get the wonky values + // used here + NiceMock default_options; + Stats::RawStatData::configureForTestsOnly(default_options); + } + + std::unique_ptr memory_; + std::unique_ptr allocator_; + NiceMock options_; + Thread::MutexBasicLockable stat_lock_; +}; + +TEST_F(BlockRawStatDataAllocatorTest, truncateKey) { + std::string key1(Stats::RawStatData::maxNameLength(), 'a'); + Stats::RawStatData* stat1 = allocator_->alloc(key1); + std::string key2 = key1 + "a"; + Stats::RawStatData* stat2 = allocator_->alloc(key2); + EXPECT_EQ(stat1, stat2); +} + +TEST_F(BlockRawStatDataAllocatorTest, uniqueNaming) { + + auto foo_1 = allocator_->alloc("foo"); + auto foo_2 = allocator_->alloc("foo"); + EXPECT_EQ(foo_1, foo_2); + auto bar_1 = allocator_->alloc("bar"); + EXPECT_NE(foo_1, bar_1); + EXPECT_NE(foo_2, bar_1); +} + +TEST_F(BlockRawStatDataAllocatorTest, allocFail) { + EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(2)); + BlockRawStatDataAllocatorTest::SetUp(); + + Stats::RawStatData* s1 = allocator_->alloc("1"); + Stats::RawStatData* s2 = allocator_->alloc("2"); + Stats::RawStatData* s3 = allocator_->alloc("3"); + EXPECT_NE(s1, nullptr); + EXPECT_NE(s2, nullptr); + EXPECT_EQ(s3, nullptr); +} + +// Because the block memory is managed manually, make sure it meets +// basic requirements: +// - Objects are correctly aligned so that std::atomic works properly +// - Objects don't overlap +class BlockRawStatDataAllocatorAlignmentTest : public BlockRawStatDataAllocatorTest, + public testing::WithParamInterface { +public: + BlockRawStatDataAllocatorAlignmentTest() : name_len_(8 + GetParam()) {} + + void SetUp() override { + EXPECT_CALL(options_, maxObjNameLength()).WillRepeatedly(Return(name_len_)); + BlockRawStatDataAllocatorTest::SetUp(); + EXPECT_EQ(name_len_, Stats::RawStatData::maxObjNameLength()); + } + + static const uint64_t num_stats_ = 8; + const uint64_t name_len_; +}; + +TEST_P(BlockRawStatDataAllocatorAlignmentTest, objectAlignment) { + std::set used; + for (uint64_t i = 0; i < num_stats_; i++) { + Stats::RawStatData* stat = allocator_->alloc(fmt::format("stat {}", i)); + EXPECT_TRUE((reinterpret_cast(stat) % alignof(decltype(*stat))) == 0); + EXPECT_TRUE(used.find(stat) == used.end()); + used.insert(stat); + } +} + +TEST_P(BlockRawStatDataAllocatorAlignmentTest, objectOverlap) { + // Iterate through all stats forwards and backwards, writing to all fields, then read them back to + // make sure that writing to an adjacent stat didn't overwrite + struct TestStat { + Stats::RawStatData* stat_; + std::string name_; + uint64_t index_; + }; + std::vector stats; + for (uint64_t i = 0; i < num_stats_; i++) { + std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + i) + .substr(0, Stats::RawStatData::maxNameLength()); + TestStat ts; + ts.stat_ = allocator_->alloc(name); + ts.name_ = ts.stat_->name_; + ts.index_ = i; + + // If this isn't true then the hard coded part of the name isn't long enough to make the test + // valid. + EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxNameLength()); + + stats.push_back(ts); + } + + auto write = [](TestStat& ts) { + ts.stat_->value_ = ts.index_; + ts.stat_->pending_increment_ = ts.index_; + ts.stat_->flags_ = ts.index_; + ts.stat_->ref_count_ = ts.index_; + ts.stat_->unused_ = ts.index_; + }; + + auto verify = [](TestStat& ts) { + EXPECT_EQ(ts.stat_->key(), ts.name_); + EXPECT_EQ(ts.stat_->value_, ts.index_); + EXPECT_EQ(ts.stat_->pending_increment_, ts.index_); + EXPECT_EQ(ts.stat_->flags_, ts.index_); + EXPECT_EQ(ts.stat_->ref_count_, ts.index_); + EXPECT_EQ(ts.stat_->unused_, ts.index_); + }; + + for (TestStat& ts : stats) { + write(ts); + } + + for (TestStat& ts : stats) { + verify(ts); + } + + for (auto it = stats.rbegin(); it != stats.rend(); ++it) { + write(*it); + } + + for (auto it = stats.rbegin(); it != stats.rend(); ++it) { + verify(*it); + } +} + +INSTANTIATE_TEST_CASE_P(BlockRawStatDataAllocatorAlignmentTest, + BlockRawStatDataAllocatorAlignmentTest, + testing::Range(std::size_t{0}, + alignof(Stats::RawStatData) + std::size_t{1})); + } // namespace Stats } // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 9b352c236d48d..fc686b47b01ff 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -4,9 +4,11 @@ #include #include "common/common/c_smart_ptr.h" +#include "common/stats/stats_impl.h" #include "common/stats/thread_local_store.h" #include "test/mocks/event/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/mocks/thread_local/mocks.h" #include "test/test_common/utility.h" @@ -24,55 +26,24 @@ using testing::_; namespace Envoy { namespace Stats { -/** - * This is a heap test allocator that works similar to how the shared memory allocator works in - * terms of reference counting, etc. - */ -class TestAllocator : public RawStatDataAllocator { -public: - ~TestAllocator() { EXPECT_TRUE(stats_.empty()); } - - RawStatData* alloc(const std::string& name) override { - CSmartPtr& stat_ref = stats_[name]; - if (!stat_ref) { - stat_ref.reset(static_cast(::calloc(RawStatData::size(), 1))); - stat_ref->initialize(name); - } else { - stat_ref->ref_count_++; - } - - return stat_ref.get(); - } - - void free(RawStatData& data) override { - if (--data.ref_count_ > 0) { - return; - } - - for (auto i = stats_.begin(); i != stats_.end(); i++) { - if (i->second.get() == &data) { - stats_.erase(i); - return; - } - } - - FAIL(); - } - -private: - static void freeAdapter(RawStatData* data) { ::free(data); } - std::unordered_map> stats_; -}; - class StatsThreadLocalStoreTest : public testing::Test, public RawStatDataAllocator { public: StatsThreadLocalStoreTest() { + Stats::RawStatData::configureForTestsOnly(options_); + BlockMemoryHashSetOptions memory_hash_set_options_ = + Stats::blockMemHashOptions(options_.maxStats()); + uint32_t num_bytes = BlockMemoryHashSet::numBytes(memory_hash_set_options_); + memory_.reset(new uint8_t[num_bytes]); + memset(memory_.get(), 0, num_bytes); + allocator_ = std::make_unique(memory_hash_set_options_, true, + memory_.get(), stat_lock_); + ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](const std::string& name) -> RawStatData* { - return alloc_.alloc(name); + return allocator_->alloc(name); })); ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { - return alloc_.free(data); + return allocator_->free(data); })); EXPECT_CALL(*this, alloc("stats.overflow")); @@ -83,9 +54,12 @@ class StatsThreadLocalStoreTest : public testing::Test, public RawStatDataAlloca MOCK_METHOD1(alloc, RawStatData*(const std::string& name)); MOCK_METHOD1(free, void(RawStatData& data)); + std::unique_ptr memory_; + std::unique_ptr allocator_; + NiceMock options_; + Thread::MutexBasicLockable stat_lock_; NiceMock main_thread_dispatcher_; NiceMock tls_; - TestAllocator alloc_; MockSink sink_; std::unique_ptr store_; }; diff --git a/test/extensions/filters/http/dynamo/dynamo_utility_test.cc b/test/extensions/filters/http/dynamo/dynamo_utility_test.cc index 2661e1819a321..ebf1b8a4cc7d9 100644 --- a/test/extensions/filters/http/dynamo/dynamo_utility_test.cc +++ b/test/extensions/filters/http/dynamo/dynamo_utility_test.cc @@ -25,7 +25,7 @@ TEST(DynamoUtility, PartitionIdStatString) { std::string expected_stat_string = "stat.prefix.table.locations.capacity.GetItem.__partition_id=c5883ca"; EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() <= Stats::RawStatData::maxNameLength()); + EXPECT_TRUE(partition_stat_string.size() <= Utility::exampleMaxNameLength()); } { @@ -40,7 +40,7 @@ TEST(DynamoUtility, PartitionIdStatString) { "partition-test-iad-mytest-rea.capacity.GetItem.__partition_" "id=c5883ca"; EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() == Stats::RawStatData::maxNameLength()); + EXPECT_TRUE(partition_stat_string.size() <= Utility::exampleMaxNameLength()); } { std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; @@ -55,7 +55,7 @@ TEST(DynamoUtility, PartitionIdStatString) { "id=c5883ca"; EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() == Stats::RawStatData::maxNameLength()); + EXPECT_TRUE(partition_stat_string.size() <= Utility::exampleMaxNameLength()); } } diff --git a/test/integration/server.cc b/test/integration/server.cc index 0a4220c3a3659..a5303d40631b6 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -84,7 +84,7 @@ void IntegrationTestServer::onWorkerListenerRemoved() { void IntegrationTestServer::threadRoutine(const Network::Address::IpVersion version) { Server::TestOptionsImpl options(config_path_, version); - Server::HotRestartNopImpl restarter; + Server::HotRestartNopImpl restarter(options); Thread::MutexBasicLockable lock; ThreadLocal::InstanceImpl tls; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 83159bf7eb855..8277f943cd278 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -55,6 +55,7 @@ MockGuardDog::~MockGuardDog() {} MockHotRestart::MockHotRestart() { ON_CALL(*this, logLock()).WillByDefault(ReturnRef(log_lock_)); + ON_CALL(*this, statLock()).WillByDefault(ReturnRef(stat_lock_)); ON_CALL(*this, accessLogLock()).WillByDefault(ReturnRef(access_log_lock_)); ON_CALL(*this, statsAllocator()).WillByDefault(ReturnRef(stats_allocator_)); } diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index fa0bdefa778a4..8e60109dead49 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -156,11 +156,13 @@ class MockHotRestart : public HotRestart { MOCK_METHOD0(shutdown, void()); MOCK_METHOD0(version, std::string()); MOCK_METHOD0(logLock, Thread::BasicLockable&()); + MOCK_METHOD0(statLock, Thread::BasicLockable&()); MOCK_METHOD0(accessLogLock, Thread::BasicLockable&()); MOCK_METHOD0(statsAllocator, Stats::RawStatDataAllocator&()); private: Thread::MutexBasicLockable log_lock_; + Thread::MutexBasicLockable stat_lock_; Thread::MutexBasicLockable access_log_lock_; Stats::HeapRawStatDataAllocator stats_allocator_; }; diff --git a/test/server/BUILD b/test/server/BUILD index 1dc049b477b06..b3b6268ef41e5 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -73,6 +73,17 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "hot_restart_nop_impl_test", + srcs = envoy_select_hot_restart(["hot_restart_nop_impl_test.cc"]), + deps = [ + "//source/common/stats:stats_lib", + "//source/server:hot_restart_nop_lib", + "//test/mocks/server:server_mocks", + "//test/test_common:threadsafe_singleton_injector_lib", + ], +) + envoy_cc_test( name = "init_manager_impl_test", srcs = ["init_manager_impl_test.cc"], diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 8fbddadb6bb07..1c4f6602810e3 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -88,13 +88,13 @@ TEST_F(HotRestartImplTest, versionString) { TEST_F(HotRestartImplTest, crossAlloc) { setup(); - Stats::RawStatData* stat1 = hot_restart_->alloc("stat1"); - Stats::RawStatData* stat2 = hot_restart_->alloc("stat2"); - Stats::RawStatData* stat3 = hot_restart_->alloc("stat3"); - Stats::RawStatData* stat4 = hot_restart_->alloc("stat4"); - Stats::RawStatData* stat5 = hot_restart_->alloc("stat5"); - hot_restart_->free(*stat2); - hot_restart_->free(*stat4); + Stats::RawStatData* stat1 = hot_restart_->statsAllocator().alloc("stat1"); + Stats::RawStatData* stat2 = hot_restart_->statsAllocator().alloc("stat2"); + Stats::RawStatData* stat3 = hot_restart_->statsAllocator().alloc("stat3"); + Stats::RawStatData* stat4 = hot_restart_->statsAllocator().alloc("stat4"); + Stats::RawStatData* stat5 = hot_restart_->statsAllocator().alloc("stat5"); + hot_restart_->statsAllocator().free(*stat2); + hot_restart_->statsAllocator().free(*stat4); stat2 = nullptr; stat4 = nullptr; @@ -103,128 +103,13 @@ TEST_F(HotRestartImplTest, crossAlloc) { EXPECT_CALL(os_sys_calls_, mmap(_, _, _, _, _, _)).WillOnce(Return(buffer_.data())); EXPECT_CALL(os_sys_calls_, bind(_, _, _)); HotRestartImpl hot_restart2(options_); - Stats::RawStatData* stat1_prime = hot_restart2.alloc("stat1"); - Stats::RawStatData* stat3_prime = hot_restart2.alloc("stat3"); - Stats::RawStatData* stat5_prime = hot_restart2.alloc("stat5"); + Stats::RawStatData* stat1_prime = hot_restart2.statsAllocator().alloc("stat1"); + Stats::RawStatData* stat3_prime = hot_restart2.statsAllocator().alloc("stat3"); + Stats::RawStatData* stat5_prime = hot_restart2.statsAllocator().alloc("stat5"); EXPECT_EQ(stat1, stat1_prime); EXPECT_EQ(stat3, stat3_prime); EXPECT_EQ(stat5, stat5_prime); } -TEST_F(HotRestartImplTest, truncateKey) { - setup(); - - std::string key1(Stats::RawStatData::maxNameLength(), 'a'); - Stats::RawStatData* stat1 = hot_restart_->alloc(key1); - std::string key2 = key1 + "a"; - Stats::RawStatData* stat2 = hot_restart_->alloc(key2); - EXPECT_EQ(stat1, stat2); -} - -TEST_F(HotRestartImplTest, allocFail) { - EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(2)); - setup(); - - Stats::RawStatData* s1 = hot_restart_->alloc("1"); - Stats::RawStatData* s2 = hot_restart_->alloc("2"); - Stats::RawStatData* s3 = hot_restart_->alloc("3"); - EXPECT_NE(s1, nullptr); - EXPECT_NE(s2, nullptr); - EXPECT_EQ(s3, nullptr); -} - -// Because the shared memory is managed manually, make sure it meets -// basic requirements: -// - Objects are correctly aligned so that std::atomic works properly -// - Objects don't overlap -class HotRestartImplAlignmentTest : public HotRestartImplTest, - public testing::WithParamInterface { -public: - HotRestartImplAlignmentTest() : name_len_(8 + GetParam()) { - EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(num_stats_)); - EXPECT_CALL(options_, maxObjNameLength()).WillRepeatedly(Return(name_len_)); - setup(); - EXPECT_EQ(name_len_, Stats::RawStatData::maxObjNameLength()); - } - - static const uint64_t num_stats_ = 8; - const uint64_t name_len_; -}; - -TEST_P(HotRestartImplAlignmentTest, objectAlignment) { - - std::set used; - for (uint64_t i = 0; i < num_stats_; i++) { - Stats::RawStatData* stat = hot_restart_->alloc(fmt::format("stat {}", i)); - EXPECT_TRUE((reinterpret_cast(stat) % alignof(decltype(*stat))) == 0); - EXPECT_TRUE(used.find(stat) == used.end()); - used.insert(stat); - } -} - -TEST_P(HotRestartImplAlignmentTest, objectOverlap) { - // Iterate through all stats forwards and backwards, writing to all fields, then read them back to - // make sure that writing to an adjacent stat didn't overwrite - struct TestStat { - Stats::RawStatData* stat_; - std::string name_; - uint64_t index_; - }; - std::vector stats; - for (uint64_t i = 0; i < num_stats_; i++) { - std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" - "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" - "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", - i) - .substr(0, Stats::RawStatData::maxNameLength()); - TestStat ts; - ts.stat_ = hot_restart_->alloc(name); - ts.name_ = ts.stat_->name_; - ts.index_ = i; - - // If this isn't true then the hard coded part of the name isn't long enough to make the test - // valid. - EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxNameLength()); - - stats.push_back(ts); - } - - auto write = [](TestStat& ts) { - ts.stat_->value_ = ts.index_; - ts.stat_->pending_increment_ = ts.index_; - ts.stat_->flags_ = ts.index_; - ts.stat_->ref_count_ = ts.index_; - ts.stat_->unused_ = ts.index_; - }; - - auto verify = [](TestStat& ts) { - EXPECT_EQ(ts.stat_->key(), ts.name_); - EXPECT_EQ(ts.stat_->value_, ts.index_); - EXPECT_EQ(ts.stat_->pending_increment_, ts.index_); - EXPECT_EQ(ts.stat_->flags_, ts.index_); - EXPECT_EQ(ts.stat_->ref_count_, ts.index_); - EXPECT_EQ(ts.stat_->unused_, ts.index_); - }; - - for (TestStat& ts : stats) { - write(ts); - } - - for (TestStat& ts : stats) { - verify(ts); - } - - for (auto it = stats.rbegin(); it != stats.rend(); ++it) { - write(*it); - } - - for (auto it = stats.rbegin(); it != stats.rend(); ++it) { - verify(*it); - } -} - -INSTANTIATE_TEST_CASE_P(HotRestartImplAlignmentTest, HotRestartImplAlignmentTest, - testing::Range(0UL, alignof(Stats::RawStatData) + 1)); - } // namespace Server } // namespace Envoy diff --git a/test/server/hot_restart_nop_impl_test.cc b/test/server/hot_restart_nop_impl_test.cc new file mode 100644 index 0000000000000..4bac18d155a88 --- /dev/null +++ b/test/server/hot_restart_nop_impl_test.cc @@ -0,0 +1,66 @@ +#include "common/stats/stats_impl.h" + +#include "server/hot_restart_nop_impl.h" + +#include "test/mocks/api/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/threadsafe_singleton_injector.h" + +#include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "gtest/gtest.h" + +using testing::Invoke; +using testing::InvokeWithoutArgs; +using testing::Return; +using testing::WithArg; +using testing::_; + +namespace Envoy { +namespace Server { + +class HotRestartNopImplTest : public testing::Test { +public: + void setup() { + Stats::RawStatData::configureForTestsOnly(options_); + + // Test we match the correct stat with empty-slots before, after, or both. + hot_restart_nop_.reset(new HotRestartNopImpl(options_)); + hot_restart_nop_->drainParentListeners(); + } + + void TearDown() { + // Configure it back so that later tests don't get the wonky values + // used here + NiceMock default_options; + Stats::RawStatData::configureForTestsOnly(default_options); + } + + NiceMock options_; + std::vector buffer_; + std::unique_ptr hot_restart_nop_; +}; + +TEST_F(HotRestartNopImplTest, sameAlloc) { + setup(); + + Stats::RawStatData* stat1 = hot_restart_nop_->statsAllocator().alloc("stat1"); + Stats::RawStatData* stat2 = hot_restart_nop_->statsAllocator().alloc("stat2"); + Stats::RawStatData* stat3 = hot_restart_nop_->statsAllocator().alloc("stat3"); + Stats::RawStatData* stat4 = hot_restart_nop_->statsAllocator().alloc("stat4"); + Stats::RawStatData* stat5 = hot_restart_nop_->statsAllocator().alloc("stat5"); + hot_restart_nop_->statsAllocator().free(*stat2); + hot_restart_nop_->statsAllocator().free(*stat4); + stat2 = nullptr; + stat4 = nullptr; + + Stats::RawStatData* stat1_prime = hot_restart_nop_->statsAllocator().alloc("stat1"); + Stats::RawStatData* stat3_prime = hot_restart_nop_->statsAllocator().alloc("stat3"); + Stats::RawStatData* stat5_prime = hot_restart_nop_->statsAllocator().alloc("stat5"); + EXPECT_EQ(stat1, stat1_prime); + EXPECT_EQ(stat3, stat3_prime); + EXPECT_EQ(stat5, stat5_prime); +} + +} // namespace Server +} // namespace Envoy