From 27f8ac0bb6119948e773b3e39919437079483521 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 24 Sep 2021 14:29:56 +0000 Subject: [PATCH 1/8] Support reactive checks in overload manager Api Signed-off-by: Kateryna Nezdolii --- envoy/server/BUILD | 1 + envoy/server/overload/BUILD | 9 ++ .../overload/proactive_resource_monitor.h | 86 ++++++++++++ .../overload/thread_local_overload_state.h | 47 +++++++ envoy/server/resource_monitor.h | 64 ++++++--- envoy/server/resource_monitor_config.h | 23 +++- .../fixed_heap/fixed_heap_monitor.cc | 2 +- .../fixed_heap/fixed_heap_monitor.h | 2 +- .../injected_resource_monitor.cc | 2 +- .../injected_resource_monitor.h | 2 +- source/server/admin/admin.h | 3 + source/server/overload_manager_impl.cc | 108 ++++++++++++--- source/server/overload_manager_impl.h | 9 +- .../fixed_heap/fixed_heap_monitor_test.cc | 2 +- .../injected_resource_monitor_test.cc | 2 +- test/mocks/server/overload_manager.cc | 4 + test/mocks/server/overload_manager.h | 3 + test/server/overload_manager_impl_test.cc | 124 +++++++++++++++++- 18 files changed, 442 insertions(+), 51 deletions(-) create mode 100644 envoy/server/overload/proactive_resource_monitor.h diff --git a/envoy/server/BUILD b/envoy/server/BUILD index ccd2478deea7..6de546726f5c 100644 --- a/envoy/server/BUILD +++ b/envoy/server/BUILD @@ -311,6 +311,7 @@ envoy_cc_library( "//envoy/api:api_interface", "//envoy/event:dispatcher_interface", "//envoy/protobuf:message_validator_interface", + "//envoy/server/overload:proactive_resource_monitor", ], ) diff --git a/envoy/server/overload/BUILD b/envoy/server/overload/BUILD index 6d937fb8d5e1..3f5da6105cc9 100644 --- a/envoy/server/overload/BUILD +++ b/envoy/server/overload/BUILD @@ -23,9 +23,18 @@ envoy_cc_library( name = "thread_local_overload_state", hdrs = ["thread_local_overload_state.h"], deps = [ + ":proactive_resource_monitor", "//envoy/event:scaled_range_timer_manager_interface", "//envoy/event:timer_interface", "//envoy/thread_local:thread_local_object", "//source/common/common:interval_value", ], ) + +envoy_cc_library( + name = "proactive_resource_monitor", + hdrs = ["proactive_resource_monitor.h"], + deps = [ + "//envoy/stats:stats_interface", + ], +) diff --git a/envoy/server/overload/proactive_resource_monitor.h b/envoy/server/overload/proactive_resource_monitor.h new file mode 100644 index 000000000000..70f89251c98c --- /dev/null +++ b/envoy/server/overload/proactive_resource_monitor.h @@ -0,0 +1,86 @@ +#pragma once + +#include + +#include "envoy/common/pure.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/stats.h" + +#include "source/common/common/assert.h" +#include "source/common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Server { + +class ProactiveResourceMonitor { +public: + ProactiveResourceMonitor() = default; + virtual ~ProactiveResourceMonitor() = default; + /** + * Tries to allocate resource for given resource monitor in thread safe manner. + * Returns true if there is enough resource quota available and allocation has succeeded, false + * otherwise. + * @param increment to add to current resource usage value and compare against configured max + * threshold. + */ + virtual bool tryAllocateResource(int64_t increment) PURE; + /** + * Tries to deallocate resource for given resource monitor in thread safe manner. + * Returns true if there is enough resource quota available and deallocation has succeeded, false + * otherwise. + * @param decrement to subtract from current resource usage value. + */ + virtual bool tryDeallocateResource(int64_t decrement) PURE; + /** + * Returns current resource usage tracked by monitor. + */ + virtual int64_t currentResourceUsage() const PURE; + /** + * Returns max resource usage configured in monitor. + */ + virtual uint64_t maxResourceUsage() const PURE; +}; + +using ProactiveResourceMonitorPtr = std::unique_ptr; + +class ProactiveResource { +public: + ProactiveResource(const std::string& name, ProactiveResourceMonitorPtr monitor, + Stats::Scope& stats_scope) + : name_(name), monitor_(std::move(monitor)), + failed_updates_counter_(makeCounter(stats_scope, name, "failed_updates")) {} + + bool tryAllocateResource(int64_t increment) { + if (monitor_->tryAllocateResource(increment)) { + return true; + } else { + failed_updates_counter_.inc(); + return false; + } + } + + bool tryDeallocateResource(int64_t decrement) { + if (monitor_->tryDeallocateResource(decrement)) { + return true; + } else { + failed_updates_counter_.inc(); + return false; + } + } + + int64_t currentResourceUsage() { return monitor_->currentResourceUsage(); } + +private: + const std::string name_; + ProactiveResourceMonitorPtr monitor_; + Stats::Counter& failed_updates_counter_; + + Stats::Counter& makeCounter(Stats::Scope& scope, absl::string_view a, absl::string_view b) { + Stats::StatNameManagedStorage stat_name(absl::StrCat("overload.", a, ".", b), + scope.symbolTable()); + return scope.counterFromStatName(stat_name.statName()); + } +}; + +} // namespace Server +} // namespace Envoy \ No newline at end of file diff --git a/envoy/server/overload/thread_local_overload_state.h b/envoy/server/overload/thread_local_overload_state.h index 9a57400d8eb3..9232b80646a9 100644 --- a/envoy/server/overload/thread_local_overload_state.h +++ b/envoy/server/overload/thread_local_overload_state.h @@ -5,6 +5,7 @@ #include "envoy/common/pure.h" #include "envoy/event/scaled_range_timer_manager.h" #include "envoy/event/timer.h" +#include "envoy/server/overload/proactive_resource_monitor.h" #include "envoy/thread_local/thread_local_object.h" #include "source/common/common/interval_value.h" @@ -12,6 +13,26 @@ namespace Envoy { namespace Server { +enum class OverloadProactiveResourceName { + GlobalDownstreamMaxConnections, +}; + +class OverloadProactiveResourceNameValues { +public: + // Overload action to stop accepting new HTTP requests. + const std::string GlobalDownstreamMaxConnections = + "envoy.resource_monitors.global_downstream_max_connections"; + + std::set proactive_resource_names_{GlobalDownstreamMaxConnections}; + + absl::flat_hash_map + proactive_action_name_to_resource_ = { + {GlobalDownstreamMaxConnections, + OverloadProactiveResourceName::GlobalDownstreamMaxConnections}}; +}; + +using OverloadProactiveResourceNames = ConstSingleton; + /** * Tracks the state of an overload action. The state is a number between 0 and 1 that represents the * level of saturation. The values are categorized in two groups: @@ -46,6 +67,32 @@ class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { public: // Get a thread-local reference to the value for the given action key. virtual const OverloadActionState& getState(const std::string& action) PURE; + /** + * Invokes corresponding resource monitor to allocate resource for given resource monitor in + * thread safe manner. Returns true if there is enough resource quota available and allocation has + * succeeded, false if allocation failed or resource is not registered. + * @param name of corresponding resource monitor. + * @param increment to add to current resource usage value within monitor. + */ + virtual bool tryAllocateResource(OverloadProactiveResourceName resource_name, + int64_t increment) PURE; + /** + * Invokes corresponding resource monitor to deallocate resource for given resource monitor in + * thread safe manner. Returns true if there is enough resource quota available and deallocation + * has succeeded, false if deallocation failed or resource is not registered. + * @param name of corresponding resource monitor. + * @param decrement to subtract from current resource usage value within monitor. + */ + virtual bool tryDeallocateResource(OverloadProactiveResourceName resource_name, + int64_t decrement) PURE; + + /** + * TODO(nezdolik) remove this method once downstream connection tracking is fully moved to + * overload manager. Checks if resource monitor is registered and resource usage tracking is + * enabled in overload manager. Returns true if resource monitor is registered, false otherwise. + * @param name of resource monitor to check. + */ + virtual bool isResourceMonitorEnabled(OverloadProactiveResourceName resource_name) PURE; }; } // namespace Server diff --git a/envoy/server/resource_monitor.h b/envoy/server/resource_monitor.h index 4eb947527ade..e22bfc9cb25f 100644 --- a/envoy/server/resource_monitor.h +++ b/envoy/server/resource_monitor.h @@ -4,6 +4,9 @@ #include "envoy/common/exception.h" #include "envoy/common/pure.h" +#include "envoy/event/dispatcher.h" + +#include "source/common/common/assert.h" namespace Envoy { namespace Server { @@ -18,39 +21,58 @@ struct ResourceUsage { double resource_pressure_; }; -class ResourceMonitor { +/** + * Notifies caller of updated resource usage. + */ +class ResourceUpdateCallbacks { public: - virtual ~ResourceMonitor() = default; + virtual ~ResourceUpdateCallbacks() = default; + + /** + * Called when the request for updated resource usage succeeds. + * @param usage the updated resource usage + */ + virtual void onSuccess(const ResourceUsage& usage) PURE; /** - * Notifies caller of updated resource usage. + * Called when the request for updated resource usage fails. + * @param error the exception caught when trying to get updated resource usage */ - class Callbacks { - public: - virtual ~Callbacks() = default; - - /** - * Called when the request for updated resource usage succeeds. - * @param usage the updated resource usage - */ - virtual void onSuccess(const ResourceUsage& usage) PURE; - - /** - * Called when the request for updated resource usage fails. - * @param error the exception caught when trying to get updated resource usage - */ - virtual void onFailure(const EnvoyException& error) PURE; - }; + virtual void onFailure(const EnvoyException& error) PURE; +}; + +class ReactiveResourceUpdateCallbacks { +public: + virtual ~ReactiveResourceUpdateCallbacks() = default; + + /** + * Called when the request for updated resource usage succeeds. + * @param usage the updated resource usage + */ + virtual void onSuccess(const uint64_t usage) PURE; + + /** + * Called when the request for updated resource usage fails. + * todo may need propagate error + */ + virtual void onFailure() PURE; + + virtual Event::Dispatcher& dispatcher() PURE; +}; + +class ResourceMonitor { +public: + virtual ~ResourceMonitor() = default; /** * Recalculate resource usage. * This must be non-blocking so if RPCs need to be made they should be * done asynchronously and invoke the callback when finished. */ - virtual void updateResourceUsage(Callbacks& callbacks) PURE; + virtual void updateResourceUsage(ResourceUpdateCallbacks& callbacks) PURE; }; using ResourceMonitorPtr = std::unique_ptr; } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/envoy/server/resource_monitor_config.h b/envoy/server/resource_monitor_config.h index 9f680f44f8f2..da1a768bfdd6 100644 --- a/envoy/server/resource_monitor_config.h +++ b/envoy/server/resource_monitor_config.h @@ -6,6 +6,7 @@ #include "envoy/event/dispatcher.h" #include "envoy/protobuf/message_validator.h" #include "envoy/server/options.h" +#include "envoy/server/overload/proactive_resource_monitor.h" #include "envoy/server/resource_monitor.h" #include "source/common/protobuf/protobuf.h" @@ -64,6 +65,26 @@ class ResourceMonitorFactory : public Config::TypedFactory { std::string category() const override { return "envoy.resource_monitors"; } }; +class ProactiveResourceMonitorFactory : public Config::TypedFactory { +public: + ~ProactiveResourceMonitorFactory() override = default; + + /** + * Create a particular proactive resource monitor implementation. + * @param config const ProtoBuf::Message& supplies the config for the proactive resource monitor + * implementation. + * @param context ProactiveResourceMonitorFactoryContext& supplies the resource monitor's context. + * @return ProactiveResourceMonitorPtr the resource monitor instance. Should not be nullptr. + * @throw EnvoyException if the implementation is unable to produce an instance with + * the provided parameters. + */ + virtual ProactiveResourceMonitorPtr + createProactiveResourceMonitor(const Protobuf::Message& config, + ResourceMonitorFactoryContext& context) PURE; + + std::string category() const override { return "envoy.resource_monitors"; } +}; + } // namespace Configuration } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc b/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc index 106c44620b68..6060adab4c97 100644 --- a/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc +++ b/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc @@ -21,7 +21,7 @@ FixedHeapMonitor::FixedHeapMonitor( ASSERT(max_heap_ > 0); } -void FixedHeapMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) { +void FixedHeapMonitor::updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) { const size_t physical = stats_->reservedHeapBytes(); const size_t unmapped = stats_->unmappedHeapBytes(); ASSERT(physical >= unmapped); diff --git a/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.h b/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.h index a54162ebb31f..84ed32c80e79 100644 --- a/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.h +++ b/source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.h @@ -31,7 +31,7 @@ class FixedHeapMonitor : public Server::ResourceMonitor { const envoy::extensions::resource_monitors::fixed_heap::v3::FixedHeapConfig& config, std::unique_ptr stats = std::make_unique()); - void updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) override; + void updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) override; private: const uint64_t max_heap_; diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index d6797ac85ec2..a2799ac6ff15 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -23,7 +23,7 @@ InjectedResourceMonitor::InjectedResourceMonitor( void InjectedResourceMonitor::onFileChanged() { file_changed_ = true; } -void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) { +void InjectedResourceMonitor::updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) { if (file_changed_) { file_changed_ = false; TRY_ASSERT_MAIN_THREAD { diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.h b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.h index 86210c112a07..59bf32375c56 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.h +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.h @@ -25,7 +25,7 @@ class InjectedResourceMonitor : public Server::ResourceMonitor { Server::Configuration::ResourceMonitorFactoryContext& context); // Server::ResourceMonitor - void updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) override; + void updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) override; protected: virtual void onFileChanged(); diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index e99330c2c8fc..13c132567161 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -270,6 +270,9 @@ class AdminImpl : public Admin, struct NullThreadLocalOverloadState : public ThreadLocalOverloadState { NullThreadLocalOverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} const OverloadActionState& getState(const std::string&) override { return inactive_; } + bool tryAllocateResource(OverloadProactiveResourceName, int64_t) override { return false; } + bool tryDeallocateResource(OverloadProactiveResourceName, int64_t) override { return false; } + bool isResourceMonitorEnabled(OverloadProactiveResourceName) override { return false; } Event::Dispatcher& dispatcher_; const OverloadActionState inactive_ = OverloadActionState::inactive(); }; diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 1e999e718ce9..90ab6280d1ca 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -25,9 +25,13 @@ namespace Server { */ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { public: - explicit ThreadLocalOverloadStateImpl(const NamedOverloadActionSymbolTable& action_symbol_table) + explicit ThreadLocalOverloadStateImpl( + const NamedOverloadActionSymbolTable& action_symbol_table, + std::shared_ptr>& + proactive_resources) : action_symbol_table_(action_symbol_table), - actions_(action_symbol_table.size(), OverloadActionState(UnitFloat::min())) {} + actions_(action_symbol_table.size(), OverloadActionState(UnitFloat::min())), + proactive_resources_(proactive_resources) {} const OverloadActionState& getState(const std::string& action) override { if (const auto symbol = action_symbol_table_.lookup(action); symbol != absl::nullopt) { @@ -40,10 +44,48 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { actions_[action.index()] = state; } + bool tryAllocateResource(OverloadProactiveResourceName resource_name, + int64_t increment) override { + const auto proactive_resource = proactive_resources_->find(resource_name); + if (proactive_resource != proactive_resources_->end()) { + if (proactive_resource->second.tryAllocateResource(increment)) { + return true; + } else { + return false; + } + } else { + ENVOY_LOG_MISC(warn, " {Failed to allocate unknown proactive resource }"); + // Resource monitor is not configured, pass through mode. + return true; + } + } + + bool tryDeallocateResource(OverloadProactiveResourceName resource_name, + int64_t decrement) override { + const auto proactive_resource = proactive_resources_->find(resource_name); + if (proactive_resource != proactive_resources_->end()) { + if (proactive_resource->second.tryDeallocateResource(decrement)) { + return true; + } else { + return false; + } + } else { + ENVOY_LOG_MISC(warn, " {Failed to deallocate unknown proactive resource }"); + return false; + } + } + + bool isResourceMonitorEnabled(OverloadProactiveResourceName resource_name) override { + const auto proactive_resource = proactive_resources_->find(resource_name); + return proactive_resource != proactive_resources_->end(); + } + private: static const OverloadActionState always_inactive_; const NamedOverloadActionSymbolTable& action_symbol_table_; std::vector actions_; + std::shared_ptr> + proactive_resources_; }; const OverloadActionState ThreadLocalOverloadStateImpl::always_inactive_{UnitFloat::min()}; @@ -268,19 +310,49 @@ OverloadManagerImpl::OverloadManagerImpl(Event::Dispatcher& dispatcher, Stats::S Api::Api& api, const Server::Options& options) : started_(false), dispatcher_(dispatcher), tls_(slot_allocator), refresh_interval_( - std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, refresh_interval, 1000))) { + std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, refresh_interval, 1000))), + proactive_resources_( + std::make_unique< + absl::node_hash_map>()) { Configuration::ResourceMonitorFactoryContextImpl context(dispatcher, options, api, validation_visitor); + // We should hide impl details from users, for them there should be no distinction between + // proactive and regular resource monitors in configuration API. But internally we will maintain + // two distinct collections of proactive and regular resources. Proactive resources are not + // subject to periodic flushes and can be recalculated/updated on demand by invoking + // `tryAllocateResource/tryDeallocateResource` via thread local overload state. for (const auto& resource : config.resource_monitors()) { const auto& name = resource.name(); - ENVOY_LOG(debug, "Adding resource monitor for {}", name); - auto& factory = - Config::Utility::getAndCheckFactory(resource); - auto config = Config::Utility::translateToFactoryConfig(resource, validation_visitor, factory); - auto monitor = factory.createResourceMonitor(*config, context); - - auto result = resources_.try_emplace(name, name, std::move(monitor), *this, stats_scope); - if (!result.second) { + // Check if it is a proactive resource. + auto proactive_resource_it = + OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.find(name); + ENVOY_LOG(debug, "Evaluating resource {}", name); + bool result = false; + if (proactive_resource_it != + OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.end()) { + ENVOY_LOG(debug, "Adding proactive resource monitor for {}", name); + auto& factory = + Config::Utility::getAndCheckFactory( + resource); + auto config = + Config::Utility::translateToFactoryConfig(resource, validation_visitor, factory); + auto monitor = factory.createProactiveResourceMonitor(*config, context); + + result = + proactive_resources_ + ->try_emplace(proactive_resource_it->second, name, std::move(monitor), stats_scope) + .second; + } else { + ENVOY_LOG(debug, "Adding resource monitor for {}", name); + auto& factory = + Config::Utility::getAndCheckFactory(resource); + auto config = + Config::Utility::translateToFactoryConfig(resource, validation_visitor, factory); + auto monitor = factory.createResourceMonitor(*config, context); + + result = resources_.try_emplace(name, name, std::move(monitor), *this, stats_scope).second; + } + if (!result) { throw EnvoyException(absl::StrCat("Duplicate resource monitor ", name)); } } @@ -315,12 +387,15 @@ OverloadManagerImpl::OverloadManagerImpl(Event::Dispatcher& dispatcher, Stats::S for (const auto& trigger : action.triggers()) { const std::string& resource = trigger.name(); + auto proactive_resource_it = + OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.find(resource); - if (resources_.find(resource) == resources_.end()) { + if (resources_.find(resource) == resources_.end() && + proactive_resource_it == + OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.end()) { throw EnvoyException( fmt::format("Unknown trigger resource {} for overload action {}", resource, name)); } - resource_to_actions_.insert(std::make_pair(resource, symbol)); } } @@ -331,7 +406,8 @@ void OverloadManagerImpl::start() { started_ = true; tls_.set([this](Event::Dispatcher&) { - return std::make_shared(action_symbol_table_); + return std::make_shared(action_symbol_table_, + proactive_resources_); }); if (resources_.empty()) { @@ -364,6 +440,8 @@ void OverloadManagerImpl::stop() { // Clear the resource map to block on any pending updates. resources_.clear(); + + // TODO(nezdolik): wrap proactive monitors into atomic? and clear it here } bool OverloadManagerImpl::registerForAction(const std::string& action, @@ -500,4 +578,4 @@ void OverloadManagerImpl::Resource::onFailure(const EnvoyException& error) { } } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/source/server/overload_manager_impl.h b/source/server/overload_manager_impl.h index 784308b1f4b8..d91be311a011 100644 --- a/source/server/overload_manager_impl.h +++ b/source/server/overload_manager_impl.h @@ -132,12 +132,12 @@ class OverloadManagerImpl : Logger::Loggable, public OverloadM private: using FlushEpochId = uint64_t; - class Resource : public ResourceMonitor::Callbacks { + class Resource : public ResourceUpdateCallbacks { public: Resource(const std::string& name, ResourceMonitorPtr monitor, OverloadManagerImpl& manager, Stats::Scope& stats_scope); - // ResourceMonitor::Callbacks + // ResourceMonitor::ResourceUpdateCallbacks void onSuccess(const ResourceUsage& usage) override; void onFailure(const EnvoyException& error) override; @@ -173,6 +173,9 @@ class OverloadManagerImpl : Logger::Loggable, public OverloadM const std::chrono::milliseconds refresh_interval_; Event::TimerPtr timer_; absl::node_hash_map resources_; + std::shared_ptr> + proactive_resources_; + absl::node_hash_map actions_; Event::ScaledTimerTypeMapConstSharedPtr timer_minimums_; @@ -194,4 +197,4 @@ class OverloadManagerImpl : Logger::Loggable, public OverloadM }; } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/extensions/resource_monitors/fixed_heap/fixed_heap_monitor_test.cc b/test/extensions/resource_monitors/fixed_heap/fixed_heap_monitor_test.cc index 24b451ee6fe7..00d8e42665f6 100644 --- a/test/extensions/resource_monitors/fixed_heap/fixed_heap_monitor_test.cc +++ b/test/extensions/resource_monitors/fixed_heap/fixed_heap_monitor_test.cc @@ -20,7 +20,7 @@ class MockMemoryStatsReader : public MemoryStatsReader { MOCK_METHOD(uint64_t, unmappedHeapBytes, ()); }; -class ResourcePressure : public Server::ResourceMonitor::Callbacks { +class ResourcePressure : public Server::ResourceUpdateCallbacks { public: void onSuccess(const Server::ResourceUsage& usage) override { pressure_ = usage.resource_pressure_; diff --git a/test/extensions/resource_monitors/injected_resource/injected_resource_monitor_test.cc b/test/extensions/resource_monitors/injected_resource/injected_resource_monitor_test.cc index 5d05210ad4f1..7010e02241b5 100644 --- a/test/extensions/resource_monitors/injected_resource/injected_resource_monitor_test.cc +++ b/test/extensions/resource_monitors/injected_resource/injected_resource_monitor_test.cc @@ -38,7 +38,7 @@ class TestableInjectedResourceMonitor : public InjectedResourceMonitor { Event::Dispatcher& dispatcher_; }; -class MockedCallbacks : public Server::ResourceMonitor::Callbacks { +class MockedCallbacks : public Server::ResourceUpdateCallbacks { public: MOCK_METHOD(void, onSuccess, (const Server::ResourceUsage&)); MOCK_METHOD(void, onFailure, (const EnvoyException&)); diff --git a/test/mocks/server/overload_manager.cc b/test/mocks/server/overload_manager.cc index 4ebf19f032b8..9ca2c36a8755 100644 --- a/test/mocks/server/overload_manager.cc +++ b/test/mocks/server/overload_manager.cc @@ -10,11 +10,15 @@ namespace Envoy { namespace Server { +using ::testing::Return; using ::testing::ReturnRef; MockThreadLocalOverloadState::MockThreadLocalOverloadState() : disabled_state_(OverloadActionState::inactive()) { ON_CALL(*this, getState).WillByDefault(ReturnRef(disabled_state_)); + ON_CALL(*this, tryAllocateResource).WillByDefault(Return(true)); + ON_CALL(*this, tryDeallocateResource).WillByDefault(Return(true)); + ON_CALL(*this, isResourceMonitorEnabled).WillByDefault(Return(false)); } MockOverloadManager::MockOverloadManager() { diff --git a/test/mocks/server/overload_manager.h b/test/mocks/server/overload_manager.h index e5ab03992836..c3c08c8895eb 100644 --- a/test/mocks/server/overload_manager.h +++ b/test/mocks/server/overload_manager.h @@ -14,6 +14,9 @@ class MockThreadLocalOverloadState : public ThreadLocalOverloadState { public: MockThreadLocalOverloadState(); MOCK_METHOD(const OverloadActionState&, getState, (const std::string&), (override)); + MOCK_METHOD(bool, tryAllocateResource, (OverloadProactiveResourceName, int64_t)); + MOCK_METHOD(bool, tryDeallocateResource, (OverloadProactiveResourceName, int64_t)); + MOCK_METHOD(bool, isResourceMonitorEnabled, (OverloadProactiveResourceName)); private: const OverloadActionState disabled_state_; diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 0886e0181d5f..4657e2a6dd5a 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -57,7 +57,7 @@ class FakeResourceMonitor : public ResourceMonitor { update_async_ = new_update_async; } - void updateResourceUsage(ResourceMonitor::Callbacks& callbacks) override { + void updateResourceUsage(ResourceUpdateCallbacks& callbacks) override { if (update_async_) { callbacks_.emplace(callbacks); } else { @@ -74,7 +74,7 @@ class FakeResourceMonitor : public ResourceMonitor { } private: - void publishUpdate(ResourceMonitor::Callbacks& callbacks) { + void publishUpdate(ResourceUpdateCallbacks& callbacks) { if (absl::holds_alternative(response_)) { Server::ResourceUsage usage; usage.resource_pressure_ = absl::get(response_); @@ -88,7 +88,39 @@ class FakeResourceMonitor : public ResourceMonitor { Event::Dispatcher& dispatcher_; absl::variant response_; bool update_async_ = false; - absl::optional> callbacks_; + absl::optional> callbacks_; +}; + +class FakeProactiveResourceMonitor : public ProactiveResourceMonitor { +public: + FakeProactiveResourceMonitor(uint64_t max) : max_(max), current_(0){}; + + bool tryAllocateResource(int64_t increment) { + int64_t new_val = (current_ += increment); + if (new_val > static_cast(max_) || new_val < 0) { + current_ -= increment; + return false; + } + return true; + } + + bool tryDeallocateResource(int64_t decrement) { + RELEASE_ASSERT(decrement <= current_, + "Cannot deallocate resource, current resource usage is lower than decrement"); + int64_t new_val = (current_ -= decrement); + if (new_val < 0) { + current_ += decrement; + return false; + } + return true; + } + + int64_t currentResourceUsage() const { return current_.load(); } + uint64_t maxResourceUsage() const { return max_; } + +private: + uint64_t max_; + std::atomic current_; }; template @@ -114,6 +146,30 @@ class FakeResourceMonitorFactory : public Server::Configuration::ResourceMonitor const std::string name_; }; +template +class FakeProactiveResourceMonitorFactory + : public Server::Configuration::ProactiveResourceMonitorFactory { +public: + FakeProactiveResourceMonitorFactory(const std::string& name) : monitor_(nullptr), name_(name) {} + + Server::ProactiveResourceMonitorPtr + createProactiveResourceMonitor(const Protobuf::Message&, + Server::Configuration::ResourceMonitorFactoryContext&) override { + auto monitor = std::make_unique(3); + monitor_ = monitor.get(); + return monitor; + } + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new ConfigType()}; + } + + std::string name() const override { return name_; } + + FakeProactiveResourceMonitor* monitor_; // not owned + const std::string name_; +}; + class TestOverloadManager : public OverloadManagerImpl { public: TestOverloadManager(Event::Dispatcher& dispatcher, Stats::Scope& stats_scope, @@ -146,8 +202,10 @@ class OverloadManagerImplTest : public testing::Test { : factory1_("envoy.resource_monitors.fake_resource1"), factory2_("envoy.resource_monitors.fake_resource2"), factory3_("envoy.resource_monitors.fake_resource3"), - factory4_("envoy.resource_monitors.fake_resource4"), register_factory1_(factory1_), - register_factory2_(factory2_), register_factory3_(factory3_), register_factory4_(factory4_), + factory4_("envoy.resource_monitors.fake_resource4"), + factory5_("envoy.resource_monitors.global_downstream_max_connections"), + register_factory1_(factory1_), register_factory2_(factory2_), register_factory3_(factory3_), + register_factory4_(factory4_), register_factory5_(factory5_), api_(Api::createApiForTest(stats_)) {} void setDispatcherExpectation() { @@ -174,10 +232,12 @@ class OverloadManagerImplTest : public testing::Test { FakeResourceMonitorFactory factory2_; FakeResourceMonitorFactory factory3_; FakeResourceMonitorFactory factory4_; + FakeProactiveResourceMonitorFactory factory5_; Registry::InjectFactory register_factory1_; Registry::InjectFactory register_factory2_; Registry::InjectFactory register_factory3_; Registry::InjectFactory register_factory4_; + Registry::InjectFactory register_factory5_; NiceMock dispatcher_; NiceMock* timer_; // not owned Stats::TestUtil::TestStore stats_; @@ -215,6 +275,20 @@ constexpr char kRegularStateConfig[] = R"YAML( saturation_threshold: 0.8 )YAML"; +constexpr char proactiveResourceConfig[] = R"YAML( + refresh_interval: + seconds: 1 + resource_monitors: + - name: envoy.resource_monitors.fake_resource1 + - name: envoy.resource_monitors.global_downstream_max_connections + actions: + - name: envoy.overload_actions.dummy_action + triggers: + - name: envoy.resource_monitors.fake_resource1 + threshold: + value: 0.9 +)YAML"; + TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { setDispatcherExpectation(); @@ -230,6 +304,9 @@ TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { [&](OverloadActionState) { EXPECT_TRUE(false); }); manager->start(); + EXPECT_FALSE(manager->getThreadLocalOverloadState().isResourceMonitorEnabled( + OverloadProactiveResourceName::GlobalDownstreamMaxConnections)); + Stats::Gauge& active_gauge = stats_.gauge("overload.envoy.overload_actions.dummy_action.active", Stats::Gauge::ImportMode::Accumulate); Stats::Gauge& scale_percent_gauge = @@ -566,6 +643,17 @@ TEST_F(OverloadManagerImplTest, DuplicateResourceMonitor) { "Duplicate resource monitor .*"); } +TEST_F(OverloadManagerImplTest, DuplicateProactiveResourceMonitor) { + const std::string config = R"EOF( + resource_monitors: + - name: "envoy.resource_monitors.global_downstream_max_connections" + - name: "envoy.resource_monitors.global_downstream_max_connections" + )EOF"; + + EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, + "Duplicate resource monitor .*"); +} + TEST_F(OverloadManagerImplTest, DuplicateOverloadAction) { const std::string config = R"EOF( actions: @@ -711,6 +799,32 @@ TEST_F(OverloadManagerImplTest, Shutdown) { manager->stop(); } +TEST_F(OverloadManagerImplTest, ProactiveResourceAllocateAndDeallocateResourceTest) { + setDispatcherExpectation(); + auto manager(createOverloadManager(proactiveResourceConfig)); + Stats::Counter& failed_updates = + stats_.counter("overload.envoy.resource_monitors.global_downstream_max_connections." + "failed_updates"); + manager->start(); + EXPECT_TRUE(manager->getThreadLocalOverloadState().isResourceMonitorEnabled( + OverloadProactiveResourceName::GlobalDownstreamMaxConnections)); + bool resource_allocated = manager->getThreadLocalOverloadState().tryAllocateResource( + Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 1); + EXPECT_EQ(true, resource_allocated); + resource_allocated = manager->getThreadLocalOverloadState().tryAllocateResource( + Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 3); + EXPECT_EQ(false, resource_allocated); + EXPECT_EQ(1, failed_updates.value()); + + bool resource_deallocated = manager->getThreadLocalOverloadState().tryDeallocateResource( + Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 1); + EXPECT_EQ(true, resource_deallocated); + EXPECT_DEATH(manager->getThreadLocalOverloadState().tryDeallocateResource( + Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 1), + ".*Cannot deallocate resource, current resource usage is lower than decrement.*"); + manager->stop(); +} + } // namespace } // namespace Server } // namespace Envoy From ae0faab581785456f6cd6250cbead6fac5647d88 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 24 Sep 2021 15:30:29 +0000 Subject: [PATCH 2/8] Fix format Signed-off-by: Kateryna Nezdolii --- .../overload/proactive_resource_monitor.h | 2 +- envoy/server/resource_monitor.h | 21 +------------------ envoy/server/resource_monitor_config.h | 2 +- source/server/overload_manager_impl.cc | 2 +- source/server/overload_manager_impl.h | 2 +- 5 files changed, 5 insertions(+), 24 deletions(-) diff --git a/envoy/server/overload/proactive_resource_monitor.h b/envoy/server/overload/proactive_resource_monitor.h index 70f89251c98c..c988d7a41123 100644 --- a/envoy/server/overload/proactive_resource_monitor.h +++ b/envoy/server/overload/proactive_resource_monitor.h @@ -83,4 +83,4 @@ class ProactiveResource { }; } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/envoy/server/resource_monitor.h b/envoy/server/resource_monitor.h index e22bfc9cb25f..a0de9518ee13 100644 --- a/envoy/server/resource_monitor.h +++ b/envoy/server/resource_monitor.h @@ -41,25 +41,6 @@ class ResourceUpdateCallbacks { virtual void onFailure(const EnvoyException& error) PURE; }; -class ReactiveResourceUpdateCallbacks { -public: - virtual ~ReactiveResourceUpdateCallbacks() = default; - - /** - * Called when the request for updated resource usage succeeds. - * @param usage the updated resource usage - */ - virtual void onSuccess(const uint64_t usage) PURE; - - /** - * Called when the request for updated resource usage fails. - * todo may need propagate error - */ - virtual void onFailure() PURE; - - virtual Event::Dispatcher& dispatcher() PURE; -}; - class ResourceMonitor { public: virtual ~ResourceMonitor() = default; @@ -75,4 +56,4 @@ class ResourceMonitor { using ResourceMonitorPtr = std::unique_ptr; } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/envoy/server/resource_monitor_config.h b/envoy/server/resource_monitor_config.h index da1a768bfdd6..d90a0fc735c7 100644 --- a/envoy/server/resource_monitor_config.h +++ b/envoy/server/resource_monitor_config.h @@ -87,4 +87,4 @@ class ProactiveResourceMonitorFactory : public Config::TypedFactory { } // namespace Configuration } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 90ab6280d1ca..54b09809bd48 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -578,4 +578,4 @@ void OverloadManagerImpl::Resource::onFailure(const EnvoyException& error) { } } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/server/overload_manager_impl.h b/source/server/overload_manager_impl.h index d91be311a011..b7e4d3e12fb0 100644 --- a/source/server/overload_manager_impl.h +++ b/source/server/overload_manager_impl.h @@ -197,4 +197,4 @@ class OverloadManagerImpl : Logger::Loggable, public OverloadM }; } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From b0afa366f1358fba6331595d0536fa62dcfd2e96 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 27 Sep 2021 17:01:15 +0000 Subject: [PATCH 3/8] Fix compile error for integration tests Signed-off-by: Kateryna Nezdolii --- test/integration/fake_resource_monitor.cc | 2 +- test/integration/fake_resource_monitor.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/fake_resource_monitor.cc b/test/integration/fake_resource_monitor.cc index 85a11084f6e1..eb57a5ba6669 100644 --- a/test/integration/fake_resource_monitor.cc +++ b/test/integration/fake_resource_monitor.cc @@ -4,7 +4,7 @@ namespace Envoy { FakeResourceMonitor::~FakeResourceMonitor() { factory_.onMonitorDestroyed(this); } -void FakeResourceMonitor::updateResourceUsage(Callbacks& callbacks) { +void FakeResourceMonitor::updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) { Server::ResourceUsage usage; usage.resource_pressure_ = pressure_; callbacks.onSuccess(usage); diff --git a/test/integration/fake_resource_monitor.h b/test/integration/fake_resource_monitor.h index 84d40eb70dcb..b32b581912d1 100644 --- a/test/integration/fake_resource_monitor.h +++ b/test/integration/fake_resource_monitor.h @@ -15,7 +15,7 @@ class FakeResourceMonitor : public Server::ResourceMonitor { : dispatcher_(dispatcher), factory_(factory), pressure_(0.0) {} // Server::ResourceMonitor ~FakeResourceMonitor() override; - void updateResourceUsage(Callbacks& callbacks) override; + void updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) override; void setResourcePressure(double pressure) { dispatcher_.post([this, pressure] { pressure_ = pressure; }); From 9ef48cc7e168bb54e52a0fdae0cc959c79ed9602 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 15 Oct 2021 14:10:10 +0000 Subject: [PATCH 4/8] Moving classes around Signed-off-by: Kateryna Nezdolii --- envoy/server/BUILD | 10 ++- envoy/server/overload/BUILD | 10 +-- .../overload/proactive_resource_monitor.h | 86 ------------------- .../overload/thread_local_overload_state.h | 4 +- envoy/server/resource_monitor_config.h | 2 +- 5 files changed, 12 insertions(+), 100 deletions(-) delete mode 100644 envoy/server/overload/proactive_resource_monitor.h diff --git a/envoy/server/BUILD b/envoy/server/BUILD index 6de546726f5c..2d3a53db1850 100644 --- a/envoy/server/BUILD +++ b/envoy/server/BUILD @@ -292,6 +292,14 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "proactive_resource_monitor_interface", + hdrs = ["proactive_resource_monitor.h"], + deps = [ + "//envoy/stats:stats_interface", + ], +) + envoy_cc_library( name = "request_id_extension_config_interface", hdrs = ["request_id_extension_config.h"], @@ -308,10 +316,10 @@ envoy_cc_library( deps = [ ":options_interface", ":resource_monitor_interface", + ":proactive_resource_monitor_interface", "//envoy/api:api_interface", "//envoy/event:dispatcher_interface", "//envoy/protobuf:message_validator_interface", - "//envoy/server/overload:proactive_resource_monitor", ], ) diff --git a/envoy/server/overload/BUILD b/envoy/server/overload/BUILD index 3f5da6105cc9..350004b6cf46 100644 --- a/envoy/server/overload/BUILD +++ b/envoy/server/overload/BUILD @@ -23,18 +23,10 @@ envoy_cc_library( name = "thread_local_overload_state", hdrs = ["thread_local_overload_state.h"], deps = [ - ":proactive_resource_monitor", + "//envoy/server:proactive_resource_monitor_interface", "//envoy/event:scaled_range_timer_manager_interface", "//envoy/event:timer_interface", "//envoy/thread_local:thread_local_object", "//source/common/common:interval_value", ], ) - -envoy_cc_library( - name = "proactive_resource_monitor", - hdrs = ["proactive_resource_monitor.h"], - deps = [ - "//envoy/stats:stats_interface", - ], -) diff --git a/envoy/server/overload/proactive_resource_monitor.h b/envoy/server/overload/proactive_resource_monitor.h deleted file mode 100644 index c988d7a41123..000000000000 --- a/envoy/server/overload/proactive_resource_monitor.h +++ /dev/null @@ -1,86 +0,0 @@ -#pragma once - -#include - -#include "envoy/common/pure.h" -#include "envoy/stats/scope.h" -#include "envoy/stats/stats.h" - -#include "source/common/common/assert.h" -#include "source/common/stats/symbol_table_impl.h" - -namespace Envoy { -namespace Server { - -class ProactiveResourceMonitor { -public: - ProactiveResourceMonitor() = default; - virtual ~ProactiveResourceMonitor() = default; - /** - * Tries to allocate resource for given resource monitor in thread safe manner. - * Returns true if there is enough resource quota available and allocation has succeeded, false - * otherwise. - * @param increment to add to current resource usage value and compare against configured max - * threshold. - */ - virtual bool tryAllocateResource(int64_t increment) PURE; - /** - * Tries to deallocate resource for given resource monitor in thread safe manner. - * Returns true if there is enough resource quota available and deallocation has succeeded, false - * otherwise. - * @param decrement to subtract from current resource usage value. - */ - virtual bool tryDeallocateResource(int64_t decrement) PURE; - /** - * Returns current resource usage tracked by monitor. - */ - virtual int64_t currentResourceUsage() const PURE; - /** - * Returns max resource usage configured in monitor. - */ - virtual uint64_t maxResourceUsage() const PURE; -}; - -using ProactiveResourceMonitorPtr = std::unique_ptr; - -class ProactiveResource { -public: - ProactiveResource(const std::string& name, ProactiveResourceMonitorPtr monitor, - Stats::Scope& stats_scope) - : name_(name), monitor_(std::move(monitor)), - failed_updates_counter_(makeCounter(stats_scope, name, "failed_updates")) {} - - bool tryAllocateResource(int64_t increment) { - if (monitor_->tryAllocateResource(increment)) { - return true; - } else { - failed_updates_counter_.inc(); - return false; - } - } - - bool tryDeallocateResource(int64_t decrement) { - if (monitor_->tryDeallocateResource(decrement)) { - return true; - } else { - failed_updates_counter_.inc(); - return false; - } - } - - int64_t currentResourceUsage() { return monitor_->currentResourceUsage(); } - -private: - const std::string name_; - ProactiveResourceMonitorPtr monitor_; - Stats::Counter& failed_updates_counter_; - - Stats::Counter& makeCounter(Stats::Scope& scope, absl::string_view a, absl::string_view b) { - Stats::StatNameManagedStorage stat_name(absl::StrCat("overload.", a, ".", b), - scope.symbolTable()); - return scope.counterFromStatName(stat_name.statName()); - } -}; - -} // namespace Server -} // namespace Envoy diff --git a/envoy/server/overload/thread_local_overload_state.h b/envoy/server/overload/thread_local_overload_state.h index 9232b80646a9..d0a1cb4a25ae 100644 --- a/envoy/server/overload/thread_local_overload_state.h +++ b/envoy/server/overload/thread_local_overload_state.h @@ -5,7 +5,7 @@ #include "envoy/common/pure.h" #include "envoy/event/scaled_range_timer_manager.h" #include "envoy/event/timer.h" -#include "envoy/server/overload/proactive_resource_monitor.h" +#include "envoy/server/proactive_resource_monitor.h" #include "envoy/thread_local/thread_local_object.h" #include "source/common/common/interval_value.h" @@ -23,8 +23,6 @@ class OverloadProactiveResourceNameValues { const std::string GlobalDownstreamMaxConnections = "envoy.resource_monitors.global_downstream_max_connections"; - std::set proactive_resource_names_{GlobalDownstreamMaxConnections}; - absl::flat_hash_map proactive_action_name_to_resource_ = { {GlobalDownstreamMaxConnections, diff --git a/envoy/server/resource_monitor_config.h b/envoy/server/resource_monitor_config.h index d90a0fc735c7..7b1a17568a24 100644 --- a/envoy/server/resource_monitor_config.h +++ b/envoy/server/resource_monitor_config.h @@ -6,7 +6,7 @@ #include "envoy/event/dispatcher.h" #include "envoy/protobuf/message_validator.h" #include "envoy/server/options.h" -#include "envoy/server/overload/proactive_resource_monitor.h" +#include "envoy/server/proactive_resource_monitor.h" #include "envoy/server/resource_monitor.h" #include "source/common/protobuf/protobuf.h" From 7e9319c75fa7c68c18983bbbce9f26433de210b6 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 15 Oct 2021 18:09:30 +0000 Subject: [PATCH 5/8] Apply review comments Signed-off-by: Kateryna Nezdolii --- envoy/server/BUILD | 2 +- envoy/server/overload/BUILD | 2 +- .../overload/thread_local_overload_state.h | 12 +-- envoy/server/proactive_resource_monitor.h | 86 +++++++++++++++++++ envoy/server/resource_monitor_config.h | 2 +- source/server/overload_manager_impl.cc | 20 ++--- test/server/overload_manager_impl_test.cc | 4 +- 7 files changed, 104 insertions(+), 24 deletions(-) create mode 100644 envoy/server/proactive_resource_monitor.h diff --git a/envoy/server/BUILD b/envoy/server/BUILD index 2d3a53db1850..9404b8f95d10 100644 --- a/envoy/server/BUILD +++ b/envoy/server/BUILD @@ -315,8 +315,8 @@ envoy_cc_library( hdrs = ["resource_monitor_config.h"], deps = [ ":options_interface", - ":resource_monitor_interface", ":proactive_resource_monitor_interface", + ":resource_monitor_interface", "//envoy/api:api_interface", "//envoy/event:dispatcher_interface", "//envoy/protobuf:message_validator_interface", diff --git a/envoy/server/overload/BUILD b/envoy/server/overload/BUILD index 350004b6cf46..fd1bf3e0bc7b 100644 --- a/envoy/server/overload/BUILD +++ b/envoy/server/overload/BUILD @@ -23,9 +23,9 @@ envoy_cc_library( name = "thread_local_overload_state", hdrs = ["thread_local_overload_state.h"], deps = [ - "//envoy/server:proactive_resource_monitor_interface", "//envoy/event:scaled_range_timer_manager_interface", "//envoy/event:timer_interface", + "//envoy/server:proactive_resource_monitor_interface", "//envoy/thread_local:thread_local_object", "//source/common/common:interval_value", ], diff --git a/envoy/server/overload/thread_local_overload_state.h b/envoy/server/overload/thread_local_overload_state.h index d0a1cb4a25ae..6a4fda14f8e5 100644 --- a/envoy/server/overload/thread_local_overload_state.h +++ b/envoy/server/overload/thread_local_overload_state.h @@ -29,7 +29,7 @@ class OverloadProactiveResourceNameValues { OverloadProactiveResourceName::GlobalDownstreamMaxConnections}}; }; -using OverloadProactiveResourceNames = ConstSingleton; +using OverloadProactiveResources = ConstSingleton; /** * Tracks the state of an overload action. The state is a number between 0 and 1 that represents the @@ -66,17 +66,17 @@ class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { // Get a thread-local reference to the value for the given action key. virtual const OverloadActionState& getState(const std::string& action) PURE; /** - * Invokes corresponding resource monitor to allocate resource for given resource monitor in - * thread safe manner. Returns true if there is enough resource quota available and allocation has - * succeeded, false if allocation failed or resource is not registered. + * Invokes the corresponding resource monitor to allocate resource for given resource monitor in + * a thread safe manner. Returns true if there is enough resource quota available and allocation + * has succeeded, false if allocation failed or resource is not registered. * @param name of corresponding resource monitor. * @param increment to add to current resource usage value within monitor. */ virtual bool tryAllocateResource(OverloadProactiveResourceName resource_name, int64_t increment) PURE; /** - * Invokes corresponding resource monitor to deallocate resource for given resource monitor in - * thread safe manner. Returns true if there is enough resource quota available and deallocation + * Invokes the corresponding resource monitor to deallocate resource for given resource monitor in + * a thread safe manner. Returns true if there is enough resource quota available and deallocation * has succeeded, false if deallocation failed or resource is not registered. * @param name of corresponding resource monitor. * @param decrement to subtract from current resource usage value within monitor. diff --git a/envoy/server/proactive_resource_monitor.h b/envoy/server/proactive_resource_monitor.h new file mode 100644 index 000000000000..ee37ccf094fd --- /dev/null +++ b/envoy/server/proactive_resource_monitor.h @@ -0,0 +1,86 @@ +#pragma once + +#include + +#include "envoy/common/pure.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/stats.h" + +#include "source/common/common/assert.h" +#include "source/common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Server { + +class ProactiveResourceMonitor { +public: + ProactiveResourceMonitor() = default; + virtual ~ProactiveResourceMonitor() = default; + /** + * Tries to allocate resource for given resource monitor in thread safe manner. + * Returns true if there is enough resource quota available and allocation has succeeded, false + * otherwise. + * @param increment to add to current resource usage value and compare against configured max + * threshold. + */ + virtual bool tryAllocateResource(int64_t increment) PURE; + /** + * Tries to deallocate resource for given resource monitor in thread safe manner. + * Returns true if there is enough resource quota available and deallocation has succeeded, false + * otherwise. + * @param decrement to subtract from current resource usage value. + */ + virtual bool tryDeallocateResource(int64_t decrement) PURE; + /** + * Returns current resource usage (most recent read) tracked by monitor. + */ + virtual int64_t currentResourceUsage() const PURE; + /** + * Returns max resource usage configured in monitor. + */ + virtual int64_t maxResourceUsage() const PURE; +}; + +using ProactiveResourceMonitorPtr = std::unique_ptr; + +class ProactiveResource { +public: + ProactiveResource(const std::string& name, ProactiveResourceMonitorPtr monitor, + Stats::Scope& stats_scope) + : name_(name), monitor_(std::move(monitor)), + failed_updates_counter_(makeCounter(stats_scope, name, "failed_updates")) {} + + bool tryAllocateResource(int64_t increment) { + if (monitor_->tryAllocateResource(increment)) { + return true; + } else { + failed_updates_counter_.inc(); + return false; + } + } + + bool tryDeallocateResource(int64_t decrement) { + if (monitor_->tryDeallocateResource(decrement)) { + return true; + } else { + failed_updates_counter_.inc(); + return false; + } + } + + int64_t currentResourceUsage() { return monitor_->currentResourceUsage(); } + +private: + const std::string name_; + ProactiveResourceMonitorPtr monitor_; + Stats::Counter& failed_updates_counter_; + + Stats::Counter& makeCounter(Stats::Scope& scope, absl::string_view a, absl::string_view b) { + Stats::StatNameManagedStorage stat_name(absl::StrCat("overload.", a, ".", b), + scope.symbolTable()); + return scope.counterFromStatName(stat_name.statName()); + } +}; + +} // namespace Server +} // namespace Envoy diff --git a/envoy/server/resource_monitor_config.h b/envoy/server/resource_monitor_config.h index 7b1a17568a24..a7d9086c28f0 100644 --- a/envoy/server/resource_monitor_config.h +++ b/envoy/server/resource_monitor_config.h @@ -73,7 +73,7 @@ class ProactiveResourceMonitorFactory : public Config::TypedFactory { * Create a particular proactive resource monitor implementation. * @param config const ProtoBuf::Message& supplies the config for the proactive resource monitor * implementation. - * @param context ProactiveResourceMonitorFactoryContext& supplies the resource monitor's context. + * @param context ResourceMonitorFactoryContext& supplies the resource monitor's context. * @return ProactiveResourceMonitorPtr the resource monitor instance. Should not be nullptr. * @throw EnvoyException if the implementation is unable to produce an instance with * the provided parameters. diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 54b09809bd48..4a274c145e1b 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -48,15 +48,11 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { int64_t increment) override { const auto proactive_resource = proactive_resources_->find(resource_name); if (proactive_resource != proactive_resources_->end()) { - if (proactive_resource->second.tryAllocateResource(increment)) { - return true; - } else { - return false; - } + return proactive_resource->second.tryAllocateResource(increment); } else { ENVOY_LOG_MISC(warn, " {Failed to allocate unknown proactive resource }"); - // Resource monitor is not configured, pass through mode. - return true; + // Resource monitor is not configured. + return false; } } @@ -325,11 +321,11 @@ OverloadManagerImpl::OverloadManagerImpl(Event::Dispatcher& dispatcher, Stats::S const auto& name = resource.name(); // Check if it is a proactive resource. auto proactive_resource_it = - OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.find(name); + OverloadProactiveResources::get().proactive_action_name_to_resource_.find(name); ENVOY_LOG(debug, "Evaluating resource {}", name); bool result = false; if (proactive_resource_it != - OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.end()) { + OverloadProactiveResources::get().proactive_action_name_to_resource_.end()) { ENVOY_LOG(debug, "Adding proactive resource monitor for {}", name); auto& factory = Config::Utility::getAndCheckFactory( @@ -337,7 +333,6 @@ OverloadManagerImpl::OverloadManagerImpl(Event::Dispatcher& dispatcher, Stats::S auto config = Config::Utility::translateToFactoryConfig(resource, validation_visitor, factory); auto monitor = factory.createProactiveResourceMonitor(*config, context); - result = proactive_resources_ ->try_emplace(proactive_resource_it->second, name, std::move(monitor), stats_scope) @@ -349,7 +344,6 @@ OverloadManagerImpl::OverloadManagerImpl(Event::Dispatcher& dispatcher, Stats::S auto config = Config::Utility::translateToFactoryConfig(resource, validation_visitor, factory); auto monitor = factory.createResourceMonitor(*config, context); - result = resources_.try_emplace(name, name, std::move(monitor), *this, stats_scope).second; } if (!result) { @@ -388,11 +382,11 @@ OverloadManagerImpl::OverloadManagerImpl(Event::Dispatcher& dispatcher, Stats::S for (const auto& trigger : action.triggers()) { const std::string& resource = trigger.name(); auto proactive_resource_it = - OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.find(resource); + OverloadProactiveResources::get().proactive_action_name_to_resource_.find(resource); if (resources_.find(resource) == resources_.end() && proactive_resource_it == - OverloadProactiveResourceNames::get().proactive_action_name_to_resource_.end()) { + OverloadProactiveResources::get().proactive_action_name_to_resource_.end()) { throw EnvoyException( fmt::format("Unknown trigger resource {} for overload action {}", resource, name)); } diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 4657e2a6dd5a..a1f97ac0f3fb 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -116,10 +116,10 @@ class FakeProactiveResourceMonitor : public ProactiveResourceMonitor { } int64_t currentResourceUsage() const { return current_.load(); } - uint64_t maxResourceUsage() const { return max_; } + int64_t maxResourceUsage() const { return max_; } private: - uint64_t max_; + int64_t max_; std::atomic current_; }; From edb87d66c8f3707299c85659cfae5fea60d9aaea Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 15 Oct 2021 20:26:46 +0000 Subject: [PATCH 6/8] fix clang tidy Signed-off-by: Kateryna Nezdolii --- envoy/server/overload/thread_local_overload_state.h | 1 + test/server/overload_manager_impl_test.cc | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/envoy/server/overload/thread_local_overload_state.h b/envoy/server/overload/thread_local_overload_state.h index 6a4fda14f8e5..35f36050945e 100644 --- a/envoy/server/overload/thread_local_overload_state.h +++ b/envoy/server/overload/thread_local_overload_state.h @@ -9,6 +9,7 @@ #include "envoy/thread_local/thread_local_object.h" #include "source/common/common/interval_value.h" +#include "source/common/singleton/const_singleton.h" namespace Envoy { namespace Server { diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index a1f97ac0f3fb..9288cc9ff7b6 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -95,7 +95,7 @@ class FakeProactiveResourceMonitor : public ProactiveResourceMonitor { public: FakeProactiveResourceMonitor(uint64_t max) : max_(max), current_(0){}; - bool tryAllocateResource(int64_t increment) { + bool tryAllocateResource(int64_t increment) override { int64_t new_val = (current_ += increment); if (new_val > static_cast(max_) || new_val < 0) { current_ -= increment; @@ -104,7 +104,7 @@ class FakeProactiveResourceMonitor : public ProactiveResourceMonitor { return true; } - bool tryDeallocateResource(int64_t decrement) { + bool tryDeallocateResource(int64_t decrement) override { RELEASE_ASSERT(decrement <= current_, "Cannot deallocate resource, current resource usage is lower than decrement"); int64_t new_val = (current_ -= decrement); @@ -115,8 +115,8 @@ class FakeProactiveResourceMonitor : public ProactiveResourceMonitor { return true; } - int64_t currentResourceUsage() const { return current_.load(); } - int64_t maxResourceUsage() const { return max_; } + int64_t currentResourceUsage() const override { return current_.load(); } + int64_t maxResourceUsage() const override { return max_; } private: int64_t max_; From 5017e97c195d33488957668668e1433ccee44607 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 19 Nov 2021 10:56:34 +0000 Subject: [PATCH 7/8] Fix merge conflict Signed-off-by: Kateryna Nezdolii --- envoy/server/resource_monitor.h | 1 - 1 file changed, 1 deletion(-) diff --git a/envoy/server/resource_monitor.h b/envoy/server/resource_monitor.h index a0de9518ee13..e2b0d0647572 100644 --- a/envoy/server/resource_monitor.h +++ b/envoy/server/resource_monitor.h @@ -4,7 +4,6 @@ #include "envoy/common/exception.h" #include "envoy/common/pure.h" -#include "envoy/event/dispatcher.h" #include "source/common/common/assert.h" From 4787f8b10f5000aaa9673c0a535515817b48c2e4 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 26 Nov 2021 10:46:41 +0000 Subject: [PATCH 8/8] Apply review comments Signed-off-by: Kateryna Nezdolii --- test/server/overload_manager_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 9288cc9ff7b6..6fa231cfeb71 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -810,15 +810,15 @@ TEST_F(OverloadManagerImplTest, ProactiveResourceAllocateAndDeallocateResourceTe OverloadProactiveResourceName::GlobalDownstreamMaxConnections)); bool resource_allocated = manager->getThreadLocalOverloadState().tryAllocateResource( Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 1); - EXPECT_EQ(true, resource_allocated); + EXPECT_TRUE(resource_allocated); resource_allocated = manager->getThreadLocalOverloadState().tryAllocateResource( Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 3); - EXPECT_EQ(false, resource_allocated); + EXPECT_FALSE(resource_allocated); EXPECT_EQ(1, failed_updates.value()); bool resource_deallocated = manager->getThreadLocalOverloadState().tryDeallocateResource( Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 1); - EXPECT_EQ(true, resource_deallocated); + EXPECT_TRUE(resource_deallocated); EXPECT_DEATH(manager->getThreadLocalOverloadState().tryDeallocateResource( Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections, 1), ".*Cannot deallocate resource, current resource usage is lower than decrement.*");