From 9edcf34854dcb5e643fc00771ad70f9fc48c9d57 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 20 Jan 2021 14:18:13 -0500 Subject: [PATCH 1/3] tcp: adding logs and debug checks Signed-off-by: Alyssa Wilk --- source/common/conn_pool/conn_pool_base.cc | 35 +++++++++++++++----- source/common/conn_pool/conn_pool_base.h | 25 ++++++++++++-- test/common/conn_pool/conn_pool_base_test.cc | 10 ++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index d94aafa389004..3c4e6d32f2e27 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -95,7 +95,8 @@ float ConnPoolImplBase::perUpstreamPreconnectRatio() const { } } -void ConnPoolImplBase::tryCreateNewConnections() { +ConnPoolImplBase::ConnectionResult ConnPoolImplBase::tryCreateNewConnections() { + ConnPoolImplBase::ConnectionResult result; // Somewhat arbitrarily cap the number of connections preconnected due to new // incoming connections. The preconnect ratio is capped at 3, so in steady // state, no more than 3 connections should be preconnected. If hosts go @@ -103,16 +104,19 @@ void ConnPoolImplBase::tryCreateNewConnections() { // many connections are desired when the host becomes healthy again, but // overwhelming it with connections is not desirable. for (int i = 0; i < 3; ++i) { - if (!tryCreateNewConnection()) { - return; + result = tryCreateNewConnection(); + if (result != ConnectionResult::CREATED_NEW_CONNECTION) { + break; } } + return result; } -bool ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { +ConnPoolImplBase::ConnectionResult +ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { // There are already enough CONNECTING connections for the number of queued streams. if (!shouldCreateNewConnection(global_preconnect_ratio)) { - return false; + return ConnectionResult::SHOULD_NOT_CONNECT; } const bool can_create_connection = @@ -135,8 +139,12 @@ bool ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { state_.incrConnectingStreamCapacity(client->effectiveConcurrentStreamLimit()); connecting_stream_capacity_ += client->effectiveConcurrentStreamLimit(); LinkedList::moveIntoList(std::move(client), owningList(client->state_)); + return can_create_connection ? ConnectionResult::CREATED_NEW_CONNECTION + : ConnectionResult::CREATED_BUT_RATE_LIMITED; + } else { + ENVOY_LOG(trace, "not creating a new connection: connection constrained"); + return ConnectionResult::NO_CREATION_RATE_LIMITED; } - return can_create_connection; } void ConnPoolImplBase::attachStreamToClient(Envoy::ConnectionPool::ActiveClient& client, @@ -218,10 +226,18 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) if (host_->cluster().resourceManager(priority_).pendingRequests().canCreate()) { ConnectionPool::Cancellable* pending = newPendingStream(context); + ENVOY_LOG(debug, "trying to create new connection"); + + auto old_capacity = connecting_stream_capacity_; // This must come after newPendingStream() because this function uses the // length of pending_streams_ to determine if a new connection is needed. - tryCreateNewConnections(); - + const ConnectionResult result = tryCreateNewConnections(); + // If there is not enough connecting capacity, the only reason to not + // increase capacity is if the connection limits are exceeded. + ENVOY_BUG(pending_streams_.size() <= connecting_stream_capacity_ || + connecting_stream_capacity_ > old_capacity || + result == ConnectionResult::NO_CREATION_RATE_LIMITED, + fmt::format("Failed to create expected connection: {}", *this)); return pending; } else { ENVOY_LOG(debug, "max pending streams overflow"); @@ -233,7 +249,8 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) } bool ConnPoolImplBase::maybePreconnect(float global_preconnect_ratio) { - return tryCreateNewConnection(global_preconnect_ratio); + return tryCreateNewConnection(global_preconnect_ratio) == + ConnectionResult::CREATED_NEW_CONNECTION; } void ConnPoolImplBase::scheduleOnUpstreamReady() { diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 38730c238e01c..bf7029f4c4cde 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -6,6 +6,7 @@ #include "envoy/stats/timespan.h" #include "envoy/upstream/cluster_manager.h" +#include "common/common/dump_state_utils.h" #include "common/common/linked_object.h" #include "absl/strings/string_view.h" @@ -192,18 +193,38 @@ class ConnPoolImplBase : protected Logger::Loggable { } bool hasPendingStreams() const { return !pending_streams_.empty(); } + void dumpState(std::ostream& os, int indent_level = 0) const { + const char* spaces = spacesForLevel(indent_level); + os << spaces << "ConnPoolImplBase " << this << DUMP_MEMBER(ready_clients_.size()) + << DUMP_MEMBER(busy_clients_.size()) << DUMP_MEMBER(connecting_clients_.size()) + << DUMP_MEMBER(connecting_stream_capacity_) << DUMP_MEMBER(num_active_streams_); + } + + friend std::ostream& operator<<(std::ostream& os, const ConnPoolImplBase& s) { + s.dumpState(os); + return os; + } + protected: // Creates up to 3 connections, based on the preconnect ratio. virtual void onConnected(Envoy::ConnectionPool::ActiveClient&) {} + enum class ConnectionResult { + CREATED_NEW_CONNECTION, + SHOULD_NOT_CONNECT, + NO_CREATION_RATE_LIMITED, + CREATED_BUT_RATE_LIMITED, + }; + // Creates up to 3 connections, based on the preconnect ratio. - void tryCreateNewConnections(); + // Returns the ConnectionResult of the last attempt. + ConnectionResult tryCreateNewConnections(); // Creates a new connection if there is sufficient demand, it is allowed by resourceManager, or // to avoid starving this pool. // Demand is determined either by perUpstreamPreconnectRatio() or global_preconnect_ratio // if this is called by maybePreconnect() - bool tryCreateNewConnection(float global_preconnect_ratio = 0); + ConnectionResult tryCreateNewConnection(float global_preconnect_ratio = 0); // A helper function which determines if a canceled pending connection should // be closed as excess or not. diff --git a/test/common/conn_pool/conn_pool_base_test.cc b/test/common/conn_pool/conn_pool_base_test.cc index d9981c7ad7bad..7e4693cacf259 100644 --- a/test/common/conn_pool/conn_pool_base_test.cc +++ b/test/common/conn_pool/conn_pool_base_test.cc @@ -12,6 +12,7 @@ namespace Envoy { namespace ConnectionPool { using testing::AnyNumber; +using testing::HasSubstr; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::Return; @@ -90,6 +91,15 @@ class ConnPoolImplBaseTest : public testing::Test { std::vector clients_; }; +TEST_F(ConnPoolImplBaseTest, DumpState) { + std::stringstream out; + pool_.dumpState(out, 0); + std::string state = out.str(); + EXPECT_THAT(state, HasSubstr("ready_clients_.size(): 0, busy_clients_.size(): 0, " + "connecting_clients_.size(): 0, connecting_stream_capacity_: 0, " + "num_active_streams_: 0")); +} + TEST_F(ConnPoolImplBaseTest, BasicPreconnect) { // Create more than one connection per new stream. ON_CALL(*cluster_, perUpstreamPreconnectRatio).WillByDefault(Return(1.5)); From a7caf30101527d6a8146a18cfd72b342ee04cb05 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 20 Jan 2021 15:39:12 -0500 Subject: [PATCH 2/3] tidy Signed-off-by: Alyssa Wilk --- source/common/conn_pool/conn_pool_base.cc | 14 +++++++------- source/common/conn_pool/conn_pool_base.h | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index 3c4e6d32f2e27..f9ec27305f255 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -105,7 +105,7 @@ ConnPoolImplBase::ConnectionResult ConnPoolImplBase::tryCreateNewConnections() { // overwhelming it with connections is not desirable. for (int i = 0; i < 3; ++i) { result = tryCreateNewConnection(); - if (result != ConnectionResult::CREATED_NEW_CONNECTION) { + if (result != ConnectionResult::CreatedNewConnection) { break; } } @@ -116,7 +116,7 @@ ConnPoolImplBase::ConnectionResult ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { // There are already enough CONNECTING connections for the number of queued streams. if (!shouldCreateNewConnection(global_preconnect_ratio)) { - return ConnectionResult::SHOULD_NOT_CONNECT; + return ConnectionResult::ShouldNotConnect; } const bool can_create_connection = @@ -139,11 +139,11 @@ ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { state_.incrConnectingStreamCapacity(client->effectiveConcurrentStreamLimit()); connecting_stream_capacity_ += client->effectiveConcurrentStreamLimit(); LinkedList::moveIntoList(std::move(client), owningList(client->state_)); - return can_create_connection ? ConnectionResult::CREATED_NEW_CONNECTION - : ConnectionResult::CREATED_BUT_RATE_LIMITED; + return can_create_connection ? ConnectionResult::CreatedNewConnection + : ConnectionResult::CreatedButRateLimited; } else { ENVOY_LOG(trace, "not creating a new connection: connection constrained"); - return ConnectionResult::NO_CREATION_RATE_LIMITED; + return ConnectionResult::NoConnectionRateLimited; } } @@ -236,7 +236,7 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) // increase capacity is if the connection limits are exceeded. ENVOY_BUG(pending_streams_.size() <= connecting_stream_capacity_ || connecting_stream_capacity_ > old_capacity || - result == ConnectionResult::NO_CREATION_RATE_LIMITED, + result == ConnectionResult::NoConnectionRateLimited, fmt::format("Failed to create expected connection: {}", *this)); return pending; } else { @@ -250,7 +250,7 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) bool ConnPoolImplBase::maybePreconnect(float global_preconnect_ratio) { return tryCreateNewConnection(global_preconnect_ratio) == - ConnectionResult::CREATED_NEW_CONNECTION; + ConnectionResult::CreatedNewConnection; } void ConnPoolImplBase::scheduleOnUpstreamReady() { diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index bf7029f4c4cde..ac8da17e980a9 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -210,10 +210,10 @@ class ConnPoolImplBase : protected Logger::Loggable { virtual void onConnected(Envoy::ConnectionPool::ActiveClient&) {} enum class ConnectionResult { - CREATED_NEW_CONNECTION, - SHOULD_NOT_CONNECT, - NO_CREATION_RATE_LIMITED, - CREATED_BUT_RATE_LIMITED, + CreatedNewConnection, + ShouldNotConnect, + NoConnectionRateLimited, + CreatedButRateLimited, }; // Creates up to 3 connections, based on the preconnect ratio. From 435ef1b1969d54094462999f6ec1c2f820027130 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 20 Jan 2021 15:55:05 -0500 Subject: [PATCH 3/3] format Signed-off-by: Alyssa Wilk --- source/common/conn_pool/conn_pool_base.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index f9ec27305f255..06b82d8eb5e46 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -249,8 +249,7 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) } bool ConnPoolImplBase::maybePreconnect(float global_preconnect_ratio) { - return tryCreateNewConnection(global_preconnect_ratio) == - ConnectionResult::CreatedNewConnection; + return tryCreateNewConnection(global_preconnect_ratio) == ConnectionResult::CreatedNewConnection; } void ConnPoolImplBase::scheduleOnUpstreamReady() {