From 4a83db85c1644805cb97cec6cfc4b00c0eec6ff2 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 13 Jul 2020 20:15:10 -0400 Subject: [PATCH] server: make overload manager non-optional (#11777) 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 Signed-off-by: scheler --- include/envoy/server/overload_manager.h | 32 ++--------------- 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 | 9 +++-- source/server/admin/admin.h | 36 +++++++++++++++++++ source/server/overload_manager_impl.cc | 25 +++++++++++-- source/server/server.cc | 35 +++++++++--------- test/common/http/BUILD | 1 + .../http/conn_manager_impl_fuzz_test.cc | 4 ++- test/common/http/conn_manager_impl_test.cc | 19 +++++----- test/mocks/server/overload_manager.cc | 6 ++++ test/mocks/server/overload_manager.h | 13 ++++++- 13 files changed, 127 insertions(+), 77 deletions(-) diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 010ac8ee9468..e10812add8fd 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -33,25 +33,8 @@ using OverloadActionCb = std::function; */ class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject { 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_; + // Get a thread-local reference to the value for the given action key. + virtual const OverloadActionState& getState(const std::string& action) PURE; }; /** @@ -106,17 +89,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 fb9c36d5d730..4a76e84be282 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 4e8f18814b09..04d3896a3ff8 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -58,7 +58,7 @@ class ConnectionManagerImpl : Logger::Loggable, Random::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 33047d76d643..3f8abde5ae24 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())}); }; } @@ -588,8 +588,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 5b3d3b3c4255..227f0a92277e 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)); @@ -679,6 +680,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())), + null_overload_manager_(server_.threadLocal()), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats("http.admin.", no_op_store_)), route_config_provider_(server.timeSource()), @@ -760,11 +762,12 @@ 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. + // 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(), nullptr, 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 9c035d123e80..278aa30c342b 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -20,9 +20,11 @@ #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" +#include "common/common/assert.h" #include "common/common/basic_resource_impl.h" #include "common/common/empty_string.h" #include "common/common/logger.h" @@ -245,6 +247,39 @@ 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 NullThreadLocalOverloadState : public ThreadLocalOverloadState { + const OverloadActionState& getState(const std::string&) override { return inactive_; } + + const OverloadActionState inactive_ = OverloadActionState::Inactive; + }; + + 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 tls_->getTyped(); + } + + bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override { + // This method shouldn't be called by the admin listener + NOT_REACHED_GCOVR_EXCL_LINE; + return false; + } + + ThreadLocal::SlotPtr tls_; + }; + /** * Helper methods for the /clusters url handler. */ @@ -389,6 +424,7 @@ class AdminImpl : public Admin, std::list access_logs_; const std::string profile_path_; Http::ConnectionManagerStats stats_; + 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_; diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index ef56fc8d7fae..40156ed9b179 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -35,6 +35,25 @@ class ThresholdTriggerImpl : public OverloadAction::Trigger { absl::optional value_; }; +/** + * Thread-local copy of the state of each configured overload action. + */ +class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { +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) { actions_[action] = 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 +167,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 +210,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 +227,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/source/server/server.cc b/source/server/server.cc index e0f7157c54c6..80c48c279c06 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -382,23 +382,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()); @@ -428,6 +411,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 diff --git a/test/common/http/BUILD b/test/common/http/BUILD index eeee6f0d6a09..ec2987c48403 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -182,6 +182,7 @@ envoy_cc_fuzz_test( "//test/mocks/network:network_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 c563f72ec381..7c85051528ab 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -34,6 +34,7 @@ #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" @@ -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 a22aa5432296..efd7222b9123 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) { @@ -5601,11 +5601,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_); @@ -5631,10 +5632,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/overload_manager.cc b/test/mocks/server/overload_manager.cc index d105df80e690..d0fd9b545ec6 100644 --- a/test/mocks/server/overload_manager.cc +++ b/test/mocks/server/overload_manager.cc @@ -9,6 +9,12 @@ namespace Envoy { namespace Server { using ::testing::ReturnRef; + +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/overload_manager.h b/test/mocks/server/overload_manager.h index 8ce63cef1b12..86c194d5586d 100644 --- a/test/mocks/server/overload_manager.h +++ b/test/mocks/server/overload_manager.h @@ -8,6 +8,16 @@ namespace Envoy { namespace Server { + +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(); @@ -20,7 +30,8 @@ class MockOverloadManager : public OverloadManager { OverloadActionCb callback)); MOCK_METHOD(ThreadLocalOverloadState&, getThreadLocalOverloadState, ()); - ThreadLocalOverloadState overload_state_; + testing::NiceMock overload_state_; }; + } // namespace Server } // namespace Envoy