From 9bfd872857d4388566ffe65c024429e474f9624b Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Fri, 15 May 2020 14:58:23 -0700 Subject: [PATCH] Refactor resource manager (#11182) This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen --- include/envoy/common/BUILD | 5 ++ include/envoy/common/resource.h | 47 ++++++++++++ include/envoy/network/BUILD | 1 + include/envoy/upstream/BUILD | 1 + include/envoy/upstream/resource_manager.h | 63 ++++------------ source/common/common/BUILD | 9 +++ source/common/common/basic_resource_impl.h | 60 +++++++++++++++ source/common/upstream/BUILD | 1 + source/common/upstream/conn_pool_map_impl.h | 2 +- .../common/upstream/resource_manager_impl.h | 60 +++++++-------- source/server/BUILD | 2 + source/server/http/BUILD | 1 + test/common/common/BUILD | 9 +++ .../common/common/basic_resource_impl_test.cc | 73 +++++++++++++++++++ test/integration/stats_integration_test.cc | 6 +- 15 files changed, 253 insertions(+), 87 deletions(-) create mode 100644 include/envoy/common/resource.h create mode 100644 source/common/common/basic_resource_impl.h create mode 100644 test/common/common/basic_resource_impl_test.cc diff --git a/include/envoy/common/BUILD b/include/envoy/common/BUILD index 105f8b4374b7..0565076813c7 100644 --- a/include/envoy/common/BUILD +++ b/include/envoy/common/BUILD @@ -24,6 +24,11 @@ envoy_cc_library( hdrs = ["mutex_tracer.h"], ) +envoy_cc_library( + name = "resource_interface", + hdrs = ["resource.h"], +) + envoy_cc_library( name = "time_interface", hdrs = ["time.h"], diff --git a/include/envoy/common/resource.h b/include/envoy/common/resource.h new file mode 100644 index 000000000000..6b04afcfdf4b --- /dev/null +++ b/include/envoy/common/resource.h @@ -0,0 +1,47 @@ +#include + +#include "envoy/common/pure.h" + +#pragma once + +namespace Envoy { + +/** + * A handle for use by any resource managers. + */ +class ResourceLimit { +public: + virtual ~ResourceLimit() = default; + + /** + * @return true if the resource can be created. + */ + virtual bool canCreate() PURE; + + /** + * Increment the resource count. + */ + virtual void inc() PURE; + + /** + * Decrement the resource count. + */ + virtual void dec() PURE; + + /** + * Decrement the resource count by a specific amount. + */ + virtual void decBy(uint64_t amount) PURE; + + /** + * @return the current maximum allowed number of this resource. + */ + virtual uint64_t max() PURE; + + /** + * @return the current resource count. + */ + virtual uint64_t count() const PURE; +}; + +} // namespace Envoy diff --git a/include/envoy/network/BUILD b/include/envoy/network/BUILD index 229ad3019523..16a0945a2770 100644 --- a/include/envoy/network/BUILD +++ b/include/envoy/network/BUILD @@ -118,6 +118,7 @@ envoy_cc_library( ":connection_interface", ":listen_socket_interface", "//include/envoy/access_log:access_log_interface", + "//include/envoy/common:resource_interface", "//include/envoy/stats:stats_interface", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/include/envoy/upstream/BUILD b/include/envoy/upstream/BUILD index 2ecf8e601b7c..c6fe37d45beb 100644 --- a/include/envoy/upstream/BUILD +++ b/include/envoy/upstream/BUILD @@ -114,6 +114,7 @@ envoy_cc_library( envoy_cc_library( name = "resource_manager_interface", hdrs = ["resource_manager.h"], + deps = ["//include/envoy/common:resource_interface"], ) envoy_cc_library( diff --git a/include/envoy/upstream/resource_manager.h b/include/envoy/upstream/resource_manager.h index c10ff89c033f..5cac59a1a0ad 100644 --- a/include/envoy/upstream/resource_manager.h +++ b/include/envoy/upstream/resource_manager.h @@ -5,6 +5,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/common/resource.h" namespace Envoy { namespace Upstream { @@ -16,54 +17,16 @@ namespace Upstream { enum class ResourcePriority { Default, High }; const size_t NumResourcePriorities = 2; -/** - * An individual resource tracked by the resource manager. - */ -class Resource { -public: - virtual ~Resource() = default; - - /** - * @return true if the resource can be created. - */ - virtual bool canCreate() PURE; - - /** - * Increment the resource count. - */ - virtual void inc() PURE; - - /** - * Decrement the resource count. - */ - virtual void dec() PURE; - - /** - * Decrement the resource count by a specific amount. - */ - virtual void decBy(uint64_t amount) PURE; - - /** - * @return the current maximum allowed number of this resource. - */ - virtual uint64_t max() PURE; - - /** - * @return the current resource count. - */ - virtual uint64_t count() const PURE; -}; - /** * RAII wrapper that increments a resource on construction and decrements it on destruction. */ class ResourceAutoIncDec { public: - ResourceAutoIncDec(Resource& resource) : resource_(resource) { resource_.inc(); } + ResourceAutoIncDec(ResourceLimit& resource) : resource_(resource) { resource_.inc(); } ~ResourceAutoIncDec() { resource_.dec(); } private: - Resource& resource_; + ResourceLimit& resource_; }; using ResourceAutoIncDecPtr = std::unique_ptr; @@ -78,31 +41,31 @@ class ResourceManager { virtual ~ResourceManager() = default; /** - * @return Resource& active TCP connections and UDP sessions. + * @return ResourceLimit& active TCP connections and UDP sessions. */ - virtual Resource& connections() PURE; + virtual ResourceLimit& connections() PURE; /** - * @return Resource& active pending requests (requests that have not yet been attached to a + * @return ResourceLimit& active pending requests (requests that have not yet been attached to a * connection pool connection). */ - virtual Resource& pendingRequests() PURE; + virtual ResourceLimit& pendingRequests() PURE; /** - * @return Resource& active requests (requests that are currently bound to a connection pool + * @return ResourceLimit& active requests (requests that are currently bound to a connection pool * connection and are awaiting response). */ - virtual Resource& requests() PURE; + virtual ResourceLimit& requests() PURE; /** - * @return Resource& active retries. + * @return ResourceLimit& active retries. */ - virtual Resource& retries() PURE; + virtual ResourceLimit& retries() PURE; /** - * @return Resource& active connection pools. + * @return ResourceLimit& active connection pools. */ - virtual Resource& connectionPools() PURE; + virtual ResourceLimit& connectionPools() PURE; }; } // namespace Upstream diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 633ff831feab..549598b738ba 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -195,6 +195,15 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "basic_resource_lib", + hdrs = ["basic_resource_impl.h"], + deps = [ + "//include/envoy/common:resource_interface", + "//include/envoy/runtime:runtime_interface", + ], +) + envoy_cc_library( name = "macros", hdrs = ["macros.h"], diff --git a/source/common/common/basic_resource_impl.h b/source/common/common/basic_resource_impl.h new file mode 100644 index 000000000000..8fe93aaabcb9 --- /dev/null +++ b/source/common/common/basic_resource_impl.h @@ -0,0 +1,60 @@ +#pragma once + +#include + +#include "envoy/common/resource.h" +#include "envoy/runtime/runtime.h" + +#include "common/common/assert.h" + +#include "absl/types/optional.h" + +namespace Envoy { + +/** + * A handle to track some limited resource. + * + * NOTE: + * This implementation makes some assumptions which favor simplicity over correctness. Though + * atomics are used, it is possible for resources to temporarily go above the supplied maximums. + * This should not effect overall behavior. + */ +class BasicResourceLimitImpl : public ResourceLimit { +public: + BasicResourceLimitImpl(uint64_t max, Runtime::Loader& runtime, const std::string& runtime_key) + : max_(max), runtime_(&runtime), runtime_key_(runtime_key) {} + BasicResourceLimitImpl(uint64_t max) : max_(max), runtime_(nullptr) {} + BasicResourceLimitImpl() : max_(std::numeric_limits::max()), runtime_(nullptr) {} + + bool canCreate() override { return current_.load() < max(); } + + void inc() override { ++current_; } + + void dec() override { decBy(1); } + + void decBy(uint64_t amount) override { + ASSERT(current_ >= amount); + current_ -= amount; + } + + uint64_t max() override { + return (runtime_ != nullptr && runtime_key_.has_value()) + ? runtime_->snapshot().getInteger(runtime_key_.value(), max_) + : max_; + } + + uint64_t count() const override { return current_.load(); } + + void setMax(uint64_t new_max) { max_ = new_max; } + void resetMax() { max_ = std::numeric_limits::max(); } + +protected: + std::atomic current_{}; + +private: + uint64_t max_; + Runtime::Loader* runtime_{nullptr}; + const absl::optional runtime_key_; +}; + +} // namespace Envoy diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 608c24bbfb3f..2dd83d85143b 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -310,6 +310,7 @@ envoy_cc_library( "//include/envoy/upstream:resource_manager_interface", "//include/envoy/upstream:upstream_interface", "//source/common/common:assert_lib", + "//source/common/common:basic_resource_lib", ], ) diff --git a/source/common/upstream/conn_pool_map_impl.h b/source/common/upstream/conn_pool_map_impl.h index 943e0b469c46..c0daca8ef37a 100644 --- a/source/common/upstream/conn_pool_map_impl.h +++ b/source/common/upstream/conn_pool_map_impl.h @@ -30,7 +30,7 @@ ConnPoolMap::getPool(KEY_TYPE key, const PoolFactory& facto if (pool_iter != active_pools_.end()) { return std::ref(*(pool_iter->second)); } - Resource& connPoolResource = host_->cluster().resourceManager(priority_).connectionPools(); + ResourceLimit& connPoolResource = host_->cluster().resourceManager(priority_).connectionPools(); // We need a new pool. Check if we have room. if (!connPoolResource.canCreate()) { // We're full. Try to free up a pool. If we can't, bail out. diff --git a/source/common/upstream/resource_manager_impl.h b/source/common/upstream/resource_manager_impl.h index e360c90206b7..12d0d498fc72 100644 --- a/source/common/upstream/resource_manager_impl.h +++ b/source/common/upstream/resource_manager_impl.h @@ -5,11 +5,13 @@ #include #include +#include "envoy/common/resource.h" #include "envoy/runtime/runtime.h" #include "envoy/upstream/resource_manager.h" #include "envoy/upstream/upstream.h" #include "common/common/assert.h" +#include "common/common/basic_resource_impl.h" namespace Envoy { namespace Upstream { @@ -44,38 +46,33 @@ class ResourceManagerImpl : public ResourceManager { pending_requests_) {} // Upstream::ResourceManager - Resource& connections() override { return connections_; } - Resource& pendingRequests() override { return pending_requests_; } - Resource& requests() override { return requests_; } - Resource& retries() override { return retries_; } - Resource& connectionPools() override { return connection_pools_; } + ResourceLimit& connections() override { return connections_; } + ResourceLimit& pendingRequests() override { return pending_requests_; } + ResourceLimit& requests() override { return requests_; } + ResourceLimit& retries() override { return retries_; } + ResourceLimit& connectionPools() override { return connection_pools_; } private: - struct ResourceImpl : public Resource { - ResourceImpl(uint64_t max, Runtime::Loader& runtime, const std::string& runtime_key, - Stats::Gauge& open_gauge, Stats::Gauge& remaining) - : max_(max), runtime_(runtime), runtime_key_(runtime_key), open_gauge_(open_gauge), + struct ManagedResourceImpl : public BasicResourceLimitImpl { + ManagedResourceImpl(uint64_t max, Runtime::Loader& runtime, const std::string& runtime_key, + Stats::Gauge& open_gauge, Stats::Gauge& remaining) + : BasicResourceLimitImpl(max, runtime, runtime_key), open_gauge_(open_gauge), remaining_(remaining) { remaining_.set(max); } - ~ResourceImpl() override { ASSERT(current_ == 0); } // Upstream::Resource bool canCreate() override { return current_ < max(); } void inc() override { - current_++; + BasicResourceLimitImpl::inc(); updateRemaining(); - open_gauge_.set(canCreate() ? 0 : 1); + open_gauge_.set(BasicResourceLimitImpl::canCreate() ? 0 : 1); } - void dec() override { decBy(1); } void decBy(uint64_t amount) override { - ASSERT(current_ >= amount); - current_ -= amount; + BasicResourceLimitImpl::decBy(amount); updateRemaining(); - open_gauge_.set(canCreate() ? 0 : 1); + open_gauge_.set(BasicResourceLimitImpl::canCreate() ? 0 : 1); } - uint64_t max() override { return runtime_.snapshot().getInteger(runtime_key_, max_); } - uint64_t count() const override { return current_.load(); } /** * We set the gauge instead of incrementing and decrementing because, @@ -91,11 +88,6 @@ class ResourceManagerImpl : public ResourceManager { remaining_.set(max() > current_copy ? max() - current_copy : 0); } - const uint64_t max_; - std::atomic current_{}; - Runtime::Loader& runtime_; - const std::string runtime_key_; - /** * A gauge to notify the live circuit breaker state. The gauge is set to 0 * to notify that the circuit breaker is not yet triggered. @@ -108,14 +100,14 @@ class ResourceManagerImpl : public ResourceManager { Stats::Gauge& remaining_; }; - class RetryBudgetImpl : public Resource { + class RetryBudgetImpl : public ResourceLimit { public: RetryBudgetImpl(absl::optional budget_percent, absl::optional min_retry_concurrency, uint64_t max_retries, Runtime::Loader& runtime, const std::string& retry_budget_runtime_key, const std::string& max_retries_runtime_key, Stats::Gauge& open_gauge, - Stats::Gauge& remaining, const Resource& requests, - const Resource& pending_requests) + Stats::Gauge& remaining, const ResourceLimit& requests, + const ResourceLimit& pending_requests) : runtime_(runtime), max_retry_resource_(max_retries, runtime, max_retries_runtime_key, open_gauge, remaining), budget_percent_(budget_percent), min_retry_concurrency_(min_retry_concurrency), @@ -123,7 +115,7 @@ class ResourceManagerImpl : public ResourceManager { min_retry_concurrency_key_(retry_budget_runtime_key + "min_retry_concurrency"), requests_(requests), pending_requests_(pending_requests), remaining_(remaining) {} - // Upstream::Resource + // Envoy::ResourceLimit bool canCreate() override { if (!useRetryBudget()) { return max_retry_resource_.canCreate(); @@ -182,20 +174,20 @@ class ResourceManagerImpl : public ResourceManager { Runtime::Loader& runtime_; // The max_retry resource is nested within the budget to maintain state if the retry budget is // toggled. - ResourceImpl max_retry_resource_; + ManagedResourceImpl max_retry_resource_; const absl::optional budget_percent_; const absl::optional min_retry_concurrency_; const std::string budget_percent_key_; const std::string min_retry_concurrency_key_; - const Resource& requests_; - const Resource& pending_requests_; + const ResourceLimit& requests_; + const ResourceLimit& pending_requests_; Stats::Gauge& remaining_; }; - ResourceImpl connections_; - ResourceImpl pending_requests_; - ResourceImpl requests_; - ResourceImpl connection_pools_; + ManagedResourceImpl connections_; + ManagedResourceImpl pending_requests_; + ManagedResourceImpl requests_; + ManagedResourceImpl connection_pools_; RetryBudgetImpl retries_; }; diff --git a/source/server/BUILD b/source/server/BUILD index da1d96e1f266..9f38b1aab361 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -363,6 +363,8 @@ envoy_cc_library( "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", "//include/envoy/server:worker_interface", + "//source/common/common:basic_resource_lib", + "//source/common/common:empty_string", "//source/common/config:utility_lib", "//source/common/config:version_converter_lib", "//source/common/http:conn_manager_lib", diff --git a/source/server/http/BUILD b/source/server/http/BUILD index d477a211afd3..f817776aea28 100644 --- a/source/server/http/BUILD +++ b/source/server/http/BUILD @@ -35,6 +35,7 @@ envoy_cc_library( "//source/common/access_log:access_log_lib", "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", + "//source/common/common:basic_resource_lib", "//source/common/common:empty_string", "//source/common/common:macros", "//source/common/common:minimal_logger_lib", diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 8dbb40bca2f3..a6961bcb25f2 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -194,6 +194,15 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "basic_resource_impl_test", + srcs = ["basic_resource_impl_test.cc"], + deps = [ + "//source/common/common:basic_resource_lib", + "//test/mocks/runtime:runtime_mocks", + ], +) + envoy_cc_test( name = "token_bucket_impl_test", srcs = ["token_bucket_impl_test.cc"], diff --git a/test/common/common/basic_resource_impl_test.cc b/test/common/common/basic_resource_impl_test.cc new file mode 100644 index 000000000000..60481535d06c --- /dev/null +++ b/test/common/common/basic_resource_impl_test.cc @@ -0,0 +1,73 @@ +#include + +#include "common/common/basic_resource_impl.h" + +#include "test/mocks/runtime/mocks.h" + +#include "gtest/gtest.h" + +using testing::NiceMock; +using testing::Return; + +namespace Envoy { + +class BasicResourceLimitImplTest : public testing::Test { +protected: + NiceMock runtime_; +}; + +TEST_F(BasicResourceLimitImplTest, NoArgsConstructorVerifyMax) { + BasicResourceLimitImpl br; + + EXPECT_EQ(br.max(), std::numeric_limits::max()); +} + +TEST_F(BasicResourceLimitImplTest, VerifySetClearMax) { + BasicResourceLimitImpl br(123); + + EXPECT_EQ(br.max(), 123); + br.setMax(321); + EXPECT_EQ(br.max(), 321); + br.resetMax(); + EXPECT_EQ(br.max(), std::numeric_limits::max()); +} + +TEST_F(BasicResourceLimitImplTest, IncDecCount) { + BasicResourceLimitImpl br; + + EXPECT_EQ(br.count(), 0); + br.inc(); + EXPECT_EQ(br.count(), 1); + br.inc(); + br.inc(); + EXPECT_EQ(br.count(), 3); + br.dec(); + EXPECT_EQ(br.count(), 2); + br.decBy(2); + EXPECT_EQ(br.count(), 0); +} + +TEST_F(BasicResourceLimitImplTest, CanCreate) { + BasicResourceLimitImpl br(2); + + EXPECT_TRUE(br.canCreate()); + br.inc(); + EXPECT_TRUE(br.canCreate()); + br.inc(); + EXPECT_FALSE(br.canCreate()); + br.dec(); + EXPECT_TRUE(br.canCreate()); + br.dec(); +} + +TEST_F(BasicResourceLimitImplTest, RuntimeMods) { + BasicResourceLimitImpl br(1337, runtime_, "trololo"); + + EXPECT_CALL(runtime_.snapshot_, getInteger("trololo", 1337)).WillOnce(Return(555)); + EXPECT_EQ(br.max(), 555); + + EXPECT_CALL(runtime_.snapshot_, getInteger("trololo", 1337)).WillOnce(Return(1337)); + EXPECT_EQ(br.max(), 1337); +} + +} // namespace Envoy diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index c08092a4b47d..9e30845a1dc8 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -271,6 +271,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/03/24 10501 44261 44600 upstream: upstream_rq_retry_limit_exceeded. // 2020/04/02 10624 43356 44000 Use 100 clusters rather than 1000 to avoid timeouts // 2020/04/07 10661 43349 44000 fix clang tidy on master + // 2020/05/13 10531 44425 44600 Refactor resource manager // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -329,6 +330,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/03/24 10501 36300 36800 upstream: upstream_rq_retry_limit_exceeded. // 2020/04/02 10624 35564 36000 Use 100 clusters rather than 1000 to avoid timeouts // 2020/04/07 10661 35557 36000 fix clang tidy on master + // 2020/05/13 10531 36537 44600 Refactor resource manager // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -342,8 +344,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 35557); - EXPECT_MEMORY_LE(m_per_cluster, 36000); + EXPECT_MEMORY_EQ(m_per_cluster, 36537); + EXPECT_MEMORY_LE(m_per_cluster, 36800); } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) {