diff --git a/api/envoy/admin/v2alpha/server_info.proto b/api/envoy/admin/v2alpha/server_info.proto index 13dea7ae8eb76..0a4506f1676b8 100644 --- a/api/envoy/admin/v2alpha/server_info.proto +++ b/api/envoy/admin/v2alpha/server_info.proto @@ -114,11 +114,9 @@ message CommandLineOptions { // See :option:`--mode` for details. Mode mode = 19; - // See :option:`--max-stats` for details. - uint64 max_stats = 20; - - // See :option:`--max-obj-name-len` for details. - uint64 max_obj_name_len = 21; + // max_stats and max_obj_name_len are now unused and have no effect. + uint64 max_stats = 20 [deprecated = true]; + uint64 max_obj_name_len = 21 [deprecated = true]; // See :option:`--disable-hot-restart` for details. bool disable_hot_restart = 22; diff --git a/api/envoy/api/v2/cds.proto b/api/envoy/api/v2/cds.proto index 6fb858efd6420..d659ef80118d0 100644 --- a/api/envoy/api/v2/cds.proto +++ b/api/envoy/api/v2/cds.proto @@ -58,9 +58,6 @@ message Cluster { // :ref:`statistics ` if :ref:`alt_stat_name // ` is not provided. // Any ``:`` in the cluster name will be converted to ``_`` when emitting statistics. - // By default, the maximum length of a cluster name is limited to 60 - // characters. This limit can be increased by setting the - // :option:`--max-obj-name-len` command line argument to the desired value. string name = 1 [(validate.rules).string.min_bytes = 1]; // An optional alternative to the cluster name to be used while emitting stats. diff --git a/api/envoy/api/v2/lds.proto b/api/envoy/api/v2/lds.proto index 4da5a461a2643..2ecfce3f50103 100644 --- a/api/envoy/api/v2/lds.proto +++ b/api/envoy/api/v2/lds.proto @@ -46,9 +46,6 @@ message Listener { // The unique name by which this listener is known. If no name is provided, // Envoy will allocate an internal UUID for the listener. If the listener is to be dynamically // updated or removed via :ref:`LDS ` a unique name must be provided. - // By default, the maximum length of a listener's name is limited to 60 characters. This limit can - // be increased by setting the :option:`--max-obj-name-len` command line argument to the desired - // value. string name = 1; // The address that the listener should listen on. In general, the address must be unique, though diff --git a/bazel/README.md b/bazel/README.md index 99adeb4d2b876..ec4f00869b70b 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -411,18 +411,6 @@ local_repository( ... ``` -## Stats Tunables - -The default maximum number of stats in shared memory, and the default -maximum length of a cluster/route config/listener name, can be -overridden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and -`ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH`, respectively, to the desired -value. For example: - -``` -bazel build --copt=-DENVOY_DEFAULT_MAX_STATS=32768 --copt=-DENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH=150 //source/exe:envoy-static -``` - # Release builds Release builds should be built in `opt` mode, processed with `strip` and have a diff --git a/docs/root/configuration/statistics.rst b/docs/root/configuration/statistics.rst index cce87dce6cb4e..5829244ed82f9 100644 --- a/docs/root/configuration/statistics.rst +++ b/docs/root/configuration/statistics.rst @@ -3,14 +3,6 @@ Statistics ========== -A few statistics are emitted to report statistics system behavior: - -.. csv-table:: - :header: Name, Type, Description - :widths: 1, 1, 2 - - stats.overflow, Counter, Total number of times Envoy cannot allocate a statistic due to a shortage of shared memory - Server ------ diff --git a/docs/root/intro/deprecated.rst b/docs/root/intro/deprecated.rst index b3a7b7d918a2d..db86c98be20ad 100644 --- a/docs/root/intro/deprecated.rst +++ b/docs/root/intro/deprecated.rst @@ -12,6 +12,7 @@ Deprecated items below are listed in chronological order. Version 1.11.0 (Pending) ======================== +* The --max-stats and --max-obj-name-len flags no longer has any effect. * Use of :ref:`cluster ` in :ref:`redis_proxy.proto ` is deprecated. Set a :ref:`catch_all_cluster ` instead. Version 1.10.0 (Apr 5, 2019) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 4ef245f4715f7..6fe432f0139f2 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -9,6 +9,7 @@ Version history * event: added :ref:`loop duration and poll delay statistics `. * ext_authz: added a `x-envoy-auth-partial-body` metadata header set to `false|true` indicating if there is a partial body sent in the authorization request message. * ext_authz: added option to `ext_authz` that allows the filter clearing route cache. +* hot restart: stats are no longer shared between hot restart parent/child via shared memory, but rather by RPC. Hot restart version incremented to 11. * http: mitigated a race condition with the :ref:`delayed_close_timeout` where it could trigger while actively flushing a pending write buffer for a downstream connection. * jwt_authn: make filter's parsing of JWT more flexible, allowing syntax like ``jwt=eyJhbGciOiJS...ZFnFIw,extra=7,realm=123`` * redis: added :ref:`prefix routing ` to enable routing commands based on their key's prefix to different upstream. @@ -643,7 +644,7 @@ Version history * runtime: added :ref:`comment capability `. * server: change default log level (:option:`-l`) to `info`. * stats: maximum stat/name sizes and maximum number of stats are now variable via the - :option:`--max-obj-name-len` and :option:`--max-stats` options. + `--max-obj-name-len` and `--max-stats` options. * tcp proxy: added :ref:`access logging `. * tcp proxy: added :ref:`configurable connect retries `. diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index 5d99e09bd7c76..1d67ce8b05924 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -211,8 +211,6 @@ modify different aspects of the server: "service_node": "", "service_zone": "", "mode": "Serve", - "max_stats": "16384", - "max_obj_name_len": "60", "disable_hot_restart": false, "enable_mutex_tracing": false, "restart_epoch": 0, diff --git a/docs/root/operations/cli.rst b/docs/root/operations/cli.rst index 1d61fc8721193..2f9e3f976ed8b 100644 --- a/docs/root/operations/cli.rst +++ b/docs/root/operations/cli.rst @@ -214,24 +214,12 @@ following are the command line options that Envoy supports. during a hot restart. See the :ref:`hot restart overview ` for more information. Defaults to 900 seconds (15 minutes). -.. option:: --max-obj-name-len - - *(optional)* The maximum name length (in bytes) of the name field in a cluster/route_config/listener. - This setting is typically used in scenarios where the cluster names are auto generated, and often exceed - the built-in limit of 60 characters. Defaults to 60, and it's not valid to set to less than 60. - .. attention:: This setting affects the output of :option:`--hot-restart-version`. If you started Envoy with this option set to a non default value, you should use the same option (and same value) for subsequent hot restarts. -.. option:: --max-stats - - *(optional)* The maximum number of stats that can be shared between hot-restarts. This setting - affects the output of :option:`--hot-restart-version`; the same value must be used to hot - restart. Defaults to 16384. It's not valid to set this larger than 100 million. - .. option:: --disable-hot-restart *(optional)* This flag disables Envoy hot restart for builds that have it enabled. By default, hot diff --git a/docs/root/start/sandboxes/front_proxy.rst b/docs/root/start/sandboxes/front_proxy.rst index 73e12cf1588a6..e14d938cadfdb 100644 --- a/docs/root/start/sandboxes/front_proxy.rst +++ b/docs/root/start/sandboxes/front_proxy.rst @@ -216,8 +216,6 @@ statistics. For example inside ``frontenvoy`` we can get:: "service_node": "", "service_zone": "", "mode": "Serve", - "max_stats": "16384", - "max_obj_name_len": "60", "disable_hot_restart": false, "enable_mutex_tracing": false, "restart_epoch": 0, diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index c81fcccfb656e..8175022dff32f 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -80,6 +80,7 @@ envoy_cc_library( deps = [ "//include/envoy/event:dispatcher_interface", "//include/envoy/thread:thread_interface", + "//source/server:hot_restart_cc", ], ) diff --git a/include/envoy/server/hot_restart.h b/include/envoy/server/hot_restart.h index 458187668e2cc..f9d80729bf2b1 100644 --- a/include/envoy/server/hot_restart.h +++ b/include/envoy/server/hot_restart.h @@ -6,8 +6,11 @@ #include "envoy/common/pure.h" #include "envoy/event/dispatcher.h" #include "envoy/stats/stat_data_allocator.h" +#include "envoy/stats/store.h" #include "envoy/thread/thread.h" +#include "source/server/hot_restart.pb.h" + namespace Envoy { namespace Server { @@ -20,13 +23,9 @@ class Instance; */ class HotRestart { public: - struct GetParentStatsInfo { - uint64_t memory_allocated_; - uint64_t num_connections_; - }; - - struct ShutdownParentAdminInfo { - time_t original_start_time_; + struct ServerStatsFromParent { + uint64_t parent_memory_allocated_ = 0; + uint64_t parent_connections_ = 0; }; virtual ~HotRestart() {} @@ -46,32 +45,37 @@ class HotRestart { virtual int duplicateParentListenSocket(const std::string& address) PURE; /** - * Retrieve stats from our parent process. - * @param info will be filled with information from our parent if it can be retrieved. - */ - virtual void getParentStats(GetParentStatsInfo& info) PURE; - - /** - * Initialize the restarter after primary server initialization begins. The hot restart - * implementation needs to be created early to deal with shared memory, logging, etc. so - * late initialization of needed interfaces is done here. + * Initialize the parent logic of our restarter. Meant to be called after initialization of a + * new child has begun. The hot restart implementation needs to be created early to deal with + * shared memory, logging, etc. so late initialization of needed interfaces is done here. */ virtual void initialize(Event::Dispatcher& dispatcher, Server::Instance& server) PURE; /** * Shutdown admin processing in the parent process if applicable. This allows admin processing * to start up in the new process. - * @param info will be filled with information from our parent if it can be retrieved. + * @param original_start_time will be filled with information from our parent, if retrieved. */ - virtual void shutdownParentAdmin(ShutdownParentAdminInfo& info) PURE; + virtual void sendParentAdminShutdownRequest(time_t& original_start_time) PURE; /** - * Tell our parent to gracefully terminate itself. + * Tell our parent process to gracefully terminate itself. */ - virtual void terminateParent() PURE; + virtual void sendParentTerminateRequest() PURE; /** - * Shutdown the hot restarter. + * Retrieve stats from our parent process and merges them into stats_store, taking into account + * the stats values we've already seen transferred. + * Skips all of the above and returns 0s if there is not currently a parent. + * @param stats_store the store whose stats will be updated. + * @param stats_proto the stats values we are updating with. + * @return special values relating to the "server" stats scope, whose + * merging has to be handled by Server::InstanceImpl. + */ + virtual ServerStatsFromParent mergeParentStatsIfAny(Stats::StoreRoot& stats_store) PURE; + + /** + * Shutdown the half of our hot restarter that acts as a parent. */ virtual void shutdown() PURE; @@ -90,11 +94,6 @@ class HotRestart { * @return Thread::BasicLockable& a lock for access logs. */ virtual Thread::BasicLockable& accessLogLock() PURE; - - /** - * @returns an allocator for stats. - */ - virtual Stats::StatDataAllocator& statsAllocator() PURE; }; } // namespace Server diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 324960b35ffa0..223aa410522c0 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -87,12 +87,6 @@ class Instance { */ virtual void failHealthcheck(bool fail) PURE; - /** - * Fetch server stats specific to this process vs. global shared stats in a hot restart scenario. - * @param info supplies the stats structure to fill. - */ - virtual void getParentStats(HotRestart::GetParentStatsInfo& info) PURE; - /** * @return whether external healthchecks are currently failed or not. */ diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h index 354a1bc53e6f5..0848fa37dcaf2 100644 --- a/include/envoy/server/options.h +++ b/include/envoy/server/options.h @@ -7,7 +7,6 @@ #include "envoy/admin/v2alpha/server_info.pb.h" #include "envoy/common/pure.h" #include "envoy/network/address.h" -#include "envoy/stats/stats_options.h" #include "spdlog/spdlog.h" @@ -148,17 +147,6 @@ class Options { */ virtual const std::string& serviceZone() const PURE; - /** - * @return uint64_t the maximum number of stats gauges and counters. - */ - virtual uint64_t maxStats() const PURE; - - /** - * @return StatsOptions& the max stat name / suffix lengths for stats. - * router/cluster/listener. - */ - virtual const Stats::StatsOptions& statsOptions() const PURE; - /** * @return bool indicating whether the hot restart functionality has been disabled via cli flags. */ diff --git a/include/envoy/stats/BUILD b/include/envoy/stats/BUILD index 9162d1b1ca410..420ad3883f52a 100644 --- a/include/envoy/stats/BUILD +++ b/include/envoy/stats/BUILD @@ -19,7 +19,6 @@ envoy_cc_library( "stat_data_allocator.h", "stats.h", "stats_matcher.h", - "stats_options.h", "store.h", "tag.h", "tag_extractor.h", diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index d462231368d5d..a593827c7c54d 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -5,7 +5,6 @@ #include "envoy/common/pure.h" #include "envoy/stats/histogram.h" -#include "envoy/stats/stats_options.h" #include "envoy/stats/symbol_table.h" namespace Envoy { @@ -15,7 +14,6 @@ class Counter; class Gauge; class Histogram; class Scope; -class StatsOptions; class NullGaugeImpl; typedef std::unique_ptr ScopePtr; @@ -86,12 +84,6 @@ class Scope { */ virtual Histogram& histogram(const std::string& name) PURE; - /** - * @return a reference to the top-level StatsOptions struct, containing information about the - * maximum allowable object name length and stat suffix length. - */ - virtual const Stats::StatsOptions& statsOptions() const PURE; - /** * @return a reference to the symbol table. */ diff --git a/include/envoy/stats/stat_data_allocator.h b/include/envoy/stats/stat_data_allocator.h index 625dc36e49e6b..8d3ec57409150 100644 --- a/include/envoy/stats/stat_data_allocator.h +++ b/include/envoy/stats/stat_data_allocator.h @@ -23,6 +23,7 @@ namespace Stats { * be created utilizing a single fixed-size block suitable for * shared-memory, or in the heap, allowing for pointers and sharing of * substrings, with an opportunity for reduced memory consumption. + * TODO(fredlas) this interface can be deleted now that the shared memory version is gone. */ class StatDataAllocator { public: @@ -48,11 +49,6 @@ class StatDataAllocator { virtual GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, const std::vector& tags) PURE; - /** - * Determines whether this stats allocator requires bounded stat-name size. - */ - virtual bool requiresBoundedStatNameSize() const PURE; - virtual const SymbolTable& symbolTable() const PURE; virtual SymbolTable& symbolTable() PURE; diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 1289d93bd1b02..e1206bf4c2d4f 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -9,6 +9,7 @@ #include "envoy/stats/symbol_table.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" namespace Envoy { namespace Stats { @@ -65,6 +66,20 @@ class Metric { */ virtual bool used() const PURE; + /** + * Flags: + * Used: used by all stats types to figure out whether they have been used. + * Logic...: used by gauges to cache how they should be combined with a parent's value. + */ + struct Flags { + static const uint8_t Used = 0x01; + // TODO(fredlas) these logic flags should be removed if we move to indicating combine logic in + // the stat declaration macros themselves. (Now that stats no longer use shared memory, it's + // safe to mess with what these flag bits mean whenever we want). + static const uint8_t LogicAccumulate = 0x02; + static const uint8_t LogicNeverImport = 0x04; + static const uint8_t LogicCached = LogicAccumulate | LogicNeverImport; + }; virtual SymbolTable& symbolTable() PURE; virtual const SymbolTable& symbolTable() const PURE; }; @@ -99,6 +114,16 @@ class Gauge : public virtual Metric { virtual void set(uint64_t value) PURE; virtual void sub(uint64_t amount) PURE; virtual uint64_t value() const PURE; + + /** + * Returns the stat's combine logic, if known. + */ + virtual absl::optional cachedShouldImport() const PURE; + + /** + * Sets the value to be returned by cachedCombineLogic(). + */ + virtual void setShouldImport(bool should_import) PURE; }; typedef std::shared_ptr GaugeSharedPtr; diff --git a/include/envoy/stats/stats_options.h b/include/envoy/stats/stats_options.h deleted file mode 100644 index 4f0b3f64f3616..0000000000000 --- a/include/envoy/stats/stats_options.h +++ /dev/null @@ -1,50 +0,0 @@ -#pragma once - -#include - -#include "envoy/common/pure.h" - -namespace Envoy { -namespace Stats { - -/** - * Struct stored under Server::Options to hold information about the maximum - * object name length and maximum stat suffix length of a stat. These have - * defaults in StatsOptionsImpl, and the maximum object name length can be - * overridden. The default initialization is used in IsolatedStatImpl, and the - * user-overridden struct is stored in Options. - * - * As noted in the comment above StatsOptionsImpl in - * source/common/stats/stats_options_impl.h, a stat name often contains both a - * string whose length is user-defined (cluster_name in the below example), and - * a specific statistic name generated by Envoy. To make room for growth on both - * fronts, we limit the max allowed length of each separately. - * - * name / stat name - * |----------------------------------------------------------------| - * cluster..outlier_detection.ejections_consecutive_5xx - * |--------------------------------------| |-----------------------| - * object name suffix - */ -class StatsOptions { -public: - virtual ~StatsOptions() {} - - /** - * The max allowed length of a complete stat name, including suffix. - */ - virtual size_t maxNameLength() const PURE; - - /** - * The max allowed length of the object part of a stat name. - */ - virtual size_t maxObjNameLength() const PURE; - - /** - * The max allowed length of a stat suffix. - */ - virtual size_t maxStatSuffixLength() const PURE; -}; - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 156a52c90eae5..4b4ebdbc94e89 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -288,16 +288,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "block_memory_hash_set_lib", - hdrs = ["block_memory_hash_set.h"], - deps = [ - ":assert_lib", - ":logger_lib", - "//source/common/stats:stats_options_lib", - ], -) - envoy_cc_library( name = "perf_annotation_lib", srcs = ["perf_annotation.cc"], diff --git a/source/common/common/block_memory_hash_set.h b/source/common/common/block_memory_hash_set.h deleted file mode 100644 index ef1e2b34c6955..0000000000000 --- a/source/common/common/block_memory_hash_set.h +++ /dev/null @@ -1,365 +0,0 @@ -#pragma once - -#include -#include -#include - -#include "envoy/common/exception.h" -#include "envoy/stats/stats_options.h" - -#include "common/common/assert.h" -#include "common/common/fmt.h" -#include "common/common/logger.h" - -#include "absl/strings/string_view.h" - -namespace Envoy { - -/** - * Initialization parameters for BlockMemoryHashSet. The options are duplicated - * to the control-block after init, to aid with sanity checking when attaching - * an existing memory segment. - */ -struct BlockMemoryHashSetOptions { - std::string toString() const { - return fmt::format("capacity={}, num_slots={}", capacity, num_slots); - } - bool operator==(const BlockMemoryHashSetOptions& that) const { - return capacity == that.capacity && num_slots == that.num_slots; - } - bool operator!=(const BlockMemoryHashSetOptions& that) const { return !(*this == that); } - - uint32_t capacity; // how many values can be stored. - uint32_t num_slots; // determines speed of hash vs size efficiency. -}; - -/** - * Implements hash_set without using pointers, suitable for use - * in shared memory. Users must commit to capacity and num_slots at - * construction time. Value must provide these methods: - * absl::string_view Value::key() - * void Value::initialize(absl::string_view key) - * static uint64_t Value::size() - * static uint64_t Value::hash() - * - * This set may also be suitable for persisting a hash-table to long - * term storage, but not across machine architectures, as it doesn't - * use network byte order for storing ints. - */ -template class BlockMemoryHashSet : public Logger::Loggable { -public: - /** - * Sentinel used for next_cell links to indicate end-of-list. - */ - static const uint32_t Sentinel = 0xffffffff; - - /** Type used by put() to indicate the value at a key, and whether it was created */ - typedef std::pair ValueCreatedPair; - - /** - * Constructs a map control structure given a set of options, which cannot be changed. - * @param hash_set_options describes the parameters controlling set layout. - * @param init true if the memory should be initialized on construction. If false, - * the data in the table will be sanity checked, and an exception thrown if - * it is incoherent or mismatches the passed-in options. - * @param memory the memory buffer for the set data. - * @param stats_options a reference to the top-level StatsOptions struct containing - * information about max allowable stat name lengths. - * - * Note that no locking of any kind is done by this class; this must be done at the - * call-site to support concurrent access. - */ - BlockMemoryHashSet(const BlockMemoryHashSetOptions& hash_set_options, bool init, uint8_t* memory, - const Stats::StatsOptions& stats_options) - : cells_(nullptr), control_(nullptr), slots_(nullptr), stats_options_(stats_options) { - mapMemorySegments(hash_set_options, memory); - if (init) { - initialize(hash_set_options); - } else if (!attach(hash_set_options)) { - throw EnvoyException("BlockMemoryHashSet: Incompatible memory block"); - } - } - - /** - * Returns the numbers of byte required for the hash-table, based on - * the control structure. This must be used to allocate the - * backing-store (eg) in memory, which we do after - * constructing the object with the desired sizing. - */ - static uint64_t numBytes(const BlockMemoryHashSetOptions& hash_set_options, - const Stats::StatsOptions& stats_options) { - uint64_t size = cellOffset(hash_set_options.capacity, stats_options) + sizeof(Control) + - hash_set_options.num_slots * sizeof(uint32_t); - return align(size); - } - - uint64_t numBytes(const Stats::StatsOptions& stats_options) const { - return numBytes(control_->hash_set_options, stats_options); - } - - /** Examines the data structures to see if they are sane, assert-failing on any trouble. */ - void sanityCheck() { - RELEASE_ASSERT(control_->size <= control_->hash_set_options.capacity, ""); - - // As a sanity check, make sure there are control_->size values - // reachable from the slots, each of which has a valid - // char_offset. - // - // Avoid infinite loops if there is a next_cell_index cycle within a - // slot. Note that the num_values message will be emitted outside - // the loop. - uint32_t num_values = 0; - for (uint32_t slot = 0; slot < control_->hash_set_options.num_slots; ++slot) { - uint32_t next = 0; // initialized to silence compilers. - for (uint32_t cell_index = slots_[slot]; - (cell_index != Sentinel) && (num_values <= control_->size); cell_index = next) { - RELEASE_ASSERT(cell_index < control_->hash_set_options.capacity, ""); - Cell& cell = getCell(cell_index); - absl::string_view key = cell.value.key(); - RELEASE_ASSERT(computeSlot(key) == slot, ""); - next = cell.next_cell_index; - ++num_values; - } - } - RELEASE_ASSERT(num_values == control_->size, ""); - - uint32_t num_free_entries = 0; - uint32_t expected_free_entries = control_->hash_set_options.capacity - control_->size; - - // Don't infinite-loop with a corruption; break when we see there's a problem. - for (uint32_t cell_index = control_->free_cell_index; - (cell_index != Sentinel) && (num_free_entries <= expected_free_entries); - cell_index = getCell(cell_index).next_cell_index) { - ++num_free_entries; - } - RELEASE_ASSERT(num_free_entries == expected_free_entries, ""); - } - - /** - * Inserts a value into the set. If successful (e.g. map has - * capacity) then put returns a pointer to the value object, which - * the caller can then write, Returns {nullptr, false} if the key - * was too large, or the capacity of the map has been exceeded. - * - * If the value was already present in the map, then {value, false} is returned. - * The caller may need to clean up an old value. - * - * If the value is newly allocated, then {value, true} is returned. - * - * @return a pair with the value-pointer (or nullptr), and a bool indicating - * whether the value is newly allocated. - */ - ValueCreatedPair insert(absl::string_view key) { - Value* value = get(key); - if (value != nullptr) { - return ValueCreatedPair(value, false); - } - if (control_->size >= control_->hash_set_options.capacity) { - return ValueCreatedPair(nullptr, false); - } - const uint32_t slot = computeSlot(key); - const uint32_t cell_index = control_->free_cell_index; - Cell& cell = getCell(cell_index); - control_->free_cell_index = cell.next_cell_index; - cell.next_cell_index = slots_[slot]; - slots_[slot] = cell_index; - value = &cell.value; - value->initialize(key, stats_options_); - ++control_->size; - return ValueCreatedPair(value, true); - } - - /** - * Removes the specified key from the map, returning true if the key - * was found. - * @param key the key to remove - */ - bool remove(absl::string_view key) { - const uint32_t slot = computeSlot(key); - uint32_t* next = nullptr; - for (uint32_t* cptr = &slots_[slot]; *cptr != Sentinel; cptr = next) { - const uint32_t cell_index = *cptr; - Cell& cell = getCell(cell_index); - if (cell.value.key() == key) { - // Splice current cell out of slot-chain. - *cptr = cell.next_cell_index; - - // Splice current cell into free-list. - cell.next_cell_index = control_->free_cell_index; - control_->free_cell_index = cell_index; - - --control_->size; - return true; - } - next = &cell.next_cell_index; - } - return false; - } - - /** Returns the number of key/values stored in the map. */ - uint32_t size() const { return control_->size; } - - /** - * Gets the value associated with a key, returning nullptr if the value was not found. - * @param key - */ - Value* get(absl::string_view key) { - const uint32_t slot = computeSlot(key); - for (uint32_t c = slots_[slot]; c != Sentinel; c = getCell(c).next_cell_index) { - Cell& cell = getCell(c); - if (cell.value.key() == key) { - return &cell.value; - } - } - return nullptr; - } - - /** - * Computes a version signature based on the options and the hash function. - */ - std::string version(const Stats::StatsOptions& stats_options) { - return fmt::format("options={} hash={} size={}", control_->hash_set_options.toString(), - control_->hash_signature, numBytes(stats_options)); - } - -private: - friend class BlockMemoryHashSetTest; - - /** - * Initializes a hash-map on raw memory. No expectations are made about the state of the memory - * coming in. - * @param memory - */ - void initialize(const BlockMemoryHashSetOptions& hash_set_options) { - control_->hash_signature = Value::hash(signatureStringToHash()); - control_->num_bytes = numBytes(hash_set_options, stats_options_); - control_->hash_set_options = hash_set_options; - control_->size = 0; - control_->free_cell_index = 0; - - // Initialize all the slots; - for (uint32_t slot = 0; slot < hash_set_options.num_slots; ++slot) { - slots_[slot] = Sentinel; - } - - // Initialize the free-cell list. - const uint32_t last_cell = hash_set_options.capacity - 1; - for (uint32_t cell_index = 0; cell_index < last_cell; ++cell_index) { - Cell& cell = getCell(cell_index); - cell.next_cell_index = cell_index + 1; - } - getCell(last_cell).next_cell_index = Sentinel; - } - - /** - * Attempts to attach to an existing memory segment. Does a (relatively) quick - * sanity check to make sure the options copied to the provided memory match, and also - * that the slot, cell, and key-string structures look sane. - */ - bool attach(const BlockMemoryHashSetOptions& hash_set_options) { - if (numBytes(hash_set_options, stats_options_) != control_->num_bytes) { - ENVOY_LOG(error, "BlockMemoryHashSet unexpected memory size {} != {}", - numBytes(hash_set_options, stats_options_), control_->num_bytes); - return false; - } - if (Value::hash(signatureStringToHash()) != control_->hash_signature) { - ENVOY_LOG(error, "BlockMemoryHashSet hash signature mismatch."); - return false; - } - sanityCheck(); - return true; - } - - uint32_t computeSlot(absl::string_view key) { - return Value::hash(key) % control_->hash_set_options.num_slots; - } - - /** - * Computes a signature string, composed of all the non-zero 8-bit characters. - * This is used for detecting if the hash algorithm changes, which invalidates - * any saved stats-set. - */ - static std::string signatureStringToHash() { - std::string signature_string; - signature_string.resize(255); - for (int i = 1; i <= 255; ++i) { - signature_string[i - 1] = i; - } - return signature_string; - } - - /** - * Represents control-values for the hash-table. - */ - struct Control { - BlockMemoryHashSetOptions hash_set_options; // Options established at map construction time. - uint64_t hash_signature; // Hash of a constant signature string. - uint64_t num_bytes; // Bytes allocated on behalf of the map. - uint32_t size; // Number of values currently stored. - uint32_t free_cell_index; // Offset of first free cell. - }; - - /** - * Represents a value-cell, which is stored in a linked-list from each slot. - */ - struct Cell { - uint32_t next_cell_index; // Index of next cell in map->cells_, terminated with Sentinel. - Value value; // Templated value field. - }; - - // It seems like this is an obvious constexpr, but it won't compile as one. - static uint64_t calculateAlignment() { - return std::max(alignof(Cell), std::max(alignof(uint32_t), alignof(Control))); - } - - static uint64_t align(uint64_t size) { - const uint64_t alignment = calculateAlignment(); - // Check that alignment is a power of 2: - // http://www.graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 - RELEASE_ASSERT((alignment > 0) && ((alignment & (alignment - 1)) == 0), ""); - return (size + alignment - 1) & ~(alignment - 1); - } - - /** - * Computes the byte offset of a cell into cells_. This is not - * simply an array index because we don't know the size of a key at - * compile-time. - */ - static uint64_t cellOffset(uint32_t cell_index, const Stats::StatsOptions& stats_options) { - // sizeof(Cell) includes 'sizeof Value' which may not be accurate. So we need to - // subtract that off, and add the template method's view of the actual value-size. - uint64_t cell_size = - align(sizeof(Cell) + Value::structSizeWithOptions(stats_options) - sizeof(Value)); - return cell_index * cell_size; - } - - /** - * Returns a reference to a Cell at the specified index. - */ - Cell& getCell(uint32_t cell_index) { - // Because the key-size is parameterizable, an array-lookup on sizeof(Cell) does not work. - char* ptr = reinterpret_cast(cells_) + cellOffset(cell_index, stats_options_); - RELEASE_ASSERT((reinterpret_cast(ptr) & (calculateAlignment() - 1)) == 0, ""); - return *reinterpret_cast(ptr); - } - - /** Maps out the segments of memory for us to work with. */ - void mapMemorySegments(const BlockMemoryHashSetOptions& hash_set_options, uint8_t* memory) { - // Note that we are not examining or mutating memory here, just looking at the pointer, - // so we don't need to hold any locks. - cells_ = reinterpret_cast(memory); // First because Value may need to be aligned. - memory += cellOffset(hash_set_options.capacity, stats_options_); - control_ = reinterpret_cast(memory); - memory += sizeof(Control); - slots_ = reinterpret_cast(memory); - } - - // Pointers into memory. Cells go first, because Value may need a more aggressive - // alignment. - Cell* cells_; - Control* control_; - uint32_t* slots_; - const Stats::StatsOptions& stats_options_; -}; - -} // namespace Envoy diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 61211cda8f2c2..581ac451a7981 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -35,7 +35,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/protobuf:utility_lib", - "//source/common/stats:stats_lib", "//source/extensions/stat_sinks:well_known_names", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", ], @@ -67,7 +66,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/network:utility_lib", - "//source/common/stats:stats_lib", "@envoy_api//envoy/api/v2:cds_cc", "@envoy_api//envoy/api/v2/cluster:circuit_breaker_cc", ], @@ -252,7 +250,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/network:utility_lib", - "//source/common/stats:stats_lib", "//source/extensions/filters/network:well_known_names", "@envoy_api//envoy/api/v2:lds_cc", ], @@ -310,7 +307,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/config:utility_lib", "//source/common/json:config_schemas_lib", - "//source/common/stats:stats_lib", "//source/extensions/filters/http:well_known_names", "@envoy_api//envoy/api/v2:rds_cc", ], diff --git a/source/common/config/bootstrap_json.cc b/source/common/config/bootstrap_json.cc index 31dbc6686bfe9..e42c8fe53b983 100644 --- a/source/common/config/bootstrap_json.cc +++ b/source/common/config/bootstrap_json.cc @@ -1,7 +1,5 @@ #include "common/config/bootstrap_json.h" -#include "envoy/stats/stats_options.h" - #include "common/common/assert.h" #include "common/config/address_json.h" #include "common/config/cds_json.h" @@ -17,8 +15,7 @@ namespace Envoy { namespace Config { void BootstrapJson::translateClusterManagerBootstrap( - const Json::Object& json_cluster_manager, envoy::config::bootstrap::v2::Bootstrap& bootstrap, - const Stats::StatsOptions& stats_options) { + const Json::Object& json_cluster_manager, envoy::config::bootstrap::v2::Bootstrap& bootstrap) { json_cluster_manager.validateSchema(Json::Schema::CLUSTER_MANAGER_SCHEMA); absl::optional eds_config; @@ -27,14 +24,13 @@ void BootstrapJson::translateClusterManagerBootstrap( auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters()->Add(); Config::CdsJson::translateCluster(*json_sds->getObject("cluster"), absl::optional(), - *cluster, stats_options); + *cluster); } if (json_cluster_manager.hasObject("cds")) { const auto json_cds = json_cluster_manager.getObject("cds"); auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters()->Add(); - Config::CdsJson::translateCluster(*json_cds->getObject("cluster"), eds_config, *cluster, - stats_options); + Config::CdsJson::translateCluster(*json_cds->getObject("cluster"), eds_config, *cluster); Config::Utility::translateCdsConfig( *json_cds, *bootstrap.mutable_dynamic_resources()->mutable_cds_config()); } @@ -42,7 +38,7 @@ void BootstrapJson::translateClusterManagerBootstrap( for (const Json::ObjectSharedPtr& json_cluster : json_cluster_manager.getObjectArray("clusters")) { auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters()->Add(); - Config::CdsJson::translateCluster(*json_cluster, eds_config, *cluster, stats_options); + Config::CdsJson::translateCluster(*json_cluster, eds_config, *cluster); } auto* cluster_manager = bootstrap.mutable_cluster_manager(); @@ -54,12 +50,10 @@ void BootstrapJson::translateClusterManagerBootstrap( } void BootstrapJson::translateBootstrap(const Json::Object& json_config, - envoy::config::bootstrap::v2::Bootstrap& bootstrap, - const Stats::StatsOptions& stats_options) { + envoy::config::bootstrap::v2::Bootstrap& bootstrap) { json_config.validateSchema(Json::Schema::TOP_LEVEL_CONFIG_SCHEMA); - translateClusterManagerBootstrap(*json_config.getObject("cluster_manager"), bootstrap, - stats_options); + translateClusterManagerBootstrap(*json_config.getObject("cluster_manager"), bootstrap); if (json_config.hasObject("lds")) { auto* lds_config = bootstrap.mutable_dynamic_resources()->mutable_lds_config(); @@ -68,7 +62,7 @@ void BootstrapJson::translateBootstrap(const Json::Object& json_config, for (const auto json_listener : json_config.getObjectArray("listeners")) { auto* listener = bootstrap.mutable_static_resources()->mutable_listeners()->Add(); - Config::LdsJson::translateListener(*json_listener, *listener, stats_options); + Config::LdsJson::translateListener(*json_listener, *listener); } JSON_UTIL_SET_STRING(json_config, bootstrap, flags_path); diff --git a/source/common/config/bootstrap_json.h b/source/common/config/bootstrap_json.h index e55397521c593..80c567d7264ad 100644 --- a/source/common/config/bootstrap_json.h +++ b/source/common/config/bootstrap_json.h @@ -2,7 +2,6 @@ #include "envoy/config/bootstrap/v2/bootstrap.pb.h" #include "envoy/json/json_object.h" -#include "envoy/stats/stats_options.h" namespace Envoy { namespace Config { @@ -15,8 +14,7 @@ class BootstrapJson { * @param bootstrap destination v2 envoy::config::bootstrap::v2::Bootstrap. */ static void translateClusterManagerBootstrap(const Json::Object& json_cluster_manager, - envoy::config::bootstrap::v2::Bootstrap& bootstrap, - const Stats::StatsOptions& stats_options); + envoy::config::bootstrap::v2::Bootstrap& bootstrap); /** * Translate a v1 JSON static config object to v2 envoy::config::bootstrap::v2::Bootstrap. @@ -24,8 +22,7 @@ class BootstrapJson { * @param bootstrap destination v2 envoy::config::bootstrap::v2::Bootstrap. */ static void translateBootstrap(const Json::Object& json_config, - envoy::config::bootstrap::v2::Bootstrap& bootstrap, - const Stats::StatsOptions& stats_options); + envoy::config::bootstrap::v2::Bootstrap& bootstrap); }; } // namespace Config diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index d98c3a9c3577a..95fede2e33c45 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -99,12 +99,10 @@ void CdsJson::translateOutlierDetection( void CdsJson::translateCluster(const Json::Object& json_cluster, const absl::optional& eds_config, - envoy::api::v2::Cluster& cluster, - const Stats::StatsOptions& stats_options) { + envoy::api::v2::Cluster& cluster) { json_cluster.validateSchema(Json::Schema::CLUSTER_SCHEMA); const std::string name = json_cluster.getString("name"); - Utility::checkObjNameLength("Invalid cluster name", name, stats_options); cluster.set_name(name); const std::string string_type = json_cluster.getString("type"); diff --git a/source/common/config/cds_json.h b/source/common/config/cds_json.h index 1cb47f2555bcc..f2995074f79ac 100644 --- a/source/common/config/cds_json.h +++ b/source/common/config/cds_json.h @@ -3,7 +3,6 @@ #include "envoy/api/v2/cds.pb.h" #include "envoy/api/v2/cluster/circuit_breaker.pb.h" #include "envoy/json/json_object.h" -#include "envoy/stats/stats_options.h" #include "envoy/upstream/cluster_manager.h" #include "absl/types/optional.h" @@ -65,8 +64,7 @@ class CdsJson { */ static void translateCluster(const Json::Object& json_cluster, const absl::optional& eds_config, - envoy::api::v2::Cluster& cluster, - const Stats::StatsOptions& stats_options); + envoy::api::v2::Cluster& cluster); }; } // namespace Config diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index 9dd65d48574b0..f46cf38275cbb 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -1,7 +1,6 @@ #include "common/config/filter_json.h" #include "envoy/config/accesslog/v2/file.pb.h" -#include "envoy/stats/stats_options.h" #include "common/common/assert.h" #include "common/common/utility.h" @@ -127,8 +126,7 @@ void FilterJson::translateAccessLog(const Json::Object& json_config, void FilterJson::translateHttpConnectionManager( const Json::Object& json_config, envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& - proto_config, - const Stats::StatsOptions& stats_options) { + proto_config) { json_config.validateSchema(Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA); envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::CodecType @@ -140,8 +138,7 @@ void FilterJson::translateHttpConnectionManager( JSON_UTIL_SET_STRING(json_config, proto_config, stat_prefix); if (json_config.hasObject("rds")) { - Utility::translateRdsConfig(*json_config.getObject("rds"), *proto_config.mutable_rds(), - stats_options); + Utility::translateRdsConfig(*json_config.getObject("rds"), *proto_config.mutable_rds()); } if (json_config.hasObject("route_config")) { if (json_config.hasObject("rds")) { @@ -149,7 +146,7 @@ void FilterJson::translateHttpConnectionManager( "http connection manager must have either rds or route_config but not both"); } RdsJson::translateRouteConfiguration(*json_config.getObject("route_config"), - *proto_config.mutable_route_config(), stats_options); + *proto_config.mutable_route_config()); } for (const auto& json_filter : json_config.getObjectArray("filters", true)) { diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index 1b59317ff2925..73ca0f7085dc3 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -15,7 +15,6 @@ #include "envoy/config/filter/network/redis_proxy/v2/redis_proxy.pb.h" #include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.h" #include "envoy/json/json_object.h" -#include "envoy/stats/stats_options.h" namespace Envoy { namespace Config { @@ -50,8 +49,7 @@ class FilterJson { static void translateHttpConnectionManager( const Json::Object& json_config, envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& - proto_config, - const Stats::StatsOptions& stats_options); + proto_config); /** * Translate a v1 JSON Redis proxy object to v2 diff --git a/source/common/config/lds_json.cc b/source/common/config/lds_json.cc index d8af91733fbbf..a8bfab61d99b5 100644 --- a/source/common/config/lds_json.cc +++ b/source/common/config/lds_json.cc @@ -1,7 +1,5 @@ #include "common/config/lds_json.h" -#include "envoy/stats/stats_options.h" - #include "common/common/assert.h" #include "common/config/address_json.h" #include "common/config/json_utility.h" @@ -17,12 +15,10 @@ namespace Envoy { namespace Config { void LdsJson::translateListener(const Json::Object& json_listener, - envoy::api::v2::Listener& listener, - const Stats::StatsOptions& stats_options) { + envoy::api::v2::Listener& listener) { json_listener.validateSchema(Json::Schema::LISTENER_SCHEMA); const std::string name = json_listener.getString("name", ""); - Utility::checkObjNameLength("Invalid listener name", name, stats_options); listener.set_name(name); AddressJson::translateAddress(json_listener.getString("address"), true, true, diff --git a/source/common/config/lds_json.h b/source/common/config/lds_json.h index 4bd1d425987af..4848192acf570 100644 --- a/source/common/config/lds_json.h +++ b/source/common/config/lds_json.h @@ -3,7 +3,6 @@ #include "envoy/api/v2/lds.pb.h" #include "envoy/api/v2/listener/listener.pb.h" #include "envoy/json/json_object.h" -#include "envoy/stats/stats_options.h" namespace Envoy { namespace Config { @@ -16,8 +15,7 @@ class LdsJson { * @param listener destination v2 envoy::api::v2::Listener. */ static void translateListener(const Json::Object& json_listener, - envoy::api::v2::Listener& listener, - const Stats::StatsOptions& stats_options); + envoy::api::v2::Listener& listener); }; } // namespace Config diff --git a/source/common/config/rds_json.cc b/source/common/config/rds_json.cc index a352c8ca3e76d..658ae6b145b20 100644 --- a/source/common/config/rds_json.cc +++ b/source/common/config/rds_json.cc @@ -116,13 +116,12 @@ void RdsJson::translateQueryParameterMatcher( } void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config, - envoy::api::v2::RouteConfiguration& route_config, - const Stats::StatsOptions& stats_options) { + envoy::api::v2::RouteConfiguration& route_config) { json_route_config.validateSchema(Json::Schema::ROUTE_CONFIGURATION_SCHEMA); for (const auto json_virtual_host : json_route_config.getObjectArray("virtual_hosts", true)) { auto* virtual_host = route_config.mutable_virtual_hosts()->Add(); - translateVirtualHost(*json_virtual_host, *virtual_host, stats_options); + translateVirtualHost(*json_virtual_host, *virtual_host); } for (const std::string& header : @@ -150,12 +149,10 @@ void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config, } void RdsJson::translateVirtualHost(const Json::Object& json_virtual_host, - envoy::api::v2::route::VirtualHost& virtual_host, - const Stats::StatsOptions& stats_options) { + envoy::api::v2::route::VirtualHost& virtual_host) { json_virtual_host.validateSchema(Json::Schema::VIRTUAL_HOST_CONFIGURATION_SCHEMA); const std::string name = json_virtual_host.getString("name", ""); - Utility::checkObjNameLength("Invalid virtual host name", name, stats_options); virtual_host.set_name(name); for (const std::string& domain : json_virtual_host.getStringArray("domains", true)) { diff --git a/source/common/config/rds_json.h b/source/common/config/rds_json.h index 1334eddca4c7a..dba4c8b5b6188 100644 --- a/source/common/config/rds_json.h +++ b/source/common/config/rds_json.h @@ -3,7 +3,6 @@ #include "envoy/api/v2/rds.pb.h" #include "envoy/api/v2/route/route.pb.h" #include "envoy/json/json_object.h" -#include "envoy/stats/stats_options.h" namespace Envoy { namespace Config { @@ -65,8 +64,7 @@ class RdsJson { * @param route_config destination v2 envoy::api::v2::RouteConfiguration. */ static void translateRouteConfiguration(const Json::Object& json_route_config, - envoy::api::v2::RouteConfiguration& route_config, - const Stats::StatsOptions& stats_options); + envoy::api::v2::RouteConfiguration& route_config); /** * Translate a v1 JSON virtual host object to v2 envoy::api::v2::route::VirtualHost. @@ -74,8 +72,7 @@ class RdsJson { * @param virtual_host destination v2 envoy::api::v2::route::VirtualHost. */ static void translateVirtualHost(const Json::Object& json_virtual_host, - envoy::api::v2::route::VirtualHost& virtual_host, - const Stats::StatsOptions& stats_options); + envoy::api::v2::route::VirtualHost& virtual_host); /** * Translate a v1 JSON decorator object to v2 envoy::api::v2::route::Decorator. diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 225e53ecdd848..6ebc2d43d3ef1 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -194,12 +194,10 @@ void Utility::translateCdsConfig(const Json::Object& json_config, void Utility::translateRdsConfig( const Json::Object& json_rds, - envoy::config::filter::network::http_connection_manager::v2::Rds& rds, - const Stats::StatsOptions& stats_options) { + envoy::config::filter::network::http_connection_manager::v2::Rds& rds) { json_rds.validateSchema(Json::Schema::RDS_CONFIGURATION_SCHEMA); const std::string name = json_rds.getString("route_config_name", ""); - checkObjNameLength("Invalid route_config name", name, stats_options); rds.set_route_config_name(name); translateApiConfigSource(json_rds.getString("cluster"), @@ -242,15 +240,6 @@ Utility::createStatsMatcher(const envoy::config::bootstrap::v2::Bootstrap& boots return std::make_unique(bootstrap.stats_config()); } -void Utility::checkObjNameLength(const std::string& error_prefix, const std::string& name, - const Stats::StatsOptions& stats_options) { - if (name.length() > stats_options.maxNameLength()) { - throw EnvoyException(fmt::format("{}: Length of {} ({}) exceeds allowed maximum length ({})", - error_prefix, name, name.length(), - stats_options.maxNameLength())); - } -} - Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource( Grpc::AsyncClientManager& async_client_manager, const envoy::api::v2::core::ApiConfigSource& api_config_source, Stats::Scope& scope) { diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 5dacc67bfa7a5..04220ea3c2503 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -12,7 +12,6 @@ #include "envoy/server/filter_config.h" #include "envoy/stats/scope.h" #include "envoy/stats/stats_matcher.h" -#include "envoy/stats/stats_options.h" #include "envoy/stats/tag_producer.h" #include "envoy/upstream/cluster_manager.h" @@ -204,8 +203,7 @@ class Utility { */ static void translateRdsConfig(const Json::Object& json_rds, - envoy::config::filter::network::http_connection_manager::v2::Rds& rds, - const Stats::StatsOptions& stats_options); + envoy::config::filter::network::http_connection_manager::v2::Rds& rds); /** * Convert a v1 LDS JSON config to v2 LDS envoy::api::v2::core::ConfigSource. @@ -289,17 +287,6 @@ class Utility { static Stats::StatsMatcherPtr createStatsMatcher(const envoy::config::bootstrap::v2::Bootstrap& bootstrap); - /** - * Check user supplied name in RDS/CDS/LDS for sanity. - * It should be within the configured length limit. Throws on error. - * @param error_prefix supplies the prefix to use in error messages. - * @param name supplies the name to check for length limits. - * @param stats_options the top-level statsOptions struct, which contains the max stat name / - * suffix lengths for stats. - */ - static void checkObjNameLength(const std::string& error_prefix, const std::string& name, - const Stats::StatsOptions& stats_options); - /** * Obtain gRPC async client factory from a envoy::api::v2::core::ApiConfigSource. * @param async_client_manager gRPC async client manager. diff --git a/source/common/protobuf/protobuf.h b/source/common/protobuf/protobuf.h index 713c877023508..c258e322909b9 100644 --- a/source/common/protobuf/protobuf.h +++ b/source/common/protobuf/protobuf.h @@ -11,6 +11,7 @@ #include "google/protobuf/io/coded_stream.h" #include "google/protobuf/io/zero_copy_stream.h" #include "google/protobuf/io/zero_copy_stream_impl.h" +#include "google/protobuf/map.h" #include "google/protobuf/message.h" #include "google/protobuf/repeated_field.h" #include "google/protobuf/service.h" diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 0dd466977fc98..b766709c0d5b6 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -47,7 +47,6 @@ envoy_cc_library( ":histogram_lib", ":scope_prefixer_lib", ":stats_lib", - ":stats_options_lib", ":store_impl_lib", "//include/envoy/stats:stats_macros", "//source/common/stats:heap_stat_data_lib", @@ -74,20 +73,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "raw_stat_data_lib", - srcs = ["raw_stat_data.cc"], - hdrs = ["raw_stat_data.h"], - deps = [ - ":stat_data_allocator_lib", - "//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:thread_lib", - ], -) - envoy_cc_library( name = "scope_prefixer_lib", srcs = ["scope_prefixer.cc"], @@ -121,14 +106,23 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "stat_merger_lib", + srcs = ["stat_merger.cc"], + hdrs = ["stat_merger.h"], + deps = [ + ":symbol_table_lib", + "//include/envoy/stats:stats_interface", + "//source/common/protobuf", + ], +) + envoy_cc_library( name = "stats_lib", deps = [ ":histogram_lib", ":metric_impl_lib", - ":raw_stat_data_lib", ":source_impl_lib", - ":stats_options_lib", ":symbol_table_lib", ":tag_extractor_lib", ":utility_lib", @@ -168,14 +162,6 @@ envoy_cc_library( deps = [":symbol_table_lib"], ) -envoy_cc_library( - name = "stats_options_lib", - hdrs = ["stats_options_impl.h"], - deps = [ - "//include/envoy/stats:stats_interface", - ], -) - envoy_cc_library( name = "tag_extractor_lib", srcs = ["tag_extractor_impl.cc"], diff --git a/source/common/stats/heap_stat_data.cc b/source/common/stats/heap_stat_data.cc index 42d48967a6087..41494ed85641b 100644 --- a/source/common/stats/heap_stat_data.cc +++ b/source/common/stats/heap_stat_data.cc @@ -13,6 +13,7 @@ HeapStatDataAllocator::~HeapStatDataAllocator() { ASSERT(stats_.empty()); } HeapStatData* HeapStatData::alloc(StatName stat_name, SymbolTable& symbol_table) { void* memory = ::malloc(sizeof(HeapStatData) + stat_name.size()); ASSERT(memory); + // TODO(fredlas) call StatMerger::verifyCombineLogicSpecified() here? symbol_table.incRefCount(stat_name); return new (memory) HeapStatData(stat_name); } diff --git a/source/common/stats/heap_stat_data.h b/source/common/stats/heap_stat_data.h index e0636b6db492a..c0423b23031fe 100644 --- a/source/common/stats/heap_stat_data.h +++ b/source/common/stats/heap_stat_data.h @@ -74,8 +74,6 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl { void free(HeapStatData& data) override; // StatDataAllocator - bool requiresBoundedStatNameSize() const override { return false; } - CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, const std::vector& tags) override { return std::make_shared>>(alloc(name), *this, diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index c0c34576e0118..8e720a6ad8e3e 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -6,12 +6,10 @@ #include #include "envoy/stats/stats.h" -#include "envoy/stats/stats_options.h" #include "envoy/stats/store.h" #include "common/common/utility.h" #include "common/stats/heap_stat_data.h" -#include "common/stats/stats_options_impl.h" #include "common/stats/store_impl.h" #include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" @@ -71,7 +69,6 @@ class IsolatedStoreImpl : public StoreImpl { Histogram& histogram = histograms_.get(name); return histogram; } - const Stats::StatsOptions& statsOptions() const override { return stats_options_; } // Stats::Store std::vector counters() const override { return counters_.toVector(); } @@ -101,7 +98,6 @@ class IsolatedStoreImpl : public StoreImpl { IsolatedStatsCache counters_; IsolatedStatsCache gauges_; IsolatedStatsCache histograms_; - const StatsOptionsImpl stats_options_; NullGaugeImpl null_gauge_; }; diff --git a/source/common/stats/metric_impl.h b/source/common/stats/metric_impl.h index 4fc6d160bbba3..d374b01ecce04 100644 --- a/source/common/stats/metric_impl.h +++ b/source/common/stats/metric_impl.h @@ -32,13 +32,6 @@ class MetricImpl : public virtual Metric { StatName tagExtractedStatName() const override; protected: - /** - * Flags used by all stats types to figure out whether they have been used. - */ - struct Flags { - static const uint8_t Used = 0x1; - }; - void clear(); private: diff --git a/source/common/stats/raw_stat_data.cc b/source/common/stats/raw_stat_data.cc deleted file mode 100644 index cf2d141fcb686..0000000000000 --- a/source/common/stats/raw_stat_data.cc +++ /dev/null @@ -1,94 +0,0 @@ -#include "common/stats/raw_stat_data.h" - -#include - -#include -#include -#include - -#include "common/common/lock_guard.h" - -namespace Envoy { -namespace Stats { - -namespace { - -// Round val up to the next multiple of the natural alignment. -// Note: this implementation only works because 8 is a power of 2. -uint64_t roundUpMultipleNaturalAlignment(uint64_t val) { - const uint64_t multiple = alignof(RawStatData); - static_assert(multiple == 1 || multiple == 2 || multiple == 4 || multiple == 8 || multiple == 16, - "multiple must be a power of 2 for this algorithm to work"); - return (val + multiple - 1) & ~(multiple - 1); -} - -} // namespace - -RawStatDataAllocator::~RawStatDataAllocator() {} - -// 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 -// it's important that each element starts on the required alignment for the type. -uint64_t RawStatData::structSize(uint64_t name_size) { - return roundUpMultipleNaturalAlignment(sizeof(RawStatData) + name_size + 1); -} - -uint64_t RawStatData::structSizeWithOptions(const StatsOptions& stats_options) { - return structSize(stats_options.maxNameLength()); -} - -void RawStatData::initialize(absl::string_view key, const StatsOptions& stats_options) { - ASSERT(!initialized()); - if (key.size() > stats_options.maxNameLength()) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), stats_options.maxNameLength()); - key = key.substr(0, stats_options.maxNameLength()); - } - ref_count_ = 1; - memcpy(name_, key.data(), key.size()); - name_[key.size()] = '\0'; -} - -Stats::RawStatData* RawStatDataAllocator::alloc(absl::string_view name) { - // Try to find the existing slot in shared memory, otherwise allocate a new one. - Thread::LockGuard lock(mutex_); - if (name.length() > options_.maxNameLength()) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", - name, name.size(), options_.maxNameLength()); - name = name.substr(0, options_.maxNameLength()); - } - auto value_created = stats_set_.insert(name); - 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 RawStatDataAllocator::free(Stats::RawStatData& data) { - // We must hold the lock since the reference decrement can race with an initialize above. - Thread::LockGuard lock(mutex_); - ASSERT(data.ref_count_ > 0); - --data.ref_count_; - if (data.ref_count_ > 0) { - return; - } - bool key_removed = stats_set_.remove(data.key()); - ASSERT(key_removed); - memset(static_cast(&data), 0, Stats::RawStatData::structSizeWithOptions(options_)); -} - -template class StatDataAllocatorImpl; - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/raw_stat_data.h b/source/common/stats/raw_stat_data.h deleted file mode 100644 index 9688b3ef1d332..0000000000000 --- a/source/common/stats/raw_stat_data.h +++ /dev/null @@ -1,156 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include "envoy/stats/stat_data_allocator.h" -#include "envoy/stats/stats_options.h" -#include "envoy/stats/symbol_table.h" - -#include "common/common/assert.h" -#include "common/common/block_memory_hash_set.h" -#include "common/common/hash.h" -#include "common/common/thread.h" -#include "common/stats/stat_data_allocator_impl.h" - -#include "absl/strings/string_view.h" - -namespace Envoy { -namespace Stats { - -/** - * This structure is the backing memory for both CounterImpl and GaugeImpl. It is designed so that - * it can be allocated from shared memory if needed. - * - * @note Due to name_ being variable size, sizeof(RawStatData) probably isn't useful. Use - * RawStatData::structSize() or RawStatData::structSizeWithOptions() instead. - */ -struct RawStatData { - - /** - * Due to the flexible-array-length of name_, c-style allocation - * and initialization are necessary. - */ - RawStatData() = delete; - ~RawStatData() = delete; - - /** - * @return uint64_t the size of this struct, accounting for the length of - * name_ and padding for alignment. - */ - static uint64_t structSize(uint64_t name_size); - - /** - * Wrapper for structSize, taking a StatsOptions struct. Required by - * BlockMemoryHashSet, which has the context to supply StatsOptions. - */ - static uint64_t structSizeWithOptions(const StatsOptions& stats_options); - - /** - * Initializes this object to have the specified key, a refcount of 1, and all - * other values zero. Required for the HeapRawStatDataAllocator, which does - * not expect stat name truncation. We pass in the number of bytes allocated - * in order to assert the copy is safe inline. - * - * @param key the key - * @param stats_options the stats options - */ - void initialize(absl::string_view key, const StatsOptions& stats_options); - - /** - * @return uint64_t a hash of the key. This is required by BlockMemoryHashSet. - */ - static uint64_t hash(absl::string_view key) { return HashUtil::xxHash64(key); } - - /** - * @return true if object is in use. - */ - bool initialized() { return name_[0] != '\0'; } - - /** - * @return absl::string_view the name as a string_view. - */ - absl::string_view key() const { return absl::string_view(name_); } - - /** - * @return const char* the name. - */ - const char* name() const { return name_; } - - std::atomic value_; - std::atomic pending_increment_; - std::atomic flags_; - std::atomic ref_count_; - std::atomic unused_; - char name_[]; -}; - -using RawStatDataSet = BlockMemoryHashSet; - -template class RawStat : public Stat { -public: - RawStat(StatName stat_name, RawStatData& data, StatDataAllocatorImpl& alloc, - absl::string_view tag_extracted_name, const std::vector& tags) - : Stat(data, alloc, tag_extracted_name, tags), - stat_name_storage_(stat_name, alloc.symbolTable()) {} - ~RawStat() { stat_name_storage_.free(this->symbolTable()); } - - StatName statName() const override { return stat_name_storage_.statName(); } - -private: - StatNameStorage stat_name_storage_; -}; - -class RawStatDataAllocator : public StatDataAllocatorImpl { -public: - RawStatDataAllocator(Thread::BasicLockable& mutex, RawStatDataSet& stats_set, - const StatsOptions& options, SymbolTable& symbol_table) - : StatDataAllocatorImpl(symbol_table), mutex_(mutex), stats_set_(stats_set), - options_(options) {} - ~RawStatDataAllocator(); - - virtual RawStatData* alloc(absl::string_view name); // Virtual only for mocking. - void free(Stats::RawStatData& data) override; - RawStatData* allocStatName(StatName stat_name) { - return alloc(symbolTable().toString(stat_name)); - } - - // StatDataAllocator - bool requiresBoundedStatNameSize() const override { return true; } - - template - std::shared_ptr makeStat(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) { - RawStatData* raw_stat_data = allocStatName(name); - if (raw_stat_data == nullptr) { - return nullptr; - } - return std::make_shared>(name, *raw_stat_data, *this, tag_extracted_name, tags); - } - - CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) override { - return makeStat>(name, tag_extracted_name, tags); - } - - GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) override { - return makeStat>(name, tag_extracted_name, tags); - } - -private: - Thread::BasicLockable& mutex_; - RawStatDataSet& stats_set_ GUARDED_BY(mutex_); - const StatsOptions& options_; -}; - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index 22bf13e8932c3..66c61072c7f42 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -35,7 +35,6 @@ class ScopePrefixer : public Scope { return histogramFromStatName(storage.statName()); } - const Stats::StatsOptions& statsOptions() const override { return scope_.statsOptions(); } const SymbolTable& symbolTable() const override { return scope_.symbolTable(); } virtual SymbolTable& symbolTable() override { return scope_.symbolTable(); } diff --git a/source/common/stats/stat_data_allocator_impl.h b/source/common/stats/stat_data_allocator_impl.h index 82db3a9562b8a..47b0858fe0a8f 100644 --- a/source/common/stats/stat_data_allocator_impl.h +++ b/source/common/stats/stat_data_allocator_impl.h @@ -23,11 +23,8 @@ namespace Stats { // hot restart stat continuity, and heap allocation for more efficient RAM usage // for when hot-restart is not required. // -// Also note that RawStatData needs to live in a shared memory block, and it's -// possible, but not obvious, that a vptr would be usable across processes. In -// any case, RawStatData is allocated from a shared-memory block rather than via -// new, so the usual C++ compiler assistance for setting up vptrs will not be -// available. This could be resolved with placed new, or another nesting level. +// TODO(fredlas) the above paragraph is obsolete; it's now only heap. So, this +// interface can hopefully be collapsed down a bit. template class StatDataAllocatorImpl : public StatDataAllocator { public: explicit StatDataAllocatorImpl(SymbolTable& symbol_table) : symbol_table_(symbol_table) {} @@ -133,24 +130,41 @@ template class GaugeImpl : public Gauge, public MetricImpl { } // Stats::Gauge - virtual void add(uint64_t amount) override { + void add(uint64_t amount) override { data_.value_ += amount; data_.flags_ |= Flags::Used; } - virtual void dec() override { sub(1); } - virtual void inc() override { add(1); } - virtual void set(uint64_t value) override { + void dec() override { sub(1); } + void inc() override { add(1); } + void set(uint64_t value) override { data_.value_ = value; data_.flags_ |= Flags::Used; } - virtual void sub(uint64_t amount) override { + void sub(uint64_t amount) override { ASSERT(data_.value_ >= amount); ASSERT(used() || amount == 0); data_.value_ -= amount; } - virtual uint64_t value() const override { return data_.value_; } + uint64_t value() const override { return data_.value_; } bool used() const override { return data_.flags_ & Flags::Used; } + // Returns true if values should be added, false if no import. + absl::optional cachedShouldImport() const override { + if ((data_.flags_ & Flags::LogicCached) == 0) { + return absl::nullopt; + } + return (data_.flags_ & Flags::LogicAccumulate) != 0; + } + + void setShouldImport(bool should_import) override { + if (should_import) { + data_.flags_ |= Flags::LogicAccumulate; + } else { + data_.flags_ |= Flags::LogicNeverImport; + } + } + +private: const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); } SymbolTable& symbolTable() override { return alloc_.symbolTable(); } @@ -180,6 +194,8 @@ class NullGaugeImpl : public Gauge, NullMetricImpl { void set(uint64_t) override {} void sub(uint64_t) override {} uint64_t value() const override { return 0; } + absl::optional cachedShouldImport() const override { return absl::nullopt; } + void setShouldImport(bool) override {} }; } // namespace Stats diff --git a/source/common/stats/stat_merger.cc b/source/common/stats/stat_merger.cc new file mode 100644 index 0000000000000..55dbb1822dc0c --- /dev/null +++ b/source/common/stats/stat_merger.cc @@ -0,0 +1,95 @@ +#include "common/stats/stat_merger.h" + +#include + +namespace Envoy { +namespace Stats { + +StatMerger::StatMerger(Stats::Store& target_store) : target_store_(target_store) {} + +bool StatMerger::shouldImport(Gauge& gauge, const std::string& gauge_name) { + absl::optional should_import = gauge.cachedShouldImport(); + if (should_import.has_value()) { + return should_import.value(); + } + + // Gauge name *substrings*, and special logic to use for combining those gauges' values. + static const auto* nonstandard_combine_logic = new std::vector{ + // Any .version is either a static property of the binary, or an opaque identifier for + // resources that are not passed across hot restart. + std::regex(".*\\.version$"), + // Once the child is up and reporting stats, its own control plane state and liveness is what + // we're interested in. + std::regex(".*\\.control_plane.connected_state$"), + std::regex("^server.live$"), + // Properties that should reasonably have some continuity across hot restart. The parent's + // last value should be a relatively accurate starting point, and then the child can update + // from there when appropriate. (All of these exceptional stats used with set() rather than + // add()/sub(), so the child's new value will in fact overwrite.) + std::regex("^cluster_manager.active_clusters$"), + std::regex("^cluster_manager.warming_clusters$"), + std::regex("^cluster\\..*\\.membership_.*$"), + std::regex("^cluster\\..*\\.max_host_weight$"), + std::regex(".*\\.total_principals$"), + std::regex("^listener_manager.total_listeners_active$"), + std::regex("^overload\\..*\\.pressure$"), + // Due to the fd passing, the parent's view of whether its listeners are in transitive states + // is not useful. + std::regex("^listener_manager.total_listeners_draining$"), + std::regex("^listener_manager.total_listeners_warming$"), + // Static properties known at startup. + std::regex("^server.concurrency$"), + std::regex("^server.hot_restart_epoch$"), + std::regex("^runtime.admin_overrides_active$"), + std::regex("^runtime.num_keys$"), + }; + for (const auto& exception : *nonstandard_combine_logic) { + std::smatch match; + if (std::regex_match(gauge_name, match, exception)) { + gauge.setShouldImport(false); + return false; + } + } + gauge.setShouldImport(true); + return true; +} + +void StatMerger::mergeCounters(const Protobuf::Map& counter_deltas) { + for (const auto& counter : counter_deltas) { + target_store_.counter(counter.first).add(counter.second); + } +} + +void StatMerger::mergeGauges(const Protobuf::Map& gauges) { + for (const auto& gauge : gauges) { + auto& gauge_ref = target_store_.gauge(gauge.first); + uint64_t& parent_value_ref = parent_gauge_values_[gauge_ref.statName()]; + uint64_t old_parent_value = parent_value_ref; + uint64_t new_parent_value = gauge.second; + parent_value_ref = new_parent_value; + + if (!StatMerger::shouldImport(gauge_ref, gauge.first)) { + continue; + } + if (new_parent_value > old_parent_value) { + gauge_ref.add(new_parent_value - old_parent_value); + } else { + gauge_ref.sub(old_parent_value - new_parent_value); + } + } +} + +// TODO(fredlas) the current implementation can "leak" obsolete parent stats into the child. +// That is, the parent had stat "foo", the child doesn't care about "foo" and back in the +// shared memory implementation would have dropped it, but the import causes it to be made into +// a real stat that stays around forever. The initial mini-consensus approach will be to +// track which stats are actually getting used by the child, and drop those that aren't when +// the hot restart completes. +void StatMerger::mergeStats(const Protobuf::Map& counter_deltas, + const Protobuf::Map& gauges) { + mergeCounters(counter_deltas); + mergeGauges(gauges); +} + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/stat_merger.h b/source/common/stats/stat_merger.h new file mode 100644 index 0000000000000..d0d25874b4626 --- /dev/null +++ b/source/common/stats/stat_merger.h @@ -0,0 +1,43 @@ +#pragma once + +#include "envoy/stats/store.h" + +#include "common/protobuf/protobuf.h" +#include "common/stats/symbol_table_impl.h" + +#include "absl/container/flat_hash_map.h" + +namespace Envoy { +namespace Stats { + +// Responsible for the sensible merging of two instances of the same stat from two different +// (typically hot restart parent+child) Envoy processes. +class StatMerger { +public: + StatMerger(Stats::Store& target_store); + + // Merge the values of stats_proto into stats_store. Counters are always straightforward + // addition, while gauges default to addition but have exceptions. + void mergeStats(const Protobuf::Map& counter_deltas, + const Protobuf::Map& gauges); + + // TODO(fredlas) add void verifyCombineLogicSpecified(absl::string_view gauge_name), to + // be called at gauge allocation, to ensure (with an ASSERT) that anyone adding a new stat + // will be forced to come across this code and explicitly specify combination logic. + // + // OR, + // switch from the combination logic table to requiring the stat macro declarations themselves + // to indicate the logic. + + // Returns true if the parent's value can be added in, false if we should do nothing. + static bool shouldImport(Gauge& gauge, const std::string& gauge_name); + +private: + void mergeCounters(const Protobuf::Map& counter_deltas); + void mergeGauges(const Protobuf::Map& gauges); + StatNameHashMap parent_gauge_values_; + Stats::Store& target_store_; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/stats_options_impl.h b/source/common/stats/stats_options_impl.h deleted file mode 100644 index 01b2e563b1a0f..0000000000000 --- a/source/common/stats/stats_options_impl.h +++ /dev/null @@ -1,32 +0,0 @@ -#pragma once - -#include - -#include "envoy/stats/stats_options.h" - -namespace Envoy { -namespace Stats { - -// The max name length is based on current set of stats. -// As of now, the longest stat is -// cluster..outlier_detection.ejections_consecutive_5xx -// which is 52 characters long without the cluster name. -// The max stat name length is 127 (default). So, in order to give room -// for growth to both the envoy generated stat characters -// (e.g., outlier_detection...) and user supplied names (e.g., cluster name), -// we set the max user supplied name length to 60, and the max internally -// generated stat suffixes to 67 (15 more characters to grow). -// If you want to increase the max user supplied name length, use the compiler -// option ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH or the CLI option -// max-obj-name-len -struct StatsOptionsImpl : public StatsOptions { - size_t maxNameLength() const override { return max_obj_name_length_ + max_stat_suffix_length_; } - size_t maxObjNameLength() const override { return max_obj_name_length_; } - size_t maxStatSuffixLength() const override { return max_stat_suffix_length_; } - - size_t max_obj_name_length_ = 60; - size_t max_stat_suffix_length_ = 67; -}; - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index b342d2793be51..589a3075e0938 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -10,7 +10,6 @@ #include "envoy/stats/sink.h" #include "envoy/stats/stat_data_allocator.h" #include "envoy/stats/stats.h" -#include "envoy/stats/stats_options.h" #include "common/common/lock_guard.h" #include "common/stats/scope_prefixer.h" @@ -22,21 +21,17 @@ namespace Envoy { namespace Stats { -ThreadLocalStoreImpl::ThreadLocalStoreImpl(const StatsOptions& stats_options, - StatDataAllocator& alloc) - : stats_options_(stats_options), alloc_(alloc), default_scope_(createScope("")), +ThreadLocalStoreImpl::ThreadLocalStoreImpl(StatDataAllocator& alloc) + : alloc_(alloc), default_scope_(createScope("")), tag_producer_(std::make_unique()), - stats_matcher_(std::make_unique()), - stats_overflow_("stats.overflow", alloc.symbolTable()), - num_last_resort_stats_(default_scope_->counterFromStatName(stats_overflow_.statName())), - heap_allocator_(alloc.symbolTable()), source_(*this), null_counter_(alloc.symbolTable()), - null_gauge_(alloc.symbolTable()), null_histogram_(alloc.symbolTable()) {} + stats_matcher_(std::make_unique()), heap_allocator_(alloc.symbolTable()), + source_(*this), null_counter_(alloc.symbolTable()), null_gauge_(alloc.symbolTable()), + null_histogram_(alloc.symbolTable()) {} ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { ASSERT(shutting_down_); default_scope_.reset(); ASSERT(scopes_.empty()); - stats_overflow_.free(symbolTable()); } void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { @@ -234,23 +229,6 @@ void ThreadLocalStoreImpl::clearScopeFromCaches(uint64_t scope_id, } } -absl::string_view ThreadLocalStoreImpl::truncateStatNameIfNeeded(absl::string_view name) { - // If the main allocator requires stat name truncation, do so now, though any - // warnings will be printed only if the truncated stat requires a new - // allocation. - if (alloc_.requiresBoundedStatNameSize()) { - const uint64_t max_length = stats_options_.maxNameLength(); - if (name.size() > max_length) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", - name, name.size(), max_length); - name = name.substr(0, max_length); - } - } - return name; -} - std::atomic ThreadLocalStoreImpl::ScopeImpl::next_scope_id_; ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix) @@ -343,12 +321,7 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( TagExtraction extraction(parent_, name); std::shared_ptr stat = make_stat(parent_.alloc_, name, extraction.tagExtractedName(), extraction.tags()); - if (stat == nullptr) { - parent_.num_last_resort_stats_.inc(); - stat = make_stat(parent_.heap_allocator_, name, extraction.tagExtractedName(), - extraction.tags()); - ASSERT(stat != nullptr); - } + ASSERT(stat != nullptr); central_ref = ¢ral_cache_map[stat->statName()]; *central_ref = stat; } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 9e223ac3a9a29..2569669559134 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -135,11 +135,11 @@ class TlsScope : public Scope { /** * Store implementation with thread local caching. For design details see - * https://github.com/envoyproxy/envoy/blob/master/docs/stats.md + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md */ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRoot { public: - ThreadLocalStoreImpl(const Stats::StatsOptions& stats_options, StatDataAllocator& alloc); + ThreadLocalStoreImpl(StatDataAllocator& alloc); ~ThreadLocalStoreImpl() override; // Stats::Scope @@ -183,9 +183,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Source& source() override { return source_; } - const Stats::StatsOptions& statsOptions() const override { return stats_options_; } - absl::string_view truncateStatNameIfNeeded(absl::string_view name); - private: template using StatMap = StatNameHashMap; @@ -221,7 +218,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Gauge& gaugeFromStatName(StatName name) override; Histogram& histogramFromStatName(StatName name) override; Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) override; - const Stats::StatsOptions& statsOptions() const override { return parent_.statsOptions(); } ScopePtr createScope(const std::string& name) override { return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name); } @@ -300,7 +296,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo bool checkAndRememberRejection(StatName name, StatNameStorageSet& central_rejected_stats, StatNameHashSet* tls_rejected_stats); - const Stats::StatsOptions& stats_options_; StatDataAllocator& alloc_; Event::Dispatcher* main_thread_dispatcher_{}; ThreadLocal::SlotPtr tls_; @@ -312,8 +307,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo StatsMatcherPtr stats_matcher_; std::atomic shutting_down_{}; std::atomic merge_in_progress_{}; - StatNameStorage stats_overflow_; - Counter& num_last_resort_stats_; HeapStatDataAllocator heap_allocator_; SourceImpl source_; diff --git a/source/docs/stats.md b/source/docs/stats.md index ad088720cf350..965dcb569d165 100644 --- a/source/docs/stats.md +++ b/source/docs/stats.md @@ -9,31 +9,12 @@ binary program restarts. The metrics are tracked as: data accumulates. Unliked counters and gauges, histogram data is not retained across binary program restarts. -## Hot-restart: `RawStatData` vs `HeapStatData` - In order to support restarting the Envoy binary program without losing counter and gauge -values, they are stored in a shared-memory block, including stats that are -created dynamically at runtime in response to discovery of new clusters at -runtime. To simplify memory management, each stat is allocated a fixed amount -of storage, controlled via [command-line -flags](https://www.envoyproxy.io/docs/envoy/latest/operations/cli): -`--max-stats` and `--max-obj-name-len`, which determine the size of the pre-allocated -shared-memory block. See -[RawStatData](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/raw_stat_data.h). - -Note in particular that the full stat name is retained in shared-memory, making -it easy to correlate stats across restarts even as the dynamic cluster -configuration changes. - -One challenge with this fixed memory allocation strategy is that it limits -cluster scalability. A deployment wishing to use a single Envoy instance to -manage tens of thousands of clusters, each with its own set of scoped stats, -will use more memory than is ideal. - -A flag `--disable-hot-restart` pivots the system toward an alternate heap-based -stat allocator that allocates stats on demand in the heap, with no preset limits -on the number of stats or their length. See -[HeapStatData](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/heap_stat_data.h). +values, they are passed from parent to child in an RPC protocol. +They were previously held in shared memory, which imposed various restrictions. +Unlike the shared memory implementation, the RPC passing *requires special indication +in source/common/stats/stat_merger.cc when simple addition is not appropriate for +combining two instances of a given stat*. ## Performance and Thread Local Storage @@ -67,10 +48,8 @@ This implementation is complicated so here is a rough overview of the threading reference the old scope which may be about to be cache flushed. * Since it's possible to have overlapping scopes, we de-dup stats when counters() or gauges() is called since these are very uncommon operations. - * Though this implementation is designed to work with a fixed shared memory space, it will fall - back to heap allocated stats if needed. NOTE: In this case, overlapping scopes will not share - the same backing store. This is to keep things simple, it could be done in the future if - needed. + * Overlapping scopes will not share the same backing store. This is to keep things simple, + it could be done in the future if needed. ### Histogram threading model @@ -101,7 +80,7 @@ followed. Stat names are replicated in several places in various forms. - * Held with the stat values, in `RawStatData` and `HeapStatData` + * Held with the stat values, in `HeapStatData` * In [MetricImpl](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/metric_impl.h) in a transformed state, with tags extracted into vectors of name/value strings. * In static strings across the codebase where stats are referenced @@ -110,10 +89,9 @@ Stat names are replicated in several places in various forms. used to perform tag extraction. There are stat maps in `ThreadLocalStore` for capturing all stats in a scope, -and each per-thread caches. However, they don't duplicate the stat -names. Instead, they reference the `char*` held in the `RawStatData` or -`HeapStatData itself, and thus are relatively cheap; effectively those maps are -all pointer-to-pointer. +and each per-thread caches. However, they don't duplicate the stat names. +Instead, they reference the `char*` held in the `HeapStatData` itself, and thus +are relatively cheap; effectively those maps are all pointer-to-pointer. For this to be safe, cache lookups from locally scoped strings must use `.find` rather than `operator[]`, as the latter would insert a pointer to a temporary as diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 6b1ac11426cb2..e7660cadb535a 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -49,7 +49,7 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti Thread::ThreadFactory& thread_factory, Filesystem::Instance& file_system) : options_(options), component_factory_(component_factory), thread_factory_(thread_factory), - file_system_(file_system) { + file_system_(file_system), stats_allocator_(symbol_table_) { ares_library_init(ARES_LIB_INIT_ALL); Event::Libevent::Global::initialize(); RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors(), ""); @@ -60,11 +60,11 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti case Server::Mode::Serve: { #ifdef ENVOY_HOT_RESTART if (!options.hotRestartDisabled()) { - restarter_ = std::make_unique(options_, symbol_table_); + restarter_ = std::make_unique(options_); } #endif if (restarter_ == nullptr) { - restarter_ = std::make_unique(symbol_table_); + restarter_ = std::make_unique(); } tls_ = std::make_unique(); @@ -80,8 +80,7 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti // block or not. std::set_new_handler([]() { PANIC("out of memory"); }); - stats_store_ = std::make_unique(options_.statsOptions(), - restarter_->statsAllocator()); + stats_store_ = std::make_unique(stats_allocator_); server_ = std::make_unique( options_, time_system, local_address, listener_hooks, *restarter_, *stats_store_, @@ -91,7 +90,7 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti break; } case Server::Mode::Validate: - restarter_ = std::make_unique(symbol_table_); + restarter_ = std::make_unique(); logging_context_ = std::make_unique(options_.logLevel(), options_.logFormat(), restarter_->logLock()); break; @@ -143,16 +142,13 @@ MainCommon::MainCommon(int argc, const char* const* argv) std::make_unique(), platform_impl_.threadFactory(), platform_impl_.fileSystem()) {} -std::string MainCommon::hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len, - bool hot_restart_enabled) { +std::string MainCommon::hotRestartVersion(bool hot_restart_enabled) { #ifdef ENVOY_HOT_RESTART if (hot_restart_enabled) { - return Server::HotRestartImpl::hotRestartVersion(max_num_stats, max_stat_name_len); + return Server::HotRestartImpl::hotRestartVersion(); } #else UNREFERENCED_PARAMETER(hot_restart_enabled); - UNREFERENCED_PARAMETER(max_num_stats); - UNREFERENCED_PARAMETER(max_stat_name_len); #endif return "disabled"; } diff --git a/source/exe/main_common.h b/source/exe/main_common.h index 14e7080c7c206..1abf89cbd70ba 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -69,6 +69,7 @@ class MainCommonBase { Server::ComponentFactory& component_factory_; Thread::ThreadFactory& thread_factory_; Filesystem::Instance& file_system_; + Stats::HeapStatDataAllocator stats_allocator_; std::unique_ptr tls_; std::unique_ptr restarter_; @@ -103,8 +104,7 @@ class MainCommon { base_.adminRequest(path_and_query, method, handler); } - static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len, - bool hot_restart_enabled); + static std::string hotRestartVersion(bool hot_restart_enabled); /** * @return a pointer to the server instance, or nullptr if initialized into diff --git a/source/extensions/filters/http/dynamo/dynamo_filter.cc b/source/extensions/filters/http/dynamo/dynamo_filter.cc index 301464b382f65..cb7ee4c357025 100644 --- a/source/extensions/filters/http/dynamo/dynamo_filter.cc +++ b/source/extensions/filters/http/dynamo/dynamo_filter.cc @@ -237,9 +237,8 @@ void DynamoFilter::chargeTablePartitionIdStats(const Json::Object& json_body) { std::vector partitions = RequestParser::parsePartitions(json_body); for (const RequestParser::PartitionDescriptor& partition : partitions) { - std::string scope_string = - Utility::buildPartitionStatString(stat_prefix_, table_descriptor_.table_name, operation_, - partition.partition_id_, scope_.statsOptions()); + std::string scope_string = Utility::buildPartitionStatString( + stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); scope_.counter(scope_string).add(partition.capacity_); } } diff --git a/source/extensions/filters/http/dynamo/dynamo_utility.cc b/source/extensions/filters/http/dynamo/dynamo_utility.cc index 6f5c60627a85b..a71408439fc31 100644 --- a/source/extensions/filters/http/dynamo/dynamo_utility.cc +++ b/source/extensions/filters/http/dynamo/dynamo_utility.cc @@ -2,8 +2,6 @@ #include -#include "envoy/stats/stats_options.h" - #include "common/common/fmt.h" namespace Envoy { @@ -14,21 +12,12 @@ namespace Dynamo { std::string Utility::buildPartitionStatString(const std::string& stat_prefix, const std::string& table_name, const std::string& operation, - const std::string& partition_id, - const Stats::StatsOptions& stats_options) { + const std::string& partition_id) { // Use the last 7 characters of the partition id. std::string stats_partition_postfix = fmt::format(".capacity.{}.__partition_id={}", operation, 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_options.maxNameLength() - 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. - if (stats_table_prefix.size() > remaining_size) { - stats_table_prefix = stats_table_prefix.substr(0, remaining_size); - } return fmt::format("{}{}", stats_table_prefix, stats_partition_postfix); } diff --git a/source/extensions/filters/http/dynamo/dynamo_utility.h b/source/extensions/filters/http/dynamo/dynamo_utility.h index 435d8f825c525..87b4bd0bc119f 100644 --- a/source/extensions/filters/http/dynamo/dynamo_utility.h +++ b/source/extensions/filters/http/dynamo/dynamo_utility.h @@ -2,8 +2,6 @@ #include -#include "envoy/stats/stats_options.h" - namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -25,8 +23,7 @@ class Utility { static std::string buildPartitionStatString(const std::string& stat_prefix, const std::string& table_name, const std::string& operation, - const std::string& partition_id, - const Stats::StatsOptions& stats_options); + const std::string& partition_id); }; } // namespace Dynamo diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 0014ccc55ef60..a00c9e552e007 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -106,8 +106,7 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( Network::FilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory( const Json::Object& json_config, Server::Configuration::FactoryContext& context) { envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager proto_config; - Config::FilterJson::translateHttpConnectionManager(json_config, proto_config, - context.scope().statsOptions()); + Config::FilterJson::translateHttpConnectionManager(json_config, proto_config); return createFilterFactoryFromProtoTyped(proto_config, context); } diff --git a/source/server/BUILD b/source/server/BUILD index 54fb3ba1aec0a..30c0395586ac3 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -107,11 +107,62 @@ envoy_cc_library( ], ) +cc_proto_library( + name = "hot_restart_cc", + deps = [":hot_restart_proto"], +) + +proto_library( + name = "hot_restart_proto", + srcs = ["hot_restart.proto"], +) + +envoy_cc_library( + name = "hot_restarting_base", + srcs = envoy_select_hot_restart(["hot_restarting_base.cc"]), + hdrs = envoy_select_hot_restart(["hot_restarting_base.h"]), + deps = [ + ":hot_restart_cc", + "//include/envoy/api:os_sys_calls_interface", + "//include/envoy/event:dispatcher_interface", + "//include/envoy/event:file_event_interface", + "//include/envoy/server:hot_restart_interface", + "//include/envoy/server:instance_interface", + "//include/envoy/server:options_interface", + "//source/common/api:os_sys_calls_lib", + "//source/common/common:assert_lib", + "//source/common/common:utility_lib", + "//source/common/network:utility_lib", + ], +) + +envoy_cc_library( + name = "hot_restarting_child", + srcs = envoy_select_hot_restart(["hot_restarting_child.cc"]), + hdrs = envoy_select_hot_restart(["hot_restarting_child.h"]), + deps = [ + ":hot_restarting_base", + "//source/common/stats:stat_merger_lib", + ], +) + +envoy_cc_library( + name = "hot_restarting_parent", + srcs = envoy_select_hot_restart(["hot_restarting_parent.cc"]), + hdrs = envoy_select_hot_restart(["hot_restarting_parent.h"]), + deps = [ + ":hot_restarting_base", + "//source/common/memory:stats_lib", + ], +) + envoy_cc_library( name = "hot_restart_lib", srcs = envoy_select_hot_restart(["hot_restart_impl.cc"]), hdrs = envoy_select_hot_restart(["hot_restart_impl.h"]), deps = [ + ":hot_restarting_child", + ":hot_restarting_parent", "//include/envoy/api:os_sys_calls_interface", "//include/envoy/event:dispatcher_interface", "//include/envoy/event:file_event_interface", @@ -120,11 +171,7 @@ envoy_cc_library( "//include/envoy/server:options_interface", "//source/common/api:os_sys_calls_lib", "//source/common/common:assert_lib", - "//source/common/common:block_memory_hash_set_lib", - "//source/common/common:utility_lib", - "//source/common/network:utility_lib", - "//source/common/stats:raw_stat_data_lib", - "//source/common/stats:stats_options_lib", + "//source/common/stats:heap_stat_data_lib", ], ) diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 2d0761d368046..b492102c86d71 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -75,7 +75,6 @@ class ValidationInstance : Logger::Loggable, DrainManager& drainManager() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } AccessLog::AccessLogManager& accessLogManager() override { return access_log_manager_; } void failHealthcheck(bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - void getParentStats(HotRestart::GetParentStatsInfo&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } HotRestart& hotRestart() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } Init::Manager& initManager() override { return init_manager_; } ServerLifecycleNotifier& lifecycleNotifier() override { return *this; } diff --git a/source/server/drain_manager_impl.cc b/source/server/drain_manager_impl.cc index e7ca1852f57c8..852452da10ab0 100644 --- a/source/server/drain_manager_impl.cc +++ b/source/server/drain_manager_impl.cc @@ -61,7 +61,7 @@ void DrainManagerImpl::startParentShutdownSequence() { parent_shutdown_timer_ = server_.dispatcher().createTimer([this]() -> void { // Shut down the parent now. It should have already been draining. ENVOY_LOG(info, "shutting down parent after drain"); - server_.hotRestart().terminateParent(); + server_.hotRestart().sendParentTerminateRequest(); }); parent_shutdown_timer_->enableTimer(std::chrono::duration_cast( diff --git a/source/server/hot_restart.proto b/source/server/hot_restart.proto new file mode 100644 index 0000000000000..7d17a3800edf3 --- /dev/null +++ b/source/server/hot_restart.proto @@ -0,0 +1,66 @@ +syntax = "proto3"; + +package envoy; + +message HotRestartMessage { + // Child->parent requests + message Request { + message PassListenSocket { + string address = 1; + } + message ShutdownAdmin { + } + message Stats { + } + message DrainListeners { + } + message Terminate { + } + oneof request { + PassListenSocket pass_listen_socket = 1; + ShutdownAdmin shutdown_admin = 2; + Stats stats = 3; + DrainListeners drain_listeners = 4; + Terminate terminate = 5; + } + } + + // Parent->child replies + message Reply { + message PassListenSocket { + int32 fd = 1; + } + message ShutdownAdmin { + uint64 original_start_time_unix_seconds = 1; + } + message Stats { + // Values for server_stats, which don't fit with the "combination logic" approach. + uint64 memory_allocated = 1; + uint64 num_connections = 2; + + // Keys are fully qualified stat names. + // + // The amount added to the counter since the last time a message included the counter in this + // map. (The first time a counter is included in this map, it's the amount added since the + // final latch() before hot restart began). + map counter_deltas = 3; + // The parent's current values for various gauges in its stats store. + map gauges = 4; + } + oneof reply { + // When this oneof is of the PassListenSocketReply type, there is a special + // implied meaning: the recvmsg that got this proto has control data to make + // the passing of the fd work, so make use of CMSG_SPACE etc. + PassListenSocket pass_listen_socket = 1; + ShutdownAdmin shutdown_admin = 2; + Stats stats = 3; + } + } + + oneof requestreply { + Request request = 1; + Reply reply = 2; + } + + bool didnt_recognize_your_last_message = 3; +} diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index a2a6dc5e002a3..1abe9b664fe56 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -17,35 +17,15 @@ #include "common/api/os_sys_calls_impl.h" #include "common/common/fmt.h" #include "common/common/lock_guard.h" -#include "common/common/utility.h" -#include "common/network/utility.h" -#include "common/stats/raw_stat_data.h" -#include "common/stats/stats_options_impl.h" #include "absl/strings/string_view.h" namespace Envoy { 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 = 10; - -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(uint64_t stats_set_size, const Options& options) { +SharedMemory* attachSharedMemory(const Options& options) { Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); - const uint64_t entry_size = Stats::RawStatData::structSizeWithOptions(options.statsOptions()); - const uint64_t total_size = sizeof(SharedMemory) + stats_set_size; - int flags = O_RDWR; const std::string shmem_name = fmt::format("/envoy_shared_memory_{}", options.baseId()); if (options.restartEpoch() == 0) { @@ -64,51 +44,44 @@ SharedMemory& SharedMemory::initialize(uint64_t stats_set_size, const Options& o } if (options.restartEpoch() == 0) { - const Api::SysCallIntResult truncateRes = os_sys_calls.ftruncate(result.rc_, total_size); + const Api::SysCallIntResult truncateRes = + os_sys_calls.ftruncate(result.rc_, sizeof(SharedMemory)); RELEASE_ASSERT(truncateRes.rc_ != -1, ""); } - const Api::SysCallPtrResult mmapRes = - os_sys_calls.mmap(nullptr, total_size, PROT_READ | PROT_WRITE, MAP_SHARED, result.rc_, 0); + const Api::SysCallPtrResult mmapRes = os_sys_calls.mmap( + nullptr, sizeof(SharedMemory), PROT_READ | PROT_WRITE, MAP_SHARED, result.rc_, 0); SharedMemory* shmem = reinterpret_cast(mmapRes.rc_); RELEASE_ASSERT(shmem != MAP_FAILED, ""); RELEASE_ASSERT((reinterpret_cast(shmem) % alignof(decltype(shmem))) == 0, ""); if (options.restartEpoch() == 0) { - shmem->size_ = total_size; - shmem->version_ = VERSION; - shmem->max_stats_ = options.maxStats(); - shmem->entry_size_ = entry_size; - shmem->initializeMutex(shmem->log_lock_); - shmem->initializeMutex(shmem->access_log_lock_); - shmem->initializeMutex(shmem->stat_lock_); - shmem->initializeMutex(shmem->init_lock_); + shmem->size_ = sizeof(SharedMemory); + shmem->version_ = HOT_RESTART_VERSION; + initializeMutex(shmem->log_lock_); + initializeMutex(shmem->access_log_lock_); } else { - RELEASE_ASSERT(shmem->size_ == total_size, ""); - RELEASE_ASSERT(shmem->version_ == VERSION, ""); - RELEASE_ASSERT(shmem->max_stats_ == options.maxStats(), ""); - RELEASE_ASSERT(shmem->entry_size_ == entry_size, ""); + RELEASE_ASSERT(shmem->size_ == sizeof(SharedMemory), + "Hot restart SharedMemory size mismatch! You must have hot restarted into a " + "not-hot-restart-compatible new version of Envoy."); + RELEASE_ASSERT(shmem->version_ == HOT_RESTART_VERSION, + "Hot restart version mismatch! You must have hot restarted into a " + "not-hot-restart-compatible new version of Envoy."); } - // Stats::RawStatData must be naturally aligned for atomics to work properly. - 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 // in a finer way. This will cause the startup to fail with an error code early, without // affecting any currently running processes. The process runner should try again later with some // back off and with the same hot restart epoch number. - uint64_t old_flags = shmem->flags_.fetch_or(Flags::INITIALIZING); - if (old_flags & Flags::INITIALIZING) { + uint64_t old_flags = shmem->flags_.fetch_or(SHMEM_FLAGS_INITIALIZING); + if (old_flags & SHMEM_FLAGS_INITIALIZING) { throw EnvoyException("previous envoy process is still initializing"); } - - return *shmem; + return shmem; } -void SharedMemory::initializeMutex(pthread_mutex_t& mutex) { +void initializeMutex(pthread_mutex_t& mutex) { pthread_mutexattr_t attribute; pthread_mutexattr_init(&attribute); pthread_mutexattr_setpshared(&attribute, PTHREAD_PROCESS_SHARED); @@ -116,369 +89,56 @@ void SharedMemory::initializeMutex(pthread_mutex_t& mutex) { pthread_mutex_init(&mutex, &attribute); } -std::string SharedMemory::version(uint64_t max_num_stats, - const Stats::StatsOptions& stats_options) { - return fmt::format("{}.{}.{}.{}", VERSION, sizeof(SharedMemory), max_num_stats, - stats_options.maxNameLength()); -} - -HotRestartImpl::HotRestartImpl(const Options& options, Stats::SymbolTable& symbol_table) - : options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), - shmem_(SharedMemory::initialize( - Stats::RawStatDataSet::numBytes(stats_set_options_, options_.statsOptions()), 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. - Thread::LockGuard lock(stat_lock_); - stats_set_ = - std::make_unique(stats_set_options_, options.restartEpoch() == 0, - shmem_.stats_set_data_, options_.statsOptions()); - } - stats_allocator_ = std::make_unique( - stat_lock_, *stats_set_, options_.statsOptions(), symbol_table); - my_domain_socket_ = bindDomainSocket(options.restartEpoch()); - child_address_ = createDomainSocketAddress((options.restartEpoch() + 1)); - initDomainSocketAddress(&parent_address_); - if (options.restartEpoch() != 0) { - parent_address_ = createDomainSocketAddress((options.restartEpoch() + -1)); - } - +HotRestartImpl::HotRestartImpl(const Options& options) + : as_child_(HotRestartingChild(options.baseId(), options.restartEpoch())), + as_parent_(HotRestartingParent(options.baseId(), options.restartEpoch())), + shmem_(attachSharedMemory(options)), log_lock_(shmem_->log_lock_), + access_log_lock_(shmem_->access_log_lock_) { // If our parent ever goes away just terminate us so that we don't have to rely on ops/launching // logic killing the entire process tree. We should never exist without our parent. int rc = prctl(PR_SET_PDEATHSIG, SIGTERM); RELEASE_ASSERT(rc != -1, ""); } -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 - // easily read single messages. - int fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0); - sockaddr_un address = createDomainSocketAddress(id); - Api::SysCallIntResult result = - os_sys_calls.bind(fd, reinterpret_cast(&address), sizeof(address)); - if (result.rc_ != 0) { - throw EnvoyException(fmt::format( - "unable to bind domain socket with id={}, (see --base-id option), address={}, errno={}: {}", - id, result.errno_, strerror(result.errno_))); - } - - return fd; -} - -void HotRestartImpl::initDomainSocketAddress(sockaddr_un* address) { - memset(address, 0, sizeof(*address)); - address->sun_family = AF_UNIX; -} - -sockaddr_un HotRestartImpl::createDomainSocketAddress(uint64_t id) { - // Right now we only allow a maximum of 3 concurrent envoy processes to be running. When the third - // starts up it will kill the oldest parent. - const uint64_t MAX_CONCURRENT_PROCESSES = 3; - id = id % MAX_CONCURRENT_PROCESSES; - - // This creates an anonymous domain socket name (where the first byte of the name of \0). - sockaddr_un address; - initDomainSocketAddress(&address); - StringUtil::strlcpy(&address.sun_path[1], - fmt::format("envoy_domain_socket_{}", options_.baseId() + id).c_str(), - sizeof(address.sun_path) - 1); - address.sun_path[0] = 0; - return address; -} - void HotRestartImpl::drainParentListeners() { - if (options_.restartEpoch() > 0) { - // No reply expected. - RpcBase rpc(RpcMessageType::DrainListenersRequest); - sendMessage(parent_address_, rpc); - } - + as_child_.drainParentListeners(); // At this point we are initialized and a new Envoy can startup if needed. - shmem_.flags_ &= ~SharedMemory::Flags::INITIALIZING; + shmem_->flags_ &= ~SHMEM_FLAGS_INITIALIZING; } int HotRestartImpl::duplicateParentListenSocket(const std::string& address) { - if (options_.restartEpoch() == 0 || parent_terminated_) { - return -1; - } - - RpcGetListenSocketRequest rpc; - ASSERT(address.length() < sizeof(rpc.address_)); - StringUtil::strlcpy(rpc.address_, address.c_str(), sizeof(rpc.address_)); - sendMessage(parent_address_, rpc); - RpcGetListenSocketReply* reply = - receiveTypedRpc(); - return reply->fd_; -} - -void HotRestartImpl::getParentStats(GetParentStatsInfo& info) { - // There exists a race condition during hot restart involving fetching parent stats. It looks like - // this: - // 1) There currently exist 2 Envoy processes (draining has not completed): P0 and P1. - // 2) New process (P2) comes up and passes the INITIALIZING check. - // 3) P2 proceeds to the parent admin shutdown phase. - // 4) This races with P1 fetching parent stats from P0. - // 5) Calling receiveTypedRpc() below picks up the wrong message. - // - // There are not any great solutions to this problem. We could potentially guard this using flags, - // but this is a legitimate race condition even under normal restart conditions, so exiting P2 - // with an error is not great. We could also rework all of this code so that P0<->P1 and P1<->P2 - // communication occur over different socket pairs. This could work, but is a large change. We - // could also potentially use connection oriented sockets and accept connections from our child, - // and connect to our parent, but again, this becomes complicated. - // - // Instead, we guard this condition with a lock. However, to avoid deadlock, we must tryLock() - // in this path, since this call runs in the same thread as the event loop that is receiving - // messages. If tryLock() fails it is sufficient to not return any parent stats. - Thread::TryLockGuard lock(init_lock_); - memset(&info, 0, sizeof(info)); - if (options_.restartEpoch() == 0 || parent_terminated_ || !lock.tryLock()) { - return; - } - - RpcBase rpc(RpcMessageType::GetStatsRequest); - sendMessage(parent_address_, rpc); - RpcGetStatsReply* reply = receiveTypedRpc(); - info.memory_allocated_ = reply->memory_allocated_; - info.num_connections_ = reply->num_connections_; + return as_child_.duplicateParentListenSocket(address); } void HotRestartImpl::initialize(Event::Dispatcher& dispatcher, Server::Instance& server) { - socket_event_ = dispatcher.createFileEvent( - my_domain_socket_, - [this](uint32_t events) -> void { - ASSERT(events == Event::FileReadyType::Read); - onSocketEvent(); - }, - Event::FileTriggerType::Edge, Event::FileReadyType::Read); - server_ = &server; -} - -HotRestartImpl::RpcBase* HotRestartImpl::receiveRpc(bool block) { - // By default the domain socket is non blocking. If we need to block, make it blocking first. - if (block) { - int rc = fcntl(my_domain_socket_, F_SETFL, 0); - RELEASE_ASSERT(rc != -1, ""); - } - - iovec iov[1]; - iov[0].iov_base = &rpc_buffer_[0]; - iov[0].iov_len = rpc_buffer_.size(); - - // We always setup to receive an FD even though most messages do not pass one. - uint8_t control_buffer[CMSG_SPACE(sizeof(int))]; - memset(control_buffer, 0, CMSG_SPACE(sizeof(int))); - - msghdr message; - memset(&message, 0, sizeof(message)); - message.msg_iov = iov; - message.msg_iovlen = 1; - message.msg_control = control_buffer; - message.msg_controllen = CMSG_SPACE(sizeof(int)); - - int rc = recvmsg(my_domain_socket_, &message, 0); - if (!block && rc == -1 && errno == EAGAIN) { - return nullptr; - } - - RELEASE_ASSERT(rc != -1, ""); - RELEASE_ASSERT(message.msg_flags == 0, ""); - - // Turn non-blocking back on if we made it blocking. - if (block) { - int rc = fcntl(my_domain_socket_, F_SETFL, O_NONBLOCK); - RELEASE_ASSERT(rc != -1, ""); - } - - RpcBase* rpc = reinterpret_cast(&rpc_buffer_[0]); - RELEASE_ASSERT(static_cast(rc) == rpc->length_, ""); - - // We should only get control data in a GetListenSocketReply. If that's the case, pull the - // cloned fd out of the control data and stick it into the RPC so that higher level code does - // need to deal with any of this. - for (cmsghdr* cmsg = CMSG_FIRSTHDR(&message); cmsg != nullptr; - cmsg = CMSG_NXTHDR(&message, cmsg)) { - - if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS && - rpc->type_ == RpcMessageType::GetListenSocketReply) { - - reinterpret_cast(rpc)->fd_ = - *reinterpret_cast(CMSG_DATA(cmsg)); - } else { - RELEASE_ASSERT(false, ""); - } - } - - return rpc; -} - -void HotRestartImpl::sendMessage(sockaddr_un& address, RpcBase& rpc) { - iovec iov[1]; - iov[0].iov_base = &rpc; - iov[0].iov_len = rpc.length_; - - msghdr message; - memset(&message, 0, sizeof(message)); - message.msg_name = &address; - message.msg_namelen = sizeof(address); - message.msg_iov = iov; - message.msg_iovlen = 1; - int rc = sendmsg(my_domain_socket_, &message, 0); - RELEASE_ASSERT(rc != -1, ""); -} - -void HotRestartImpl::onGetListenSocket(RpcGetListenSocketRequest& rpc) { - RpcGetListenSocketReply reply; - reply.fd_ = -1; - - Network::Address::InstanceConstSharedPtr addr = - Network::Utility::resolveUrl(std::string(rpc.address_)); - for (const auto& listener : server_->listenerManager().listeners()) { - if (*listener.get().socket().localAddress() == *addr) { - reply.fd_ = listener.get().socket().ioHandle().fd(); - break; - } - } - - if (reply.fd_ == -1) { - // In this case there is no fd to duplicate so we just send a normal message. - sendMessage(child_address_, reply); - } else { - iovec iov[1]; - iov[0].iov_base = &reply; - iov[0].iov_len = reply.length_; - - uint8_t control_buffer[CMSG_SPACE(sizeof(int))]; - memset(control_buffer, 0, CMSG_SPACE(sizeof(int))); - - msghdr message; - memset(&message, 0, sizeof(message)); - message.msg_name = &child_address_; - message.msg_namelen = sizeof(child_address_); - message.msg_iov = iov; - message.msg_iovlen = 1; - message.msg_control = control_buffer; - message.msg_controllen = CMSG_SPACE(sizeof(int)); - - cmsghdr* control_message = CMSG_FIRSTHDR(&message); - control_message->cmsg_level = SOL_SOCKET; - control_message->cmsg_type = SCM_RIGHTS; - control_message->cmsg_len = CMSG_LEN(sizeof(int)); - *reinterpret_cast(CMSG_DATA(control_message)) = reply.fd_; - - int rc = sendmsg(my_domain_socket_, &message, 0); - RELEASE_ASSERT(rc != -1, ""); - } + as_parent_.initialize(dispatcher, server); } -void HotRestartImpl::onSocketEvent() { - while (true) { - RpcBase* base_message = receiveRpc(false); - if (!base_message) { - return; - } - - switch (base_message->type_) { - case RpcMessageType::ShutdownAdminRequest: { - server_->shutdownAdmin(); - RpcShutdownAdminReply rpc; - rpc.original_start_time_ = server_->startTimeFirstEpoch(); - sendMessage(child_address_, rpc); - break; - } - - case RpcMessageType::GetListenSocketRequest: { - RpcGetListenSocketRequest* message = - reinterpret_cast(base_message); - onGetListenSocket(*message); - break; - } - - case RpcMessageType::GetStatsRequest: { - GetParentStatsInfo info; - server_->getParentStats(info); - RpcGetStatsReply rpc; - rpc.memory_allocated_ = info.memory_allocated_; - rpc.num_connections_ = info.num_connections_; - sendMessage(child_address_, rpc); - break; - } - - case RpcMessageType::DrainListenersRequest: { - server_->drainListeners(); - break; - } - - case RpcMessageType::TerminateRequest: { - ENVOY_LOG(info, "shutting down due to child request"); - kill(getpid(), SIGTERM); - break; - } - - default: { - RpcBase rpc(RpcMessageType::UnknownRequestReply); - sendMessage(child_address_, rpc); - break; - } - } - } +void HotRestartImpl::sendParentAdminShutdownRequest(time_t& original_start_time) { + as_child_.sendParentAdminShutdownRequest(original_start_time); } -void HotRestartImpl::shutdownParentAdmin(ShutdownParentAdminInfo& info) { - // See large comment in getParentStats() on why this operation is locked. - Thread::LockGuard lock(init_lock_); - if (options_.restartEpoch() == 0) { - return; - } - - RpcBase rpc(RpcMessageType::ShutdownAdminRequest); - sendMessage(parent_address_, rpc); - RpcShutdownAdminReply* reply = - receiveTypedRpc(); - info.original_start_time_ = reply->original_start_time_; -} +void HotRestartImpl::sendParentTerminateRequest() { as_child_.sendParentTerminateRequest(); } -void HotRestartImpl::terminateParent() { - if (options_.restartEpoch() == 0 || parent_terminated_) { - return; +HotRestart::ServerStatsFromParent +HotRestartImpl::mergeParentStatsIfAny(Stats::StoreRoot& stats_store) { + std::unique_ptr wrapper_msg = as_child_.getParentStats(); + ServerStatsFromParent response; + // getParentStats() will happily and cleanly return nullptr if we have no parent. + if (wrapper_msg) { + as_child_.mergeParentStats(stats_store, wrapper_msg->reply().stats()); + response.parent_memory_allocated_ = wrapper_msg->reply().stats().memory_allocated(); + response.parent_connections_ = wrapper_msg->reply().stats().num_connections(); } - - RpcBase rpc(RpcMessageType::TerminateRequest); - sendMessage(parent_address_, rpc); - parent_terminated_ = true; -} - -void HotRestartImpl::shutdown() { socket_event_.reset(); } - -std::string HotRestartImpl::version() { - Thread::LockGuard lock(stat_lock_); - return versionHelper(shmem_.maxStats(), options_.statsOptions(), *stats_set_); + return response; } -// Called from envoy --hot-restart-version -- needs to instantiate a RawStatDataSet so it -// can generate the version string. -std::string HotRestartImpl::hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len) { - Stats::StatsOptionsImpl stats_options; - stats_options.max_obj_name_length_ = max_stat_name_len - stats_options.maxStatSuffixLength(); - - const BlockMemoryHashSetOptions hash_set_options = blockMemHashOptions(max_num_stats); - const uint64_t bytes = Stats::RawStatDataSet::numBytes(hash_set_options, stats_options); - std::unique_ptr mem_buffer_for_dry_run_(new uint8_t[bytes]); +void HotRestartImpl::shutdown() { as_parent_.shutdown(); } - Stats::RawStatDataSet stats_set(hash_set_options, true /* init */, mem_buffer_for_dry_run_.get(), - stats_options); - - return versionHelper(max_num_stats, stats_options, stats_set); -} +std::string HotRestartImpl::version() { return hotRestartVersion(); } -std::string HotRestartImpl::versionHelper(uint64_t max_num_stats, - const Stats::StatsOptions& stats_options, - Stats::RawStatDataSet& stats_set) { - return SharedMemory::version(max_num_stats, stats_options) + "." + - stats_set.version(stats_options); +std::string HotRestartImpl::hotRestartVersion() { + return fmt::format("{}.{}", HOT_RESTART_VERSION, sizeof(SharedMemory)); } } // namespace Server diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index 8f77c7d616694..0244c20f69274 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -10,63 +10,43 @@ #include "envoy/common/platform.h" #include "envoy/server/hot_restart.h" -#include "envoy/server/options.h" -#include "envoy/stats/stats_options.h" #include "common/common/assert.h" -#include "common/stats/raw_stat_data.h" +#include "common/stats/heap_stat_data.h" + +#include "server/hot_restarting_child.h" +#include "server/hot_restarting_parent.h" namespace Envoy { 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 HOT_RESTART_VERSION = 11; + /** * Shared memory segment. This structure is laid directly into shared memory and is used amongst * all running envoy processes. */ -class SharedMemory { -public: - static void configure(uint64_t max_num_stats, uint64_t max_stat_name_len); - static std::string version(uint64_t max_num_stats, const Stats::StatsOptions& stats_options); - - // Made public for testing. - static const uint64_t VERSION; - - int64_t maxStats() const { return max_stats_; } - -private: - struct Flags { - static const uint64_t INITIALIZING = 0x1; - }; - - // Due to the flexible-array-length of stats_set_data_, c-style allocation - // and initialization are necessary. - SharedMemory() = delete; - ~SharedMemory() = delete; - - /** - * Initialize the shared memory segment, depending on whether we should be the first running - * envoy, or a host restarted envoy process. - */ - static SharedMemory& initialize(uint64_t stats_set_size, const Options& options); - - /** - * Initialize a pthread mutex for process shared locking. - */ - void initializeMutex(pthread_mutex_t& mutex); - +struct SharedMemory { uint64_t size_; uint64_t version_; - uint64_t max_stats_; - uint64_t entry_size_; - std::atomic flags_; pthread_mutex_t log_lock_; pthread_mutex_t access_log_lock_; - pthread_mutex_t stat_lock_; - pthread_mutex_t init_lock_; - alignas(BlockMemoryHashSet) uint8_t stats_set_data_[]; - - friend class HotRestartImpl; + std::atomic flags_; }; +static const uint64_t SHMEM_FLAGS_INITIALIZING = 0x1; + +/** + * Initialize the shared memory segment, depending on whether we are the first running + * envoy, or a host restarted envoy process. + */ +SharedMemory* attachSharedMemory(const Options& options); + +/** + * Initialize a pthread mutex for process shared locking. + */ +void initializeMutex(pthread_mutex_t& mutex); /** * Implementation of Thread::BasicLockable that operates on a process shared pthread mutex. @@ -110,118 +90,39 @@ class ProcessSharedMutex : public Thread::BasicLockable { }; /** - * Implementation of HotRestart built for Linux. + * Implementation of HotRestart built for Linux. Most of the "protocol" type logic is split out into + * HotRestarting{Base,Parent,Child}. This class ties all that to shared memory and version logic. */ -class HotRestartImpl : public HotRestart, Logger::Loggable { +class HotRestartImpl : public HotRestart { public: - HotRestartImpl(const Options& options, Stats::SymbolTable& symbol_table); + HotRestartImpl(const Options& options); // Server::HotRestart void drainParentListeners() override; int duplicateParentListenSocket(const std::string& address) override; - void getParentStats(GetParentStatsInfo& info) override; void initialize(Event::Dispatcher& dispatcher, Server::Instance& server) override; - void shutdownParentAdmin(ShutdownParentAdminInfo& info) override; - void terminateParent() override; + void sendParentAdminShutdownRequest(time_t& original_start_time) override; + void sendParentTerminateRequest() override; + ServerStatsFromParent mergeParentStatsIfAny(Stats::StoreRoot& stats_store) override; void shutdown() override; std::string version() override; Thread::BasicLockable& logLock() override { return log_lock_; } Thread::BasicLockable& accessLogLock() override { return access_log_lock_; } - Stats::RawStatDataAllocator& statsAllocator() override { return *stats_allocator_; } /** * envoy --hot_restart_version doesn't initialize Envoy, but computes the version string * based on the configured options. */ - static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len); + static std::string hotRestartVersion(); private: - enum class RpcMessageType { - DrainListenersRequest = 1, - GetListenSocketRequest = 2, - GetListenSocketReply = 3, - ShutdownAdminRequest = 4, - ShutdownAdminReply = 5, - TerminateRequest = 6, - UnknownRequestReply = 7, - GetStatsRequest = 8, - GetStatsReply = 9 - }; - - PACKED_STRUCT(struct RpcBase { - RpcBase(RpcMessageType type, uint64_t length = sizeof(RpcBase)) - : type_(type), length_(length) {} - - RpcMessageType type_; - uint64_t length_; - }); - - PACKED_STRUCT(struct RpcGetListenSocketRequest - : public RpcBase { - RpcGetListenSocketRequest() - : RpcBase(RpcMessageType::GetListenSocketRequest, sizeof(*this)) {} - - char address_[256]{0}; - }); - - PACKED_STRUCT(struct RpcGetListenSocketReply - : public RpcBase { - RpcGetListenSocketReply() - : RpcBase(RpcMessageType::GetListenSocketReply, sizeof(*this)) {} - - int fd_{0}; - }); - - PACKED_STRUCT(struct RpcShutdownAdminReply - : public RpcBase { - RpcShutdownAdminReply() - : RpcBase(RpcMessageType::ShutdownAdminReply, sizeof(*this)) {} - - uint64_t original_start_time_{0}; - }); - - PACKED_STRUCT(struct RpcGetStatsReply - : public RpcBase { - RpcGetStatsReply() : RpcBase(RpcMessageType::GetStatsReply, sizeof(*this)) {} - - uint64_t memory_allocated_{0}; - uint64_t num_connections_{0}; - uint64_t unused_[16]{0}; - }); - - template rpc_class* receiveTypedRpc() { - RpcBase* base_message = receiveRpc(true); - RELEASE_ASSERT(base_message->length_ == sizeof(rpc_class), ""); - RELEASE_ASSERT(base_message->type_ == rpc_type, ""); - return reinterpret_cast(base_message); - } - - int bindDomainSocket(uint64_t id); - void initDomainSocketAddress(sockaddr_un* address); - sockaddr_un createDomainSocketAddress(uint64_t id); - void onGetListenSocket(RpcGetListenSocketRequest& rpc); - void onSocketEvent(); - RpcBase* receiveRpc(bool block); - void sendMessage(sockaddr_un& address, RpcBase& rpc); - static std::string versionHelper(uint64_t max_num_stats, const Stats::StatsOptions& stats_options, - Stats::RawStatDataSet& stats_set); - - const Options& options_; - BlockMemoryHashSetOptions stats_set_options_; - SharedMemory& shmem_; - std::unique_ptr stats_set_ GUARDED_BY(stat_lock_); - std::unique_ptr stats_allocator_; + HotRestartingChild as_child_; + HotRestartingParent as_parent_; + // This pointer is shared memory, and is expected to exist until process end. + // It will automatically be unmapped when the process terminates. + SharedMemory* shmem_; ProcessSharedMutex log_lock_; ProcessSharedMutex access_log_lock_; - ProcessSharedMutex stat_lock_; - ProcessSharedMutex init_lock_; - int my_domain_socket_{-1}; - sockaddr_un parent_address_; - sockaddr_un child_address_; - Event::FileEventPtr socket_event_; - std::array rpc_buffer_; - Server::Instance* server_{}; - bool parent_terminated_{}; }; } // namespace Server diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index 756bb8d217fde..010f93340ed52 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -15,25 +15,23 @@ namespace Server { */ class HotRestartNopImpl : public Server::HotRestart { public: - explicit HotRestartNopImpl(Stats::SymbolTable& symbol_table) : stats_allocator_(symbol_table) {} - // Server::HotRestart void drainParentListeners() override {} int duplicateParentListenSocket(const std::string&) override { return -1; } - void getParentStats(GetParentStatsInfo& info) override { memset(&info, 0, sizeof(info)); } void initialize(Event::Dispatcher&, Server::Instance&) override {} - void shutdownParentAdmin(ShutdownParentAdminInfo&) override {} - void terminateParent() override {} + void sendParentAdminShutdownRequest(time_t&) override {} + void sendParentTerminateRequest() override {} + ServerStatsFromParent mergeParentStatsIfAny(Stats::StoreRoot&) override { + return ServerStatsFromParent(); + } void shutdown() override {} std::string version() override { return "disabled"; } Thread::BasicLockable& logLock() override { return log_lock_; } Thread::BasicLockable& accessLogLock() override { return access_log_lock_; } - Stats::StatDataAllocator& statsAllocator() override { return stats_allocator_; } private: Thread::MutexBasicLockable log_lock_; Thread::MutexBasicLockable access_log_lock_; - Stats::HeapStatDataAllocator stats_allocator_; }; } // namespace Server diff --git a/source/server/hot_restarting_base.cc b/source/server/hot_restarting_base.cc new file mode 100644 index 0000000000000..20e0a6c0347bc --- /dev/null +++ b/source/server/hot_restarting_base.cc @@ -0,0 +1,213 @@ +#include "server/hot_restarting_base.h" + +#include "common/api/os_sys_calls_impl.h" +#include "common/common/utility.h" + +namespace Envoy { +namespace Server { + +using HotRestartMessage = envoy::HotRestartMessage; + +void HotRestartingBase::initDomainSocketAddress(sockaddr_un* address) { + memset(address, 0, sizeof(*address)); + address->sun_family = AF_UNIX; +} + +sockaddr_un HotRestartingBase::createDomainSocketAddress(uint64_t id, const std::string& role) { + // Right now we only allow a maximum of 3 concurrent envoy processes to be running. When the third + // starts up it will kill the oldest parent. + const uint64_t MAX_CONCURRENT_PROCESSES = 3; + id = id % MAX_CONCURRENT_PROCESSES; + + // This creates an anonymous domain socket name (where the first byte of the name of \0). + sockaddr_un address; + initDomainSocketAddress(&address); + StringUtil::strlcpy(&address.sun_path[1], + fmt::format("envoy_domain_socket_{}_{}", role, base_id_ + id).c_str(), + sizeof(address.sun_path) - 1); + address.sun_path[0] = 0; + return address; +} + +void HotRestartingBase::bindDomainSocket(uint64_t id, const std::string& role) { + 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 + // easily read single messages. + my_domain_socket_ = socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0); + sockaddr_un address = createDomainSocketAddress(id, role); + Api::SysCallIntResult result = + os_sys_calls.bind(my_domain_socket_, reinterpret_cast(&address), sizeof(address)); + if (result.rc_ != 0) { + throw EnvoyException( + fmt::format("unable to bind domain socket with id={} (see --base-id option)", id)); + } +} + +void HotRestartingBase::sendHotRestartMessage(sockaddr_un& address, + const HotRestartMessage& proto) { + const uint64_t serialized_size = proto.ByteSizeLong(); + const uint64_t total_size = sizeof(uint64_t) + serialized_size; + // Fill with uint64_t 'length' followed by the serialized HotRestartMessage. + std::vector send_buf; + send_buf.resize(total_size); + *reinterpret_cast(send_buf.data()) = htobe64(serialized_size); + RELEASE_ASSERT(proto.SerializeWithCachedSizesToArray(send_buf.data() + sizeof(uint64_t)), + "failed to serialize a HotRestartMessage"); + + RELEASE_ASSERT(fcntl(my_domain_socket_, F_SETFL, 0) != -1, + fmt::format("Set domain socket blocking failed, errno = {}", errno)); + + uint8_t* next_byte_to_send = send_buf.data(); + uint64_t sent = 0; + while (sent < total_size) { + const uint64_t cur_chunk_size = std::min(MaxSendmsgSize, total_size - sent); + iovec iov[1]; + iov[0].iov_base = next_byte_to_send; + iov[0].iov_len = cur_chunk_size; + next_byte_to_send += cur_chunk_size; + sent += cur_chunk_size; + msghdr message; + memset(&message, 0, sizeof(message)); + message.msg_name = &address; + message.msg_namelen = sizeof(address); + message.msg_iov = iov; + message.msg_iovlen = 1; + + // Control data stuff, only relevant for the fd passing done with PassListenSocketReply. + uint8_t control_buffer[CMSG_SPACE(sizeof(int))]; + if (replyIsExpectedType(&proto, HotRestartMessage::Reply::kPassListenSocket) && + proto.reply().pass_listen_socket().fd() != -1) { + memset(control_buffer, 0, CMSG_SPACE(sizeof(int))); + message.msg_control = control_buffer; + message.msg_controllen = CMSG_SPACE(sizeof(int)); + cmsghdr* control_message = CMSG_FIRSTHDR(&message); + control_message->cmsg_level = SOL_SOCKET; + control_message->cmsg_type = SCM_RIGHTS; + control_message->cmsg_len = CMSG_LEN(sizeof(int)); + *reinterpret_cast(CMSG_DATA(control_message)) = proto.reply().pass_listen_socket().fd(); + ASSERT(sent == total_size, "an fd passing message was too long for one sendmsg()."); + } + + const int rc = sendmsg(my_domain_socket_, &message, 0); + RELEASE_ASSERT(rc == static_cast(cur_chunk_size), + fmt::format("hot restart sendmsg() failed: returned {}, errno {}", rc, errno)); + } + RELEASE_ASSERT(fcntl(my_domain_socket_, F_SETFL, O_NONBLOCK) != -1, + fmt::format("Set domain socket nonblocking failed, errno = {}", errno)); +} + +bool HotRestartingBase::replyIsExpectedType(const HotRestartMessage* proto, + HotRestartMessage::Reply::ReplyCase oneof_type) const { + return proto != nullptr && proto->requestreply_case() == HotRestartMessage::kReply && + proto->reply().reply_case() == oneof_type; +} + +// Pull the cloned fd, if present, out of the control data and write it into the +// PassListenSocketReply proto; the higher level code will see a listening fd that Just Works. We +// should only get control data in a PassListenSocketReply, it should only be the fd passing type, +// and there should only be one at a time. Crash on any other control data. +void HotRestartingBase::getPassedFdIfPresent(HotRestartMessage* out, msghdr* message) { + cmsghdr* cmsg = CMSG_FIRSTHDR(message); + if (cmsg != nullptr) { + RELEASE_ASSERT(cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS && + replyIsExpectedType(out, HotRestartMessage::Reply::kPassListenSocket), + "recvmsg() came with control data when the message's purpose was not to pass a " + "file descriptor."); + + out->mutable_reply()->mutable_pass_listen_socket()->set_fd( + *reinterpret_cast(CMSG_DATA(cmsg))); + + RELEASE_ASSERT(CMSG_NXTHDR(message, cmsg) == nullptr, + "More than one control data on a single hot restart recvmsg()."); + } +} + +// While in use, recv_buf_ is always >= MaxSendmsgSize. In between messages, it is kept empty, to be +// grown back to MaxSendmsgSize at the start of the next message. +void HotRestartingBase::initRecvBufIfNewMessage() { + if (recv_buf_.empty()) { + ASSERT(cur_msg_recvd_bytes_ == 0); + ASSERT(!expected_proto_length_.has_value()); + recv_buf_.resize(MaxSendmsgSize); + } +} + +// Must only be called when recv_buf_ contains a full proto. Returns that proto, and resets all of +// our receive-buffering state back to empty, to await a new message. +std::unique_ptr HotRestartingBase::parseProtoAndResetState() { + auto ret = std::make_unique(); + RELEASE_ASSERT( + ret->ParseFromArray(recv_buf_.data() + sizeof(uint64_t), expected_proto_length_.value()), + "failed to parse a HotRestartMessage."); + recv_buf_.resize(0); + cur_msg_recvd_bytes_ = 0; + expected_proto_length_.reset(); + return ret; +} + +std::unique_ptr HotRestartingBase::receiveHotRestartMessage(Blocking block) { + // By default the domain socket is non blocking. If we need to block, make it blocking first. + if (block == Blocking::Yes) { + RELEASE_ASSERT(fcntl(my_domain_socket_, F_SETFL, 0) != -1, + fmt::format("Set domain socket blocking failed, errno = {}", errno)); + } + + initRecvBufIfNewMessage(); + + iovec iov[1]; + msghdr message; + uint8_t control_buffer[CMSG_SPACE(sizeof(int))]; + std::unique_ptr ret = nullptr; + while (!ret) { + iov[0].iov_base = recv_buf_.data() + cur_msg_recvd_bytes_; + iov[0].iov_len = MaxSendmsgSize; + + // We always setup to receive an FD even though most messages do not pass one. + memset(control_buffer, 0, CMSG_SPACE(sizeof(int))); + memset(&message, 0, sizeof(message)); + message.msg_iov = iov; + message.msg_iovlen = 1; + message.msg_control = control_buffer; + message.msg_controllen = CMSG_SPACE(sizeof(int)); + + const int recvmsg_rc = recvmsg(my_domain_socket_, &message, 0); + if (block == Blocking::No && recvmsg_rc == -1 && errno == EAGAIN) { + return nullptr; + } + RELEASE_ASSERT(recvmsg_rc != -1, fmt::format("recvmsg() returned -1, errno = {}", errno)); + RELEASE_ASSERT(message.msg_flags == 0, + fmt::format("recvmsg() left msg_flags = {}", message.msg_flags)); + cur_msg_recvd_bytes_ += recvmsg_rc; + + // If we don't already know 'length', we're at the start of a new length+protobuf message! + if (!expected_proto_length_.has_value()) { + // We are not ok with messages so fragmented that the length doesn't even come in one piece. + RELEASE_ASSERT(recvmsg_rc >= 8, "received a brokenly tiny message fragment."); + + expected_proto_length_ = be64toh(*reinterpret_cast(recv_buf_.data())); + // Expand the buffer from its default 4096 if this message is going to be longer. + if (expected_proto_length_.value() > MaxSendmsgSize - sizeof(uint64_t)) { + recv_buf_.resize(expected_proto_length_.value() + sizeof(uint64_t)); + cur_msg_recvd_bytes_ = recvmsg_rc; + } + } + // If we have received beyond the end of the current in-flight proto, then next is misaligned. + RELEASE_ASSERT(cur_msg_recvd_bytes_ <= sizeof(uint64_t) + expected_proto_length_.value(), + "received a length+protobuf message not aligned to start of sendmsg()."); + + if (cur_msg_recvd_bytes_ == sizeof(uint64_t) + expected_proto_length_.value()) { + ret = parseProtoAndResetState(); + } + } + + // Turn non-blocking back on if we made it blocking. + if (block == Blocking::Yes) { + RELEASE_ASSERT(fcntl(my_domain_socket_, F_SETFL, O_NONBLOCK) != -1, + fmt::format("Set domain socket nonblocking failed, errno = {}", errno)); + } + getPassedFdIfPresent(ret.get(), &message); + return ret; +} + +} // namespace Server +} // namespace Envoy diff --git a/source/server/hot_restarting_base.h b/source/server/hot_restarting_base.h new file mode 100644 index 0000000000000..a5a0fbb264afd --- /dev/null +++ b/source/server/hot_restarting_base.h @@ -0,0 +1,91 @@ +#pragma once + +#include +#include + +#include +#include +#include +#include + +#include "envoy/common/platform.h" +#include "envoy/server/hot_restart.h" +#include "envoy/server/options.h" + +#include "common/common/assert.h" + +namespace Envoy { +namespace Server { + +/** + * Logic shared by the implementations of both sides of the child<-->parent hot restart protocol: + * domain socket communication, and our ad hoc RPC protocol. + */ +class HotRestartingBase { +protected: + HotRestartingBase(uint64_t base_id) : base_id_(base_id) {} + + void initDomainSocketAddress(sockaddr_un* address); + sockaddr_un createDomainSocketAddress(uint64_t id, const std::string& role); + void bindDomainSocket(uint64_t id, const std::string& role); + int myDomainSocket() const { return my_domain_socket_; } + + // Protocol description: + // + // In each direction between parent<-->child, a series of pairs of: + // A uint64 'length' (bytes in network order), + // followed by 'length' bytes of a serialized HotRestartMessage. + // Each new message must start in a new sendmsg datagram, i.e. 'length' must always start at byte + // 0. Each sendmsg datagram can be up to 4096 bytes (including 'length' if present). When the + // serialized protobuf is longer than 4096-8 bytes, and so cannot fit in just one datagram, it is + // delivered by a series of datagrams. In each of these continuation datagrams, the protobuf data + // starts at byte 0. + // + // There is no mechanism to explicitly pair responses to requests. However, the child initiates + // all exchanges, and blocks until a reply is received, so there is implicit pairing. + void sendHotRestartMessage(sockaddr_un& address, const envoy::HotRestartMessage& proto); + + enum class Blocking { Yes, No }; + // Receive data, possibly enough to build one of our protocol messages. + // If block is true, blocks until a full protocol message is available. + // If block is false, returns nullptr if we run out of data to receive before a full protocol + // message is available. In either case, the HotRestartingBase may end up buffering some data for + // the next protocol message, even if the function returns a protobuf. + std::unique_ptr receiveHotRestartMessage(Blocking block); + + bool replyIsExpectedType(const envoy::HotRestartMessage* proto, + envoy::HotRestartMessage::Reply::ReplyCase oneof_type) const; + +private: + void getPassedFdIfPresent(envoy::HotRestartMessage* out, msghdr* message); + std::unique_ptr parseProtoAndResetState(); + void initRecvBufIfNewMessage(); + + // An int in [0, MAX_CONCURRENT_PROCESSES). As hot restarts happen, each next process gets the + // next of 0,1,2,0,1,... + // A HotRestartingBase's domain socket's name contains its base_id_ value, and so we can use + // this value to determine which domain socket name to treat as our parent, and which to treat as + // our child. (E.g. if we are 2, 1 is parent and 0 is child). + const uint64_t base_id_; + int my_domain_socket_{-1}; + + const uint64_t MaxSendmsgSize = 4096; + + // State for the receiving half of the protocol. + // + // When filled, the size in bytes that the in-flight HotRestartMessage should be. + // When empty, we're ready to start receiving a new message (starting with a uint64 'length'). + absl::optional expected_proto_length_; + // How much of the current in-flight message (including both the uint64 'length', plus the proto + // itself) we have received. Once this equals expected_proto_length_ + sizeof(uint64_t), we're + // ready to parse the HotRestartMessage. Should be set to 0 in between messages, to indicate + // readiness for a new message. + uint64_t cur_msg_recvd_bytes_{}; + // The first 8 bytes will always be the raw net-order bytes of the current value of + // expected_proto_length_. The protobuf partial data starts at byte 8. + // Should be resized to 0 in between messages, to indicate readiness for a new message. + std::vector recv_buf_; +}; + +} // namespace Server +} // namespace Envoy diff --git a/source/server/hot_restarting_child.cc b/source/server/hot_restarting_child.cc new file mode 100644 index 0000000000000..3ff34ef0a844f --- /dev/null +++ b/source/server/hot_restarting_child.cc @@ -0,0 +1,97 @@ +#include "server/hot_restarting_child.h" + +#include "common/common/utility.h" + +namespace Envoy { +namespace Server { + +using HotRestartMessage = envoy::HotRestartMessage; + +HotRestartingChild::HotRestartingChild(int base_id, int restart_epoch) + : HotRestartingBase(base_id), restart_epoch_(restart_epoch) { + initDomainSocketAddress(&parent_address_); + if (restart_epoch_ != 0) { + parent_address_ = createDomainSocketAddress(restart_epoch_ + -1, "parent"); + } + bindDomainSocket(restart_epoch_, "child"); +} + +int HotRestartingChild::duplicateParentListenSocket(const std::string& address) { + if (restart_epoch_ == 0 || parent_terminated_) { + return -1; + } + + HotRestartMessage wrapped_request; + wrapped_request.mutable_request()->mutable_pass_listen_socket()->set_address(address); + sendHotRestartMessage(parent_address_, wrapped_request); + + std::unique_ptr wrapped_reply = receiveHotRestartMessage(Blocking::Yes); + if (!replyIsExpectedType(wrapped_reply.get(), HotRestartMessage::Reply::kPassListenSocket)) { + return -1; + } + return wrapped_reply->reply().pass_listen_socket().fd(); +} + +std::unique_ptr HotRestartingChild::getParentStats() { + if (restart_epoch_ == 0 || parent_terminated_) { + return nullptr; + } + + HotRestartMessage wrapped_request; + wrapped_request.mutable_request()->mutable_stats(); + sendHotRestartMessage(parent_address_, wrapped_request); + + std::unique_ptr wrapped_reply = receiveHotRestartMessage(Blocking::Yes); + RELEASE_ASSERT(replyIsExpectedType(wrapped_reply.get(), HotRestartMessage::Reply::kStats), + "Hot restart parent did not respond as expected to get stats request."); + return wrapped_reply; +} + +void HotRestartingChild::drainParentListeners() { + if (restart_epoch_ == 0 || parent_terminated_) { + return; + } + // No reply expected. + HotRestartMessage wrapped_request; + wrapped_request.mutable_request()->mutable_drain_listeners(); + sendHotRestartMessage(parent_address_, wrapped_request); +} + +void HotRestartingChild::sendParentAdminShutdownRequest(time_t& original_start_time) { + if (restart_epoch_ == 0 || parent_terminated_) { + return; + } + + HotRestartMessage wrapped_request; + wrapped_request.mutable_request()->mutable_shutdown_admin(); + sendHotRestartMessage(parent_address_, wrapped_request); + + std::unique_ptr wrapped_reply = receiveHotRestartMessage(Blocking::Yes); + RELEASE_ASSERT(replyIsExpectedType(wrapped_reply.get(), HotRestartMessage::Reply::kShutdownAdmin), + "Hot restart parent did not respond as expected to ShutdownParentAdmin."); + original_start_time = wrapped_reply->reply().shutdown_admin().original_start_time_unix_seconds(); +} + +void HotRestartingChild::sendParentTerminateRequest() { + if (restart_epoch_ == 0 || parent_terminated_) { + return; + } + HotRestartMessage wrapped_request; + wrapped_request.mutable_request()->mutable_terminate(); + sendHotRestartMessage(parent_address_, wrapped_request); + parent_terminated_ = true; + // Once setting parent_terminated_ == true, we can send no more hot restart RPCs, and therefore + // receive no more responses, including stats. So, now safe to forget our stat transferral state. + stat_merger_.reset(); +} + +void HotRestartingChild::mergeParentStats(Stats::Store& stats_store, + const HotRestartMessage::Reply::Stats& stats_proto) { + if (!stat_merger_) { + stat_merger_ = std::make_unique(stats_store); + } + stat_merger_->mergeStats(stats_proto.counter_deltas(), stats_proto.gauges()); +} + +} // namespace Server +} // namespace Envoy diff --git a/source/server/hot_restarting_child.h b/source/server/hot_restarting_child.h new file mode 100644 index 0000000000000..08c3cc27359f1 --- /dev/null +++ b/source/server/hot_restarting_child.h @@ -0,0 +1,33 @@ +#pragma once + +#include "common/stats/stat_merger.h" + +#include "server/hot_restarting_base.h" + +namespace Envoy { +namespace Server { + +/** + * The child half of hot restarting. Issues requests and commands to the parent. + */ +class HotRestartingChild : HotRestartingBase, Logger::Loggable { +public: + HotRestartingChild(int base_id, int restart_epoch); + + int duplicateParentListenSocket(const std::string& address); + std::unique_ptr getParentStats(); + void drainParentListeners(); + void sendParentAdminShutdownRequest(time_t& original_start_time); + void sendParentTerminateRequest(); + void mergeParentStats(Stats::Store& stats_store, + const envoy::HotRestartMessage::Reply::Stats& stats_proto); + +private: + const int restart_epoch_; + bool parent_terminated_{}; + sockaddr_un parent_address_; + std::unique_ptr stat_merger_{}; +}; + +} // namespace Server +} // namespace Envoy diff --git a/source/server/hot_restarting_parent.cc b/source/server/hot_restarting_parent.cc new file mode 100644 index 0000000000000..becef491b5fae --- /dev/null +++ b/source/server/hot_restarting_parent.cc @@ -0,0 +1,130 @@ +#include "server/hot_restarting_parent.h" + +#include "envoy/server/instance.h" + +#include "common/memory/stats.h" +#include "common/network/utility.h" + +namespace Envoy { +namespace Server { + +using HotRestartMessage = envoy::HotRestartMessage; + +HotRestartingParent::HotRestartingParent(int base_id, int restart_epoch) + : HotRestartingBase(base_id), restart_epoch_(restart_epoch) { + child_address_ = createDomainSocketAddress(restart_epoch_ + 1, "child"); + bindDomainSocket(restart_epoch_, "parent"); +} + +void HotRestartingParent::initialize(Event::Dispatcher& dispatcher, Server::Instance& server) { + socket_event_ = dispatcher.createFileEvent( + myDomainSocket(), + [this](uint32_t events) -> void { + ASSERT(events == Event::FileReadyType::Read); + onSocketEvent(); + }, + Event::FileTriggerType::Edge, Event::FileReadyType::Read); + internal_ = std::make_unique(&server); +} + +void HotRestartingParent::onSocketEvent() { + std::unique_ptr wrapped_request; + while ((wrapped_request = receiveHotRestartMessage(Blocking::No))) { + if (wrapped_request->requestreply_case() == HotRestartMessage::kReply) { + ENVOY_LOG(error, "child sent us a HotRestartMessage reply (we want requests); ignoring."); + HotRestartMessage wrapped_reply; + wrapped_reply.set_didnt_recognize_your_last_message(true); + sendHotRestartMessage(child_address_, wrapped_reply); + continue; + } + switch (wrapped_request->request().request_case()) { + case HotRestartMessage::Request::kShutdownAdmin: { + sendHotRestartMessage(child_address_, internal_->shutdownAdmin()); + break; + } + + case HotRestartMessage::Request::kPassListenSocket: { + sendHotRestartMessage(child_address_, + internal_->getListenSocketsForChild(wrapped_request->request())); + break; + } + + case HotRestartMessage::Request::kStats: { + HotRestartMessage wrapped_reply; + internal_->exportStatsToChild(wrapped_reply.mutable_reply()->mutable_stats()); + sendHotRestartMessage(child_address_, wrapped_reply); + break; + } + + case HotRestartMessage::Request::kDrainListeners: { + internal_->drainListeners(); + break; + } + + case HotRestartMessage::Request::kTerminate: { + ENVOY_LOG(info, "shutting down due to child request"); + kill(getpid(), SIGTERM); + break; + } + + default: { + ENVOY_LOG(error, "child sent us an unfamiliar type of HotRestartMessage; ignoring."); + HotRestartMessage wrapped_reply; + wrapped_reply.set_didnt_recognize_your_last_message(true); + sendHotRestartMessage(child_address_, wrapped_reply); + break; + } + } + } +} + +void HotRestartingParent::shutdown() { socket_event_.reset(); } + +HotRestartMessage HotRestartingParent::Internal::shutdownAdmin() { + server_->shutdownAdmin(); + HotRestartMessage wrapped_reply; + wrapped_reply.mutable_reply()->mutable_shutdown_admin()->set_original_start_time_unix_seconds( + server_->startTimeFirstEpoch()); + return wrapped_reply; +} + +HotRestartMessage +HotRestartingParent::Internal::getListenSocketsForChild(const HotRestartMessage::Request& request) { + HotRestartMessage wrapped_reply; + wrapped_reply.mutable_reply()->mutable_pass_listen_socket()->set_fd(-1); + Network::Address::InstanceConstSharedPtr addr = + Network::Utility::resolveUrl(request.pass_listen_socket().address()); + for (const auto& listener : server_->listenerManager().listeners()) { + if (*listener.get().socket().localAddress() == *addr) { + wrapped_reply.mutable_reply()->mutable_pass_listen_socket()->set_fd( + listener.get().socket().ioHandle().fd()); + break; + } + } + return wrapped_reply; +} + +// TODO(fredlas) if there are enough stats for stat name length to become an issue, this current +// implementation can negate the benefit of symbolized stat names by periodically reaching the +// magnitude of memory usage that they are meant to avoid, since this map holds full-string +// names. The problem can be solved by splitting the export up over many chunks. +void HotRestartingParent::Internal::exportStatsToChild(HotRestartMessage::Reply::Stats* stats) { + for (const auto& gauge : server_->stats().gauges()) { + (*stats->mutable_gauges())[gauge->name()] = gauge->value(); + } + for (const auto& counter : server_->stats().counters()) { + // The hot restart parent is expected to have stopped its normal stat exporting (and so + // latching) by the time it begins exporting to the hot restart child. + uint64_t latched_value = counter->latch(); + if (latched_value > 0) { + (*stats->mutable_counter_deltas())[counter->name()] = latched_value; + } + } + stats->set_memory_allocated(Memory::Stats::totalCurrentlyAllocated()); + stats->set_num_connections(server_->listenerManager().numConnections()); +} + +void HotRestartingParent::Internal::drainListeners() { server_->drainListeners(); } + +} // namespace Server +} // namespace Envoy diff --git a/source/server/hot_restarting_parent.h b/source/server/hot_restarting_parent.h new file mode 100644 index 0000000000000..4afef96a14a6b --- /dev/null +++ b/source/server/hot_restarting_parent.h @@ -0,0 +1,49 @@ +#pragma once + +#include "common/common/hash.h" + +#include "server/hot_restarting_base.h" + +namespace Envoy { +namespace Server { + +/** + * The parent half of hot restarting. Listens for requests and commands from the child. + * This outer class only handles evented socket I/O. The actual hot restart logic lives in + * HotRestartingParent::Internal. + */ +class HotRestartingParent : HotRestartingBase, Logger::Loggable { +public: + HotRestartingParent(int base_id, int restart_epoch); + void initialize(Event::Dispatcher& dispatcher, Server::Instance& server); + void shutdown(); + + // The hot restarting parent's hot restart logic. Each function is meant to be called to fulfill a + // request from the child for that action. + class Internal { + public: + explicit Internal(Server::Instance* server) : server_(server) {} + // Return value is the response to return to the child. + envoy::HotRestartMessage shutdownAdmin(); + // Return value is the response to return to the child. + envoy::HotRestartMessage + getListenSocketsForChild(const envoy::HotRestartMessage::Request& request); + // 'stats' is a field in the reply protobuf to be sent to the child, which we should populate. + void exportStatsToChild(envoy::HotRestartMessage::Reply::Stats* stats); + void drainListeners(); + + private: + Server::Instance* const server_{}; + }; + +private: + void onSocketEvent(); + + const int restart_epoch_; + sockaddr_un child_address_; + Event::FileEventPtr socket_event_; + std::unique_ptr internal_; +}; + +} // namespace Server +} // namespace Envoy diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 5f6a199136d65..ef62769408778 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -17,22 +17,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. -#ifndef ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH -#define ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH 60 -#endif - -#if ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH < 60 -#error "ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH must be >= 60" -#endif - namespace Envoy { OptionsImpl::OptionsImpl(int argc, const char* const* argv, const HotRestartVersionCb& hot_restart_version_cb, @@ -106,15 +90,11 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, "traffic normally) or 'validate' (validate configs and exit).", false, "serve", "string", cmd); TCLAP::ValueArg max_stats("", "max-stats", - "Maximum number of stats gauges and counters " - "that can be allocated in shared memory.", - false, ENVOY_DEFAULT_MAX_STATS, "uint64_t", cmd); + "Deprecated and unused; please do not specify.", false, 123, + "uint64_t", cmd); TCLAP::ValueArg max_obj_name_len("", "max-obj-name-len", - "Maximum name length for a field in the config " - "(applies to listener name, route config name and" - " the cluster name)", - false, ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH, "uint64_t", - cmd); + "Deprecated and unused; please do not specify.", false, + 123, "uint64_t", cmd); TCLAP::SwitchArg disable_hot_restart("", "disable-hot-restart", "Disable hot restart functionality", cmd, false); TCLAP::SwitchArg enable_mutex_tracing( @@ -144,20 +124,6 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, throw NoServingException(); } - auto check_numeric_arg = [](bool is_error, uint64_t value, absl::string_view pattern) { - if (is_error) { - const std::string message = fmt::format(std::string(pattern), value); - throw MalformedArgvException(message); - } - }; - check_numeric_arg(max_obj_name_len.getValue() < 60, max_obj_name_len.getValue(), - "error: the 'max-obj-name-len' value specified ({}) is less than the minimum " - "value of 60"); - check_numeric_arg(max_stats.getValue() > 100 * 1000 * 1000, max_stats.getValue(), - "error: the 'max-stats' value specified ({}) is more than the maximum value " - "of 100M"); - // TODO(jmarantz): should we also multiply these to bound the total amount of memory? - hot_restart_disabled_ = disable_hot_restart.getValue(); mutex_tracing_enabled_ = enable_mutex_tracing.getValue(); @@ -228,12 +194,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, file_flush_interval_msec_ = std::chrono::milliseconds(file_flush_interval_msec.getValue()); drain_time_ = std::chrono::seconds(drain_time_s.getValue()); parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue()); - max_stats_ = max_stats.getValue(); - stats_options_.max_obj_name_length_ = max_obj_name_len.getValue(); if (hot_restart_version_option.getValue()) { - std::cerr << hot_restart_version_cb(max_stats.getValue(), stats_options_.maxNameLength(), - !hot_restart_disabled_); + std::cerr << hot_restart_version_cb(!hot_restart_disabled_); throw NoServingException(); } } @@ -311,8 +274,6 @@ Server::CommandLineOptionsPtr OptionsImpl::toCommandLineOptions() const { Protobuf::util::TimeUtil::SecondsToDuration(parentShutdownTime().count())); command_line_options->mutable_drain_time()->MergeFrom( Protobuf::util::TimeUtil::SecondsToDuration(drainTime().count())); - command_line_options->set_max_stats(maxStats()); - command_line_options->set_max_obj_name_len(statsOptions().maxObjNameLength()); command_line_options->set_disable_hot_restart(hotRestartDisabled()); command_line_options->set_enable_mutex_tracing(mutexTracingEnabled()); command_line_options->set_cpuset_threads(cpusetThreadsEnabled()); @@ -327,8 +288,7 @@ OptionsImpl::OptionsImpl(const std::string& service_cluster, const std::string& log_format_(Logger::Logger::DEFAULT_LOG_FORMAT), restart_epoch_(0u), service_cluster_(service_cluster), service_node_(service_node), service_zone_(service_zone), file_flush_interval_msec_(10000), drain_time_(600), parent_shutdown_time_(900), - mode_(Server::Mode::Serve), max_stats_(ENVOY_DEFAULT_MAX_STATS), hot_restart_disabled_(false), - signal_handling_enabled_(true), mutex_tracing_enabled_(false), cpuset_threads_(false), - libevent_buffer_enabled_(false) {} + mode_(Server::Mode::Serve), hot_restart_disabled_(false), signal_handling_enabled_(true), + mutex_tracing_enabled_(false), cpuset_threads_(false), libevent_buffer_enabled_(false) {} } // namespace Envoy diff --git a/source/server/options_impl.h b/source/server/options_impl.h index 08e8e13927c3e..2441d4893e649 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -6,10 +6,8 @@ #include "envoy/common/exception.h" #include "envoy/server/options.h" -#include "envoy/stats/stats_options.h" #include "common/common/logger.h" -#include "common/stats/stats_options_impl.h" #include "spdlog/spdlog.h" @@ -20,9 +18,9 @@ namespace Envoy { class OptionsImpl : public Server::Options, protected Logger::Loggable { public: /** - * Parameters are max_num_stats, max_stat_name_len, hot_restart_enabled + * Parameters are max_stat_name_len, hot_restart_enabled */ - typedef std::function HotRestartVersionCb; + typedef std::function HotRestartVersionCb; /** * @throw NoServingException if Envoy has already done everything specified by the argv (e.g. @@ -66,8 +64,6 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable void { - HotRestart::GetParentStatsInfo info; - restarter_.getParentStats(info); + // mergeParentStatsIfAny() does nothing and returns a struct of 0s if there is no parent. + HotRestart::ServerStatsFromParent parent_stats = restarter_.mergeParentStatsIfAny(stats_store_); + server_stats_->uptime_.set(time(nullptr) - original_start_time_); server_stats_->memory_allocated_.set(Memory::Stats::totalCurrentlyAllocated() + - info.memory_allocated_); + parent_stats.parent_memory_allocated_); server_stats_->memory_heap_size_.set(Memory::Stats::totalCurrentlyReserved()); - server_stats_->parent_connections_.set(info.num_connections_); - server_stats_->total_connections_.set(numConnections() + info.num_connections_); + server_stats_->parent_connections_.set(parent_stats.parent_connections_); + server_stats_->total_connections_.set(listener_manager_->numConnections() + + parent_stats.parent_connections_); server_stats_->days_until_first_cert_expiring_.set( sslContextManager().daysUntilFirstCertExpires()); InstanceUtil::flushMetricsToSinks(config_.statsSinks(), stats_store_.source()); @@ -166,11 +168,6 @@ void InstanceImpl::flushStats() { }); } -void InstanceImpl::getParentStats(HotRestart::GetParentStatsInfo& info) { - info.memory_allocated_ = Memory::Stats::totalCurrentlyAllocated(); - info.num_connections_ = numConnections(); -} - bool InstanceImpl::healthCheckFailed() { return server_stats_->live_.value() == 0; } InstanceUtil::BootstrapVersion @@ -269,10 +266,8 @@ void InstanceImpl::initialize(const Options& options, Configuration::InitialImpl initial_config(bootstrap_); - HotRestart::ShutdownParentAdminInfo info; - info.original_start_time_ = original_start_time_; - restarter_.shutdownParentAdmin(info); - original_start_time_ = info.original_start_time_; + // Learn original_start_time_ if our parent is still around to inform us of it. + restarter_.sendParentAdminShutdownRequest(original_start_time_); admin_ = std::make_unique(initial_config.admin().profilePath(), *this); if (initial_config.admin().address()) { if (initial_config.admin().accessLogPath().empty()) { @@ -413,8 +408,6 @@ void InstanceImpl::loadServerFlags(const absl::optional& flags_path } } -uint64_t InstanceImpl::numConnections() { return listener_manager_->numConnections(); } - RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, Init::Manager& init_manager, OverloadManager& overload_manager, @@ -533,7 +526,7 @@ Runtime::Loader& InstanceImpl::runtime() { return Runtime::LoaderSingleton::get( void InstanceImpl::shutdown() { ENVOY_LOG(info, "shutting down server instance"); shutdown_ = true; - restarter_.terminateParent(); + restarter_.sendParentTerminateRequest(); notifyCallbacksForStage(Stage::ShutdownExit, [this] { dispatcher_->exit(); }); } @@ -546,8 +539,9 @@ void InstanceImpl::shutdownAdmin() { handler_->stopListeners(); admin_->closeSocket(); + // If we still have a parent, it should be terminated now that we have a child. ENVOY_LOG(warn, "terminating parent process"); - restarter_.terminateParent(); + restarter_.sendParentTerminateRequest(); } void InstanceImpl::registerCallback(Stage stage, StageCallback callback) { diff --git a/source/server/server.h b/source/server/server.h index 876c268d808c6..6b2fdf0c0cd3d 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -166,7 +166,6 @@ class InstanceImpl : Logger::Loggable, DrainManager& drainManager() override { return *drain_manager_; } AccessLog::AccessLogManager& accessLogManager() override { return access_log_manager_; } void failHealthcheck(bool fail) override; - void getParentStats(HotRestart::GetParentStatsInfo& info) override; HotRestart& hotRestart() override { return restarter_; } Init::Manager& initManager() override { return init_manager_; } ServerLifecycleNotifier& lifecycleNotifier() override { return *this; } @@ -204,7 +203,6 @@ class InstanceImpl : Logger::Loggable, void initialize(const Options& options, Network::Address::InstanceConstSharedPtr local_address, ComponentFactory& component_factory, ListenerHooks& hooks); void loadServerFlags(const absl::optional& flags_path); - uint64_t numConnections(); void startWorkers(); void terminate(); void notifyCallbacksForStage( diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 598ad406adc9f..2aa4a66dc28f3 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -182,17 +182,6 @@ envoy_cc_test( deps = ["//source/common/common:callback_impl_lib"], ) -envoy_cc_test( - name = "block_memory_hash_set_test", - srcs = ["block_memory_hash_set_test.cc"], - deps = [ - "//include/envoy/stats:stats_interface", - "//source/common/common:block_memory_hash_set_lib", - "//source/common/common:hash_lib", - "//source/common/stats:stats_lib", - ], -) - envoy_cc_binary( name = "utility_speed_test", srcs = ["utility_speed_test.cc"], diff --git a/test/common/common/block_memory_hash_set_test.cc b/test/common/common/block_memory_hash_set_test.cc deleted file mode 100644 index 37b4957feca1f..0000000000000 --- a/test/common/common/block_memory_hash_set_test.cc +++ /dev/null @@ -1,217 +0,0 @@ -#include -#include -#include -#include -#include - -#include "common/common/block_memory_hash_set.h" -#include "common/common/fmt.h" -#include "common/common/hash.h" -#include "common/stats/stats_options_impl.h" - -#include "absl/strings/string_view.h" -#include "gtest/gtest.h" - -namespace Envoy { - -// Tests BlockMemoryHashSet. -class BlockMemoryHashSetTest : public testing::Test { -protected: - // TestValue that doesn't define a hash. - struct TestValueBase { - absl::string_view key() const { return name; } - void initialize(absl::string_view key, const Stats::StatsOptions& stats_options) { - ASSERT(key.size() <= stats_options.maxNameLength()); - memcpy(name, key.data(), key.size()); - name[key.size()] = '\0'; - } - static uint64_t structSizeWithOptions(const Stats::StatsOptions& stats_options) { - UNREFERENCED_PARAMETER(stats_options); - return sizeof(TestValue); - } - - int64_t number; - char name[256]; - }; - - // TestValue that uses an always-zero hash. - struct TestValueZeroHash : public TestValueBase { - static uint64_t hash(absl::string_view /* key */) { return 0; } - }; - - // TestValue that uses a real hash function. - struct TestValue : public TestValueBase { - static uint64_t hash(absl::string_view key) { return HashUtil::xxHash64(key); } - }; - - typedef BlockMemoryHashSet::ValueCreatedPair ValueCreatedPair; - - template void setUp() { - hash_set_options_.capacity = 100; - hash_set_options_.num_slots = 5; - const uint32_t mem_size = - BlockMemoryHashSet::numBytes(hash_set_options_, stats_options_); - memory_ = std::make_unique(mem_size); - memset(memory_.get(), 0, mem_size); - } - - /** - * Returns a string describing the contents of the map, including the control - * bits and the keys in each slot. - */ - template - std::string hashSetToString(BlockMemoryHashSet& hs) { - std::string ret; - static const uint32_t sentinel = BlockMemoryHashSet::Sentinel; - std::string control_string = - fmt::format("{} size={} free_cell_index={}", hs.control_->hash_set_options.toString(), - hs.control_->size, hs.control_->free_cell_index); - ret = fmt::format("options={}\ncontrol={}\n", hs.control_->hash_set_options.toString(), - control_string); - for (uint32_t i = 0; i < hs.control_->hash_set_options.num_slots; ++i) { - ret += fmt::format("slot {}:", i); - for (uint32_t j = hs.slots_[i]; j != sentinel; j = hs.getCell(j).next_cell_index) { - ret += " " + std::string(hs.getCell(j).value.key()); - } - ret += "\n"; - } - return ret; - } - - BlockMemoryHashSetOptions hash_set_options_; - Stats::StatsOptionsImpl stats_options_; - std::unique_ptr memory_; -}; - -TEST_F(BlockMemoryHashSetTest, initAndAttach) { - setUp(); - { - BlockMemoryHashSet hash_set1(hash_set_options_, true, memory_.get(), - stats_options_); // init - BlockMemoryHashSet hash_set2(hash_set_options_, false, memory_.get(), - stats_options_); // attach - } - - // If we tweak an option, we can no longer attach it. - bool constructor_completed = false; - bool constructor_threw = false; - try { - hash_set_options_.capacity = 99; - BlockMemoryHashSet hash_set3(hash_set_options_, false, memory_.get(), - stats_options_); - constructor_completed = false; - } catch (const std::exception& e) { - constructor_threw = true; - } - EXPECT_TRUE(constructor_threw); - EXPECT_FALSE(constructor_completed); -} - -TEST_F(BlockMemoryHashSetTest, putRemove) { - setUp(); - { - BlockMemoryHashSet hash_set1(hash_set_options_, true, memory_.get(), stats_options_); - hash_set1.sanityCheck(); - EXPECT_EQ(0, hash_set1.size()); - EXPECT_EQ(nullptr, hash_set1.get("no such key")); - ValueCreatedPair vc = hash_set1.insert("good key"); - EXPECT_TRUE(vc.second); - vc.first->number = 12345; - hash_set1.sanityCheck(); - EXPECT_EQ(1, hash_set1.size()); - EXPECT_EQ(12345, hash_set1.get("good key")->number); - EXPECT_EQ(nullptr, hash_set1.get("no such key")); - - vc = hash_set1.insert("good key"); - EXPECT_FALSE(vc.second) << "re-used, not newly created"; - vc.first->number = 6789; - EXPECT_EQ(6789, hash_set1.get("good key")->number); - EXPECT_EQ(1, hash_set1.size()); - } - - { - // Now init a new hash-map with the same memory. - BlockMemoryHashSet hash_set2(hash_set_options_, false, memory_.get(), - stats_options_); - EXPECT_EQ(1, hash_set2.size()); - EXPECT_EQ(nullptr, hash_set2.get("no such key")); - EXPECT_EQ(6789, hash_set2.get("good key")->number) << hashSetToString(hash_set2); - EXPECT_FALSE(hash_set2.remove("no such key")); - hash_set2.sanityCheck(); - EXPECT_TRUE(hash_set2.remove("good key")); - hash_set2.sanityCheck(); - EXPECT_EQ(nullptr, hash_set2.get("good key")); - EXPECT_EQ(0, hash_set2.size()); - } -} - -TEST_F(BlockMemoryHashSetTest, tooManyValues) { - setUp(); - BlockMemoryHashSet hash_set1(hash_set_options_, true, memory_.get(), stats_options_); - std::vector keys; - for (uint32_t i = 0; i < hash_set_options_.capacity + 1; ++i) { - keys.push_back(fmt::format("key{}", i)); - } - - for (uint32_t i = 0; i < hash_set_options_.capacity; ++i) { - TestValue* value = hash_set1.insert(keys[i]).first; - ASSERT_NE(nullptr, value); - value->number = i; - } - hash_set1.sanityCheck(); - EXPECT_EQ(hash_set_options_.capacity, hash_set1.size()); - - for (uint32_t i = 0; i < hash_set_options_.capacity; ++i) { - const TestValue* value = hash_set1.get(keys[i]); - ASSERT_NE(nullptr, value); - EXPECT_EQ(i, value->number); - } - hash_set1.sanityCheck(); - - // We can't fit one more value. - EXPECT_EQ(nullptr, hash_set1.insert(keys[hash_set_options_.capacity]).first); - hash_set1.sanityCheck(); - EXPECT_EQ(hash_set_options_.capacity, hash_set1.size()); - - // Now remove everything one by one. - for (uint32_t i = 0; i < hash_set_options_.capacity; ++i) { - EXPECT_TRUE(hash_set1.remove(keys[i])); - } - hash_set1.sanityCheck(); - EXPECT_EQ(0, hash_set1.size()); - - // Now we can put in that last key we weren't able to before. - TestValue* value = hash_set1.insert(keys[hash_set_options_.capacity]).first; - EXPECT_NE(nullptr, value); - value->number = 314519; - EXPECT_EQ(1, hash_set1.size()); - EXPECT_EQ(314519, hash_set1.get(keys[hash_set_options_.capacity])->number); - hash_set1.sanityCheck(); -} - -TEST_F(BlockMemoryHashSetTest, severalKeysZeroHash) { - setUp(); - BlockMemoryHashSet hash_set1(hash_set_options_, true, memory_.get(), - stats_options_); - hash_set1.insert("one").first->number = 1; - hash_set1.insert("two").first->number = 2; - hash_set1.insert("three").first->number = 3; - EXPECT_TRUE(hash_set1.remove("two")); - hash_set1.sanityCheck(); - hash_set1.insert("four").first->number = 4; - hash_set1.sanityCheck(); - EXPECT_FALSE(hash_set1.remove("two")); - hash_set1.sanityCheck(); -} - -class BlockMemoryHashSetDeathTest : public BlockMemoryHashSetTest {}; - -TEST_F(BlockMemoryHashSetDeathTest, sanityCheckZeroedMemoryDeathTest) { - setUp(); - BlockMemoryHashSet hash_set1(hash_set_options_, true, memory_.get(), - stats_options_); - memset(memory_.get(), 0, hash_set1.numBytes(stats_options_)); - EXPECT_DEATH(hash_set1.sanityCheck(), ""); -} - -} // namespace Envoy diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index a2cd07509b6b3..c576c8f9fd25f 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -129,65 +129,6 @@ TEST(UtilityTest, createTagProducer) { ASSERT_EQ(tags.size(), 1); } -TEST(UtilityTest, ObjNameLength) { - Stats::StatsOptionsImpl stats_options; - std::string name = "listenerwithareallyreallyreallyreallyreallyreallyreallyreallyreallyreallyreal" - "lyreallyreallyreallyreallyreallylongnamemorethanmaxcharsallowedbyschema"; - std::string err_prefix; - std::string err_suffix = fmt::format(": Length of {} ({}) exceeds allowed maximum length ({})", - name, name.length(), stats_options.maxNameLength()); - { - err_prefix = "test"; - EXPECT_THROW_WITH_MESSAGE(Utility::checkObjNameLength(err_prefix, name, stats_options), - EnvoyException, err_prefix + err_suffix); - } - - { - err_prefix = "Invalid listener name"; - std::string json = - R"EOF({ "name": ")EOF" + name + R"EOF(", "address": "foo", "filters":[]})EOF"; - auto json_object_ptr = Json::Factory::loadFromString(json); - - envoy::api::v2::Listener listener; - EXPECT_THROW_WITH_MESSAGE( - Config::LdsJson::translateListener(*json_object_ptr, listener, stats_options), - EnvoyException, err_prefix + err_suffix); - } - - { - err_prefix = "Invalid virtual host name"; - std::string json = R"EOF({ "name": ")EOF" + name + R"EOF(", "domains": [], "routes": []})EOF"; - auto json_object_ptr = Json::Factory::loadFromString(json); - envoy::api::v2::route::VirtualHost vhost; - EXPECT_THROW_WITH_MESSAGE( - Config::RdsJson::translateVirtualHost(*json_object_ptr, vhost, stats_options), - EnvoyException, err_prefix + err_suffix); - } - - { - err_prefix = "Invalid cluster name"; - std::string json = - R"EOF({ "name": ")EOF" + name + - R"EOF(", "type": "static", "lb_type": "random", "connect_timeout_ms" : 1})EOF"; - auto json_object_ptr = Json::Factory::loadFromString(json); - envoy::api::v2::Cluster cluster; - envoy::api::v2::core::ConfigSource eds_config; - EXPECT_THROW_WITH_MESSAGE( - Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster, stats_options), - EnvoyException, err_prefix + err_suffix); - } - - { - err_prefix = "Invalid route_config name"; - std::string json = R"EOF({ "route_config_name": ")EOF" + name + R"EOF(", "cluster": "foo"})EOF"; - auto json_object_ptr = Json::Factory::loadFromString(json); - envoy::config::filter::network::http_connection_manager::v2::Rds rds; - EXPECT_THROW_WITH_MESSAGE( - Config::Utility::translateRdsConfig(*json_object_ptr, rds, stats_options), EnvoyException, - err_prefix + err_suffix); - } -} - TEST(UtilityTest, UnixClusterDns) { std::string cluster_type; @@ -198,10 +139,9 @@ TEST(UtilityTest, UnixClusterDns) { auto json_object_ptr = Json::Factory::loadFromString(json); envoy::api::v2::Cluster cluster; envoy::api::v2::core::ConfigSource eds_config; - Stats::StatsOptionsImpl stats_options; EXPECT_THROW_WITH_MESSAGE( - Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster, stats_options), - EnvoyException, "unresolved URL must be TCP scheme, got: unix:///test.sock"); + Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster), EnvoyException, + "unresolved URL must be TCP scheme, got: unix:///test.sock"); } TEST(UtilityTest, UnixClusterStatic) { @@ -214,8 +154,7 @@ TEST(UtilityTest, UnixClusterStatic) { auto json_object_ptr = Json::Factory::loadFromString(json); envoy::api::v2::Cluster cluster; envoy::api::v2::core::ConfigSource eds_config; - Stats::StatsOptionsImpl stats_options; - Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster, stats_options); + Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster); EXPECT_EQ("/test.sock", cluster.hosts(0).pipe().path()); } diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index c73ea59e01901..fb38da5c5b42f 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -38,12 +38,12 @@ namespace Router { namespace { envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager -parseHttpConnectionManagerFromJson(const std::string& json_string, const Stats::Scope& scope) { +parseHttpConnectionManagerFromJson(const std::string& json_string) { envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager http_connection_manager; auto json_object_ptr = Json::Factory::loadFromString(json_string); - Envoy::Config::FilterJson::translateHttpConnectionManager( - *json_object_ptr, http_connection_manager, scope.statsOptions()); + Envoy::Config::FilterJson::translateHttpConnectionManager(*json_object_ptr, + http_connection_manager); return http_connection_manager; } @@ -124,7 +124,7 @@ class RdsImplTest : public RdsTestBase { interval_timer_ = new Event::MockTimer(&factory_context_.dispatcher_); EXPECT_CALL(factory_context_.init_manager_, add(_)); rds_ = - RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json, scope_), + RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), factory_context_, "foo.", *route_config_provider_manager_); expectRequest(); factory_context_.init_manager_.initialize(init_watcher_); @@ -154,10 +154,10 @@ TEST_F(RdsImplTest, RdsAndStatic) { } )EOF"; - EXPECT_THROW( - RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json, scope_), - factory_context_, "foo.", *route_config_provider_manager_), - EnvoyException); + EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), + factory_context_, "foo.", + *route_config_provider_manager_), + EnvoyException); } TEST_F(RdsImplTest, LocalInfoNotDefined) { @@ -177,10 +177,10 @@ TEST_F(RdsImplTest, LocalInfoNotDefined) { factory_context_.local_info_.node_.set_cluster(""); factory_context_.local_info_.node_.set_id(""); - EXPECT_THROW( - RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json, scope_), - factory_context_, "foo.", *route_config_provider_manager_), - EnvoyException); + EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), + factory_context_, "foo.", + *route_config_provider_manager_), + EnvoyException); } TEST_F(RdsImplTest, UnknownCluster) { @@ -201,7 +201,7 @@ TEST_F(RdsImplTest, UnknownCluster) { Upstream::ClusterManager::ClusterInfoMap cluster_map; EXPECT_CALL(factory_context_.cluster_manager_, clusters()).WillOnce(Return(cluster_map)); EXPECT_THROW_WITH_MESSAGE( - RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json, scope_), + RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), factory_context_, "foo.", *route_config_provider_manager_), EnvoyException, "envoy::api::v2::core::ConfigSource must have a statically defined non-EDS " @@ -388,7 +388,7 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase { )EOF"; Json::ObjectSharedPtr config = Json::Factory::loadFromString(config_json); - Envoy::Config::Utility::translateRdsConfig(*config, rds_, stats_options_); + Envoy::Config::Utility::translateRdsConfig(*config, rds_); // Get a RouteConfigProvider. This one should create an entry in the RouteConfigProviderManager. Upstream::ClusterManager::ClusterInfoMap cluster_map; @@ -411,7 +411,6 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase { ~RouteConfigProviderManagerImplTest() { factory_context_.thread_local_.shutdownThread(); } - Stats::StatsOptionsImpl stats_options_; envoy::config::filter::network::http_connection_manager::v2::Rds rds_; std::unique_ptr route_config_provider_manager_; RouteConfigProviderPtr provider_; @@ -576,7 +575,7 @@ name: foo_route_config Json::ObjectSharedPtr config2 = Json::Factory::loadFromString(config_json2); envoy::config::filter::network::http_connection_manager::v2::Rds rds2; - Envoy::Config::Utility::translateRdsConfig(*config2, rds2, stats_options_); + Envoy::Config::Utility::translateRdsConfig(*config2, rds2); Upstream::ClusterManager::ClusterInfoMap cluster_map; Upstream::MockClusterMockPrioritySet cluster; diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index b021cc07cb49e..525eab03727b1 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -97,7 +97,6 @@ class RateLimitConfiguration : public testing::Test { Http::TestHeaderMapImpl header_; const RouteEntry* route_; Network::Address::Ipv4Instance default_remote_address_{"10.0.0.1"}; - Stats::StatsOptionsImpl stats_options; }; TEST_F(RateLimitConfiguration, NoApplicableRateLimit) { diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index 3e8184772c919..4f47932660fed 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -16,7 +16,6 @@ envoy_cc_test( deps = [ "//source/common/stats:fake_symbol_table_lib", "//source/common/stats:heap_stat_data_lib", - "//source/common/stats:stats_options_lib", "//test/test_common:logging_lib", ], ) @@ -30,22 +29,20 @@ envoy_cc_test( ) envoy_cc_test( - name = "raw_stat_data_test", - srcs = ["raw_stat_data_test.cc"], + name = "source_impl_test", + srcs = ["source_impl_test.cc"], deps = [ - "//source/common/stats:raw_stat_data_lib", - "//source/common/stats:stats_options_lib", - "//test/test_common:logging_lib", - "//test/test_common:utility_lib", + "//source/common/stats:source_impl_lib", + "//test/mocks/stats:stats_mocks", ], ) envoy_cc_test( - name = "source_impl_test", - srcs = ["source_impl_test.cc"], + name = "stat_merger_test", + srcs = ["stat_merger_test.cc"], deps = [ - "//source/common/stats:source_impl_lib", - "//test/mocks/stats:stats_mocks", + "//source/common/stats:isolated_store_lib", + "//source/common/stats:stat_merger_lib", ], ) diff --git a/test/common/stats/heap_stat_data_test.cc b/test/common/stats/heap_stat_data_test.cc index faab893e4e50a..2fe0d4ccd3d9d 100644 --- a/test/common/stats/heap_stat_data_test.cc +++ b/test/common/stats/heap_stat_data_test.cc @@ -2,7 +2,6 @@ #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/heap_stat_data.h" -#include "common/stats/stats_options_impl.h" #include "test/test_common/logging.h" @@ -40,10 +39,8 @@ class HeapStatDataTest : public testing::Test { }; // No truncation occurs in the implementation of HeapStatData. -// Note: a similar test using RawStatData* is in raw_stat_data_test.cc. TEST_F(HeapStatDataTest, HeapNoTruncate) { - StatsOptionsImpl stats_options; - const std::string long_string(stats_options.maxNameLength() + 1, 'A'); + const std::string long_string(128, 'A'); StatName stat_name = makeStat(long_string); HeapStatData* stat{}; EXPECT_NO_LOGS(stat = &alloc_.alloc(stat_name)); @@ -51,7 +48,6 @@ TEST_F(HeapStatDataTest, HeapNoTruncate) { alloc_.free(*stat); }; -// Note: a similar test using RawStatData* is in raw_stat_data_test.cc. TEST_F(HeapStatDataTest, HeapAlloc) { HeapStatData* stat_1 = &alloc_.alloc(makeStat("ref_name")); ASSERT_NE(stat_1, nullptr); diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc index c9e9c2cd49b7e..6b49fffb13559 100644 --- a/test/common/stats/isolated_store_impl_test.cc +++ b/test/common/stats/isolated_store_impl_test.cc @@ -123,8 +123,7 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) { } TEST_F(StatsIsolatedStoreImplTest, LongStatName) { - Stats::StatsOptionsImpl stats_options; - const std::string long_string(stats_options.maxNameLength() + 1, 'A'); + const std::string long_string(128, 'A'); ScopePtr scope = store_.createScope("scope."); Counter& counter = scope->counter(long_string); diff --git a/test/common/stats/raw_stat_data_test.cc b/test/common/stats/raw_stat_data_test.cc deleted file mode 100644 index e1e0f1365b4aa..0000000000000 --- a/test/common/stats/raw_stat_data_test.cc +++ /dev/null @@ -1,57 +0,0 @@ -#include - -#include "common/stats/raw_stat_data.h" -#include "common/stats/stats_options_impl.h" - -#include "test/test_common/logging.h" -#include "test/test_common/utility.h" - -#include "gtest/gtest.h" - -namespace Envoy { -namespace Stats { -namespace { - -class RawStatDataTest : public testing::Test { -public: - RawStatDataTest() : allocator_(stats_options_, symbol_table_) {} - - StatsOptionsImpl stats_options_; - FakeSymbolTableImpl symbol_table_; - TestAllocator allocator_; // This is RawStatDataAllocator with some size settings. -}; - -// Note: a similar test using HeapStatData* is in heap_stat_data_test.cc. -TEST_F(RawStatDataTest, RawTruncate) { - const std::string long_string(stats_options_.maxNameLength() + 1, 'A'); - RawStatData* stat{}; - EXPECT_LOG_CONTAINS("warning", " is too long ", stat = allocator_.alloc(long_string)); - EXPECT_NE(stat->key(), long_string); - - // If I add more bytes to the key, I'll get the same allocation back - // due to the truncated map lookup. - EXPECT_EQ(stat, allocator_.alloc(long_string + " ignored")); - - allocator_.free(*stat); - allocator_.free(*stat); // Have to free it twice as second allocation bumped ref-count. -} - -// Note: a similar test using HeapStatData* is in heap_stat_data_test.cc. -TEST_F(RawStatDataTest, RawAlloc) { - Stats::RawStatData* stat_1 = allocator_.alloc("ref_name"); - ASSERT_NE(stat_1, nullptr); - Stats::RawStatData* stat_2 = allocator_.alloc("ref_name"); - ASSERT_NE(stat_2, nullptr); - Stats::RawStatData* stat_3 = allocator_.alloc("not_ref_name"); - ASSERT_NE(stat_3, nullptr); - EXPECT_EQ(stat_1, stat_2); - EXPECT_NE(stat_1, stat_3); - EXPECT_NE(stat_2, stat_3); - allocator_.free(*stat_1); - allocator_.free(*stat_2); - allocator_.free(*stat_3); -} - -} // namespace -} // namespace Stats -} // namespace Envoy diff --git a/test/common/stats/stat_merger_test.cc b/test/common/stats/stat_merger_test.cc new file mode 100644 index 0000000000000..5d6d70b5073df --- /dev/null +++ b/test/common/stats/stat_merger_test.cc @@ -0,0 +1,169 @@ +#include + +#include "common/stats/isolated_store_impl.h" +#include "common/stats/stat_merger.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Stats { +namespace { + +class StatMergerTest : public testing::Test { +public: + StatMergerTest() : stat_merger_(store_) { store_.gauge("whywassixafraidofseven").set(678); } + + Stats::IsolatedStoreImpl store_; + StatMerger stat_merger_; + Protobuf::Map empty_counter_deltas_; + Protobuf::Map empty_gauges_; +}; + +TEST_F(StatMergerTest, counterMerge) { + // Child's value of the counter might already be non-zero by the first merge. + store_.counter("draculaer").inc(); + EXPECT_EQ(1, store_.counter("draculaer").latch()); + + Protobuf::Map counter_deltas; + counter_deltas["draculaer"] = 1; + stat_merger_.mergeStats(counter_deltas, empty_gauges_); + // Initial combined value: 1+1. + EXPECT_EQ(2, store_.counter("draculaer").value()); + EXPECT_EQ(1, store_.counter("draculaer").latch()); + + // The parent's counter increases by 1. + counter_deltas["draculaer"] = 1; + stat_merger_.mergeStats(counter_deltas, empty_gauges_); + EXPECT_EQ(3, store_.counter("draculaer").value()); + EXPECT_EQ(1, store_.counter("draculaer").latch()); + + // Our own counter increases by 4, while the parent's stays constant. Total increase of 4. + store_.counter("draculaer").add(4); + counter_deltas["draculaer"] = 0; + stat_merger_.mergeStats(counter_deltas, empty_gauges_); + EXPECT_EQ(7, store_.counter("draculaer").value()); + EXPECT_EQ(4, store_.counter("draculaer").latch()); + + // Our counter and the parent's counter both increase by 2, total increase of 4. + store_.counter("draculaer").add(2); + counter_deltas["draculaer"] = 2; + stat_merger_.mergeStats(counter_deltas, empty_gauges_); + EXPECT_EQ(11, store_.counter("draculaer").value()); + EXPECT_EQ(4, store_.counter("draculaer").latch()); +} + +// It should be fine for the parent to send us stats we haven't ourselves instantiated. +// TODO(6756) This is how things currently work, but this is actually what 6756 is looking to avoid. +TEST_F(StatMergerTest, newStatFromParent) { + Protobuf::Map counter_values; + Protobuf::Map counter_deltas; + Protobuf::Map gauges; + counter_deltas["newcounter0"] = 0; + counter_deltas["newcounter1"] = 1; + gauges["newgauge"] = 5; + stat_merger_.mergeStats(counter_deltas, gauges); + EXPECT_EQ(0, store_.counter("newcounter0").value()); + EXPECT_EQ(0, store_.counter("newcounter0").latch()); + EXPECT_EQ(1, store_.counter("newcounter1").value()); + EXPECT_EQ(1, store_.counter("newcounter1").latch()); + EXPECT_EQ(5, store_.gauge("newgauge").value()); +} + +TEST_F(StatMergerTest, basicDefaultAccumulationImport) { + Protobuf::Map gauges; + gauges["whywassixafraidofseven"] = 111; + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + EXPECT_EQ(789, store_.gauge("whywassixafraidofseven").value()); +} + +TEST_F(StatMergerTest, multipleImportsWithAccumulationLogic) { + { + Protobuf::Map gauges; + gauges["whywassixafraidofseven"] = 100; + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + // Initial combined values: 678+100 and 1+2. + EXPECT_EQ(778, store_.gauge("whywassixafraidofseven").value()); + } + { + Protobuf::Map gauges; + // The parent's gauge drops by 1, and its counter increases by 1. + gauges["whywassixafraidofseven"] = 99; + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + EXPECT_EQ(777, store_.gauge("whywassixafraidofseven").value()); + } + { + Protobuf::Map gauges; + // Our own gauge increases by 12, while the parent's stays constant. Total increase of 12. + // Our own counter increases by 4, while the parent's stays constant. Total increase of 4. + store_.gauge("whywassixafraidofseven").add(12); + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + EXPECT_EQ(789, store_.gauge("whywassixafraidofseven").value()); + } + { + Protobuf::Map gauges; + // Our gauge decreases by 5, parent's increases by 5. Net zero change. + // Our counter and the parent's counter both increase by 1, total increase of 2. + store_.gauge("whywassixafraidofseven").sub(5); + gauges["whywassixafraidofseven"] = 104; + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + EXPECT_EQ(789, store_.gauge("whywassixafraidofseven").value()); + } +} + +// Stat names that have NoImport logic should leave the child gauge value alone upon import, even if +// the child has that gauge undefined. +TEST_F(StatMergerTest, exclusionsNotImported) { + store_.gauge("some.sort.of.version").set(12345); + + Protobuf::Map gauges; + gauges["some.sort.of.version"] = 67890; + gauges["child.doesnt.have.this.version"] = 111; + + // Check defined values are not changed, and undefined remain undefined. + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + EXPECT_EQ(12345, store_.gauge("some.sort.of.version").value()); + EXPECT_FALSE(store_.gauge("child.doesnt.have.this.version").used()); + + // Check the "undefined remains undefined" behavior for a bunch of other names. + gauges["runtime.admin_overrides_active"] = 111; + gauges["runtime.num_keys"] = 111; + gauges["listener_manager.total_listeners_draining"] = 111; + gauges["listener_manager.total_listeners_warming"] = 111; + gauges["server.hot_restart_epoch"] = 111; + gauges["server.live"] = 1; + gauges["server.concurrency"] = 1; + gauges["some.control_plane.connected_state"] = 1; + gauges["cluster_manager.active_clusters"] = 33; + gauges["cluster_manager.warming_clusters"] = 33; + gauges["cluster.rds.membership_total"] = 33; + gauges["cluster.rds.membership_healthy"] = 33; + gauges["cluster.rds.membership_degraded"] = 33; + gauges["cluster.rds.max_host_weight"] = 33; + gauges["anything.total_principals"] = 33; + gauges["listener_manager.total_listeners_active"] = 33; + gauges["overload.something.pressure"] = 33; + + stat_merger_.mergeStats(empty_counter_deltas_, gauges); + EXPECT_FALSE(store_.gauge("child.doesnt.have.this.version").used()); + EXPECT_FALSE(store_.gauge("runtime.admin_overrides_active").used()); + EXPECT_FALSE(store_.gauge("runtime.num_keys").used()); + EXPECT_FALSE(store_.gauge("listener_manager.total_listeners_draining").used()); + EXPECT_FALSE(store_.gauge("listener_manager.total_listeners_warming").used()); + EXPECT_FALSE(store_.gauge("server.hot_restart_epoch").used()); + EXPECT_FALSE(store_.gauge("server.live").used()); + EXPECT_FALSE(store_.gauge("server.concurrency").used()); + EXPECT_FALSE(store_.gauge("some.control_plane.connected_state").used()); + EXPECT_FALSE(store_.gauge("cluster_manager.active_clusters").used()); + EXPECT_FALSE(store_.gauge("cluster_manager.warming_clusters").used()); + EXPECT_FALSE(store_.gauge("cluster.rds.membership_total").used()); + EXPECT_FALSE(store_.gauge("cluster.rds.membership_healthy").used()); + EXPECT_FALSE(store_.gauge("cluster.rds.membership_degraded").used()); + EXPECT_FALSE(store_.gauge("cluster.rds.max_host_weight").used()); + EXPECT_FALSE(store_.gauge("anything.total_principals").used()); + EXPECT_FALSE(store_.gauge("listener_manager.total_listeners_active").used()); + EXPECT_FALSE(store_.gauge("overload.something.pressure").used()); +} + +} // namespace +} // namespace Stats +} // namespace Envoy diff --git a/test/common/stats/thread_local_store_speed_test.cc b/test/common/stats/thread_local_store_speed_test.cc index 76ac8117a95f6..30413ff6b7655 100644 --- a/test/common/stats/thread_local_store_speed_test.cc +++ b/test/common/stats/thread_local_store_speed_test.cc @@ -6,7 +6,6 @@ #include "common/event/dispatcher_impl.h" #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/heap_stat_data.h" -#include "common/stats/stats_options_impl.h" #include "common/stats/tag_producer_impl.h" #include "common/stats/thread_local_store.h" #include "common/thread_local/thread_local_impl.h" @@ -23,7 +22,7 @@ namespace Envoy { class ThreadLocalStorePerf { public: ThreadLocalStorePerf() - : heap_alloc_(symbol_table_), store_(options_, heap_alloc_), + : heap_alloc_(symbol_table_), store_(heap_alloc_), api_(Api::createApiForTest(store_, time_system_)) { store_.setTagProducer(std::make_unique(stats_config_)); @@ -57,7 +56,6 @@ class ThreadLocalStorePerf { private: Stats::FakeSymbolTableImpl symbol_table_; Event::SimulatedTimeSystem time_system_; - Stats::StatsOptionsImpl options_; Stats::HeapStatDataAllocator heap_alloc_; Stats::ThreadLocalStoreImpl store_; Api::ApiPtr api_; @@ -96,9 +94,6 @@ BENCHMARK(BM_StatsWithTls); // TODO(jmarantz): add multi-threaded variant of this test, that aggressively // looks up stats in multiple threads to try to trigger contention issues. -// TODO(jmarantz): add version using the RawStatDataAllocator, or better yet, -// the full hot-restart mechanism so that actual shared-memory is used. - // Boilerplate main(), which discovers benchmarks in the same file and runs them. int main(int argc, char** argv) { benchmark::Initialize(&argc, argv); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 86c883b7412a4..05aaa84f10931 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -33,23 +33,24 @@ using testing::Return; namespace Envoy { namespace Stats { +const uint64_t MaxStatNameLength = 127; + class StatsThreadLocalStoreTest : public testing::Test { public: - void SetUp() override { - alloc_ = std::make_unique(options_, symbol_table_); - resetStoreWithAlloc(*alloc_); + StatsThreadLocalStoreTest() + : alloc_(symbol_table_), store_(std::make_unique(alloc_)) { + store_->addSink(sink_); } void resetStoreWithAlloc(StatDataAllocator& alloc) { - store_ = std::make_unique(options_, alloc); + store_ = std::make_unique(alloc); store_->addSink(sink_); } Stats::FakeSymbolTableImpl symbol_table_; NiceMock main_thread_dispatcher_; NiceMock tls_; - StatsOptionsImpl options_; - std::unique_ptr alloc_; + HeapStatDataAllocator alloc_; MockSink sink_; std::unique_ptr store_; }; @@ -76,10 +77,10 @@ class HistogramTest : public testing::Test { public: using NameHistogramMap = std::map; - HistogramTest() : alloc_(options_, symbol_table_) {} + HistogramTest() : alloc_(symbol_table_) {} void SetUp() override { - store_ = std::make_unique(options_, alloc_); + store_ = std::make_unique(alloc_); store_->addSink(sink_); store_->initializeThreading(main_thread_dispatcher_, tls_); } @@ -87,8 +88,6 @@ class HistogramTest : public testing::Test { void TearDown() override { store_->shutdownThreading(); tls_.shutdownThread(); - // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)); } NameHistogramMap makeHistogramMap(const std::vector& hist_list) { @@ -166,14 +165,10 @@ class HistogramTest : public testing::Test { } } - MOCK_METHOD1(alloc, RawStatData*(const std::string& name)); - MOCK_METHOD1(free, void(RawStatData& data)); - FakeSymbolTableImpl symbol_table_; NiceMock main_thread_dispatcher_; NiceMock tls_; - StatsOptionsImpl options_; - MockedTestAllocator alloc_; + HeapStatDataAllocator alloc_; MockSink sink_; std::unique_ptr store_; InSequence s; @@ -183,7 +178,6 @@ class HistogramTest : public testing::Test { TEST_F(StatsThreadLocalStoreTest, NoTls) { InSequence s; - EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -199,16 +193,13 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 100)); store_->deliverHistogramToSinks(h1, 100); - EXPECT_EQ(2UL, store_->counters().size()); + EXPECT_EQ(1UL, store_->counters().size()); EXPECT_EQ(&c1, TestUtility::findCounter(*store_, "c1").get()); EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c1").use_count()); EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(3); - store_->shutdownThreading(); } @@ -216,8 +207,6 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*alloc_, alloc(_)).Times(2); - Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -227,7 +216,7 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { Histogram& h1 = store_->histogram("h1"); EXPECT_EQ(&h1, &store_->histogram("h1")); - EXPECT_EQ(2UL, store_->counters().size()); + EXPECT_EQ(1UL, store_->counters().size()); EXPECT_EQ(&c1, TestUtility::findCounter(*store_, "c1").get()); EXPECT_EQ(3L, TestUtility::findCounter(*store_, "c1").use_count()); EXPECT_EQ(1UL, store_->gauges().size()); @@ -237,15 +226,12 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { store_->shutdownThreading(); tls_.shutdownThread(); - EXPECT_EQ(2UL, store_->counters().size()); + EXPECT_EQ(1UL, store_->counters().size()); EXPECT_EQ(&c1, TestUtility::findCounter(*store_, "c1").get()); EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c1").use_count()); EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, BasicScope) { @@ -253,7 +239,6 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*alloc_, alloc(_)).Times(4); Counter& c1 = store_->counter("c1"); Counter& c2 = scope1->counter("c2"); EXPECT_EQ("c1", c1.name()); @@ -277,9 +262,6 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { scope1->deliverHistogramToSinks(h1, 100); scope1->deliverHistogramToSinks(h2, 200); tls_.shutdownThread(); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(5); } // Validate that we sanitize away bad characters in the stats prefix. @@ -288,15 +270,11 @@ TEST_F(StatsThreadLocalStoreTest, SanitizePrefix) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope(std::string("scope1:\0:foo.", 13)); - EXPECT_CALL(*alloc_, alloc(_)); Counter& c1 = scope1->counter("c1"); EXPECT_EQ("scope1___foo.c1", c1.name()); store_->shutdownThreading(); tls_.shutdownThread(); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(2); } TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { @@ -304,9 +282,8 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*alloc_, alloc(_)); scope1->counter("c1"); - EXPECT_EQ(2UL, store_->counters().size()); + EXPECT_EQ(1UL, store_->counters().size()); CounterSharedPtr c1 = TestUtility::findCounter(*store_, "scope1.c1"); EXPECT_EQ("scope1.c1", c1->name()); EXPECT_EQ(TestUtility::findByName(store_->source().cachedCounters(), "scope1.c1"), c1); @@ -314,20 +291,16 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { EXPECT_CALL(main_thread_dispatcher_, post(_)); EXPECT_CALL(tls_, runOnAllThreads(_, _)); scope1.reset(); - EXPECT_EQ(1UL, store_->counters().size()); - EXPECT_EQ(2UL, store_->source().cachedCounters().size()); - store_->source().clearCache(); + EXPECT_EQ(0UL, store_->counters().size()); EXPECT_EQ(1UL, store_->source().cachedCounters().size()); + store_->source().clearCache(); + EXPECT_EQ(0UL, store_->source().cachedCounters().size()); - EXPECT_CALL(*alloc_, free(_)); EXPECT_EQ(1L, c1.use_count()); c1.reset(); store_->shutdownThreading(); tls_.shutdownThread(); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)); } TEST_F(StatsThreadLocalStoreTest, NestedScopes) { @@ -335,12 +308,10 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*alloc_, alloc(_)); Counter& c1 = scope1->counter("foo.bar"); EXPECT_EQ("scope1.foo.bar", c1.name()); ScopePtr scope2 = scope1->createScope("foo."); - EXPECT_CALL(*alloc_, alloc(_)); Counter& c2 = scope2->counter("bar"); EXPECT_NE(&c1, &c2); EXPECT_EQ("scope1.foo.bar", c2.name()); @@ -350,15 +321,11 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { EXPECT_EQ(1UL, c1.value()); EXPECT_EQ(c1.value(), c2.value()); - EXPECT_CALL(*alloc_, alloc(_)); Gauge& g1 = scope2->gauge("some_gauge"); EXPECT_EQ("scope1.foo.some_gauge", g1.name()); store_->shutdownThreading(); tls_.shutdownThread(); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(4); } TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { @@ -371,7 +338,6 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { ScopePtr scope2 = store_->createScope("scope1."); // We will call alloc twice, but they should point to the same backing storage. - EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = scope1->counter("c"); Counter& c2 = scope2->counter("c"); EXPECT_NE(&c1, &c2); @@ -383,10 +349,9 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(2UL, c2.value()); // We should dedup when we fetch all counters to handle the overlapping case. - EXPECT_EQ(2UL, store_->counters().size()); + EXPECT_EQ(1UL, store_->counters().size()); // Gauges should work the same way. - EXPECT_CALL(*alloc_, alloc(_)).Times(2); Gauge& g1 = scope1->gauge("g"); Gauge& g2 = scope2->gauge("g"); EXPECT_NE(&g1, &g2); @@ -399,88 +364,21 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(1UL, store_->gauges().size()); // Deleting scope 1 will call free but will be reference counted. It still leaves scope 2 valid. - EXPECT_CALL(*alloc_, free(_)).Times(2); scope1.reset(); c2.inc(); EXPECT_EQ(3UL, c2.value()); - EXPECT_EQ(2UL, store_->counters().size()); + EXPECT_EQ(1UL, store_->counters().size()); g2.set(10); EXPECT_EQ(10UL, g2.value()); EXPECT_EQ(1UL, store_->gauges().size()); store_->shutdownThreading(); tls_.shutdownThread(); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(3); -} - -TEST_F(StatsThreadLocalStoreTest, AllocFailed) { - InSequence s; - store_->initializeThreading(main_thread_dispatcher_, tls_); - - EXPECT_CALL(*alloc_, alloc(absl::string_view("foo"))).WillOnce(Return(nullptr)); - Counter& c1 = store_->counter("foo"); - EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); - - c1.inc(); - EXPECT_EQ(1UL, c1.value()); - - store_->shutdownThreading(); - tls_.shutdownThread(); - - // Includes overflow but not the failsafe stat which we allocated from the heap. - EXPECT_CALL(*alloc_, free(_)); -} - -TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { - InSequence s; - store_->initializeThreading(main_thread_dispatcher_, tls_); - - // First, with a successful RawStatData allocation: - const uint64_t max_name_length = options_.maxNameLength(); - const std::string name_1(max_name_length + 1, 'A'); - - EXPECT_CALL(*alloc_, alloc(_)); - EXPECT_LOG_CONTAINS("warning", "is too long with", store_->counter(name_1)); - - // The stats did not overflow yet. - EXPECT_EQ(0UL, store_->counter("stats.overflow").value()); - - // Truncation occurs in the underlying representation, but the by-name lookups - // are all based on the untruncated name. - EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1).get()); - - // Outside the stats system, no Envoy code can see the truncated view, so - // lookups for truncated names will fail. - EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); - - // The same should be true with heap allocation, which occurs when the default - // allocator fails. - const std::string name_2(max_name_length + 1, 'B'); - EXPECT_CALL(*alloc_, alloc(_)).WillOnce(Return(nullptr)); - store_->counter(name_2); - - // Same deal: the name will be truncated, but we find it with the entire name. - EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1).get()); - - // But we can't find it based on the truncation -- that name is not visible at the API. - EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); - - // Now the stats have overflowed. - EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); - - store_->shutdownThreading(); - tls_.shutdownThread(); - - // Includes overflow, and the first raw-allocated stat, but not the failsafe stat which we - // allocated from the heap. - EXPECT_CALL(*alloc_, free(_)).Times(2); } class LookupWithStatNameTest : public testing::Test { public: - LookupWithStatNameTest() : alloc_(symbol_table_), store_(options_, alloc_) {} + LookupWithStatNameTest() : alloc_(symbol_table_), store_(alloc_) {} ~LookupWithStatNameTest() override { store_.shutdownThreading(); clearStorage(); @@ -505,7 +403,6 @@ class LookupWithStatNameTest : public testing::Test { Stats::FakeSymbolTableImpl symbol_table_; HeapStatDataAllocator alloc_; - StatsOptionsImpl options_; ThreadLocalStoreImpl store_; std::vector stat_name_storage_; }; @@ -549,7 +446,7 @@ TEST_F(LookupWithStatNameTest, All) { ScopePtr scope3 = scope1->createScope(std::string("foo:\0:.", 7)); EXPECT_EQ("scope1.foo___.bar", scope3->counter("bar").name()); - EXPECT_EQ(5UL, store_.counters().size()); // The 4 objects created plus stats.overflow. + EXPECT_EQ(4UL, store_.counters().size()); EXPECT_EQ(2UL, store_.gauges().size()); } @@ -561,8 +458,6 @@ class StatsMatcherTLSTest : public StatsThreadLocalStoreTest { TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { InSequence s; - EXPECT_CALL(*alloc_, alloc(_)).Times(0); - stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( "noop"); store_->setStatsMatcher(std::make_unique(stats_config_)); @@ -605,9 +500,6 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { Histogram& noop_histogram_2 = store_->histogram("noop_histogram_2"); EXPECT_EQ(&noop_histogram, &noop_histogram_2); - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(1); - store_->shutdownThreading(); } @@ -617,7 +509,6 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { InSequence s; // Expected to alloc lowercase_counter, lowercase_gauge, valid_counter, valid_gauge - EXPECT_CALL(*alloc_, alloc(_)).Times(4); // Will block all stats containing any capital alphanumeric letter. stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_regex( @@ -693,10 +584,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { Histogram& invalid_histogram_2 = store_->histogram("also_INVALID_histogram"); EXPECT_EQ(invalid_histogram_2.name(), ""); - // Expected to free lowercase_counter, lowercase_gauge, valid_counter, - // valid_gauge, overflow.stats - EXPECT_CALL(*alloc_, free(_)).Times(5); - + // Expected to free lowercase_counter, lowercase_gauge, valid_counter, valid_gauge store_->shutdownThreading(); } @@ -709,8 +597,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { class RememberStatsMatcherTest : public testing::TestWithParam { public: RememberStatsMatcherTest() - : heap_alloc_(symbol_table_), store_(options_, heap_alloc_), - scope_(store_.createScope("scope.")) { + : heap_alloc_(symbol_table_), store_(heap_alloc_), scope_(store_.createScope("scope.")) { if (GetParam()) { store_.initializeThreading(main_thread_dispatcher_, tls_); } @@ -730,8 +617,6 @@ class RememberStatsMatcherTest : public testing::TestWithParam { InSequence s; MockStatsMatcher* matcher = new MockStatsMatcher; - EXPECT_CALL(*matcher, rejects("stats.overflow")).WillRepeatedly(Return(false)); - StatsMatcherPtr matcher_ptr(matcher); store_.setStatsMatcher(std::move(matcher_ptr)); @@ -748,7 +633,6 @@ class RememberStatsMatcherTest : public testing::TestWithParam { InSequence s; MockStatsMatcher* matcher = new MockStatsMatcher; - EXPECT_CALL(*matcher, rejects("stats.overflow")).WillRepeatedly(Return(false)); matcher->rejects_all_ = true; StatsMatcherPtr matcher_ptr(matcher); store_.setStatsMatcher(std::move(matcher_ptr)); @@ -765,7 +649,6 @@ class RememberStatsMatcherTest : public testing::TestWithParam { InSequence s; MockStatsMatcher* matcher = new MockStatsMatcher; - EXPECT_CALL(*matcher, rejects("stats.overflow")).WillRepeatedly(Return(false)); matcher->accepts_all_ = true; StatsMatcherPtr matcher_ptr(matcher); store_.setStatsMatcher(std::move(matcher_ptr)); @@ -808,7 +691,6 @@ class RememberStatsMatcherTest : public testing::TestWithParam { Stats::FakeSymbolTableImpl symbol_table_; NiceMock main_thread_dispatcher_; NiceMock tls_; - StatsOptionsImpl options_; HeapStatDataAllocator heap_alloc_; ThreadLocalStoreImpl store_; ScopePtr scope_; @@ -847,29 +729,12 @@ TEST_P(RememberStatsMatcherTest, HistogramRejectsAll) { testRejectsAll(lookupHis TEST_P(RememberStatsMatcherTest, HistogramAcceptsAll) { testAcceptsAll(lookupHistogramFn()); } -class HeapStatsThreadLocalStoreTest : public StatsThreadLocalStoreTest { -public: - HeapStatsThreadLocalStoreTest() : heap_alloc_(symbol_table_) {} - - void SetUp() override { - resetStoreWithAlloc(heap_alloc_); - // Note: we do not call StatsThreadLocalStoreTest::SetUp here as that - // sets up a thread_local_store with raw stat alloc. - } - void TearDown() override { - store_->shutdownThreading(); - tls_.shutdownThread(); - store_.reset(); // delete before the allocator. - } - - HeapStatDataAllocator heap_alloc_; -}; - -TEST_F(HeapStatsThreadLocalStoreTest, RemoveRejectedStats) { +TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { + store_->initializeThreading(main_thread_dispatcher_, tls_); Counter& counter = store_->counter("c1"); Gauge& gauge = store_->gauge("g1"); Histogram& histogram = store_->histogram("h1"); - ASSERT_EQ(2, store_->counters().size()); // "stats.overflow" and "c1". + ASSERT_EQ(1, store_->counters().size()); // "c1". EXPECT_TRUE(&counter == store_->counters()[0].get() || &counter == store_->counters()[1].get()); // counters() order is non-deterministic. ASSERT_EQ(1, store_->gauges().size()); @@ -893,74 +758,96 @@ TEST_F(HeapStatsThreadLocalStoreTest, RemoveRejectedStats) { gauge.inc(); EXPECT_CALL(sink_, onHistogramComplete(Ref(histogram), 42)); histogram.recordValue(42); + store_->shutdownThreading(); + tls_.shutdownThread(); } -TEST_F(HeapStatsThreadLocalStoreTest, NonHotRestartNoTruncation) { +TEST_F(StatsThreadLocalStoreTest, NonHotRestartNoTruncation) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); // Allocate a stat greater than the max name length. - const uint64_t max_name_length = options_.maxNameLength(); - const std::string name_1(max_name_length + 1, 'A'); + const std::string name_1(MaxStatNameLength + 1, 'A'); store_->counter(name_1); // This works fine, and we can find it by its long name because heap-stats do not // get truncated. EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1).get()); + store_->shutdownThreading(); + tls_.shutdownThread(); } // Tests how much memory is consumed allocating 100k stats. -TEST_F(HeapStatsThreadLocalStoreTest, MemoryWithoutTls) { +TEST(StatsThreadLocalStoreTestNoFixture, MemoryWithoutTls) { if (!TestUtil::hasDeterministicMallocStats()) { return; } + MockSink sink; + Stats::FakeSymbolTableImpl symbol_table; + HeapStatDataAllocator alloc(symbol_table); + auto store = std::make_unique(alloc); + store->addSink(sink); + // Use a tag producer that will produce tags. envoy::config::metrics::v2::StatsConfig stats_config; - store_->setTagProducer(std::make_unique(stats_config)); + store->setTagProducer(std::make_unique(stats_config)); - const size_t million = 1000 * 1000; const size_t start_mem = Memory::Stats::totalCurrentlyAllocated(); if (start_mem == 0) { // Skip this test for platforms where we can't measure memory. return; } TestUtil::forEachSampleStat( - 1000, [this](absl::string_view name) { store_->counter(std::string(name)); }); + 1000, [&store](absl::string_view name) { store->counter(std::string(name)); }); const size_t end_mem = Memory::Stats::totalCurrentlyAllocated(); EXPECT_LT(start_mem, end_mem); + const size_t million = 1000 * 1000; EXPECT_LT(end_mem - start_mem, 20 * million); // actual value: 19601552 as of March 14, 2019 + + // HACK: doesn't like shutting down without threading having started. + NiceMock main_thread_dispatcher; + NiceMock tls; + store->initializeThreading(main_thread_dispatcher, tls); + store->shutdownThreading(); + tls.shutdownThread(); } -TEST_F(HeapStatsThreadLocalStoreTest, MemoryWithTls) { +TEST(StatsThreadLocalStoreTestNoFixture, MemoryWithTls) { if (!TestUtil::hasDeterministicMallocStats()) { return; } + Stats::FakeSymbolTableImpl symbol_table; + HeapStatDataAllocator alloc(symbol_table); + auto store = std::make_unique(alloc); // Use a tag producer that will produce tags. envoy::config::metrics::v2::StatsConfig stats_config; - store_->setTagProducer(std::make_unique(stats_config)); + store->setTagProducer(std::make_unique(stats_config)); - const size_t million = 1000 * 1000; - store_->initializeThreading(main_thread_dispatcher_, tls_); + NiceMock main_thread_dispatcher; + NiceMock tls; + store->initializeThreading(main_thread_dispatcher, tls); const size_t start_mem = Memory::Stats::totalCurrentlyAllocated(); if (start_mem == 0) { // Skip this test for platforms where we can't measure memory. return; } TestUtil::forEachSampleStat( - 1000, [this](absl::string_view name) { store_->counter(std::string(name)); }); + 1000, [&store](absl::string_view name) { store->counter(std::string(name)); }); const size_t end_mem = Memory::Stats::totalCurrentlyAllocated(); EXPECT_LT(start_mem, end_mem); + const size_t million = 1000 * 1000; EXPECT_LT(end_mem - start_mem, 23 * million); // actual value: 22880912 as of March 14, 2019 + store->shutdownThreading(); + tls.shutdownThread(); } TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*alloc_, alloc(_)).Times(4); store_->counter("c1"); store_->gauge("g1"); store_->shutdownThreading(); @@ -973,10 +860,8 @@ TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c2").use_count()); EXPECT_EQ(2L, TestUtility::findGauge(*store_, "g2").use_count()); + store_->shutdownThreading(); tls_.shutdownThread(); - - // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(5); } TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { @@ -996,10 +881,8 @@ TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { store_->mergeHistograms([&merge_called]() -> void { merge_called = true; }); EXPECT_TRUE(merge_called); - + store_->shutdownThreading(); tls_.shutdownThread(); - - EXPECT_CALL(*alloc_, free(_)); } // Histogram tests @@ -1171,59 +1054,5 @@ TEST_F(HistogramTest, BasicHistogramUsed) { } } -class TruncatingAllocTest : public HeapStatsThreadLocalStoreTest { -protected: - TruncatingAllocTest() - : test_alloc_(options_, symbol_table_), long_name_(options_.maxNameLength() + 1, 'A') {} - - void SetUp() override { - store_ = std::make_unique(options_, test_alloc_); - // Do not call superclass SetUp. - } - - FakeSymbolTableImpl symbol_table_; - TestAllocator test_alloc_; - std::string long_name_; -}; - -TEST_F(TruncatingAllocTest, CounterNotTruncated) { - EXPECT_NO_LOGS({ - Counter& counter = store_->counter("simple"); - EXPECT_EQ(&counter, &store_->counter("simple")); - }); -} - -TEST_F(TruncatingAllocTest, GaugeNotTruncated) { - EXPECT_NO_LOGS({ - Gauge& gauge = store_->gauge("simple"); - EXPECT_EQ(&gauge, &store_->gauge("simple")); - }); -} - -TEST_F(TruncatingAllocTest, CounterTruncated) { - Counter* counter = nullptr; - EXPECT_LOG_CONTAINS("warning", "is too long with", { - Counter& c = store_->counter(long_name_); - counter = &c; - }); - EXPECT_NO_LOGS(EXPECT_EQ(counter, &store_->counter(long_name_))); -} - -TEST_F(TruncatingAllocTest, GaugeTruncated) { - Gauge* gauge = nullptr; - EXPECT_LOG_CONTAINS("warning", "is too long with", { - Gauge& g = store_->gauge(long_name_); - gauge = &g; - }); - EXPECT_NO_LOGS(EXPECT_EQ(gauge, &store_->gauge(long_name_))); -} - -TEST_F(TruncatingAllocTest, HistogramWithLongNameNotTruncated) { - EXPECT_NO_LOGS({ - Histogram& histogram = store_->histogram(long_name_); - EXPECT_EQ(&histogram, &store_->histogram(long_name_)); - }); -} - } // namespace Stats } // namespace Envoy diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index e60a51b00ed02..5647c8d8a6e24 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -281,9 +281,7 @@ class ClusterManagerImplTest : public testing::Test { envoy::config::bootstrap::v2::Bootstrap parseBootstrapFromJson(const std::string& json_string) { envoy::config::bootstrap::v2::Bootstrap bootstrap; auto json_object_ptr = Json::Factory::loadFromString(json_string); - Stats::StatsOptionsImpl stats_options; - Config::BootstrapJson::translateClusterManagerBootstrap(*json_object_ptr, bootstrap, - stats_options); + Config::BootstrapJson::translateClusterManagerBootstrap(*json_object_ptr, bootstrap); return bootstrap; } diff --git a/test/common/upstream/utility.h b/test/common/upstream/utility.h index e5bae8d39fbdb..d7f03be8c6800 100644 --- a/test/common/upstream/utility.h +++ b/test/common/upstream/utility.h @@ -6,7 +6,6 @@ #include "common/config/cds_json.h" #include "common/json/json_loader.h" #include "common/network/utility.h" -#include "common/stats/stats_options_impl.h" #include "common/upstream/upstream_impl.h" #include "fmt/printf.h" @@ -47,10 +46,8 @@ inline std::string clustersJson(const std::vector& clusters) { inline envoy::api::v2::Cluster parseClusterFromJson(const std::string& json_string) { envoy::api::v2::Cluster cluster; auto json_object_ptr = Json::Factory::loadFromString(json_string); - Stats::StatsOptionsImpl stats_options; Config::CdsJson::translateCluster(*json_object_ptr, - absl::optional(), cluster, - stats_options); + absl::optional(), cluster); return cluster; } @@ -69,8 +66,7 @@ parseSdsClusterFromJson(const std::string& json_string, const envoy::api::v2::core::ConfigSource eds_config) { envoy::api::v2::Cluster cluster; auto json_object_ptr = Json::Factory::loadFromString(json_string); - Stats::StatsOptionsImpl stats_options; - Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster, stats_options); + Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster); return cluster; } diff --git a/test/extensions/filters/http/dynamo/dynamo_utility_test.cc b/test/extensions/filters/http/dynamo/dynamo_utility_test.cc index 3ec3088d136c7..b00fd773e036d 100644 --- a/test/extensions/filters/http/dynamo/dynamo_utility_test.cc +++ b/test/extensions/filters/http/dynamo/dynamo_utility_test.cc @@ -18,20 +18,16 @@ namespace Dynamo { namespace { TEST(DynamoUtility, PartitionIdStatString) { - Stats::StatsOptionsImpl stats_options; - stats_options.max_obj_name_length_ = 60; - { std::string stat_prefix = "stat.prefix."; std::string table_name = "locations"; std::string operation = "GetItem"; std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; - std::string partition_stat_string = Utility::buildPartitionStatString( - stat_prefix, table_name, operation, partition_id, stats_options); + std::string partition_stat_string = + Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); 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_options.maxNameLength()); } { @@ -40,13 +36,12 @@ TEST(DynamoUtility, PartitionIdStatString) { std::string operation = "GetItem"; std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; - std::string partition_stat_string = Utility::buildPartitionStatString( - stat_prefix, table_name, operation, partition_id, stats_options); - std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" - "partition-test-iad-mytest-rea.capacity.GetItem.__partition_" - "id=c5883ca"; + std::string partition_stat_string = + Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = + "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-partition-test-iad-mytest-" + "really-long-name.capacity.GetItem.__partition_id=c5883ca"; EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() <= stats_options.maxNameLength()); } { std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; @@ -54,14 +49,13 @@ TEST(DynamoUtility, PartitionIdStatString) { std::string operation = "GetItem"; std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; - std::string partition_stat_string = Utility::buildPartitionStatString( - stat_prefix, table_name, operation, partition_id, stats_options); + std::string partition_stat_string = + Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" "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_options.maxNameLength()); } } diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh index 2bbdb149f2d81..15a8558a04ac7 100755 --- a/test/integration/hotrestart_test.sh +++ b/test/integration/hotrestart_test.sh @@ -91,33 +91,27 @@ do # string, compare it against a hard-coded string. start_test Checking for consistency of /hot_restart_version CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" 2>&1) - EXPECTED_CLI_HOT_RESTART_VERSION="10.200.16384.127.options=capacity=16384, num_slots=8209 hash=228984379728933363 size=2654312" + EXPECTED_CLI_HOT_RESTART_VERSION="11.104" + echo "The Envoy's hot restart version is ${CLI_HOT_RESTART_VERSION}" + echo "Now checking that the above version is what we expected." check [ "${CLI_HOT_RESTART_VERSION}" = "${EXPECTED_CLI_HOT_RESTART_VERSION}" ] + # TODO(fredlas) max-obj-name-len is a deprecated no-op; can probably remove this test soon. start_test Checking for consistency of /hot_restart_version with --max-obj-name-len 500 CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" \ --max-obj-name-len 500 2>&1) - EXPECTED_CLI_HOT_RESTART_VERSION="10.200.16384.567.options=capacity=16384, num_slots=8209 hash=228984379728933363 size=9863272" + EXPECTED_CLI_HOT_RESTART_VERSION="11.104" check [ "${CLI_HOT_RESTART_VERSION}" = "${EXPECTED_CLI_HOT_RESTART_VERSION}" ] start_test Checking for match of --hot-restart-version and admin /hot_restart_version ADMIN_ADDRESS_0=$(cat "${ADMIN_ADDRESS_PATH_0}") echo fetching hot restart version from http://${ADMIN_ADDRESS_0}/hot_restart_version ... ADMIN_HOT_RESTART_VERSION=$(curl -sg http://${ADMIN_ADDRESS_0}/hot_restart_version) + echo "Fetched ADMIN_HOT_RESTART_VERSION is ${ADMIN_HOT_RESTART_VERSION}" CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" \ --max-obj-name-len 500 2>&1) check [ "${ADMIN_HOT_RESTART_VERSION}" = "${CLI_HOT_RESTART_VERSION}" ] - start_test Checking for hot-restart-version mismatch when max-obj-name-len differs - CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" \ - --max-obj-name-len 1234 2>&1) - check [ "${ADMIN_HOT_RESTART_VERSION}" != "${CLI_HOT_RESTART_VERSION}" ] - - start_test Checking for hot-start-version mismatch when max-stats differs - CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" \ - --max-stats 12345 2>&1) - check [ "${ADMIN_HOT_RESTART_VERSION}" != "${CLI_HOT_RESTART_VERSION}" ] - enableHeapCheck start_test Starting epoch 1 diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 688d6fcd8be70..26889d884fbec 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -508,14 +508,10 @@ TEST_P(StatsMatcherIntegrationTest, ExcludeMultipleExact) { EXPECT_THAT(response_->body(), testing::Not(testing::HasSubstr("server.live"))); } -// TODO(ambuc): Find a cleaner way to test this. This test has two unfortunate compromises: -// -// - a) `listener_manager.listener_create_success` must be instantiated, because BaseIntegrationTest -// blocks on its creation (see waitForCounterGe and the suite of waitFor* functions), and -// - b) `stats.overflow` isn't blocked from instantiation, because it occurs at ThreadLocalStore -// construction time, before setStatsMatcher() is called. -// -// If either of these invariants is changed, this test must be rewritten. +// TODO(ambuc): Find a cleaner way to test this. This test has an unfortunate compromise: +// `listener_manager.listener_create_success` must be instantiated, because BaseIntegrationTest +// blocks on its creation (see waitForCounterGe and the suite of waitFor* functions). +// If this invariant is changed, this test must be rewritten. TEST_P(StatsMatcherIntegrationTest, IncludeExact) { stats_matcher_.mutable_inclusion_list()->add_patterns()->set_exact( "listener_manager.listener_create_success"); diff --git a/test/integration/run_envoy_test.sh b/test/integration/run_envoy_test.sh index 0ef2e661acb23..409f84bb34f17 100755 --- a/test/integration/run_envoy_test.sh +++ b/test/integration/run_envoy_test.sh @@ -37,9 +37,3 @@ expect_fail_with_error "error: invalid log level specified" --component-log-leve start_test Launching envoy with invalid component. expect_fail_with_error "error: invalid component specified" --component-log-level foo:debug - -start_test Launching envoy with max-obj-name-len value less than minimum value of 60. -expect_fail_with_error "error: the 'max-obj-name-len' value specified .* is less than the minimum" --max-obj-name-len 1 - -start_test Launching envoy with max-stats value more than maximum value of 100M. -expect_fail_with_error "error: the 'max-stats' value specified .* is more than the maximum value" --max-stats 1000000000 diff --git a/test/integration/server.cc b/test/integration/server.cc index 6076179aa3d47..5cfa3d21f26e1 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -37,7 +37,6 @@ OptionsImpl createTestOptionsImpl(const std::string& config_path, const std::str test_options.setFileFlushIntervalMsec(std::chrono::milliseconds(50)); test_options.setDrainTime(std::chrono::seconds(1)); test_options.setParentShutdownTime(std::chrono::seconds(2)); - test_options.setMaxStats(16384u); return test_options; } @@ -170,10 +169,10 @@ void IntegrationTestServerImpl::createAndRunEnvoyServer( Thread::BasicLockable& access_log_lock, Server::ComponentFactory& component_factory, Runtime::RandomGeneratorPtr&& random_generator) { Stats::FakeSymbolTableImpl symbol_table; - Server::HotRestartNopImpl restarter(symbol_table); + Server::HotRestartNopImpl restarter; ThreadLocal::InstanceImpl tls; Stats::HeapStatDataAllocator stats_allocator(symbol_table); - Stats::ThreadLocalStoreImpl stat_store(options.statsOptions(), stats_allocator); + Stats::ThreadLocalStoreImpl stat_store(stats_allocator); Server::InstanceImpl server(options, time_system, local_address, hooks, restarter, stat_store, access_log_lock, component_factory, std::move(random_generator), tls, diff --git a/test/integration/server.h b/test/integration/server.h index d8cd3cbb840a3..52a130b3cff71 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -108,12 +108,10 @@ class TestScopeWrapper : public Scope { const SymbolTable& symbolTable() const override { return wrapped_scope_->symbolTable(); } SymbolTable& symbolTable() override { return wrapped_scope_->symbolTable(); } - const StatsOptions& statsOptions() const override { return stats_options_; } private: Thread::MutexBasicLockable& lock_; ScopePtr wrapped_scope_; - StatsOptionsImpl stats_options_; }; /** @@ -154,7 +152,6 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.histogram(name); } - const StatsOptions& statsOptions() const override { return stats_options_; } const SymbolTable& symbolTable() const override { return store_.symbolTable(); } SymbolTable& symbolTable() override { return store_.symbolTable(); } @@ -186,7 +183,6 @@ class TestIsolatedStoreImpl : public StoreRoot { mutable Thread::MutexBasicLockable lock_; IsolatedStoreImpl store_; SourceImpl source_; - StatsOptionsImpl stats_options_; }; } // namespace Stats diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index d352a3a3836eb..b7e8504864995 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -640,7 +640,6 @@ TEST_P(XfccIntegrationTest, TagExtractedNameGenerationTest) { {"http.admin.downstream_rq_tx_reset", "http.downstream_rq_tx_reset"}, {"http.admin.downstream_flow_control_resumed_reading_total", "http.downstream_flow_control_resumed_reading_total"}, - {"stats.overflow", "stats.overflow"}, {"http.admin.downstream_cx_total", "http.downstream_cx_total"}, {"http.admin.downstream_rq_3xx", "http.downstream_rq_xx"}, {"http.admin.downstream_cx_idle_timeout", "http.downstream_cx_idle_timeout"}, diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 1713385299647..3f8397a68b503 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -28,8 +28,6 @@ MockOptions::MockOptions(const std::string& config_path) : config_path_(config_p ON_CALL(*this, serviceZone()).WillByDefault(ReturnRef(service_zone_name_)); ON_CALL(*this, logLevel()).WillByDefault(Return(log_level_)); ON_CALL(*this, logPath()).WillByDefault(ReturnRef(log_path_)); - ON_CALL(*this, maxStats()).WillByDefault(Return(1000)); - ON_CALL(*this, statsOptions()).WillByDefault(ReturnRef(stats_options_)); ON_CALL(*this, restartEpoch()).WillByDefault(ReturnPointee(&hot_restart_epoch_)); ON_CALL(*this, hotRestartDisabled()).WillByDefault(ReturnPointee(&hot_restart_disabled_)); ON_CALL(*this, signalHandlingEnabled()).WillByDefault(ReturnPointee(&signal_handling_enabled_)); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 3046f17672bab..93ccbc954df52 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -18,7 +18,6 @@ #include "envoy/server/worker.h" #include "envoy/ssl/context_manager.h" #include "envoy/stats/scope.h" -#include "envoy/stats/stats_options.h" #include "envoy/thread/thread.h" #include "common/http/context_impl.h" @@ -44,7 +43,6 @@ #include "absl/strings/string_view.h" #include "gmock/gmock.h" -#include "gtest/gtest.h" #include "spdlog/spdlog.h" namespace Envoy { @@ -75,8 +73,6 @@ class MockOptions : public Options { MOCK_CONST_METHOD0(serviceClusterName, const std::string&()); MOCK_CONST_METHOD0(serviceNodeName, const std::string&()); MOCK_CONST_METHOD0(serviceZone, const std::string&()); - MOCK_CONST_METHOD0(maxStats, uint64_t()); - MOCK_CONST_METHOD0(statsOptions, const Stats::StatsOptions&()); MOCK_CONST_METHOD0(hotRestartDisabled, bool()); MOCK_CONST_METHOD0(signalHandlingEnabled, bool()); MOCK_CONST_METHOD0(mutexTracingEnabled, bool()); @@ -92,7 +88,6 @@ class MockOptions : public Options { std::string service_zone_name_; spdlog::level::level_enum log_level_{spdlog::level::trace}; std::string log_path_; - Stats::StatsOptionsImpl stats_options_; uint32_t concurrency_{1}; uint64_t hot_restart_epoch_{}; bool hot_restart_disabled_{}; @@ -199,10 +194,11 @@ class MockHotRestart : public HotRestart { // Server::HotRestart MOCK_METHOD0(drainParentListeners, void()); MOCK_METHOD1(duplicateParentListenSocket, int(const std::string& address)); - MOCK_METHOD1(getParentStats, void(GetParentStatsInfo& info)); + MOCK_METHOD0(getParentStats, std::unique_ptr()); MOCK_METHOD2(initialize, void(Event::Dispatcher& dispatcher, Server::Instance& server)); - MOCK_METHOD1(shutdownParentAdmin, void(ShutdownParentAdminInfo& info)); - MOCK_METHOD0(terminateParent, void()); + MOCK_METHOD1(sendParentAdminShutdownRequest, void(time_t& original_start_time)); + MOCK_METHOD0(sendParentTerminateRequest, void()); + MOCK_METHOD1(mergeParentStatsIfAny, ServerStatsFromParent(Stats::StoreRoot& stats_store)); MOCK_METHOD0(shutdown, void()); MOCK_METHOD0(version, std::string()); MOCK_METHOD0(logLock, Thread::BasicLockable&()); @@ -348,7 +344,7 @@ class MockInstance : public Instance { MOCK_METHOD0(drainManager, DrainManager&()); MOCK_METHOD0(accessLogManager, AccessLog::AccessLogManager&()); MOCK_METHOD1(failHealthcheck, void(bool fail)); - MOCK_METHOD1(getParentStats, void(HotRestart::GetParentStatsInfo&)); + MOCK_METHOD1(exportStatsToChild, void(envoy::HotRestartMessage::Reply::Stats*)); MOCK_METHOD0(healthCheckFailed, bool()); MOCK_METHOD0(hotRestart, HotRestart&()); MOCK_METHOD0(initManager, Init::Manager&()); diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index bea653c71e84c..53d2e88812d41 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -91,7 +91,6 @@ MockStore::MockStore() : StoreImpl(*fake_symbol_table_) { histograms_.emplace_back(histogram); return *histogram; })); - ON_CALL(*this, statsOptions()).WillByDefault(ReturnRef(stats_options_)); } MockStore::~MockStore() {} diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 37edb5e6a6a38..1d841f6ca1318 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -103,6 +103,8 @@ class MockGauge : public Gauge, public MockMetric { MOCK_METHOD1(sub, void(uint64_t amount)); MOCK_CONST_METHOD0(used, bool()); MOCK_CONST_METHOD0(value, uint64_t()); + MOCK_CONST_METHOD0(cachedShouldImport, absl::optional()); + MOCK_METHOD1(setShouldImport, void(bool should_import)); bool used_; uint64_t value_; @@ -184,7 +186,6 @@ class MockStore : public SymbolTableProvider, public StoreImpl { MOCK_CONST_METHOD0(gauges, std::vector()); MOCK_METHOD1(histogram, Histogram&(const std::string& name)); MOCK_CONST_METHOD0(histograms, std::vector()); - MOCK_CONST_METHOD0(statsOptions, const StatsOptions&()); Counter& counterFromStatName(StatName name) override { return counter(symbol_table_->toString(name)); @@ -200,7 +201,6 @@ class MockStore : public SymbolTableProvider, public StoreImpl { Test::Global symbol_table_; testing::NiceMock counter_; std::vector> histograms_; - StatsOptionsImpl stats_options_; }; /** diff --git a/test/server/BUILD b/test/server/BUILD index 3ebf82386d89f..6bec1b1689361 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -80,6 +80,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "hot_restarting_parent_test", + srcs = envoy_select_hot_restart(["hot_restarting_parent_test.cc"]), + deps = [ + "//source/common/stats:stats_lib", + "//source/server:hot_restart_lib", + "//test/mocks/server:server_mocks", + ], +) + envoy_cc_test( name = "guarddog_impl_test", srcs = ["guarddog_impl_test.cc"], diff --git a/test/server/drain_manager_impl_test.cc b/test/server/drain_manager_impl_test.cc index 219b8a6c212f4..ba69e4dffdb18 100644 --- a/test/server/drain_manager_impl_test.cc +++ b/test/server/drain_manager_impl_test.cc @@ -36,7 +36,7 @@ TEST_F(DrainManagerImplTest, Default) { EXPECT_CALL(*shutdown_timer, enableTimer(std::chrono::milliseconds(900000))); drain_manager.startParentShutdownSequence(); - EXPECT_CALL(server_.hot_restart_, terminateParent()); + EXPECT_CALL(server_.hot_restart_, sendParentTerminateRequest()); shutdown_timer->callback_(); // Verify basic drain close. diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 035c44d013013..ce287407b5dc7 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -15,6 +15,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::AnyNumber; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::Return; @@ -28,7 +29,7 @@ namespace { class HotRestartImplTest : public testing::Test { public: void setup() { - EXPECT_CALL(os_sys_calls_, shmUnlink(_)); + EXPECT_CALL(os_sys_calls_, shmUnlink(_)).Times(AnyNumber()); EXPECT_CALL(os_sys_calls_, shmOpen(_, _, _)); EXPECT_CALL(os_sys_calls_, ftruncate(_, _)).WillOnce(WithArg<1>(Invoke([this](off_t size) { buffer_.resize(size); @@ -37,37 +38,32 @@ class HotRestartImplTest : public testing::Test { EXPECT_CALL(os_sys_calls_, mmap(_, _, _, _, _, _)).WillOnce(InvokeWithoutArgs([this]() { return Api::SysCallPtrResult{buffer_.data(), 0}; })); - EXPECT_CALL(os_sys_calls_, bind(_, _, _)); - EXPECT_CALL(options_, statsOptions()).WillRepeatedly(ReturnRef(stats_options_)); + // We bind two sockets: one to talk to parent, one to talk to our (hypothetical eventual) child + EXPECT_CALL(os_sys_calls_, bind(_, _, _)).Times(2); // Test we match the correct stat with empty-slots before, after, or both. - hot_restart_ = std::make_unique(options_, symbol_table_); + hot_restart_ = std::make_unique(options_); hot_restart_->drainParentListeners(); } - Stats::FakeSymbolTableImpl symbol_table_; Api::MockOsSysCalls os_sys_calls_; TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; NiceMock options_; - Stats::StatsOptionsImpl stats_options_; std::vector buffer_; std::unique_ptr hot_restart_; }; TEST_F(HotRestartImplTest, versionString) { - // Tests that the version-string will be consistent and SharedMemory::VERSION, + // Tests that the version-string will be consistent and HOT_RESTART_VERSION, // between multiple instantiations. std::string version; - 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. { setup(); 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_.statsOptions().maxObjNameLength(); + EXPECT_TRUE(absl::StartsWith(version, fmt::format("{}.", HOT_RESTART_VERSION))) << version; TearDown(); } @@ -76,179 +72,8 @@ TEST_F(HotRestartImplTest, versionString) { EXPECT_EQ(version, hot_restart_->version()) << "Version string deterministic from options"; TearDown(); } - - { - ON_CALL(options_, maxStats()).WillByDefault(Return(2 * max_stats)); - setup(); - EXPECT_NE(version, hot_restart_->version()) << "Version changes when max-stats change"; - TearDown(); - } - - { - stats_options_.max_obj_name_length_ = 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. - } -} - -// Check consistency of internal raw stat representation by comparing hash of -// memory contents against a previously recorded value. -TEST_F(HotRestartImplTest, Consistency) { - setup(); - - // Generate a stat, encode it to hex, and take the hash of that hex string. We - // expect the hash to vary only when the internal representation of a stat has - // been intentionally changed, in which case SharedMemory::VERSION should be - // incremented as well. - const uint64_t expected_hash = 1874506077228772558; - const uint64_t max_name_length = stats_options_.maxNameLength(); - - const std::string name_1(max_name_length, 'A'); - Stats::RawStatData* stat_1 = hot_restart_->statsAllocator().alloc(name_1); - const uint64_t stat_size = sizeof(Stats::RawStatData) + max_name_length; - const std::string stat_hex_dump_1 = Hex::encode(reinterpret_cast(stat_1), stat_size); - EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); - EXPECT_EQ(name_1, stat_1->key()); - hot_restart_->statsAllocator().free(*stat_1); -} - -TEST_F(HotRestartImplTest, crossAlloc) { - setup(); - - 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; - - EXPECT_CALL(options_, restartEpoch()).WillRepeatedly(Return(1)); - EXPECT_CALL(os_sys_calls_, shmOpen(_, _, _)); - EXPECT_CALL(os_sys_calls_, mmap(_, _, _, _, _, _)) - .WillOnce(Return(Api::SysCallPtrResult{buffer_.data(), 0})); - EXPECT_CALL(os_sys_calls_, bind(_, _, _)); - HotRestartImpl hot_restart2(options_, symbol_table_); - 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, allocFail) { - EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(2)); - setup(); - - Stats::RawStatData* s1 = hot_restart_->statsAllocator().alloc("1"); - Stats::RawStatData* s2 = hot_restart_->statsAllocator().alloc("2"); - Stats::RawStatData* s3 = hot_restart_->statsAllocator().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()) { - stats_options_.max_obj_name_length_ = name_len_; - EXPECT_CALL(options_, statsOptions()).WillRepeatedly(ReturnRef(stats_options_)); - EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(num_stats_)); - - setup(); - EXPECT_EQ(name_len_ + stats_options_.maxStatSuffixLength(), stats_options_.maxNameLength()); - } - - Stats::StatsOptionsImpl stats_options_; - 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_->statsAllocator().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_options_.maxNameLength()); - TestStat ts; - ts.stat_ = hot_restart_->statsAllocator().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_options_.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_SUITE_P(HotRestartImplAlignmentTest, HotRestartImplAlignmentTest, - testing::Range(0UL, alignof(Stats::RawStatData) + 1)); - } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/hot_restarting_parent_test.cc b/test/server/hot_restarting_parent_test.cc new file mode 100644 index 0000000000000..5d5496504ab16 --- /dev/null +++ b/test/server/hot_restarting_parent_test.cc @@ -0,0 +1,87 @@ +#include + +#include "server/hot_restarting_parent.h" + +#include "test/mocks/server/mocks.h" + +#include "gtest/gtest.h" + +using testing::_; +using testing::Return; +using testing::ReturnRef; + +namespace Envoy { +namespace Server { +namespace { + +using HotRestartMessage = envoy::HotRestartMessage; + +class HotRestartingParentTest : public testing::Test { +public: + MockInstance server_; + HotRestartingParent::Internal hot_restarting_parent_{&server_}; +}; + +TEST_F(HotRestartingParentTest, shutdownAdmin) { + EXPECT_CALL(server_, shutdownAdmin()); + EXPECT_CALL(server_, startTimeFirstEpoch()).WillOnce(Return(12345)); + HotRestartMessage message = hot_restarting_parent_.shutdownAdmin(); + EXPECT_EQ(12345, message.reply().shutdown_admin().original_start_time_unix_seconds()); +} + +TEST_F(HotRestartingParentTest, getListenSocketsForChildNotFound) { + MockListenerManager listener_manager; + std::vector> listeners; + EXPECT_CALL(server_, listenerManager()).WillOnce(ReturnRef(listener_manager)); + EXPECT_CALL(listener_manager, listeners()).WillOnce(Return(listeners)); + + HotRestartMessage::Request request; + request.mutable_pass_listen_socket()->set_address("tcp://127.0.0.1:80"); + HotRestartMessage message = hot_restarting_parent_.getListenSocketsForChild(request); + EXPECT_EQ(-1, message.reply().pass_listen_socket().fd()); +} + +TEST_F(HotRestartingParentTest, exportStatsToChild) { + Stats::IsolatedStoreImpl store; + MockListenerManager listener_manager; + EXPECT_CALL(server_, listenerManager()).WillRepeatedly(ReturnRef(listener_manager)); + EXPECT_CALL(listener_manager, numConnections()).WillRepeatedly(Return(0)); + EXPECT_CALL(server_, stats()).WillRepeatedly(ReturnRef(store)); + + { + store.counter("c1").inc(); + store.counter("c2").add(2); + store.gauge("g0").set(0); + store.gauge("g1").set(123); + store.gauge("g2").set(456); + HotRestartMessage::Reply::Stats stats; + hot_restarting_parent_.exportStatsToChild(&stats); + EXPECT_EQ(1, stats.counter_deltas().at("c1")); + EXPECT_EQ(2, stats.counter_deltas().at("c2")); + EXPECT_EQ(0, stats.gauges().at("g0")); + EXPECT_EQ(123, stats.gauges().at("g1")); + EXPECT_EQ(456, stats.gauges().at("g2")); + } + // When a counter has not changed since its last export, it should not be included in the message. + { + store.counter("c2").add(2); + store.gauge("g1").add(1); + store.gauge("g2").sub(1); + HotRestartMessage::Reply::Stats stats; + hot_restarting_parent_.exportStatsToChild(&stats); + EXPECT_EQ(stats.counter_deltas().end(), stats.counter_deltas().find("c1")); + EXPECT_EQ(2, stats.counter_deltas().at("c2")); // 4 is the value, but 2 is the delta + EXPECT_EQ(0, stats.gauges().at("g0")); + EXPECT_EQ(124, stats.gauges().at("g1")); + EXPECT_EQ(455, stats.gauges().at("g2")); + } +} + +TEST_F(HotRestartingParentTest, drainListeners) { + EXPECT_CALL(server_, drainListeners()); + hot_restarting_parent_.drainListeners(); +} + +} // namespace +} // namespace Server +} // namespace Envoy diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 52ac66b6b980d..21ece6c4336a7 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -50,8 +50,8 @@ namespace Server { class AdminStatsTest : public testing::TestWithParam { public: - AdminStatsTest() : alloc_(options_, symbol_table_) { - store_ = std::make_unique(options_, alloc_); + AdminStatsTest() : alloc_(symbol_table_) { + store_ = std::make_unique(alloc_); store_->addSink(sink_); } @@ -66,8 +66,7 @@ class AdminStatsTest : public testing::TestWithParam main_thread_dispatcher_; NiceMock tls_; - Stats::StatsOptionsImpl options_; - Stats::MockedTestAllocator alloc_; + Stats::HeapStatDataAllocator alloc_; Stats::MockSink sink_; std::unique_ptr store_; }; @@ -114,14 +113,11 @@ TEST_P(AdminStatsTest, StatsAsJson) { store_->mergeHistograms([]() -> void {}); - EXPECT_CALL(alloc_, free(_)); - - std::map all_stats; - std::vector histograms = store_->histograms(); std::sort(histograms.begin(), histograms.end(), [](const Stats::ParentHistogramSharedPtr& a, const Stats::ParentHistogramSharedPtr& b) -> bool { return a->name() < b->name(); }); + std::map all_stats; std::string actual_json = statsAsJsonHandler(all_stats, histograms, false); const std::string expected_json = R"EOF({ @@ -263,10 +259,7 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJson) { store_->mergeHistograms([]() -> void {}); - EXPECT_CALL(alloc_, free(_)); - std::map all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms(), true); // Expected JSON should not have h2 values as it is not used. @@ -364,10 +357,7 @@ TEST_P(AdminStatsTest, StatsAsJsonFilterString) { store_->mergeHistograms([]() -> void {}); - EXPECT_CALL(alloc_, free(_)); - std::map all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms(), false, absl::optional{std::regex("[a-z]1")}); @@ -473,10 +463,7 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJsonFilterString) { store_->mergeHistograms([]() -> void {}); - EXPECT_CALL(alloc_, free(_)); - std::map all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms(), true, absl::optional{std::regex("h[12]")}); @@ -1283,7 +1270,6 @@ class PrometheusStatsFormatterTest : public testing::Test { } Stats::FakeSymbolTableImpl symbol_table_; - Stats::StatsOptionsImpl stats_options_; Stats::HeapStatDataAllocator alloc_; std::vector counters_; std::vector gauges_; diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index ff408e7f8bf7b..2fe7db220c2e3 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -41,8 +41,7 @@ class OptionsImplTest : public testing::Test { argv.push_back(s.c_str()); } return std::make_unique( - argv.size(), argv.data(), [](uint64_t, uint64_t, bool) { return "1"; }, - spdlog::level::warn); + argv.size(), argv.data(), [](bool) { return "1"; }, spdlog::level::warn); } }; @@ -106,9 +105,6 @@ TEST_F(OptionsImplTest, SetAll) { bool hot_restart_disabled = options->hotRestartDisabled(); bool signal_handling_enabled = options->signalHandlingEnabled(); bool cpuset_threads_enabled = options->cpusetThreadsEnabled(); - Stats::StatsOptionsImpl stats_options; - stats_options.max_obj_name_length_ = 54321; - stats_options.max_stat_suffix_length_ = 1234; options->setBaseId(109876); options->setConcurrency(42); @@ -127,8 +123,6 @@ TEST_F(OptionsImplTest, SetAll) { options->setServiceClusterName("cluster_foo"); options->setServiceNodeName("node_foo"); options->setServiceZone("zone_foo"); - options->setMaxStats(12345); - options->setStatsOptions(stats_options); options->setHotRestartDisabled(!options->hotRestartDisabled()); options->setSignalHandling(!options->signalHandlingEnabled()); options->setCpusetThreads(!options->cpusetThreadsEnabled()); @@ -150,9 +144,6 @@ TEST_F(OptionsImplTest, SetAll) { EXPECT_EQ("cluster_foo", options->serviceClusterName()); EXPECT_EQ("node_foo", options->serviceNodeName()); EXPECT_EQ("zone_foo", options->serviceZone()); - EXPECT_EQ(12345U, options->maxStats()); - EXPECT_EQ(stats_options.max_obj_name_length_, options->statsOptions().maxObjNameLength()); - EXPECT_EQ(stats_options.max_stat_suffix_length_, options->statsOptions().maxStatSuffixLength()); EXPECT_EQ(!hot_restart_disabled, options->hotRestartDisabled()); EXPECT_EQ(!signal_handling_enabled, options->signalHandlingEnabled()); EXPECT_EQ(!cpuset_threads_enabled, options->cpusetThreadsEnabled()); @@ -182,8 +173,6 @@ TEST_F(OptionsImplTest, SetAll) { EXPECT_EQ(options->serviceClusterName(), command_line_options->service_cluster()); EXPECT_EQ(options->serviceNodeName(), command_line_options->service_node()); EXPECT_EQ(options->serviceZone(), command_line_options->service_zone()); - EXPECT_EQ(options->maxStats(), command_line_options->max_stats()); - EXPECT_EQ(options->statsOptions().maxObjNameLength(), command_line_options->max_obj_name_len()); EXPECT_EQ(options->hotRestartDisabled(), command_line_options->disable_hot_restart()); EXPECT_EQ(options->mutexTracingEnabled(), command_line_options->enable_mutex_tracing()); EXPECT_EQ(options->cpusetThreadsEnabled(), command_line_options->cpuset_threads()); @@ -231,16 +220,6 @@ TEST_F(OptionsImplTest, BadCliOption) { MalformedArgvException, "error: unknown IP address version 'foo'"); } -TEST_F(OptionsImplTest, BadObjNameLenOption) { - EXPECT_THROW_WITH_REGEX(createOptionsImpl("envoy --max-obj-name-len 1"), MalformedArgvException, - "'max-obj-name-len' value specified"); -} - -TEST_F(OptionsImplTest, BadMaxStatsOption) { - EXPECT_THROW_WITH_REGEX(createOptionsImpl("envoy --max-stats 1000000000"), MalformedArgvException, - "'max-stats' value specified"); -} - TEST_F(OptionsImplTest, ParseComponentLogLevels) { std::unique_ptr options = createOptionsImpl("envoy --mode init_only"); options->parseComponentLogLevels("upstream:debug,connection:trace"); @@ -314,13 +293,6 @@ TEST_F(OptionsImplTest, SaneTestConstructor) { EXPECT_EQ(regular_options_impl->mode(), test_options_impl.mode()); EXPECT_EQ(regular_options_impl->fileFlushIntervalMsec(), test_options_impl.fileFlushIntervalMsec()); - EXPECT_EQ(regular_options_impl->maxStats(), test_options_impl.maxStats()); - EXPECT_EQ(regular_options_impl->statsOptions().maxNameLength(), - test_options_impl.statsOptions().maxNameLength()); - EXPECT_EQ(regular_options_impl->statsOptions().maxObjNameLength(), - test_options_impl.statsOptions().maxObjNameLength()); - EXPECT_EQ(regular_options_impl->statsOptions().maxStatSuffixLength(), - test_options_impl.statsOptions().maxStatSuffixLength()); EXPECT_EQ(regular_options_impl->hotRestartDisabled(), test_options_impl.hotRestartDisabled()); EXPECT_EQ(regular_options_impl->cpusetThreadsEnabled(), test_options_impl.cpusetThreadsEnabled()); } diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 293640e4d3cee..32a7e991b2324 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -99,7 +99,6 @@ envoy_cc_test_library( "//include/envoy/http:codec_interface", "//include/envoy/network:address_interface", "//source/common/api:api_lib", - "//source/common/common:block_memory_hash_set_lib", "//source/common/common:empty_string", "//source/common/common:thread_lib", "//source/common/common:utility_lib", @@ -113,7 +112,6 @@ envoy_cc_test_library( "//source/common/protobuf:utility_lib", "//source/common/stats:fake_symbol_table_lib", "//source/common/stats:stats_lib", - "//source/common/stats:stats_options_lib", "//test/mocks/stats:stats_mocks", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", ], diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index c3f924dff3a99..d3a40f94eb29d 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -164,7 +164,7 @@ std::vector TestEnvironment::getIpVersionsForTest() Server::Options& TestEnvironment::getOptions() { static OptionsImpl* options = new OptionsImpl( - argc_, argv_, [](uint64_t, uint64_t, bool) { return "1"; }, spdlog::level::err); + argc_, argv_, [](bool) { return "1"; }, spdlog::level::err); return *options; } diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index f77209822b07e..f7b2a49da7fe5 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -34,7 +34,6 @@ #include "common/json/json_loader.h" #include "common/network/address_impl.h" #include "common/network/utility.h" -#include "common/stats/stats_options_impl.h" #include "common/filesystem/directory.h" #include "common/filesystem/filesystem_impl.h" @@ -179,8 +178,7 @@ envoy::config::bootstrap::v2::Bootstrap TestUtility::parseBootstrapFromJson(const std::string& json_string) { envoy::config::bootstrap::v2::Bootstrap bootstrap; auto json_object_ptr = Json::Factory::loadFromString(json_string); - Stats::StatsOptionsImpl stats_options; - Config::BootstrapJson::translateBootstrap(*json_object_ptr, bootstrap, stats_options); + Config::BootstrapJson::translateBootstrap(*json_object_ptr, bootstrap); return bootstrap; } @@ -381,26 +379,6 @@ bool TestHeaderMapImpl::has(const LowerCaseString& key) { return get(key) != nul } // namespace Http -namespace Stats { - -MockedTestAllocator::MockedTestAllocator(const StatsOptions& stats_options, - SymbolTable& symbol_table) - : TestAllocator(stats_options, symbol_table) { - ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](absl::string_view name) -> RawStatData* { - return TestAllocator::alloc(name); - })); - - ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { - return TestAllocator::free(data); - })); - - EXPECT_CALL(*this, alloc(absl::string_view("stats.overflow"))); -} - -MockedTestAllocator::~MockedTestAllocator() {} - -} // namespace Stats - namespace Api { class TestImplProvider { diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 650160733bda7..822c159a870d6 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -16,13 +16,11 @@ #include "envoy/thread/thread.h" #include "common/buffer/buffer_impl.h" -#include "common/common/block_memory_hash_set.h" #include "common/common/c_smart_ptr.h" #include "common/common/thread.h" #include "common/http/header_map_impl.h" #include "common/protobuf/utility.h" #include "common/stats/fake_symbol_table_impl.h" -#include "common/stats/raw_stat_data.h" #include "test/test_common/file_system_for_test.h" #include "test/test_common/printers.h" @@ -495,48 +493,6 @@ makeHeaderMap(const std::initializer_list>& } // namespace Http -namespace Stats { - -/** - * Implements a RawStatDataAllocator using a contiguous block of heap-allocated - * memory, but is otherwise identical to the shared memory allocator in terms of - * reference counting, data structures, etc. - */ -class TestAllocator : public RawStatDataAllocator { -public: - struct TestBlockMemoryHashSetOptions : public BlockMemoryHashSetOptions { - TestBlockMemoryHashSetOptions() { - capacity = 200; - num_slots = 131; - } - }; - - TestAllocator(const StatsOptions& stats_options, SymbolTable& symbol_table) - : RawStatDataAllocator(mutex_, hash_set_, stats_options, symbol_table), - block_memory_(std::make_unique( - RawStatDataSet::numBytes(block_hash_options_, stats_options))), - hash_set_(block_hash_options_, true /* init */, block_memory_.get(), stats_options) {} - ~TestAllocator() { EXPECT_EQ(0, hash_set_.size()); } - -private: - FakeSymbolTableImpl symbol_table_; - Thread::MutexBasicLockable mutex_; - TestBlockMemoryHashSetOptions block_hash_options_; - std::unique_ptr block_memory_; - RawStatDataSet hash_set_; -}; - -class MockedTestAllocator : public TestAllocator { -public: - MockedTestAllocator(const StatsOptions& stats_options, SymbolTable& symbol_table); - virtual ~MockedTestAllocator(); - - MOCK_METHOD1(alloc, RawStatData*(absl::string_view name)); - MOCK_METHOD1(free, void(RawStatData& data)); -}; - -} // namespace Stats - namespace Api { ApiPtr createApiForTest(); ApiPtr createApiForTest(Stats::Store& stat_store); diff --git a/test/tools/schema_validator/validator.cc b/test/tools/schema_validator/validator.cc index 22055bb1270ce..35be4b896b04b 100644 --- a/test/tools/schema_validator/validator.cc +++ b/test/tools/schema_validator/validator.cc @@ -55,10 +55,7 @@ void Validator::validate(const std::string& json_path, Schema::Type schema_type) // Construct a envoy::api::v2::RouteConfiguration to validate the Route configuration and // ignore the output since nothing will consume it. envoy::api::v2::RouteConfiguration route_config; - // TODO(ambuc): Add a CLI option to the schema_validator to allow for a maxStatNameLength - // constraint - Stats::StatsOptionsImpl stats_options; - Config::RdsJson::translateRouteConfiguration(*loader, route_config, stats_options); + Config::RdsJson::translateRouteConfiguration(*loader, route_config); break; } default: diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index c1fcb1afbd5cb..cee943f454975 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -21,6 +21,7 @@ CHACHA CIDR CLA CLI +CMSG CN CNAME CORS @@ -719,6 +720,7 @@ tokenize tokenizes tokenizing traceid +transferral trie tuple tuples