From 09a4d7cfdd81af078fc388866e0a19d5a331beb0 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 17 Jun 2020 06:53:07 -0400 Subject: [PATCH 01/14] Make OverloadManager argument non-optional The only user of a null OverloadManager was the admin console. Switch that to using a no-op OverloadManager subclass so that the OM can be provided via reference in all cases. This makes the HCM initialization simpler since it doesn't need to worry about the null pointer. Signed-off-by: Alex Konradi --- include/envoy/server/overload_manager.h | 11 ------- source/common/http/conn_manager_impl.cc | 14 ++++----- source/common/http/conn_manager_impl.h | 2 +- .../network/http_connection_manager/config.cc | 8 ++--- source/server/admin/admin.cc | 11 +++++-- source/server/admin/admin.h | 30 +++++++++++++++++++ test/common/http/BUILD | 1 + .../http/conn_manager_impl_fuzz_test.cc | 4 ++- test/common/http/conn_manager_impl_test.cc | 2 +- 9 files changed, 53 insertions(+), 30 deletions(-) diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 010ac8ee9468..6a214cd2669c 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -106,17 +106,6 @@ class OverloadManager { * an alternative to registering a callback for overload action state changes. */ virtual ThreadLocalOverloadState& getThreadLocalOverloadState() PURE; - - /** - * Convenience method to get a statically allocated reference to the inactive overload - * action state. Useful for code that needs to initialize a reference either to an - * entry in the ThreadLocalOverloadState map (if overload behavior is enabled) or to - * some other static memory location set to the inactive state (if overload behavior - * is disabled). - */ - static const OverloadActionState& getInactiveState() { - CONSTRUCT_ON_FIRST_USE(OverloadActionState, OverloadActionState::Inactive); - } }; } // namespace Server diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 243260c98bf2..50906dcb6ca7 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -104,7 +104,7 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, Http::Context& http_context, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info, Upstream::ClusterManager& cluster_manager, - Server::OverloadManager* overload_manager, + Server::OverloadManager& overload_manager, TimeSource& time_source) : config_(config), stats_(config_.stats()), conn_length_(new Stats::HistogramCompletableTimespanImpl( @@ -113,14 +113,10 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, random_generator_(random_generator), http_context_(http_context), runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager), listener_stats_(config_.listenerStats()), - overload_stop_accepting_requests_ref_( - overload_manager ? overload_manager->getThreadLocalOverloadState().getState( - Server::OverloadActionNames::get().StopAcceptingRequests) - : Server::OverloadManager::getInactiveState()), - overload_disable_keepalive_ref_( - overload_manager ? overload_manager->getThreadLocalOverloadState().getState( - Server::OverloadActionNames::get().DisableHttpKeepAlive) - : Server::OverloadManager::getInactiveState()), + overload_stop_accepting_requests_ref_(overload_manager.getThreadLocalOverloadState().getState( + Server::OverloadActionNames::get().StopAcceptingRequests)), + overload_disable_keepalive_ref_(overload_manager.getThreadLocalOverloadState().getState( + Server::OverloadActionNames::get().DisableHttpKeepAlive)), time_source_(time_source) {} const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index f51240bbc149..c6965a261896 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -57,7 +57,7 @@ class ConnectionManagerImpl : Logger::Loggable, Runtime::RandomGenerator& random_generator, Http::Context& http_context, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info, Upstream::ClusterManager& cluster_manager, - Server::OverloadManager* overload_manager, TimeSource& time_system); + Server::OverloadManager& overload_manager, TimeSource& time_system); ~ConnectionManagerImpl() override; static ConnectionManagerStats generateStats(const std::string& prefix, Stats::Scope& scope); diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 1c0958ad6f1d..171cb4647268 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -144,8 +144,8 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( return [singletons, filter_config, &context](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *filter_config, context.drainDecision(), context.random(), context.httpContext(), - context.runtime(), context.localInfo(), context.clusterManager(), - &context.overloadManager(), context.dispatcher().timeSource())}); + context.runtime(), context.localInfo(), context.clusterManager(), context.overloadManager(), + context.dispatcher().timeSource())}); }; } @@ -591,8 +591,8 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( return [singletons, filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { auto conn_manager = std::make_unique( *filter_config, context.drainDecision(), context.random(), context.httpContext(), - context.runtime(), context.localInfo(), context.clusterManager(), - &context.overloadManager(), context.dispatcher().timeSource()); + context.runtime(), context.localInfo(), context.clusterManager(), context.overloadManager(), + context.dispatcher().timeSource()); // This factory creates a new ConnectionManagerImpl in the absence of its usual environment as // an L4 filter, so this factory needs to take a few actions. diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 20b08bc3100a..9d99babd11cc 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -650,6 +650,12 @@ ConfigTracker& AdminImpl::getConfigTracker() { return config_tracker_; } AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider(TimeSource& time_source) : config_(new Router::NullConfigImpl()), time_source_(time_source) {} +OverloadTimerFactory AdminImpl::NullOverloadManager::NullThreadOverloadState::getTimerFactory() { + return [this](absl::string_view, Event::TimerCb callback) { + return dispatcher_.createTimer(callback); + }; +} + void AdminImpl::startHttpListener(const std::string& access_log_path, const std::string& address_out_path, Network::Address::InstanceConstSharedPtr address, @@ -679,6 +685,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) request_id_extension_(Http::RequestIDExtensionFactory::defaultInstance(server_.random())), profile_path_(profile_path), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), + overload_manager_(server_.dispatcher()), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats("http.admin.", no_op_store_)), route_config_provider_(server.timeSource()), @@ -760,11 +767,9 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, const std::vector&) { - // Don't pass in the overload manager so that the admin interface is accessible even when - // the envoy is overloaded. connection.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *this, server_.drainManager(), server_.random(), server_.httpContext(), server_.runtime(), - server_.localInfo(), server_.clusterManager(), nullptr, server_.timeSource())}); + server_.localInfo(), server_.clusterManager(), overload_manager_, server_.timeSource())}); return true; } diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 51818a6a714e..5f0658b8db7c 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -12,6 +12,7 @@ #include "envoy/admin/v3/server_info.pb.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/route/v3/route.pb.h" +#include "envoy/event/timer.h" #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" #include "envoy/http/filter.h" #include "envoy/http/request_id_extension.h" @@ -20,6 +21,7 @@ #include "envoy/server/admin.h" #include "envoy/server/instance.h" #include "envoy/server/listener_manager.h" +#include "envoy/server/overload_manager.h" #include "envoy/upstream/outlier_detection.h" #include "envoy/upstream/resource_manager.h" @@ -244,6 +246,33 @@ class AdminImpl : public Admin, TimeSource& time_source_; }; + /** + * Implementation of OverloadManager that is never overloaded. Using this instead of the real + * OverloadManager keeps the admin interface accessible even when the proxy is overloaded. + */ + struct NullOverloadManager : public OverloadManager { + struct NullThreadOverloadState : public ThreadLocalOverloadState { + NullThreadOverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} + const OverloadActionState& getState(const std::string&) override { return inactive_; } + void setState(const std::string&, OverloadActionState) override {} + OverloadTimerFactory getTimerFactory() override; + + const OverloadActionState inactive_ = OverloadActionState::Inactive; + Event::Dispatcher& dispatcher_; + }; + + NullOverloadManager(Event::Dispatcher& dispatcher) : thread_local_overload_state_(dispatcher) {} + void start() override {} + ThreadLocalOverloadState& getThreadLocalOverloadState() override { + return thread_local_overload_state_; + } + bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override { + return false; + } + + NullThreadOverloadState thread_local_overload_state_; + }; + /** * Helper methods for the /clusters url handler. */ @@ -386,6 +415,7 @@ class AdminImpl : public Admin, std::list access_logs_; const std::string profile_path_; Http::ConnectionManagerStats stats_; + NullOverloadManager overload_manager_; // Note: this is here to essentially blackhole the tracing stats since they aren't used in the // Admin case. Stats::IsolatedStoreImpl no_op_store_; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index e723a48abeb0..08f8ed274521 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -180,6 +180,7 @@ envoy_cc_fuzz_test( "//test/mocks/http:http_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/router:router_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/ssl:ssl_mocks", diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 196b8bdc24d2..a19bb30f7f57 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -30,6 +30,7 @@ #include "test/mocks/access_log/mocks.h" #include "test/mocks/common.h" #include "test/mocks/http/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/router/mocks.h" @@ -541,6 +542,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { NiceMock local_info; NiceMock cluster_manager; NiceMock filter_callbacks; + NiceMock overload_manager; auto ssl_connection = std::make_shared(); bool connection_alive = true; @@ -554,7 +556,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { std::make_shared("0.0.0.0"); ConnectionManagerImpl conn_manager(config, drain_close, random, http_context, runtime, local_info, - cluster_manager, nullptr, config.time_system_); + cluster_manager, overload_manager, config.time_system_); conn_manager.initializeReadFilterCallbacks(filter_callbacks); std::vector streams; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 7083b4f92d9d..41ed87ca5a12 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -133,7 +133,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::make_shared("0.0.0.0"); conn_manager_ = std::make_unique( *this, drain_close_, random_, http_context_, runtime_, local_info_, cluster_manager_, - &overload_manager_, test_time_.timeSystem()); + overload_manager_, test_time_.timeSystem()); conn_manager_->initializeReadFilterCallbacks(filter_callbacks_); if (tracing) { From 76f494bdd5d9b35a225e9eb536a1223165aa971b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 15 Jun 2020 17:46:23 -0400 Subject: [PATCH 02/14] Make ThreadLocalOverloadState virtual Make ThreadLocalOverloadState an interface that the Overload Manager can return an implementation of. This decouples the behavior specified by the header from the details of the implementation. Signed-off-by: Alex Konradi --- include/envoy/server/overload_manager.h | 25 ++++------------ source/server/admin/admin.cc | 6 ---- source/server/admin/admin.h | 2 -- source/server/overload_manager_impl.cc | 33 ++++++++++++++++++++-- test/common/http/conn_manager_impl_test.cc | 17 ++++++----- test/mocks/server/mocks.cc | 5 ++++ test/mocks/server/mocks.h | 11 +++++++- 7 files changed, 60 insertions(+), 39 deletions(-) diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 6a214cd2669c..0affffd4b125 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -31,27 +31,12 @@ using OverloadActionCb = std::function; /** * Thread-local copy of the state of each configured overload action. */ -class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { +class ThreadLocalOverloadState { public: - const OverloadActionState& getState(const std::string& action) { - auto it = actions_.find(action); - if (it == actions_.end()) { - it = actions_.insert(std::make_pair(action, OverloadActionState::Inactive)).first; - } - return it->second; - } - - void setState(const std::string& action, OverloadActionState state) { - auto it = actions_.find(action); - if (it == actions_.end()) { - actions_[action] = state; - } else { - it->second = state; - } - } - -private: - std::unordered_map actions_; + virtual ~ThreadLocalOverloadState() = default; + + // Get a thread-local reference to the value for the given action key. + virtual const OverloadActionState& getState(const std::string& action) PURE; }; /** diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 9d99babd11cc..75f18b66a9ae 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -650,12 +650,6 @@ ConfigTracker& AdminImpl::getConfigTracker() { return config_tracker_; } AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider(TimeSource& time_source) : config_(new Router::NullConfigImpl()), time_source_(time_source) {} -OverloadTimerFactory AdminImpl::NullOverloadManager::NullThreadOverloadState::getTimerFactory() { - return [this](absl::string_view, Event::TimerCb callback) { - return dispatcher_.createTimer(callback); - }; -} - void AdminImpl::startHttpListener(const std::string& access_log_path, const std::string& address_out_path, Network::Address::InstanceConstSharedPtr address, diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 5f0658b8db7c..b156119f450a 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -254,8 +254,6 @@ class AdminImpl : public Admin, struct NullThreadOverloadState : public ThreadLocalOverloadState { NullThreadOverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} const OverloadActionState& getState(const std::string&) override { return inactive_; } - void setState(const std::string&, OverloadActionState) override {} - OverloadTimerFactory getTimerFactory() override; const OverloadActionState inactive_ = OverloadActionState::Inactive; Event::Dispatcher& dispatcher_; diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index ef56fc8d7fae..e84e04458d76 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -35,6 +35,33 @@ class ThresholdTriggerImpl : public OverloadAction::Trigger { absl::optional value_; }; +/** + * Thread-local copy of the state of each configured overload action. + */ +class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState, + public ThreadLocal::ThreadLocalObject { +public: + const OverloadActionState& getState(const std::string& action) override { + auto it = actions_.find(action); + if (it == actions_.end()) { + it = actions_.insert(std::make_pair(action, OverloadActionState::Inactive)).first; + } + return it->second; + } + + void setState(const std::string& action, OverloadActionState state) { + auto it = actions_.find(action); + if (it == actions_.end()) { + actions_[action] = state; + } else { + it->second = state; + } + } + +private: + std::unordered_map actions_; +}; + 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()); @@ -148,7 +175,7 @@ void OverloadManagerImpl::start() { started_ = true; tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { - return std::make_shared(); + return std::make_shared(); }); if (resources_.empty()) { @@ -191,7 +218,7 @@ bool OverloadManagerImpl::registerForAction(const std::string& action, } ThreadLocalOverloadState& OverloadManagerImpl::getThreadLocalOverloadState() { - return tls_->getTyped(); + return tls_->getTyped(); } void OverloadManagerImpl::updateResourcePressure(const std::string& resource, double pressure) { @@ -208,7 +235,7 @@ void OverloadManagerImpl::updateResourcePressure(const std::string& resource, do ENVOY_LOG(info, "Overload action {} became {}", action, is_active ? "active" : "inactive"); tls_->runOnAllThreads([this, action, state] { - tls_->getTyped().setState(action, state); + tls_->getTyped().setState(action, state); }); auto callback_range = action_to_callbacks_.equal_range(action); std::for_each(callback_range.first, callback_range.second, diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 41ed87ca5a12..c7bb5e9c3ae5 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -5600,11 +5600,12 @@ TEST(HttpConnectionManagerTracingStatsTest, verifyTracingStats) { } TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { - setup(false, ""); + Server::OverloadActionState stop_accepting_requests = Server::OverloadActionState::Active; + ON_CALL(overload_manager_.overload_state_, + getState(Server::OverloadActionNames::get().StopAcceptingRequests)) + .WillByDefault(ReturnRef(stop_accepting_requests)); - overload_manager_.overload_state_.setState( - Server::OverloadActionNames::get().StopAcceptingRequests, - Server::OverloadActionState::Active); + setup(false, ""); EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance&) -> Http::Status { RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_); @@ -5630,10 +5631,12 @@ TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { } TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenOverloaded) { - setup(false, ""); + Server::OverloadActionState disable_http_keep_alive = Server::OverloadActionState::Active; + ON_CALL(overload_manager_.overload_state_, + getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)) + .WillByDefault(ReturnRef(disable_http_keep_alive)); - overload_manager_.overload_state_.setState( - Server::OverloadActionNames::get().DisableHttpKeepAlive, Server::OverloadActionState::Active); + setup(false, ""); std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index e9fc293558c5..b7bb88a6f98d 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -76,6 +76,11 @@ MockHotRestart::MockHotRestart() : stats_allocator_(*symbol_table_) { } MockHotRestart::~MockHotRestart() = default; +MockThreadLocalOverloadState::MockThreadLocalOverloadState() + : disabled_state_(OverloadActionState::Inactive) { + ON_CALL(*this, getState).WillByDefault(ReturnRef(disabled_state_)); +} + MockOverloadManager::MockOverloadManager() { ON_CALL(*this, getThreadLocalOverloadState()).WillByDefault(ReturnRef(overload_state_)); } diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 1685d0f706d2..7125bbc5108d 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -331,6 +331,15 @@ class MockWorker : public Worker { std::function remove_filter_chains_completion_; }; +class MockThreadLocalOverloadState : public ThreadLocalOverloadState { +public: + MockThreadLocalOverloadState(); + MOCK_METHOD(const OverloadActionState&, getState, (const std::string&), (override)); + +private: + const OverloadActionState disabled_state_; +}; + class MockOverloadManager : public OverloadManager { public: MockOverloadManager(); @@ -343,7 +352,7 @@ class MockOverloadManager : public OverloadManager { OverloadActionCb callback)); MOCK_METHOD(ThreadLocalOverloadState&, getThreadLocalOverloadState, ()); - ThreadLocalOverloadState overload_state_; + NiceMock overload_state_; }; class MockInstance : public Instance { From 0a402c42b203e877ca1f1e36a99ba6151661514b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 26 Jun 2020 15:03:16 -0400 Subject: [PATCH 03/14] Fix formatting Signed-off-by: Alex Konradi --- test/common/http/BUILD | 2 +- test/common/http/conn_manager_impl_fuzz_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 08f8ed274521..9b5e71fad07c 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -180,9 +180,9 @@ envoy_cc_fuzz_test( "//test/mocks/http:http_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", - "//test/mocks/server:server_mocks", "//test/mocks/router:router_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/ssl:ssl_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:upstream_mocks", diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index a19bb30f7f57..f85146f42bcf 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -30,11 +30,11 @@ #include "test/mocks/access_log/mocks.h" #include "test/mocks/common.h" #include "test/mocks/http/mocks.h" -#include "test/mocks/server/mocks.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/router/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/ssl/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" From 2609dbee8a7afe46de26f72b2ea08d9de7d161a9 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 29 Jun 2020 16:00:28 -0400 Subject: [PATCH 04/14] Mark ThreadLocalOverloadState as thread-local All implementations of the interface are for thread-local objects so specify that with interface inheritance. Signed-off-by: Alex Konradi --- include/envoy/server/overload_manager.h | 2 +- source/server/overload_manager_impl.cc | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 0affffd4b125..5c534be33367 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -31,7 +31,7 @@ using OverloadActionCb = std::function; /** * Thread-local copy of the state of each configured overload action. */ -class ThreadLocalOverloadState { +class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { public: virtual ~ThreadLocalOverloadState() = default; diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index e84e04458d76..de5ac292a74b 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -38,8 +38,7 @@ class ThresholdTriggerImpl : public OverloadAction::Trigger { /** * Thread-local copy of the state of each configured overload action. */ -class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState, - public ThreadLocal::ThreadLocalObject { +class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { public: const OverloadActionState& getState(const std::string& action) override { auto it = actions_.find(action); From dac81f841bf1a94e33d058caf8142d4df5e25cd3 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 29 Jun 2020 16:01:21 -0400 Subject: [PATCH 05/14] Address feedback on null overload manager - rename member - use thread-local store - remove dispatcher Signed-off-by: Alex Konradi --- source/server/admin/admin.cc | 4 ++-- source/server/admin/admin.h | 22 ++++++++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 75f18b66a9ae..85924a8e5242 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -679,7 +679,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) request_id_extension_(Http::RequestIDExtensionFactory::defaultInstance(server_.random())), profile_path_(profile_path), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), - overload_manager_(server_.dispatcher()), + null_overload_manager_(server_.threadLocal()), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats("http.admin.", no_op_store_)), route_config_provider_(server.timeSource()), @@ -763,7 +763,7 @@ bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, const std::vector&) { connection.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *this, server_.drainManager(), server_.random(), server_.httpContext(), server_.runtime(), - server_.localInfo(), server_.clusterManager(), overload_manager_, server_.timeSource())}); + server_.localInfo(), server_.clusterManager(), null_overload_manager_, server_.timeSource())}); return true; } diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index b156119f450a..becce193bf22 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -251,24 +251,30 @@ class AdminImpl : public Admin, * OverloadManager keeps the admin interface accessible even when the proxy is overloaded. */ struct NullOverloadManager : public OverloadManager { - struct NullThreadOverloadState : public ThreadLocalOverloadState { - NullThreadOverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} + struct NullThreadLocalOverloadState : public ThreadLocalOverloadState { const OverloadActionState& getState(const std::string&) override { return inactive_; } const OverloadActionState inactive_ = OverloadActionState::Inactive; - Event::Dispatcher& dispatcher_; }; - NullOverloadManager(Event::Dispatcher& dispatcher) : thread_local_overload_state_(dispatcher) {} - void start() override {} + NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator) + : tls_(slot_allocator.allocateSlot()) {} + + void start() override { + tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { + return std::make_shared(); + }); + } + ThreadLocalOverloadState& getThreadLocalOverloadState() override { - return thread_local_overload_state_; + return tls_->getTyped(); } + bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override { return false; } - NullThreadOverloadState thread_local_overload_state_; + ThreadLocal::SlotPtr tls_; }; /** @@ -413,7 +419,7 @@ class AdminImpl : public Admin, std::list access_logs_; const std::string profile_path_; Http::ConnectionManagerStats stats_; - NullOverloadManager overload_manager_; + NullOverloadManager null_overload_manager_; // Note: this is here to essentially blackhole the tracing stats since they aren't used in the // Admin case. Stats::IsolatedStoreImpl no_op_store_; From 159f96496d662dc01c82362307fbfb8b185efdfa Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 1 Jul 2020 11:32:38 -0400 Subject: [PATCH 06/14] Address NullOverloadManager feedback Signed-off-by: Alex Konradi --- source/server/admin/admin.cc | 5 ++++- source/server/admin/admin.h | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 85924a8e5242..afcf795c603e 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -761,9 +761,12 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, const std::vector&) { + // Pass in the null overload manager so that the admin interface is accessible even when Envoy is + // overloaded. connection.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *this, server_.drainManager(), server_.random(), server_.httpContext(), server_.runtime(), - server_.localInfo(), server_.clusterManager(), null_overload_manager_, server_.timeSource())}); + server_.localInfo(), server_.clusterManager(), null_overload_manager_, + server_.timeSource())}); return true; } diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index becce193bf22..fab540fa3703 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -258,19 +258,21 @@ class AdminImpl : public Admin, }; NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator) - : tls_(slot_allocator.allocateSlot()) {} - - void start() override { + : tls_(slot_allocator.allocateSlot()) { tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(); }); } + void start() override {} + ThreadLocalOverloadState& getThreadLocalOverloadState() override { return tls_->getTyped(); } bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override { + // This method shouldn't be called by the admin listener + ASSERT(false); return false; } From 5f402603c585216fc6776297ef5384df8fa1f41b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 1 Jul 2020 16:23:12 -0400 Subject: [PATCH 07/14] Fix clang tidy issue Signed-off-by: Alex Konradi --- include/envoy/server/overload_manager.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 5c534be33367..e10812add8fd 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -33,8 +33,6 @@ using OverloadActionCb = std::function; */ class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { public: - virtual ~ThreadLocalOverloadState() = default; - // Get a thread-local reference to the value for the given action key. virtual const OverloadActionState& getState(const std::string& action) PURE; }; From 44ac271fa746dae86ff358462cb955bcb30e7120 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 1 Jul 2020 16:24:43 -0400 Subject: [PATCH 08/14] Remove unused #include Signed-off-by: Alex Konradi --- source/server/admin/admin.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index fab540fa3703..69991044f020 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -12,7 +12,6 @@ #include "envoy/admin/v3/server_info.pb.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/route/v3/route.pb.h" -#include "envoy/event/timer.h" #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" #include "envoy/http/filter.h" #include "envoy/http/request_id_extension.h" From 733486103f7bb6c4f2e0014029870a3527cdc001 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 1 Jul 2020 22:29:00 -0400 Subject: [PATCH 09/14] Move TLS initialization to start() Doing it in the constructor was causing ASAN errors. Signed-off-by: Alex Konradi --- source/server/admin/admin.cc | 1 + source/server/admin/admin.h | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index afcf795c603e..42d295cd3327 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -660,6 +660,7 @@ void AdminImpl::startHttpListener(const std::string& access_log_path, access_logs_.emplace_back(new Extensions::AccessLoggers::File::FileAccessLog( access_log_path, {}, Formatter::SubstitutionFormatUtils::defaultSubstitutionFormatter(), server_.accessLogManager())); + null_overload_manager_.start(); socket_ = std::make_shared(address, socket_options, true); socket_factory_ = std::make_shared(socket_); listener_ = std::make_unique(*this, std::move(listener_scope)); diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 69991044f020..a449ca67cd86 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -257,14 +257,14 @@ class AdminImpl : public Admin, }; NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator) - : tls_(slot_allocator.allocateSlot()) { + : tls_(slot_allocator.allocateSlot()) {} + + void start() override { tls_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(); }); } - void start() override {} - ThreadLocalOverloadState& getThreadLocalOverloadState() override { return tls_->getTyped(); } From 438f711baec9d285ee59b0d688d52cede4ffb3d0 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 6 Jul 2020 18:21:08 -0400 Subject: [PATCH 10/14] Move admin initialization later Initialize the AdminImpl after the dispatchers so that it can create thread-local data structures. Without this, integration tests fail ASAN runs. Signed-off-by: Alex Konradi --- source/server/server.cc | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/source/server/server.cc b/source/server/server.cc index 5b201c6c1e4c..96f945520642 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -379,23 +379,6 @@ void InstanceImpl::initialize(const Options& options, // 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()) { - throw EnvoyException("An admin access log path is required for a listening server."); - } - ENVOY_LOG(info, "admin address: {}", initial_config.admin().address()->asString()); - admin_->startHttpListener(initial_config.admin().accessLogPath(), options.adminAddressPath(), - initial_config.admin().address(), - initial_config.admin().socketOptions(), - stats_store_.createScope("listener.admin.")); - } else { - ENVOY_LOG(warn, "No admin address given, so no admin HTTP server started."); - } - config_tracker_entry_ = - admin_->getConfigTracker().add("bootstrap", [this] { return dumpBootstrapConfig(); }); - if (initial_config.admin().address()) { - admin_->addListenerToHandler(handler_.get()); - } loadServerFlags(initial_config.flagsPath()); @@ -425,6 +408,24 @@ void InstanceImpl::initialize(const Options& options, dispatcher_->initializeStats(stats_store_, "server."); } + if (initial_config.admin().address()) { + if (initial_config.admin().accessLogPath().empty()) { + throw EnvoyException("An admin access log path is required for a listening server."); + } + ENVOY_LOG(info, "admin address: {}", initial_config.admin().address()->asString()); + admin_->startHttpListener(initial_config.admin().accessLogPath(), options.adminAddressPath(), + initial_config.admin().address(), + initial_config.admin().socketOptions(), + stats_store_.createScope("listener.admin.")); + } else { + ENVOY_LOG(warn, "No admin address given, so no admin HTTP server started."); + } + config_tracker_entry_ = + admin_->getConfigTracker().add("bootstrap", [this] { return dumpBootstrapConfig(); }); + if (initial_config.admin().address()) { + admin_->addListenerToHandler(handler_.get()); + } + // The broad order of initialization from this point on is the following: // 1. Statically provisioned configuration (bootstrap) are loaded. // 2. Cluster manager is created and all primary clusters (i.e. with endpoint assignments From 3bc1f888cf8d5b34df86434e6b829b2a9ec3c66c Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 9 Jul 2020 09:30:41 -0400 Subject: [PATCH 11/14] Simplify ThreadLocalOverloadStateImpl::setState Signed-off-by: Alex Konradi --- source/server/overload_manager_impl.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index de5ac292a74b..02f7e6439d89 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -49,12 +49,7 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { } void setState(const std::string& action, OverloadActionState state) { - auto it = actions_.find(action); - if (it == actions_.end()) { - actions_[action] = state; - } else { - it->second = state; - } + actions_[action] = state; } private: From 04053d40c4d8629decceba3d1025a4a42028cdda Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 9 Jul 2020 09:53:09 -0400 Subject: [PATCH 12/14] Fix formatting Signed-off-by: Alex Konradi --- source/server/overload_manager_impl.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 02f7e6439d89..40156ed9b179 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -48,9 +48,7 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { return it->second; } - void setState(const std::string& action, OverloadActionState state) { - actions_[action] = state; - } + void setState(const std::string& action, OverloadActionState state) { actions_[action] = state; } private: std::unordered_map actions_; From 82146f64870496ea5376201ffc092ce096950955 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 10 Jul 2020 13:54:12 -0400 Subject: [PATCH 13/14] Use correct macro for "this is never called" Signed-off-by: Alex Konradi --- source/server/admin/admin.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index b2235590115f..b79143b86ff9 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -8,6 +8,7 @@ #include #include +#include "common/common/assert.h" #include "envoy/admin/v3/config_dump.pb.h" #include "envoy/admin/v3/server_info.pb.h" #include "envoy/config/core/v3/base.pb.h" @@ -272,7 +273,7 @@ class AdminImpl : public Admin, bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override { // This method shouldn't be called by the admin listener - ASSERT(false); + NOT_REACHED_GCOVR_EXCL_LINE; return false; } From c190617412ec7d3efd2d23772b7db38c3c23275f Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 10 Jul 2020 14:29:36 -0400 Subject: [PATCH 14/14] Fix formatting Signed-off-by: Alex Konradi --- source/server/admin/admin.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index b79143b86ff9..278aa30c342b 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -8,7 +8,6 @@ #include #include -#include "common/common/assert.h" #include "envoy/admin/v3/config_dump.pb.h" #include "envoy/admin/v3/server_info.pb.h" #include "envoy/config/core/v3/base.pb.h" @@ -25,6 +24,7 @@ #include "envoy/upstream/outlier_detection.h" #include "envoy/upstream/resource_manager.h" +#include "common/common/assert.h" #include "common/common/basic_resource_impl.h" #include "common/common/empty_string.h" #include "common/common/logger.h"