From ba2f255ddf3589ce059cef11e7d4b955cdce84ff Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Thu, 11 Jul 2019 12:46:32 +0200 Subject: [PATCH 01/18] upstream: Introducing close_connections_on_rebalance property Signed-off-by: Kateryna Nezdolii --- api/envoy/api/v2/cds.proto | 4 + .../common/upstream/cluster_manager_impl.cc | 23 +- source/common/upstream/cluster_manager_impl.h | 1 + .../upstream/cluster_manager_impl_test.cc | 263 ++++++++++++++++++ 4 files changed, 286 insertions(+), 5 deletions(-) diff --git a/api/envoy/api/v2/cds.proto b/api/envoy/api/v2/cds.proto index 44d2d7501479..652e3fbd2dc1 100644 --- a/api/envoy/api/v2/cds.proto +++ b/api/envoy/api/v2/cds.proto @@ -580,6 +580,10 @@ message Cluster { // If panic mode is triggered, new hosts are still eligible for traffic; they simply do not // contribute to the calculation when deciding whether panic mode is enabled or not. bool ignore_new_hosts_until_first_hc = 5; + + // If field is set to `true` tls cluster managers will close + // all existing connections to upstream hosts on hosts set change + bool close_connections_on_host_set_change = 6; } // Common configuration for all load balancer implementations. diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index d4a666f97af4..3d430b76fe64 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -313,12 +313,25 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // Now setup for cross-thread updates. cluster.prioritySet().addMemberUpdateCb( [&cluster, this](const HostVector&, const HostVector& hosts_removed) -> void { - // TODO(snowp): Should this be subject to merge windows? + const bool close_connections_on_host_set_change = + cluster.info()->lbConfig().close_connections_on_host_set_change(); - // Whenever hosts are removed from the cluster, we make each TLS cluster drain it's - // connection pools for the removed hosts. - if (!hosts_removed.empty()) { - postThreadLocalHostRemoval(cluster, hosts_removed); + if (close_connections_on_host_set_change) { + for (const auto& host_set : cluster.prioritySet().hostSetsPerPriority()) { + // this will drain all tcp and http connection pools + postThreadLocalHostRemoval(cluster, host_set->hosts()); + } + cm_stats_.upstream_connections_closed_on_host_set_change_.inc(); + } else { + // TODO(snowp): Should this be subject to merge windows? + + // Whenever hosts are removed from the cluster, we make each TLS cluster drain it's + // connection pools for the removed hosts. If `close_connections_on_host_set_change` is + // enabled, this case will be covered by first if statement, where all + // connection pools are drained. + if (!hosts_removed.empty()) { + postThreadLocalHostRemoval(cluster, hosts_removed); + } } }); diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 055cb9fd2cce..75724f1ca955 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -149,6 +149,7 @@ class ClusterManagerInitHelper : Logger::Loggable { COUNTER(cluster_removed) \ COUNTER(cluster_updated) \ COUNTER(cluster_updated_via_merge) \ + COUNTER(upstream_connections_closed_on_host_set_change) \ COUNTER(update_merge_cancelled) \ COUNTER(update_out_of_merge_window) \ GAUGE(active_clusters, NeverImport) \ diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 039490906a11..3e7bccecb415 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1,3 +1,4 @@ +#include #include #include @@ -3265,6 +3266,268 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveWithAllOptions) { expectSetsockoptSoKeepalive(7, 4, 1); } +TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { + const std::string yaml = R"EOF( + { + "static_resources": { + "clusters": [ + { + "name": "cluster_1", + "connect_timeout": "0.250s", + "type": "STATIC", + "lb_policy": "ROUND_ROBIN", + "common_lb_config": { + "close_connections_on_host_set_change": true + } + } + ] + } + } + )EOF"; + + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + + create(parseBootstrapFromV2Yaml(yaml)); + + // Set up for an initialize callback. + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::unique_ptr callbacks(new NiceMock()); + ClusterUpdateCallbacksHandlePtr cb = + cluster_manager_->addThreadLocalClusterUpdateCallbacks(*callbacks); + + EXPECT_FALSE(cluster_manager_->get("cluster_1")->info()->addedViaApi()); + + // Test for no hosts returning the correct values before we have hosts. + EXPECT_EQ(nullptr, cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + EXPECT_EQ(nullptr, cluster_manager_->tcpConnPoolForCluster("cluster_1", ResourcePriority::Default, + nullptr, nullptr)); + EXPECT_EQ(nullptr, + cluster_manager_->tcpConnForCluster("cluster_1", nullptr, nullptr).connection_); + + Cluster& cluster = cluster_manager_->activeClusters().begin()->second; + + // Set up the HostSet + HostSharedPtr host1 = makeTestHost(cluster.info(), "tcp://127.0.0.1:80"); + HostSharedPtr host2 = makeTestHost(cluster.info(), "tcp://127.0.0.1:81"); + + HostVector hosts{host1, host2}; + auto hosts_ptr = std::make_shared(hosts); + + // sending non-mergeable updates + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, + 100); + + //here actually no conn pools are being drained, as this is initial addition of hosts + EXPECT_EQ( + 1, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") + .value()); + + EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); + EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); + EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); + + EXPECT_CALL(factory_, allocateConnPool_(_, _)) + .Times(3) + .WillRepeatedly(ReturnNew()); + + EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + .Times(3) + .WillRepeatedly(ReturnNew()); + + // This should provide us a CP for each of the above hosts. + Http::ConnectionPool::MockInstance* cp1 = + dynamic_cast(cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + + Http::ConnectionPool::MockInstance* cp2 = + dynamic_cast(cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + + Tcp::ConnectionPool::MockInstance* tcp1 = + dynamic_cast(cluster_manager_->tcpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, nullptr, nullptr)); + + Tcp::ConnectionPool::MockInstance* tcp2 = + dynamic_cast(cluster_manager_->tcpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, nullptr, nullptr)); + + EXPECT_NE(cp1, cp2); + EXPECT_NE(tcp1, tcp2); + + EXPECT_CALL(*cp2, addDrainedCallback(_)) + .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + EXPECT_CALL(*cp1, addDrainedCallback(_)) + .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + EXPECT_CALL(*tcp1, addDrainedCallback(_)) + .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + EXPECT_CALL(*tcp2, addDrainedCallback(_)) + .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + HostVector hosts_removed; + hosts_removed.push_back(host2); + + // this update should drain all conn pools (host1, host2) + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, + hosts_removed, 100); + + EXPECT_EQ( + 2, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") + .value()); + + // recreate conn pool for host1 + cp1 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + + tcp1 = dynamic_cast(cluster_manager_->tcpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, nullptr, nullptr)); + + HostSharedPtr host3 = makeTestHost(cluster.info(), "tcp://127.0.0.1:82"); + + HostVector hosts_added; + hosts_added.push_back(host3); + + EXPECT_CALL(*cp1, addDrainedCallback(_)) + .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + EXPECT_CALL(*tcp1, addDrainedCallback(_)) + .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + // adding host3 should drain conn pool to host1 + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, + hosts_added, {}, 100); + + EXPECT_EQ(3, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change").value()); +} + +TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { + const std::string yaml = R"EOF( + { + "static_resources": { + "clusters": [ + { + "name": "cluster_1", + "connect_timeout": "0.250s", + "type": "STATIC", + "lb_policy": "ROUND_ROBIN" + } + ] + } + } + )EOF"; + + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + + create(parseBootstrapFromV2Yaml(yaml)); + + // Set up for an initialize callback. + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::unique_ptr callbacks(new NiceMock()); + ClusterUpdateCallbacksHandlePtr cb = + cluster_manager_->addThreadLocalClusterUpdateCallbacks(*callbacks); + + EXPECT_FALSE(cluster_manager_->get("cluster_1")->info()->addedViaApi()); + + // Test for no hosts returning the correct values before we have hosts. + EXPECT_EQ(nullptr, cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + EXPECT_EQ(nullptr, cluster_manager_->tcpConnPoolForCluster("cluster_1", ResourcePriority::Default, + nullptr, nullptr)); + EXPECT_EQ(nullptr, + cluster_manager_->tcpConnForCluster("cluster_1", nullptr, nullptr).connection_); + + Cluster& cluster = cluster_manager_->activeClusters().begin()->second; + + // Set up the HostSet + HostSharedPtr host1 = makeTestHost(cluster.info(), "tcp://127.0.0.1:80"); + HostSharedPtr host2 = makeTestHost(cluster.info(), "tcp://127.0.0.1:81"); + + HostVector hosts{host1, host2}; + auto hosts_ptr = std::make_shared(hosts); + + // sending non-mergeable updates + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, + 100); + + EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); + EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); + EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); + + EXPECT_CALL(factory_, allocateConnPool_(_, _)) + .Times(2) + .WillRepeatedly(ReturnNew()); + + EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + .Times(2) + .WillRepeatedly(ReturnNew()); + + // This should provide us a CP for each of the above hosts. + Http::ConnectionPool::MockInstance* cp1 = + dynamic_cast(cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + + Http::ConnectionPool::MockInstance* cp2 = + dynamic_cast(cluster_manager_->httpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + + Tcp::ConnectionPool::MockInstance* tcp1 = + dynamic_cast(cluster_manager_->tcpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, nullptr, nullptr)); + + Tcp::ConnectionPool::MockInstance* tcp2 = + dynamic_cast(cluster_manager_->tcpConnPoolForCluster( + "cluster_1", ResourcePriority::Default, nullptr, nullptr)); + + EXPECT_NE(cp1, cp2); + EXPECT_NE(tcp1, tcp2); + + HostSharedPtr host3 = makeTestHost(cluster.info(), "tcp://127.0.0.1:82"); + + HostVector hosts_added; + hosts_added.push_back(host3); + + // no conn pools should be drained + EXPECT_CALL(*cp1, drainConnections()).Times(0); + EXPECT_CALL(*cp2, drainConnections()).Times(0); + EXPECT_CALL(*tcp1, drainConnections()).Times(0); + EXPECT_CALL(*tcp2, drainConnections()).Times(0); + + // no conn pools should be drained + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, + hosts_added, {}, 100); + + EXPECT_CALL(*cp2, addDrainedCallback(_)) + .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + EXPECT_CALL(*tcp2, addDrainedCallback(_)) + .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + + EXPECT_CALL(*cp1, drainConnections()).Times(0); + EXPECT_CALL(*tcp1, drainConnections()).Times(0); + + HostVector hosts_removed; + hosts_removed.push_back(host2); + + // conn pool for host2 should be drained + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, + hosts_removed, 100); + + EXPECT_EQ(0, factory_.stats_.counter("upstream_connections_closed_on_host_set_change").value()); +} + } // namespace } // namespace Upstream } // namespace Envoy From 92ad8d50d1a6d574c92061976a8b52d8ff8db295 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Wed, 31 Jul 2019 14:39:59 +0200 Subject: [PATCH 02/18] fix format Signed-off-by: Kateryna Nezdolii --- test/common/upstream/cluster_manager_impl_test.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 3e7bccecb415..c5a8989fabab 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1,4 +1,3 @@ -#include #include #include @@ -3321,10 +3320,10 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, 100); - //here actually no conn pools are being drained, as this is initial addition of hosts - EXPECT_EQ( - 1, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") - .value()); + // here actually no conn pools are being drained, as this is initial addition of hosts + EXPECT_EQ( + 1, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") + .value()); EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); @@ -3405,7 +3404,9 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); - EXPECT_EQ(3, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change").value()); + EXPECT_EQ( + 3, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") + .value()); } TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { From 888640c4a03b3fdce0c1bcbb9d062a3c84c8f91a Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Wed, 31 Jul 2019 15:53:53 +0200 Subject: [PATCH 03/18] Correct mistake in test Signed-off-by: Kateryna Nezdolii --- test/common/upstream/cluster_manager_impl_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index c5a8989fabab..46955d265c55 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3526,7 +3526,9 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, hosts_removed, 100); - EXPECT_EQ(0, factory_.stats_.counter("upstream_connections_closed_on_host_set_change").value()); + EXPECT_EQ( + 0, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") + .value()); } } // namespace From 77cf3c6c31a144d3eb402e30b2cd861267f9feb4 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Wed, 31 Jul 2019 17:10:37 +0200 Subject: [PATCH 04/18] Removing extra lines to reduce test size Signed-off-by: Kateryna Nezdolii --- test/common/upstream/cluster_manager_impl_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 46955d265c55..e2ab91931e41 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3427,7 +3427,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { ReadyWatcher initialized; EXPECT_CALL(initialized, ready()); - create(parseBootstrapFromV2Yaml(yaml)); // Set up for an initialize callback. @@ -3477,7 +3476,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { Http::ConnectionPool::MockInstance* cp1 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); - Http::ConnectionPool::MockInstance* cp2 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); @@ -3485,7 +3483,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { Tcp::ConnectionPool::MockInstance* tcp1 = dynamic_cast(cluster_manager_->tcpConnPoolForCluster( "cluster_1", ResourcePriority::Default, nullptr, nullptr)); - Tcp::ConnectionPool::MockInstance* tcp2 = dynamic_cast(cluster_manager_->tcpConnPoolForCluster( "cluster_1", ResourcePriority::Default, nullptr, nullptr)); @@ -3494,7 +3491,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { EXPECT_NE(tcp1, tcp2); HostSharedPtr host3 = makeTestHost(cluster.info(), "tcp://127.0.0.1:82"); - HostVector hosts_added; hosts_added.push_back(host3); From 853343b571e6469458ea9f2ccc64770a7ba8eb25 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 5 Aug 2019 11:34:30 +0200 Subject: [PATCH 05/18] Apply review comments Signed-off-by: Kateryna Nezdolii --- api/envoy/api/v2/cds.proto | 4 +-- .../common/upstream/cluster_manager_impl.cc | 2 +- .../upstream/cluster_manager_impl_test.cc | 28 +++++++++---------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/api/envoy/api/v2/cds.proto b/api/envoy/api/v2/cds.proto index 652e3fbd2dc1..28de7b4bbebd 100644 --- a/api/envoy/api/v2/cds.proto +++ b/api/envoy/api/v2/cds.proto @@ -581,8 +581,8 @@ message Cluster { // contribute to the calculation when deciding whether panic mode is enabled or not. bool ignore_new_hosts_until_first_hc = 5; - // If field is set to `true` tls cluster managers will close - // all existing connections to upstream hosts on hosts set change + // If field is set to `true`, tls cluster managers will close all existing + // connections to upstream hosts whenever hosts are added or removed from cluster. bool close_connections_on_host_set_change = 6; } diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 3d430b76fe64..001e73c588e7 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -327,7 +327,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // Whenever hosts are removed from the cluster, we make each TLS cluster drain it's // connection pools for the removed hosts. If `close_connections_on_host_set_change` is - // enabled, this case will be covered by first if statement, where all + // enabled, this case will be covered by first `if` statement, where all // connection pools are drained. if (!hosts_removed.empty()) { postThreadLocalHostRemoval(cluster, hosts_removed); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index e2ab91931e41..21f8d7766bf8 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3298,7 +3298,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { EXPECT_FALSE(cluster_manager_->get("cluster_1")->info()->addedViaApi()); - // Test for no hosts returning the correct values before we have hosts. + // Verify that we get no hosts when the HostSet is empty. EXPECT_EQ(nullptr, cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); EXPECT_EQ(nullptr, cluster_manager_->tcpConnPoolForCluster("cluster_1", ResourcePriority::Default, @@ -3308,14 +3308,14 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { Cluster& cluster = cluster_manager_->activeClusters().begin()->second; - // Set up the HostSet + // Set up the HostSet. HostSharedPtr host1 = makeTestHost(cluster.info(), "tcp://127.0.0.1:80"); HostSharedPtr host2 = makeTestHost(cluster.info(), "tcp://127.0.0.1:81"); HostVector hosts{host1, host2}; auto hosts_ptr = std::make_shared(hosts); - // sending non-mergeable updates + // Sending non-mergeable updates. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, 100); @@ -3341,10 +3341,10 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { Http::ConnectionPool::MockInstance* cp1 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); - + // Create persistent connection for host2. Http::ConnectionPool::MockInstance* cp2 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( - "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); + "cluster_1", ResourcePriority::Default, Http::Protocol::Http2, nullptr)); Tcp::ConnectionPool::MockInstance* tcp1 = dynamic_cast(cluster_manager_->tcpConnPoolForCluster( @@ -3372,7 +3372,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { HostVector hosts_removed; hosts_removed.push_back(host2); - // this update should drain all conn pools (host1, host2) + // This update should drain all connection pools (host1, host2). cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, hosts_removed, 100); @@ -3381,7 +3381,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { 2, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") .value()); - // recreate conn pool for host1 + // Recreate connection pool for host1. cp1 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); @@ -3399,7 +3399,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { EXPECT_CALL(*tcp1, addDrainedCallback(_)) .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - // adding host3 should drain conn pool to host1 + // Adding host3 should drain connection pool for host1. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); @@ -3438,7 +3438,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { EXPECT_FALSE(cluster_manager_->get("cluster_1")->info()->addedViaApi()); - // Test for no hosts returning the correct values before we have hosts. + // Verify that we get no hosts when the HostSet is empty. EXPECT_EQ(nullptr, cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); EXPECT_EQ(nullptr, cluster_manager_->tcpConnPoolForCluster("cluster_1", ResourcePriority::Default, @@ -3448,14 +3448,14 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { Cluster& cluster = cluster_manager_->activeClusters().begin()->second; - // Set up the HostSet + // Set up the HostSet. HostSharedPtr host1 = makeTestHost(cluster.info(), "tcp://127.0.0.1:80"); HostSharedPtr host2 = makeTestHost(cluster.info(), "tcp://127.0.0.1:81"); HostVector hosts{host1, host2}; auto hosts_ptr = std::make_shared(hosts); - // sending non-mergeable updates + // Sending non-mergeable updates. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, 100); @@ -3494,13 +3494,13 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { HostVector hosts_added; hosts_added.push_back(host3); - // no conn pools should be drained + // No connection pools should be drained. EXPECT_CALL(*cp1, drainConnections()).Times(0); EXPECT_CALL(*cp2, drainConnections()).Times(0); EXPECT_CALL(*tcp1, drainConnections()).Times(0); EXPECT_CALL(*tcp2, drainConnections()).Times(0); - // no conn pools should be drained + // No connection pools should be drained. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); @@ -3517,7 +3517,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { HostVector hosts_removed; hosts_removed.push_back(host2); - // conn pool for host2 should be drained + // Connection pool for host2 should be drained. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, hosts_removed, 100); From b336b6b467421284f5d52063b1b017bad536a13e Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Thu, 8 Aug 2019 15:45:26 +0200 Subject: [PATCH 06/18] apply review comments Signed-off-by: Kateryna Nezdolii --- source/common/upstream/cluster_manager_impl.cc | 8 ++++---- source/common/upstream/cluster_manager_impl.h | 3 ++- test/common/upstream/cluster_manager_impl_test.cc | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 001e73c588e7..be0e72a1e0e3 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -319,7 +319,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { if (close_connections_on_host_set_change) { for (const auto& host_set : cluster.prioritySet().hostSetsPerPriority()) { // this will drain all tcp and http connection pools - postThreadLocalHostRemoval(cluster, host_set->hosts()); + postThreadLocalDrainConnections(cluster, host_set->hosts()); } cm_stats_.upstream_connections_closed_on_host_set_change_.inc(); } else { @@ -330,7 +330,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // enabled, this case will be covered by first `if` statement, where all // connection pools are drained. if (!hosts_removed.empty()) { - postThreadLocalHostRemoval(cluster, hosts_removed); + postThreadLocalDrainConnections(cluster, hosts_removed); } } }); @@ -724,8 +724,8 @@ Tcp::ConnectionPool::Instance* ClusterManagerImpl::tcpConnPoolForCluster( return entry->second->tcpConnPool(priority, context, transport_socket_options); } -void ClusterManagerImpl::postThreadLocalHostRemoval(const Cluster& cluster, - const HostVector& hosts_removed) { +void ClusterManagerImpl::postThreadLocalDrainConnections(const Cluster& cluster, + const HostVector& hosts_removed) { tls_->runOnAllThreads([this, name = cluster.info()->name(), hosts_removed]() { ThreadLocalClusterManagerImpl::removeHosts(name, hosts_removed, *tls_); }); diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 75724f1ca955..f03273dfdf24 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -233,7 +233,8 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable Date: Thu, 8 Aug 2019 16:02:56 +0200 Subject: [PATCH 07/18] Fix indentation in test Signed-off-by: Kateryna Nezdolii --- test/common/upstream/cluster_manager_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 53b0c32c6d15..9c778e2db36e 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3276,8 +3276,8 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { "type": "STATIC", "lb_policy": "ROUND_ROBIN", "common_lb_config": { - "close_connections_on_host_set_change": true - } + "close_connections_on_host_set_change": true + } } ] } From d351954baa4f837fb5b9e6738b8f738c07c74511 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Wed, 14 Aug 2019 12:22:01 +0200 Subject: [PATCH 08/18] Update api/envoy/api/v2/cds.proto Co-Authored-By: Matt Klein Signed-off-by: Kateryna Nezdolii --- api/envoy/api/v2/cds.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/api/v2/cds.proto b/api/envoy/api/v2/cds.proto index 28de7b4bbebd..93eb17dadc6b 100644 --- a/api/envoy/api/v2/cds.proto +++ b/api/envoy/api/v2/cds.proto @@ -581,7 +581,7 @@ message Cluster { // contribute to the calculation when deciding whether panic mode is enabled or not. bool ignore_new_hosts_until_first_hc = 5; - // If field is set to `true`, tls cluster managers will close all existing + // If set to `true`, the cluster manager will close all existing // connections to upstream hosts whenever hosts are added or removed from cluster. bool close_connections_on_host_set_change = 6; } From 09b185aa32c9949cbb087b3a1bcbff9d87a1ecb3 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Wed, 14 Aug 2019 12:22:27 +0200 Subject: [PATCH 09/18] Update source/common/upstream/cluster_manager_impl.cc Co-Authored-By: Matt Klein Signed-off-by: Kateryna Nezdolii --- source/common/upstream/cluster_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index be0e72a1e0e3..4b71cfdd5be4 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -318,7 +318,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { if (close_connections_on_host_set_change) { for (const auto& host_set : cluster.prioritySet().hostSetsPerPriority()) { - // this will drain all tcp and http connection pools + // This will drain all tcp and http connection pools. postThreadLocalDrainConnections(cluster, host_set->hosts()); } cm_stats_.upstream_connections_closed_on_host_set_change_.inc(); From 42cfdb2000b0c8f99051e90534815ee518621dd4 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Wed, 14 Aug 2019 14:24:19 +0200 Subject: [PATCH 10/18] apply review comments Signed-off-by: Kateryna Nezdolii --- .../common/upstream/cluster_manager_impl.cc | 5 +-- source/common/upstream/cluster_manager_impl.h | 2 +- .../upstream/cluster_manager_impl_test.cc | 41 +++++++------------ 3 files changed, 16 insertions(+), 32 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 4b71cfdd5be4..170720bb4af6 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -313,10 +313,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // Now setup for cross-thread updates. cluster.prioritySet().addMemberUpdateCb( [&cluster, this](const HostVector&, const HostVector& hosts_removed) -> void { - const bool close_connections_on_host_set_change = - cluster.info()->lbConfig().close_connections_on_host_set_change(); - - if (close_connections_on_host_set_change) { + if (cluster.info()->lbConfig().close_connections_on_host_set_change()) { for (const auto& host_set : cluster.prioritySet().hostSetsPerPriority()) { // This will drain all tcp and http connection pools. postThreadLocalDrainConnections(cluster, host_set->hosts()); diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index f03273dfdf24..d5d36f0161ce 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -149,9 +149,9 @@ class ClusterManagerInitHelper : Logger::Loggable { COUNTER(cluster_removed) \ COUNTER(cluster_updated) \ COUNTER(cluster_updated_via_merge) \ - COUNTER(upstream_connections_closed_on_host_set_change) \ COUNTER(update_merge_cancelled) \ COUNTER(update_out_of_merge_window) \ + COUNTER(upstream_connections_closed_on_host_set_change) \ GAUGE(active_clusters, NeverImport) \ GAUGE(warming_clusters, NeverImport) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 9c778e2db36e..6aaf3a8b4a81 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3267,21 +3267,14 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveWithAllOptions) { TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { const std::string yaml = R"EOF( - { - "static_resources": { - "clusters": [ - { - "name": "cluster_1", - "connect_timeout": "0.250s", - "type": "STATIC", - "lb_policy": "ROUND_ROBIN", - "common_lb_config": { - "close_connections_on_host_set_change": true - } - } - ] - } - } + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.250s + lb_policy: ROUND_ROBIN + type: STATIC + common_lb_config: + close_connections_on_host_set_change: true )EOF"; ReadyWatcher initialized; @@ -3411,18 +3404,12 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { const std::string yaml = R"EOF( - { - "static_resources": { - "clusters": [ - { - "name": "cluster_1", - "connect_timeout": "0.250s", - "type": "STATIC", - "lb_policy": "ROUND_ROBIN" - } - ] - } - } + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.250s + lb_policy: ROUND_ROBIN + type: STATIC )EOF"; ReadyWatcher initialized; From 213ed5ab6e1d53030f6f4f893906780b2b2b59d9 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 19 Aug 2019 12:27:37 +0200 Subject: [PATCH 11/18] Clean up test, fix DCO Signed-off-by: Kateryna Nezdolii --- .../upstream/cluster_manager_impl_test.cc | 53 ++----------------- 1 file changed, 5 insertions(+), 48 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 6aaf3a8b4a81..8bb520e9340e 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3423,23 +3423,12 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { ClusterUpdateCallbacksHandlePtr cb = cluster_manager_->addThreadLocalClusterUpdateCallbacks(*callbacks); - EXPECT_FALSE(cluster_manager_->get("cluster_1")->info()->addedViaApi()); - - // Verify that we get no hosts when the HostSet is empty. - EXPECT_EQ(nullptr, cluster_manager_->httpConnPoolForCluster( - "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); - EXPECT_EQ(nullptr, cluster_manager_->tcpConnPoolForCluster("cluster_1", ResourcePriority::Default, - nullptr, nullptr)); - EXPECT_EQ(nullptr, - cluster_manager_->tcpConnForCluster("cluster_1", nullptr, nullptr).connection_); - Cluster& cluster = cluster_manager_->activeClusters().begin()->second; // Set up the HostSet. HostSharedPtr host1 = makeTestHost(cluster.info(), "tcp://127.0.0.1:80"); - HostSharedPtr host2 = makeTestHost(cluster.info(), "tcp://127.0.0.1:81"); - HostVector hosts{host1, host2}; + HostVector hosts{host1}; auto hosts_ptr = std::make_shared(hosts); // Sending non-mergeable updates. @@ -3447,68 +3436,36 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, 100); - EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); - EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); - EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); - EXPECT_CALL(factory_, allocateConnPool_(_, _)) - .Times(2) + .Times(1) .WillRepeatedly(ReturnNew()); EXPECT_CALL(factory_, allocateTcpConnPool_(_)) - .Times(2) + .Times(1) .WillRepeatedly(ReturnNew()); // This should provide us a CP for each of the above hosts. Http::ConnectionPool::MockInstance* cp1 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); - Http::ConnectionPool::MockInstance* cp2 = - dynamic_cast(cluster_manager_->httpConnPoolForCluster( - "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); Tcp::ConnectionPool::MockInstance* tcp1 = dynamic_cast(cluster_manager_->tcpConnPoolForCluster( "cluster_1", ResourcePriority::Default, nullptr, nullptr)); - Tcp::ConnectionPool::MockInstance* tcp2 = - dynamic_cast(cluster_manager_->tcpConnPoolForCluster( - "cluster_1", ResourcePriority::Default, nullptr, nullptr)); - - EXPECT_NE(cp1, cp2); - EXPECT_NE(tcp1, tcp2); - HostSharedPtr host3 = makeTestHost(cluster.info(), "tcp://127.0.0.1:82"); + HostSharedPtr host2 = makeTestHost(cluster.info(), "tcp://127.0.0.1:82"); HostVector hosts_added; - hosts_added.push_back(host3); + hosts_added.push_back(host2); // No connection pools should be drained. EXPECT_CALL(*cp1, drainConnections()).Times(0); - EXPECT_CALL(*cp2, drainConnections()).Times(0); EXPECT_CALL(*tcp1, drainConnections()).Times(0); - EXPECT_CALL(*tcp2, drainConnections()).Times(0); // No connection pools should be drained. cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); - EXPECT_CALL(*cp2, addDrainedCallback(_)) - .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*tcp2, addDrainedCallback(_)) - .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*cp1, drainConnections()).Times(0); - EXPECT_CALL(*tcp1, drainConnections()).Times(0); - - HostVector hosts_removed; - hosts_removed.push_back(host2); - - // Connection pool for host2 should be drained. - cluster.prioritySet().updateHosts( - 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, - hosts_removed, 100); - EXPECT_EQ( 0, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") .value()); From ad3ba43ff9932b23afe133b2d1184f6f4cb155ad Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 19 Aug 2019 13:12:54 +0200 Subject: [PATCH 12/18] Update version history Signed-off-by: Kateryna Nezdolii --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 8442328f715e..b9c0ade22293 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -10,6 +10,7 @@ Version history * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`. * tls: added verification of IP address SAN fields in certificates against configured SANs in the certificate validation context. +* upstream: added :ref:`an option ` that allows to drain http, tcp connection pools on cluster membership change. 1.11.0 (July 11, 2019) ====================== From 6900e8a1834841bc5e77616d7d6b2b20e6fd5fb7 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 19 Aug 2019 13:12:54 +0200 Subject: [PATCH 13/18] Update version history Signed-off-by: Kateryna Nezdolii --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index f1f8419705f2..3a8180daf2d8 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -50,6 +50,7 @@ Version history Runtime feature `envoy.reloadable_features.http2_protocol_options.max_outbound_control_frames` overrides :ref:`max_outbound_control_frames config setting `. Large override value effectively disables flood mitigation of outbound frames of types PING, SETTINGS and RST_STREAM. * http: enabled strict validation of HTTP/2 messaging. Previous behavior can be restored using :ref:`stream_error_on_invalid_http_messaging config setting `. Runtime feature `envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging` overrides :ref:`stream_error_on_invalid_http_messaging config setting `. +* upstream: added :ref:`an option ` that allows to drain http, tcp connection pools on cluster membership change. 1.11.0 (July 11, 2019) ====================== From 88abfa48cdc9ce3e61a3d2b30731a4c62b5eab8f Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 19 Aug 2019 13:53:23 +0200 Subject: [PATCH 14/18] Update version history Signed-off-by: Kateryna Nezdolii --- docs/root/intro/version_history.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 3a8180daf2d8..f1f8419705f2 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -50,7 +50,6 @@ Version history Runtime feature `envoy.reloadable_features.http2_protocol_options.max_outbound_control_frames` overrides :ref:`max_outbound_control_frames config setting `. Large override value effectively disables flood mitigation of outbound frames of types PING, SETTINGS and RST_STREAM. * http: enabled strict validation of HTTP/2 messaging. Previous behavior can be restored using :ref:`stream_error_on_invalid_http_messaging config setting `. Runtime feature `envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging` overrides :ref:`stream_error_on_invalid_http_messaging config setting `. -* upstream: added :ref:`an option ` that allows to drain http, tcp connection pools on cluster membership change. 1.11.0 (July 11, 2019) ====================== From 5c4129cd5173bfce25b98db259efc48efefc71d4 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 19 Aug 2019 17:29:35 +0200 Subject: [PATCH 15/18] Apply api review comments Signed-off-by: Kateryna Nezdolii --- api/envoy/api/v2/cds.proto | 2 +- docs/root/intro/version_history.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/envoy/api/v2/cds.proto b/api/envoy/api/v2/cds.proto index 3635cc6609af..47809dad3c6e 100644 --- a/api/envoy/api/v2/cds.proto +++ b/api/envoy/api/v2/cds.proto @@ -584,7 +584,7 @@ message Cluster { // contribute to the calculation when deciding whether panic mode is enabled or not. bool ignore_new_hosts_until_first_hc = 5; - // If set to `true`, the cluster manager will close all existing + // If set to `true`, the cluster manager will drain all existing // connections to upstream hosts whenever hosts are added or removed from cluster. bool close_connections_on_host_set_change = 6; } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index f1f8419705f2..36297455c4b0 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -32,7 +32,7 @@ Version history certificate validation context. * upstream: added network filter chains to upstream connections, see :ref:`filters`. * upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1. -* upstream: added :ref:`an option ` that allows to drain http, tcp connection pools on cluster membership change. +* upstream: added :ref:`an option ` that allows to drain HTTP, TCP connection pools on cluster membership change. * zookeeper: parse responses and emit latency stats. 1.11.1 (August 13, 2019) From ba07c8e29bf62b3d58ed6c0bc8f7dc87d69ba534 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Tue, 20 Aug 2019 10:35:51 +0200 Subject: [PATCH 16/18] Update api/envoy/api/v2/cds.proto Co-Authored-By: Matt Klein Signed-off-by: Kateryna Nezdolii --- api/envoy/api/v2/cds.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/api/v2/cds.proto b/api/envoy/api/v2/cds.proto index 47809dad3c6e..e7df4a940bc6 100644 --- a/api/envoy/api/v2/cds.proto +++ b/api/envoy/api/v2/cds.proto @@ -585,7 +585,7 @@ message Cluster { bool ignore_new_hosts_until_first_hc = 5; // If set to `true`, the cluster manager will drain all existing - // connections to upstream hosts whenever hosts are added or removed from cluster. + // connections to upstream hosts whenever hosts are added or removed from the cluster. bool close_connections_on_host_set_change = 6; } From 22f49f417211a872059b8bfa0d618d5b15e37e64 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Tue, 20 Aug 2019 10:36:03 +0200 Subject: [PATCH 17/18] Update docs/root/intro/version_history.rst Co-Authored-By: Matt Klein Signed-off-by: Kateryna Nezdolii --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 36297455c4b0..36920e32e264 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -32,7 +32,7 @@ Version history certificate validation context. * upstream: added network filter chains to upstream connections, see :ref:`filters`. * upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1. -* upstream: added :ref:`an option ` that allows to drain HTTP, TCP connection pools on cluster membership change. +* upstream: added :ref:`an option ` that allows draining HTTP, TCP connection pools on cluster membership change. * zookeeper: parse responses and emit latency stats. 1.11.1 (August 13, 2019) From f4a1a4a6f30e1f65fe33052d3b4020eb90026977 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Tue, 20 Aug 2019 12:17:03 +0200 Subject: [PATCH 18/18] apply review comments Signed-off-by: Kateryna Nezdolii --- source/common/upstream/cluster_manager_impl.cc | 1 - source/common/upstream/cluster_manager_impl.h | 1 - .../upstream/cluster_manager_impl_test.cc | 17 ----------------- 3 files changed, 19 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 50accd1ad793..490b77bc87ac 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -318,7 +318,6 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // This will drain all tcp and http connection pools. postThreadLocalDrainConnections(cluster, host_set->hosts()); } - cm_stats_.upstream_connections_closed_on_host_set_change_.inc(); } else { // TODO(snowp): Should this be subject to merge windows? diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index d5d36f0161ce..6c538e9aa30a 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -151,7 +151,6 @@ class ClusterManagerInitHelper : Logger::Loggable { COUNTER(cluster_updated_via_merge) \ COUNTER(update_merge_cancelled) \ COUNTER(update_out_of_merge_window) \ - COUNTER(upstream_connections_closed_on_host_set_change) \ GAUGE(active_clusters, NeverImport) \ GAUGE(warming_clusters, NeverImport) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 03d8d0507d4f..473a3c80d204 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3369,11 +3369,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, 100); - // here actually no conn pools are being drained, as this is initial addition of hosts - EXPECT_EQ( - 1, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") - .value()); - EXPECT_EQ(1, factory_.stats_.counter("cluster_manager.cluster_updated").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.cluster_updated_via_merge").value()); EXPECT_EQ(0, factory_.stats_.counter("cluster_manager.update_merge_cancelled").value()); @@ -3426,10 +3421,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, {}, hosts_removed, 100); - EXPECT_EQ( - 2, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") - .value()); - // Recreate connection pool for host1. cp1 = dynamic_cast(cluster_manager_->httpConnPoolForCluster( "cluster_1", ResourcePriority::Default, Http::Protocol::Http11, nullptr)); @@ -3452,10 +3443,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); - - EXPECT_EQ( - 3, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") - .value()); } TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { @@ -3521,10 +3508,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); - - EXPECT_EQ( - 0, factory_.stats_.counter("cluster_manager.upstream_connections_closed_on_host_set_change") - .value()); } } // namespace