diff --git a/envoy/common/conn_pool.h b/envoy/common/conn_pool.h index 17cf77eca6824..8fd7f1061bf95 100644 --- a/envoy/common/conn_pool.h +++ b/envoy/common/conn_pool.h @@ -36,6 +36,21 @@ class Cancellable { virtual void cancel(CancelPolicy cancel_policy) PURE; }; +/** + * Controls the behavior when draining a connection pool. + */ +enum class DrainBehavior { + // Starts draining a pool, by gracefully completing all requests and gracefully closing all + // connections, in preparation for deletion. It is invalid to create new streams or + // connections from this pool after draining a pool with this behavior. + DrainAndDelete, + // Actively drain all existing connection pool connections. This can be used in cases where + // the connection pool is not being destroyed, but the caller wishes to make sure that + // all new streams take place on a new connection. For example, when a health check failure + // occurs. + DrainExistingConnections, +}; + /** * An instance of a generic connection pool. */ @@ -59,20 +74,10 @@ class Instance { virtual bool isIdle() const PURE; /** - * Starts draining a pool, by gracefully completing all requests and gracefully closing all - * connections, in preparation for deletion. When the process completes, the function registered - * via `addIdleCallback()` is called. The callback may occur before this call returns if the pool - * can be immediately drained. - */ - virtual void startDrain() PURE; - - /** - * Actively drain all existing connection pool connections. This method can be used in cases - * where the connection pool is not being destroyed, but the caller wishes to make sure that - * all new streams take place on a new connection. For example, when a health check failure - * occurs. + * Drains the connections in a pool. + * @param drain_behavior A DrainBehavior that controls the behavior of the draining. */ - virtual void drainConnections() PURE; + virtual void drainConnections(DrainBehavior drain_behavior) PURE; /** * @return Upstream::HostDescriptionConstSharedPtr the host for which connections are pooled. diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index e91cc10709790..113836ab13648 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -229,6 +229,7 @@ void ConnPoolImplBase::onStreamClosed(Envoy::ConnectionPool::ActiveClient& clien } ConnectionPool::Cancellable* ConnPoolImplBase::newStreamImpl(AttachContext& context) { + ASSERT(!is_draining_for_deletion_); ASSERT(!deferred_deleting_); ASSERT(static_cast(connecting_stream_capacity_) == @@ -331,11 +332,6 @@ void ConnPoolImplBase::transitionActiveClientState(ActiveClient& client, void ConnPoolImplBase::addIdleCallbackImpl(Instance::IdleCb cb) { idle_callbacks_.push_back(cb); } -void ConnPoolImplBase::startDrainImpl() { - is_draining_ = true; - checkForIdleAndCloseIdleConnsIfDraining(); -} - void ConnPoolImplBase::closeIdleConnectionsForDrainingPool() { // Create a separate list of elements to close to avoid mutate-while-iterating problems. std::list to_close; @@ -359,7 +355,12 @@ void ConnPoolImplBase::closeIdleConnectionsForDrainingPool() { } } -void ConnPoolImplBase::drainConnectionsImpl() { +void ConnPoolImplBase::drainConnectionsImpl(DrainBehavior drain_behavior) { + if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) { + is_draining_for_deletion_ = true; + checkForIdleAndCloseIdleConnsIfDraining(); + return; + } closeIdleConnectionsForDrainingPool(); // closeIdleConnections() closes all connections in ready_clients_ with no active streams, @@ -387,12 +388,13 @@ bool ConnPoolImplBase::isIdleImpl() const { } void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() { - if (is_draining_) { + if (is_draining_for_deletion_) { closeIdleConnectionsForDrainingPool(); } if (isIdleImpl()) { - ENVOY_LOG(debug, "invoking idle callbacks - is_draining_={}", is_draining_); + ENVOY_LOG(debug, "invoking idle callbacks - is_draining_for_deletion_={}", + is_draining_for_deletion_); for (const Instance::IdleCb& cb : idle_callbacks_) { cb(); } diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 5ce4d9936feef..dafc845f88915 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -166,8 +166,7 @@ class ConnPoolImplBase : protected Logger::Loggable { void addIdleCallbackImpl(Instance::IdleCb cb); // Returns true if the pool is idle. bool isIdleImpl() const; - void startDrainImpl(); - void drainConnectionsImpl(); + void drainConnectionsImpl(DrainBehavior drain_behavior); const Upstream::HostConstSharedPtr& host() const { return host_; } // Called if this pool is likely to be picked soon, to determine if it's worth preconnecting. bool maybePreconnectImpl(float global_preconnect_ratio); @@ -335,7 +334,7 @@ class ConnPoolImplBase : protected Logger::Loggable { // Whether the connection pool is currently in the process of closing // all connections so that it can be gracefully deleted. - bool is_draining_{false}; + bool is_draining_for_deletion_{false}; // True iff this object is in the deferred delete list. bool deferred_deleting_{false}; diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h index e14a6ffa2ae3a..47bdfdad7f3f2 100644 --- a/source/common/http/conn_pool_base.h +++ b/source/common/http/conn_pool_base.h @@ -61,8 +61,9 @@ class HttpConnPoolImplBase : public Envoy::ConnectionPool::ConnPoolImplBase, // ConnectionPool::Instance void addIdleCallback(IdleCb cb) override { addIdleCallbackImpl(cb); } bool isIdle() const override { return isIdleImpl(); } - void startDrain() override { startDrainImpl(); } - void drainConnections() override { drainConnectionsImpl(); } + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) override { + drainConnectionsImpl(drain_behavior); + } Upstream::HostDescriptionConstSharedPtr host() const override { return host_; } ConnectionPool::Cancellable* newStream(Http::ResponseDecoder& response_decoder, Http::ConnectionPool::Callbacks& callbacks) override; diff --git a/source/common/http/conn_pool_grid.cc b/source/common/http/conn_pool_grid.cc index f5e38a1a9e2ef..bb70fad9978c9 100644 --- a/source/common/http/conn_pool_grid.cc +++ b/source/common/http/conn_pool_grid.cc @@ -296,24 +296,20 @@ void ConnectivityGrid::addIdleCallback(IdleCb cb) { idle_callbacks_.emplace_back(cb); } -void ConnectivityGrid::startDrain() { +void ConnectivityGrid::drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) { if (draining_) { // A drain callback has already been set, and only needs to happen once. return; } - // Note that no new pools can be created from this point on - // as createNextPool fast-fails if `draining_` is true. - draining_ = true; - - for (auto& pool : pools_) { - pool->startDrain(); + if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) { + // Note that no new pools can be created from this point on + // as createNextPool fast-fails if `draining_` is true. + draining_ = true; } -} -void ConnectivityGrid::drainConnections() { for (auto& pool : pools_) { - pool->drainConnections(); + pool->drainConnections(drain_behavior); } } diff --git a/source/common/http/conn_pool_grid.h b/source/common/http/conn_pool_grid.h index c9b01a74e1622..2c4832067f6ad 100644 --- a/source/common/http/conn_pool_grid.h +++ b/source/common/http/conn_pool_grid.h @@ -148,8 +148,7 @@ class ConnectivityGrid : public ConnectionPool::Instance, ConnectionPool::Callbacks& callbacks) override; void addIdleCallback(IdleCb cb) override; bool isIdle() const override; - void startDrain() override; - void drainConnections() override; + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) override; Upstream::HostDescriptionConstSharedPtr host() const override; bool maybePreconnect(float preconnect_ratio) override; absl::string_view protocolDescription() const override { return "connection grid"; } diff --git a/source/common/tcp/conn_pool.h b/source/common/tcp/conn_pool.h index 949feeb7ee7a2..d3a4a6d81d37e 100644 --- a/source/common/tcp/conn_pool.h +++ b/source/common/tcp/conn_pool.h @@ -150,9 +150,11 @@ class ConnPoolImpl : public Envoy::ConnectionPool::ConnPoolImplBase, void addIdleCallback(IdleCb cb) override { addIdleCallbackImpl(cb); } bool isIdle() const override { return isIdleImpl(); } - void startDrain() override { startDrainImpl(); } - void drainConnections() override { - drainConnectionsImpl(); + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) override { + drainConnectionsImpl(drain_behavior); + if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) { + return; + } // Legacy behavior for the TCP connection pool marks all connecting clients // as draining. for (auto& connecting_client : connecting_clients_) { diff --git a/source/common/tcp/original_conn_pool.cc b/source/common/tcp/original_conn_pool.cc index 325c424aa61c6..7321d3723a5c6 100644 --- a/source/common/tcp/original_conn_pool.cc +++ b/source/common/tcp/original_conn_pool.cc @@ -37,7 +37,13 @@ OriginalConnPoolImpl::~OriginalConnPoolImpl() { dispatcher_.clearDeferredDeleteList(); } -void OriginalConnPoolImpl::drainConnections() { +void OriginalConnPoolImpl::drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) { + if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) { + is_draining_ = true; + checkForIdleAndCloseIdleConnsIfDraining(); + return; + } + ENVOY_LOG(debug, "draining connections"); while (!ready_conns_.empty()) { ready_conns_.front()->conn_->close(Network::ConnectionCloseType::NoFlush); @@ -70,11 +76,6 @@ void OriginalConnPoolImpl::closeConnections() { void OriginalConnPoolImpl::addIdleCallback(IdleCb cb) { idle_callbacks_.push_back(cb); } -void OriginalConnPoolImpl::startDrain() { - is_draining_ = true; - checkForIdleAndCloseIdleConnsIfDraining(); -} - void OriginalConnPoolImpl::assignConnection(ActiveConn& conn, ConnectionPool::Callbacks& callbacks) { ASSERT(conn.wrapper_ == nullptr); diff --git a/source/common/tcp/original_conn_pool.h b/source/common/tcp/original_conn_pool.h index d5e79580c5b46..8e4d07a1f9e2c 100644 --- a/source/common/tcp/original_conn_pool.h +++ b/source/common/tcp/original_conn_pool.h @@ -34,8 +34,7 @@ class OriginalConnPoolImpl : Logger::Loggable, public Connecti // ConnectionPool::Instance void addIdleCallback(IdleCb cb) override; bool isIdle() const override; - void startDrain() override; - void drainConnections() override; + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) override; void closeConnections() override; ConnectionPool::Cancellable* newConnection(ConnectionPool::Callbacks& callbacks) override; // The old pool does not implement preconnecting. diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index f2c9ff1271b99..cbe3507907f5d 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -1197,7 +1197,7 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools( // guarding deletion with `do_not_delete_` in the registered idle callback, and then checking // afterwards whether it is empty and deleting it if necessary. container.do_not_delete_ = true; - pools->startDrain(); + pools->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); container.do_not_delete_ = false; if (container.pools_->size() == 0) { @@ -1217,7 +1217,7 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainTcpConnPools( container.draining_ = true; for (auto pool : pools) { - pool->startDrain(); + pool->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); } } @@ -1389,7 +1389,8 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainAllConnPoolsWorker( const auto container = getHttpConnPoolsContainer(host); if (container != nullptr) { container->do_not_delete_ = true; - container->pools_->drainConnections(); + container->pools_->drainConnections( + Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); container->do_not_delete_ = false; if (container->pools_->size() == 0) { @@ -1417,7 +1418,7 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainAllConnPoolsWorker( ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE) { pool->closeConnections(); } else { - pool->drainConnections(); + pool->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } } } diff --git a/source/common/upstream/conn_pool_map.h b/source/common/upstream/conn_pool_map.h index 4d6a557d7e680..b3840c3600cd6 100644 --- a/source/common/upstream/conn_pool_map.h +++ b/source/common/upstream/conn_pool_map.h @@ -3,6 +3,7 @@ #include #include +#include "envoy/common/conn_pool.h" #include "envoy/event/dispatcher.h" #include "envoy/upstream/resource_manager.h" #include "envoy/upstream/upstream.h" @@ -58,15 +59,10 @@ template class ConnPoolMap { */ void addIdleCallback(const IdleCb& cb); - /** - * See `Envoy::ConnectionPool::Instance::startDrain()`. - */ - void startDrain(); - /** * See `Envoy::ConnectionPool::Instance::drainConnections()`. */ - void drainConnections(); + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior); private: /** diff --git a/source/common/upstream/conn_pool_map_impl.h b/source/common/upstream/conn_pool_map_impl.h index 18176d0a18ec8..63db84a047412 100644 --- a/source/common/upstream/conn_pool_map_impl.h +++ b/source/common/upstream/conn_pool_map_impl.h @@ -99,7 +99,8 @@ void ConnPoolMap::addIdleCallback(const IdleCb& cb) { } template -void ConnPoolMap::startDrain() { +void ConnPoolMap::drainConnections( + Envoy::ConnectionPool::DrainBehavior drain_behavior) { // Copy the `active_pools_` so that it is safe for the call to result // in deletion, and avoid iteration through a mutating container. std::vector pools; @@ -109,22 +110,7 @@ void ConnPoolMap::startDrain() { } for (auto* pool : pools) { - pool->startDrain(); - } -} - -template -void ConnPoolMap::drainConnections() { - // Copy the `active_pools_` so that it is safe for the call to result - // in deletion, and avoid iteration through a mutating container. - std::vector pools; - pools.reserve(active_pools_.size()); - for (auto& pool_pair : active_pools_) { - pools.push_back(pool_pair.second.get()); - } - - for (auto* pool : pools) { - pool->drainConnections(); + pool->drainConnections(drain_behavior); } } diff --git a/source/common/upstream/priority_conn_pool_map.h b/source/common/upstream/priority_conn_pool_map.h index c43ba46c06ea5..d3c3c66bd4714 100644 --- a/source/common/upstream/priority_conn_pool_map.h +++ b/source/common/upstream/priority_conn_pool_map.h @@ -51,15 +51,10 @@ template class PriorityConnPoolMap { */ void addIdleCallback(const IdleCb& cb); - /** - * See `Envoy::ConnectionPool::Instance::startDrain()`. - */ - void startDrain(); - /** * See `Envoy::ConnectionPool::Instance::drainConnections()`. */ - void drainConnections(); + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior); private: size_t getPriorityIndex(ResourcePriority priority) const; diff --git a/source/common/upstream/priority_conn_pool_map_impl.h b/source/common/upstream/priority_conn_pool_map_impl.h index 66cc9ff4407ea..a706938b8e182 100644 --- a/source/common/upstream/priority_conn_pool_map_impl.h +++ b/source/common/upstream/priority_conn_pool_map_impl.h @@ -55,16 +55,10 @@ void PriorityConnPoolMap::addIdleCallback(const IdleCb& cb) } template -void PriorityConnPoolMap::startDrain() { +void PriorityConnPoolMap::drainConnections( + ConnectionPool::DrainBehavior drain_behavior) { for (auto& pool_map : conn_pool_maps_) { - pool_map->startDrain(); - } -} - -template -void PriorityConnPoolMap::drainConnections() { - for (auto& pool_map : conn_pool_maps_) { - pool_map->drainConnections(); + pool_map->drainConnections(drain_behavior); } } diff --git a/test/common/conn_pool/conn_pool_base_test.cc b/test/common/conn_pool/conn_pool_base_test.cc index 869dafa68a678..17dcfe7e8473a 100644 --- a/test/common/conn_pool/conn_pool_base_test.cc +++ b/test/common/conn_pool/conn_pool_base_test.cc @@ -227,7 +227,7 @@ TEST_F(ConnPoolImplBaseTest, PoolIdleCallbackTriggeredRemoteClose) { clients_.back()->onEvent(Network::ConnectionEvent::RemoteClose); EXPECT_CALL(idle_pool_callback, Call()); - pool_.startDrainImpl(); + pool_.drainConnectionsImpl(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); } // Local close simulates what would happen for an idle timeout on a connection. @@ -255,7 +255,7 @@ TEST_F(ConnPoolImplBaseTest, PoolIdleCallbackTriggeredLocalClose) { clients_.back()->onEvent(Network::ConnectionEvent::LocalClose); EXPECT_CALL(idle_pool_callback, Call()); - pool_.startDrainImpl(); + pool_.drainConnectionsImpl(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); } } // namespace ConnectionPool diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 908e82bf1282b..873a813be4501 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -382,20 +382,23 @@ TEST_F(ConnectivityGridTest, TestCancel) { // Make sure drains get sent to all active pools. TEST_F(ConnectivityGridTest, Drain) { - grid_.drainConnections(); + grid_.drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); // Synthetically create a pool. grid_.createNextPool(); { - EXPECT_CALL(*grid_.first(), drainConnections()); - grid_.drainConnections(); + EXPECT_CALL(*grid_.first(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + grid_.drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } grid_.createNextPool(); { - EXPECT_CALL(*grid_.first(), drainConnections()); - EXPECT_CALL(*grid_.second(), drainConnections()); - grid_.drainConnections(); + EXPECT_CALL(*grid_.first(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + EXPECT_CALL(*grid_.second(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + grid_.drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } } @@ -411,16 +414,22 @@ TEST_F(ConnectivityGridTest, DrainCallbacks) { // The first time a drain is started, both pools should start draining. { - EXPECT_CALL(*grid_.first(), startDrain()); - EXPECT_CALL(*grid_.second(), startDrain()); - grid_.startDrain(); + EXPECT_CALL(*grid_.first(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)); + EXPECT_CALL(*grid_.second(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)); + grid_.drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); } // The second time, the pools will not see any change. { - EXPECT_CALL(*grid_.first(), startDrain()).Times(0); - EXPECT_CALL(*grid_.second(), startDrain()).Times(0); - grid_.startDrain(); + EXPECT_CALL(*grid_.first(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .Times(0); + EXPECT_CALL(*grid_.second(), + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .Times(0); + grid_.drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); } { // Notify the grid the second pool has been drained. This should not be @@ -481,7 +490,7 @@ TEST_F(ConnectivityGridTest, NoDrainOnTeardown) { { grid_.addIdleCallback([&drain_received]() -> void { drain_received = true; }); - grid_.startDrain(); + grid_.drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); } grid_.setDestroying(); // Fake being in the destructor. diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index f603967b14c97..75b124986a808 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -261,7 +261,7 @@ TEST_F(Http1ConnPoolImplTest, DrainConnections) { r1.completeResponse(false); // This will destroy the ready client and set requests remaining to 1 on the busy client. - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); conn_pool_->expectAndRunUpstreamReady(); @@ -951,7 +951,7 @@ TEST_F(Http1ConnPoolImplTest, DrainCallback) { ActiveTestRequest r2(*this, 0, ActiveTestRequest::Type::Pending); conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); - conn_pool_->startDrain(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); r2.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::Default); EXPECT_EQ(1U, cluster_->stats_.upstream_rq_total_.value()); @@ -978,7 +978,7 @@ TEST_F(Http1ConnPoolImplTest, DrainWhileConnecting) { EXPECT_NE(nullptr, handle); conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); - conn_pool_->startDrain(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(*conn_pool_->test_clients_[0].connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_CALL(drained, ready()).Times(AtLeast(1)); @@ -1046,7 +1046,7 @@ TEST_F(Http1ConnPoolImplTest, ActiveRequestHasActiveConnectionsTrue) { // cleanup conn_pool_->expectEnableUpstreamReady(); r1.completeResponse(false); - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); conn_pool_->expectAndRunUpstreamReady(); @@ -1060,7 +1060,7 @@ TEST_F(Http1ConnPoolImplTest, ResponseCompletedConnectionReadyNoActiveConnection EXPECT_FALSE(conn_pool_->hasActiveConnections()); - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); conn_pool_->expectAndRunUpstreamReady(); @@ -1075,7 +1075,7 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { EXPECT_CALL(*conn_pool_, onClientDestroy()); r1.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::Default); EXPECT_EQ(0U, cluster_->stats_.upstream_rq_total_.value()); - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); dispatcher_.clearDeferredDeleteList(); diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index 4f1b8e6f4bbec..7a1c272a1796c 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -320,7 +320,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionIdle) { completeRequest(r); EXPECT_CALL(*this, onClientDestroy()); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } /** @@ -389,7 +389,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionReadyWithRequest) { ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) .ok()); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_CALL(r.decoder_, decodeHeaders_(_, true)); EXPECT_CALL(*this, onClientDestroy()); @@ -414,7 +414,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionBusy) { ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) .ok()); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_CALL(r.decoder_, decodeHeaders_(_, true)); EXPECT_CALL(*this, onClientDestroy()); @@ -434,12 +434,12 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionConnecting) { ActiveTestRequest r(*this, 0, false); // Pending request prevents the connection from being drained - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); // Cancel the pending request, and then the connection can be closed. r.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::Default); EXPECT_CALL(*this, onClientDestroy()); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } /** @@ -452,7 +452,7 @@ TEST_F(Http2ConnPoolImplTest, CloseExcess) { ActiveTestRequest r(*this, 0, false); // Pending request prevents the connection from being drained - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_CALL(*this, onClientDestroy()); r.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); @@ -593,7 +593,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnections) { cluster_->max_requests_per_connection_ = 1; // Test drain connections call prior to any connections being created. - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); expectClientCreate(); ActiveTestRequest r1(*this, 0, false); @@ -616,7 +616,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnections) { .ok()); // This will move the second connection to draining. - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); // This will destroy the 2 draining connections. test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -1091,7 +1091,7 @@ TEST_F(Http2ConnPoolImplTest, DrainDisconnectWithActiveRequest) { .ok()); ReadyWatcher drained; pool_->addIdleCallback([&]() -> void { drained.ready(); }); - pool_->startDrain(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(drained, ready()); @@ -1128,7 +1128,7 @@ TEST_F(Http2ConnPoolImplTest, DrainDisconnectDrainingWithActiveRequest) { ReadyWatcher drained; pool_->addIdleCallback([&]() -> void { drained.ready(); }); - pool_->startDrain(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); @@ -1172,7 +1172,7 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimary) { ReadyWatcher drained; pool_->addIdleCallback([&]() -> void { drained.ready(); }); - pool_->startDrain(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); @@ -1228,7 +1228,7 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimaryNoActiveRequest) { ReadyWatcher drained; EXPECT_CALL(drained, ready()); pool_->addIdleCallback([&]() -> void { drained.ready(); }); - pool_->startDrain(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(*this, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); @@ -1382,7 +1382,7 @@ TEST_F(Http2ConnPoolImplTest, DrainingConnectionsConsideredActive) { expectClientCreate(); ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); EXPECT_TRUE(pool_->hasActiveConnections()); @@ -1396,7 +1396,7 @@ TEST_F(Http2ConnPoolImplTest, DrainedConnectionsNotActive) { expectClientCreate(); ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); completeRequest(r1); EXPECT_FALSE(pool_->hasActiveConnections()); @@ -1429,7 +1429,7 @@ TEST_F(Http2ConnPoolImplTest, PreconnectWithoutMultiplexing) { r1.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); r2.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); r3.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1483,7 +1483,7 @@ TEST_F(Http2ConnPoolImplTest, PreconnectWithMultiplexing) { // Clean up. r1.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); r2.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1527,7 +1527,7 @@ TEST_F(Http2ConnPoolImplTest, PreconnectWithSettings) { CHECK_STATE(2 /*active*/, 0 /*pending*/, 0 /*capacity*/); // Clean up. - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1553,7 +1553,7 @@ TEST_F(Http2ConnPoolImplTest, PreconnectWithGoaway) { CHECK_STATE(1 /*active*/, 0 /*pending*/, 0 /*capacity*/); // Clean up. - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1579,7 +1579,7 @@ TEST_F(Http2ConnPoolImplTest, PreconnectEvenWhenReady) { // Clean up. completeRequest(r1); completeRequest(r2); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1600,7 +1600,7 @@ TEST_F(Http2ConnPoolImplTest, PreconnectAfterTimeout) { // Clean up. completeRequest(r1); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1625,7 +1625,7 @@ TEST_F(Http2ConnPoolImplTest, CloseExcessWithPreconnect) { // Clean up. r1.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1638,7 +1638,7 @@ TEST_F(Http2ConnPoolImplTest, MaybePreconnect) { expectClientsCreate(1); EXPECT_TRUE(pool_->maybePreconnect(2)); - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); } @@ -1675,7 +1675,7 @@ TEST_F(Http2ConnPoolImplTest, TestStateWithMultiplexing) { CHECK_STATE(0 /*active*/, 0 /*pending*/, 1 /*capacity*/); // Clean up with an outstanding stream. - pool_->drainConnections(); + pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); closeAllClients(); CHECK_STATE(0 /*active*/, 0 /*pending*/, 0 /*capacity*/); } diff --git a/test/common/http/mixed_conn_pool_test.cc b/test/common/http/mixed_conn_pool_test.cc index 3f22bfc5b876c..90b0fc2cee33d 100644 --- a/test/common/http/mixed_conn_pool_test.cc +++ b/test/common/http/mixed_conn_pool_test.cc @@ -86,7 +86,7 @@ void MixedConnPoolImplTest::testAlpnHandshake(absl::optional protocol) EXPECT_EQ(protocol.value(), conn_pool_->protocol()); } - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); connection->raiseEvent(Network::ConnectionEvent::RemoteClose); dispatcher_.clearDeferredDeleteList(); conn_pool_.reset(); diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index de4f489e05f46..04178b7207b38 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -107,8 +107,9 @@ class ConnPoolBase : public Tcp::ConnectionPool::Instance { void addIdleCallback(IdleCb cb) override { conn_pool_->addIdleCallback(cb); } bool isIdle() const override { return conn_pool_->isIdle(); } - void startDrain() override { return conn_pool_->startDrain(); } - void drainConnections() override { conn_pool_->drainConnections(); } + void drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) override { + conn_pool_->drainConnections(drain_behavior); + } void closeConnections() override { conn_pool_->closeConnections(); } ConnectionPool::Cancellable* newConnection(Tcp::ConnectionPool::Callbacks& callbacks) override { return conn_pool_->newConnection(callbacks); @@ -442,7 +443,7 @@ TEST_P(TcpConnPoolImplTest, DrainConnections) { // This will destroy the ready connection and set requests remaining to 1 on the busy and // pending connections. EXPECT_CALL(*conn_pool_, onConnDestroyedForTest()); - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); dispatcher_.clearDeferredDeleteList(); } { @@ -975,12 +976,11 @@ TEST_P(TcpConnPoolImplTest, ConnectionStateWithConcurrentConnections) { TEST_P(TcpConnPoolImplTest, DrainCallback) { initialize(); ReadyWatcher drained; - EXPECT_CALL(drained, ready()); conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); - conn_pool_->startDrain(); ActiveTestConn c1(*this, 0, ActiveTestConn::Type::CreateConnection); ActiveTestConn c2(*this, 0, ActiveTestConn::Type::Pending); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); c2.handle_->cancel(ConnectionPool::CancelPolicy::Default); EXPECT_CALL(*conn_pool_, onConnReleasedForTest()); @@ -1005,7 +1005,7 @@ TEST_P(TcpConnPoolImplTest, DrainWhileConnecting) { EXPECT_NE(nullptr, handle); conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); - conn_pool_->startDrain(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); if (test_new_connection_pool_) { // The shared connection pool removes and closes connecting clients if there are no @@ -1029,12 +1029,11 @@ TEST_P(TcpConnPoolImplTest, DrainWhileConnecting) { TEST_P(TcpConnPoolImplTest, DrainOnClose) { initialize(); ReadyWatcher drained; - EXPECT_CALL(drained, ready()); conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); - conn_pool_->startDrain(); - ActiveTestConn c1(*this, 0, ActiveTestConn::Type::CreateConnection); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); EXPECT_CALL(c1.callbacks_.callbacks_, onEvent(Network::ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](Network::ConnectionEvent event) -> void { @@ -1077,7 +1076,7 @@ TEST_P(TcpConnPoolImplTest, RequestCapacity) { // This should set the number of requests remaining to 1 on the active // connections, and the connecting_request_capacity to 2 as well. - conn_pool_->drainConnections(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); // Cancel the connections. Because neither used CloseExcess, the two connections should persist. handle1->cancel(ConnectionPool::CancelPolicy::Default); @@ -1130,7 +1129,7 @@ TEST_P(TcpConnPoolImplTest, TestIdleTimeout) { testing::MockFunction drained_callback; EXPECT_CALL(idle_callback, Call()); - conn_pool_->startDrain(); + conn_pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(*conn_pool_, onConnDestroyedForTest()); dispatcher_.clearDeferredDeleteList(); } diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 652fdac6ba909..480be4477bd92 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1813,8 +1813,8 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { // Now remove the cluster. This should drain the connection pools, but not affect // tcp connections. EXPECT_CALL(*callbacks, onClusterRemoval(_)); - EXPECT_CALL(*cp, startDrain()); - EXPECT_CALL(*cp2, startDrain()); + EXPECT_CALL(*cp, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)); + EXPECT_CALL(*cp2, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)); EXPECT_TRUE(cluster_manager_->removeCluster("fake_cluster")); EXPECT_EQ(nullptr, cluster_manager_->getThreadLocalCluster("fake_cluster")); EXPECT_EQ(0UL, cluster_manager_->clusters().active_clusters_.size()); @@ -1953,7 +1953,8 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { outlier_detector.runCallbacks(test_host); health_checker.runCallbacks(test_host, HealthTransition::Unchanged); - EXPECT_CALL(*cp1, drainConnections()); + EXPECT_CALL(*cp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); test_host->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK); outlier_detector.runCallbacks(test_host); @@ -1963,8 +1964,10 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { } // Order of these calls is implementation dependent, so can't sequence them! - EXPECT_CALL(*cp1, drainConnections()); - EXPECT_CALL(*cp2, drainConnections()); + EXPECT_CALL(*cp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + EXPECT_CALL(*cp2, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); test_host->healthFlagSet(Host::HealthFlag::FAILED_ACTIVE_HC); health_checker.runCallbacks(test_host, HealthTransition::Changed); @@ -2017,7 +2020,9 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsAndDeletePoolOnHealthFailure) outlier_detector.runCallbacks(test_host); health_checker.runCallbacks(test_host, HealthTransition::Unchanged); - EXPECT_CALL(*cp1, drainConnections()).WillOnce(Invoke([&]() { cp1->idle_cb_(); })); + EXPECT_CALL(*cp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)) + .WillOnce(Invoke([&]() { cp1->idle_cb_(); })); test_host->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK); outlier_detector.runCallbacks(test_host); @@ -2064,7 +2069,8 @@ TEST_F(ClusterManagerImplTest, CloseTcpConnectionPoolsOnHealthFailure) { outlier_detector.runCallbacks(test_host); health_checker.runCallbacks(test_host, HealthTransition::Unchanged); - EXPECT_CALL(*cp1, drainConnections()); + EXPECT_CALL(*cp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); test_host->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK); outlier_detector.runCallbacks(test_host); @@ -2074,8 +2080,10 @@ TEST_F(ClusterManagerImplTest, CloseTcpConnectionPoolsOnHealthFailure) { } // Order of these calls is implementation dependent, so can't sequence them! - EXPECT_CALL(*cp1, drainConnections()); - EXPECT_CALL(*cp2, drainConnections()); + EXPECT_CALL(*cp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + EXPECT_CALL(*cp2, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); test_host->healthFlagSet(Host::HealthFlag::FAILED_ACTIVE_HC); health_checker.runCallbacks(test_host, HealthTransition::Changed); @@ -3000,14 +3008,16 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { ->tcpConnPool(ResourcePriority::Default, nullptr)); // Immediate drain, since this can happen with the HTTP codecs. - EXPECT_CALL(*cp, startDrain()).WillOnce(Invoke([&]() { - cp->idle_cb_(); - cp->idle_cb_ = nullptr; - })); - EXPECT_CALL(*tcp, startDrain()).WillOnce(Invoke([&]() { - tcp->idle_cb_(); - tcp->idle_cb_ = nullptr; - })); + EXPECT_CALL(*cp, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + cp->idle_cb_(); + cp->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + tcp->idle_cb_(); + tcp->idle_cb_ = nullptr; + })); // Remove the first host, this should lead to the cp being drained, without // crash. @@ -3079,13 +3089,13 @@ TEST_F(ClusterManagerImplTest, ConnPoolDestroyWithDraining) { Http::ConnectionPool::Instance::IdleCb drained_cb; EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(mock_cp)); EXPECT_CALL(*mock_cp, addIdleCallback(_)).WillOnce(SaveArg<0>(&drained_cb)); - EXPECT_CALL(*mock_cp, startDrain()); + EXPECT_CALL(*mock_cp, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)); MockTcpConnPoolWithDestroy* mock_tcp = new NiceMock(); Tcp::ConnectionPool::Instance::IdleCb tcp_drained_cb; EXPECT_CALL(factory_, allocateTcpConnPool_).WillOnce(Return(mock_tcp)); EXPECT_CALL(*mock_tcp, addIdleCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb)); - EXPECT_CALL(*mock_tcp, startDrain()); + EXPECT_CALL(*mock_tcp, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)); HttpPoolDataPeer::getPool( cluster_manager_->getThreadLocalCluster("cluster_1") @@ -4647,22 +4657,26 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { EXPECT_NE(cp1, cp2); EXPECT_NE(tcp1, tcp2); - EXPECT_CALL(*cp2, startDrain()).WillOnce(Invoke([&]() { - cp2->idle_cb_(); - cp2->idle_cb_ = nullptr; - })); - EXPECT_CALL(*cp1, startDrain()).WillOnce(Invoke([&]() { - cp1->idle_cb_(); - cp1->idle_cb_ = nullptr; - })); - EXPECT_CALL(*tcp1, startDrain()).WillOnce(Invoke([&]() { - tcp1->idle_cb_(); - tcp1->idle_cb_ = nullptr; - })); - EXPECT_CALL(*tcp2, startDrain()).WillOnce(Invoke([&]() { - tcp2->idle_cb_(); - tcp2->idle_cb_ = nullptr; - })); + EXPECT_CALL(*cp2, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + cp2->idle_cb_(); + cp2->idle_cb_ = nullptr; + })); + EXPECT_CALL(*cp1, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + cp1->idle_cb_(); + cp1->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp1, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + tcp1->idle_cb_(); + tcp1->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp2, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + tcp2->idle_cb_(); + tcp2->idle_cb_ = nullptr; + })); HostVector hosts_removed; hosts_removed.push_back(host2); @@ -4685,14 +4699,16 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { HostVector hosts_added; hosts_added.push_back(host3); - EXPECT_CALL(*cp1, startDrain()).WillOnce(Invoke([&]() { - cp1->idle_cb_(); - cp1->idle_cb_ = nullptr; - })); - EXPECT_CALL(*tcp1, startDrain()).WillOnce(Invoke([&]() { - tcp1->idle_cb_(); - tcp1->idle_cb_ = nullptr; - })); + EXPECT_CALL(*cp1, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + cp1->idle_cb_(); + cp1->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp1, drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete)) + .WillOnce(Invoke([&]() { + tcp1->idle_cb_(); + tcp1->idle_cb_ = nullptr; + })); // Adding host3 should drain connection pool for host1. cluster.prioritySet().updateHosts( @@ -4756,8 +4772,12 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { hosts_added.push_back(host2); // No connection pools should be drained. - EXPECT_CALL(*cp1, drainConnections()).Times(0); - EXPECT_CALL(*tcp1, drainConnections()).Times(0); + EXPECT_CALL(*cp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)) + .Times(0); + EXPECT_CALL(*tcp1, + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)) + .Times(0); // No connection pools should be drained. cluster.prioritySet().updateHosts( diff --git a/test/common/upstream/conn_pool_map_impl_test.cc b/test/common/upstream/conn_pool_map_impl_test.cc index 5b8cd99f2b78b..8ee79ece9599c 100644 --- a/test/common/upstream/conn_pool_map_impl_test.cc +++ b/test/common/upstream/conn_pool_map_impl_test.cc @@ -160,7 +160,7 @@ TEST_F(ConnPoolMapImplTest, CallbacksPassedToPools) { ReadyWatcher watcher; test_map->addIdleCallback([&watcher]() { watcher.ready(); }); - test_map->startDrain(); + test_map->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); EXPECT_CALL(watcher, ready()).Times(2); cb1(); @@ -173,7 +173,7 @@ TEST_F(ConnPoolMapImplTest, CallbacksCachedAndPassedOnCreation) { ReadyWatcher watcher; test_map->addIdleCallback([&watcher]() { watcher.ready(); }); - test_map->startDrain(); + test_map->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainAndDelete); Http::ConnectionPool::Instance::IdleCb cb1; test_map->getPool(1, getFactoryExpectIdleCb(&cb1)); @@ -189,7 +189,7 @@ TEST_F(ConnPoolMapImplTest, CallbacksCachedAndPassedOnCreation) { // Tests that if we drain connections on an empty map, nothing happens. TEST_F(ConnPoolMapImplTest, EmptyMapDrainConnectionsNop) { TestMapPtr test_map = makeTestMap(); - test_map->drainConnections(); + test_map->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } // Tests that we forward drainConnections to the pools. @@ -198,10 +198,12 @@ TEST_F(ConnPoolMapImplTest, DrainConnectionsForwarded) { test_map->getPool(1, getBasicFactory()); test_map->getPool(2, getBasicFactory()); - EXPECT_CALL(*mock_pools_[0], drainConnections()); - EXPECT_CALL(*mock_pools_[1], drainConnections()); + EXPECT_CALL(*mock_pools_[0], + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + EXPECT_CALL(*mock_pools_[1], + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); - test_map->drainConnections(); + test_map->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } TEST_F(ConnPoolMapImplTest, ClearDefersDelete) { diff --git a/test/common/upstream/priority_conn_pool_map_impl_test.cc b/test/common/upstream/priority_conn_pool_map_impl_test.cc index a7ade68348547..db48e6afc799d 100644 --- a/test/common/upstream/priority_conn_pool_map_impl_test.cc +++ b/test/common/upstream/priority_conn_pool_map_impl_test.cc @@ -156,10 +156,12 @@ TEST_F(PriorityConnPoolMapImplTest, TestDrainConnectionsProxiedThrough) { test_map->getPool(ResourcePriority::High, 0, getBasicFactory()); test_map->getPool(ResourcePriority::Default, 0, getBasicFactory()); - EXPECT_CALL(*mock_pools_[0], drainConnections()); - EXPECT_CALL(*mock_pools_[1], drainConnections()); + EXPECT_CALL(*mock_pools_[0], + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); + EXPECT_CALL(*mock_pools_[1], + drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections)); - test_map->drainConnections(); + test_map->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections); } } // namespace diff --git a/test/mocks/http/conn_pool.h b/test/mocks/http/conn_pool.h index c1f55c2f92ef8..323eb7a087ac6 100644 --- a/test/mocks/http/conn_pool.h +++ b/test/mocks/http/conn_pool.h @@ -31,8 +31,7 @@ class MockInstance : public Instance { MOCK_METHOD(Http::Protocol, protocol, (), (const)); MOCK_METHOD(void, addIdleCallback, (IdleCb cb)); MOCK_METHOD(bool, isIdle, (), (const)); - MOCK_METHOD(void, startDrain, ()); - MOCK_METHOD(void, drainConnections, ()); + MOCK_METHOD(void, drainConnections, (Envoy::ConnectionPool::DrainBehavior drain_behavior)); MOCK_METHOD(bool, hasActiveConnections, (), (const)); MOCK_METHOD(Cancellable*, newStream, (ResponseDecoder & response_decoder, Callbacks& callbacks)); MOCK_METHOD(bool, maybePreconnect, (float)); diff --git a/test/mocks/tcp/mocks.h b/test/mocks/tcp/mocks.h index 75e79e7aea932..ff3e985e7bd6f 100644 --- a/test/mocks/tcp/mocks.h +++ b/test/mocks/tcp/mocks.h @@ -61,8 +61,7 @@ class MockInstance : public Instance { // Tcp::ConnectionPool::Instance MOCK_METHOD(void, addIdleCallback, (IdleCb cb)); MOCK_METHOD(bool, isIdle, (), (const)); - MOCK_METHOD(void, startDrain, ()); - MOCK_METHOD(void, drainConnections, ()); + MOCK_METHOD(void, drainConnections, (Envoy::ConnectionPool::DrainBehavior drain_behavior)); MOCK_METHOD(void, closeConnections, ()); MOCK_METHOD(Cancellable*, newConnection, (Tcp::ConnectionPool::Callbacks & callbacks)); MOCK_METHOD(bool, maybePreconnect, (float), ());