From b21eb32161eb435248661b5aad69d133cd59bf9a Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Thu, 21 Feb 2019 16:02:16 +0530 Subject: [PATCH 01/14] copy active cluster hosts Signed-off-by: Rama Chavali --- .../common/upstream/cluster_manager_impl.cc | 31 ++++++++ .../upstream/cluster_manager_impl_test.cc | 78 ++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 259af7062d061..9d82de57a1cdc 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -477,6 +477,37 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // Otherwise, applyUpdates() will fire with a dangling cluster reference. updates_map_.erase(cluster_name); + // If the active cluster has priority sets and the warming cluster does not have them, it + // means that onConfigUpdate is triggered of EDS type of cluster is triggered with no + // reference to this cluster. In such cases, copy the active cluster priority set to the + // warming cluster otherwise the active cluster hosts will be cleared after warming is + // completed. See https://github.com/envoyproxy/envoy/issues/5168 for more context. + const auto active_it = active_clusters_.find(cluster_name); + if (active_it != active_clusters_.end()) { + auto& active_cluster_entry = *active_it->second; + if (active_cluster_entry.cluster_->prioritySet().hostSetsPerPriority().size() != 0 && + (cluster_entry.cluster_->prioritySet().hostSetsPerPriority().size() == 0 || + (cluster_entry.cluster_->prioritySet().hostSetsPerPriority().size() == 1 && + cluster_entry.cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size() == + 0))) { + ENVOY_LOG( + debug, + "copying host set from active cluster {} as warming cluster does not have host set", + cluster_name); + + const auto& host_sets = + active_cluster_entry.cluster_->prioritySet().hostSetsPerPriority(); + for (size_t priority = 0; priority < host_sets.size(); ++priority) { + const auto& host_set = host_sets[priority]; + HostVectorConstSharedPtr hosts_copy(new HostVector(host_set->hosts())); + HostsPerLocalityConstSharedPtr hosts_per_locality_copy = + host_set->hostsPerLocality().clone(); + cluster_entry.cluster_->prioritySet().updateHosts( + priority, HostSetImpl::partitionHosts(hosts_copy, hosts_per_locality_copy), + host_set->localityWeights(), {}, {}, absl::nullopt); + } + } + } active_clusters_[cluster_name] = std::move(warming_it->second); warming_clusters_.erase(warming_it); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 2823c6e502675..094c6426cc873 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1189,10 +1189,86 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(callbacks.get())); } +// This test validates that if warming cluster's initialization is triggered with empty hosts, it +// does not clear the active cluster hosts. Regression to test to validate the behaviour observed in +// https://github.com/envoyproxy/envoy/issues/5168. +TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { + const std::string json = R"EOF( + { + "clusters": [] + } + )EOF"; + + create(parseBootstrapFromJson(json)); + + InSequence s; + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::shared_ptr cluster1(new NiceMock()); + + // Set up the HostSet with 1 healthy, 1 degraded and 1 unhealthy for active cluster. + HostSharedPtr host1 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + host1->healthFlagSet(HostImpl::HealthFlag::DEGRADED_ACTIVE_HC); + HostSharedPtr host2 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + host2->healthFlagSet(HostImpl::HealthFlag::FAILED_ACTIVE_HC); + HostSharedPtr host3 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + + HostVector hosts{host1, host2, host3}; + auto hosts_ptr = std::make_shared(hosts); + + cluster1->priority_set_.updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, + {}); + + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(*cluster1, initializePhase()).Times(0); + EXPECT_CALL(*cluster1, initialize(_)); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "")); + checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); + EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); + cluster1->initialize_callback_(); + + EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); + checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); + + // Now trigger warming of this cluster with no hosts. + auto update_cluster = defaultStaticCluster("fake_cluster"); + update_cluster.mutable_per_connection_buffer_limit_bytes()->set_value(12345); + + std::shared_ptr cluster2(new NiceMock()); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(*cluster2, initializePhase()).Times(0); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "")); + checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 1 /*warming*/); + cluster2->initialize_callback_(); + + checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); + + EXPECT_EQ(cluster2->info_, cluster_manager_->get("fake_cluster")->info()); + EXPECT_EQ(1UL, cluster_manager_->clusters().size()); + + // Validate that the host updates are pushed to tls clusters. + auto* tls_cluster = cluster_manager_->get(cluster2->info_->name()); + + EXPECT_EQ(1, tls_cluster->prioritySet().hostSetsPerPriority().size()); + EXPECT_EQ(1, tls_cluster->prioritySet().hostSetsPerPriority()[0]->degradedHosts().size()); + EXPECT_EQ(host1, tls_cluster->prioritySet().hostSetsPerPriority()[0]->degradedHosts()[0]); + EXPECT_EQ(1, tls_cluster->prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); + EXPECT_EQ(host3, tls_cluster->prioritySet().hostSetsPerPriority()[0]->healthyHosts()[0]); + EXPECT_EQ(3, tls_cluster->prioritySet().hostSetsPerPriority()[0]->hosts().size()); + + factory_.tls_.shutdownThread(); + + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); +} + TEST_F(ClusterManagerImplTest, addOrUpdateClusterStaticExists) { const std::string json = fmt::sprintf("{%s}", clustersJson({defaultStaticClusterJson("some_cluster")})); - std::shared_ptr cluster1(new NiceMock()); + std::shared_ptr cluster1(new NiceMock()); InSequence s; EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); From e8aeaf6dad693051227e5826e74eae5a9ea7f92f Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Thu, 21 Feb 2019 16:08:25 +0530 Subject: [PATCH 02/14] revert unnecessary change Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 2 +- test/common/upstream/cluster_manager_impl_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 9d82de57a1cdc..d74ed87629646 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -478,7 +478,7 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust updates_map_.erase(cluster_name); // If the active cluster has priority sets and the warming cluster does not have them, it - // means that onConfigUpdate is triggered of EDS type of cluster is triggered with no + // means that onConfigUpdate is triggered for EDS type of cluster is triggered with no // reference to this cluster. In such cases, copy the active cluster priority set to the // warming cluster otherwise the active cluster hosts will be cleared after warming is // completed. See https://github.com/envoyproxy/envoy/issues/5168 for more context. diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 094c6426cc873..f6c1ea21a1fab 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1268,7 +1268,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { TEST_F(ClusterManagerImplTest, addOrUpdateClusterStaticExists) { const std::string json = fmt::sprintf("{%s}", clustersJson({defaultStaticClusterJson("some_cluster")})); - std::shared_ptr cluster1(new NiceMock()); + std::shared_ptr cluster1(new NiceMock()); InSequence s; EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); From 2e6f53be193fc2607d1ebd0169e6d85c8c995bcd Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Thu, 21 Feb 2019 17:19:31 +0530 Subject: [PATCH 03/14] fix compilation after merge Signed-off-by: Rama Chavali --- test/common/upstream/cluster_manager_impl_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index f76677f866333..af06fa6b4c89c 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1245,7 +1245,8 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(*cluster1, initializePhase()).Times(0); EXPECT_CALL(*cluster1, initialize(_)); - EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "")); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "", + dummyWarmingCb)); checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); cluster1->initialize_callback_(); @@ -1260,7 +1261,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { std::shared_ptr cluster2(new NiceMock()); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); - EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "")); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "", dummyWarmingCb)); checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 1 /*warming*/); cluster2->initialize_callback_(); From 2d472ea7555a6adb892c9b9aad420461f2368abd Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Fri, 22 Feb 2019 12:34:00 +0530 Subject: [PATCH 04/14] address review feedback Signed-off-by: Rama Chavali --- include/envoy/upstream/upstream.h | 5 ++ .../common/upstream/cluster_manager_impl.cc | 47 +++++++++---------- source/common/upstream/subset_lb.h | 2 +- source/common/upstream/upstream_impl.h | 9 ++++ .../upstream/cluster_manager_impl_test.cc | 2 +- test/mocks/upstream/mocks.h | 1 + 6 files changed, 38 insertions(+), 28 deletions(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 0eca9224fdea6..1834af5f7fbf1 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -354,6 +354,11 @@ class PrioritySet { */ virtual const std::vector& hostSetsPerPriority() const PURE; + /** + * @return true if the priority set does have any hosts in any priorities. + */ + virtual bool empty() const PURE; + /** * Parameter class for updateHosts. */ diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index ba7a2330933a0..def7b50a7b008 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -473,40 +473,35 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust cluster_warming_cb(cluster_name, ClusterWarmingState::Starting); cluster_entry->cluster_->initialize([this, cluster_name, cluster_warming_cb] { auto warming_it = warming_clusters_.find(cluster_name); - auto& cluster_entry = *warming_it->second; + auto& warming_cluster_entry = *warming_it->second; // If the cluster is being updated, we need to cancel any pending merged updates. // Otherwise, applyUpdates() will fire with a dangling cluster reference. updates_map_.erase(cluster_name); - // If the active cluster has priority sets and the warming cluster does not have them, it - // means that onConfigUpdate is triggered for EDS type of cluster is triggered with no - // reference to this cluster. In such cases, copy the active cluster priority set to the - // warming cluster otherwise the active cluster hosts will be cleared after warming is - // completed. See https://github.com/envoyproxy/envoy/issues/5168 for more context. + // If the active cluster has hosts in priority set and the warming cluster does not have them, + // means that onConfigUpdate was triggered by an EDS update that had no references to this + // cluster. In such cases, copy the active cluster priority set to the warming cluster to + // prevent the hosts from being cleared after warming. See + // https://github.com/envoyproxy/envoy/issues/5168 for more context. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { - auto& active_cluster_entry = *active_it->second; - if (active_cluster_entry.cluster_->prioritySet().hostSetsPerPriority().size() != 0 && - (cluster_entry.cluster_->prioritySet().hostSetsPerPriority().size() == 0 || - (cluster_entry.cluster_->prioritySet().hostSetsPerPriority().size() == 1 && - cluster_entry.cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size() == - 0))) { - ENVOY_LOG( - debug, - "copying host set from active cluster {} as warming cluster does not have host set", - cluster_name); - - const auto& host_sets = + const auto& active_cluster_entry = *active_it->second; + if (!active_cluster_entry.cluster_->prioritySet().empty() && + warming_cluster_entry.cluster_->prioritySet().empty()) { + ENVOY_LOG(debug, "copying host set from active cluster {} to warming cluster", + cluster_name); + const auto& active_host_sets = active_cluster_entry.cluster_->prioritySet().hostSetsPerPriority(); - for (size_t priority = 0; priority < host_sets.size(); ++priority) { - const auto& host_set = host_sets[priority]; - HostVectorConstSharedPtr hosts_copy(new HostVector(host_set->hosts())); + for (size_t priority = 0; priority < active_host_sets.size(); ++priority) { + const auto& active_host_set = active_host_sets[priority]; + HostVectorConstSharedPtr hosts_copy(new HostVector(active_host_set->hosts())); HostsPerLocalityConstSharedPtr hosts_per_locality_copy = - host_set->hostsPerLocality().clone(); - cluster_entry.cluster_->prioritySet().updateHosts( + active_host_set->hostsPerLocality().clone(); + warming_cluster_entry.cluster_->prioritySet().updateHosts( priority, HostSetImpl::partitionHosts(hosts_copy, hosts_per_locality_copy), - host_set->localityWeights(), {}, {}, absl::nullopt); + active_host_set->localityWeights(), {}, {}, + active_host_set->overprovisioningFactor()); } } } @@ -514,8 +509,8 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust warming_clusters_.erase(warming_it); ENVOY_LOG(info, "warming cluster {} complete", cluster_name); - createOrUpdateThreadLocalCluster(cluster_entry); - onClusterInit(*cluster_entry.cluster_); + createOrUpdateThreadLocalCluster(warming_cluster_entry); + onClusterInit(*warming_cluster_entry.cluster_); cluster_warming_cb(cluster_name, ClusterWarmingState::Finished); updateGauges(); }); diff --git a/source/common/upstream/subset_lb.h b/source/common/upstream/subset_lb.h index aba48fa8cb0d9..a342e8ec1a949 100644 --- a/source/common/upstream/subset_lb.h +++ b/source/common/upstream/subset_lb.h @@ -67,7 +67,7 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable(&getOrCreateHostSet(priority)); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 54e41f9a71750..aadd603c21b3d 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -413,6 +413,15 @@ class PrioritySetImpl : public PrioritySet { const HostVector& hosts_removed, absl::optional overprovisioning_factor = absl::nullopt) override; + bool empty() const override { + for (auto const& host_set : host_sets_) { + if (host_set->hosts().size() > 0) { + return false; + } + } + return true; + } + protected: // Allows subclasses of PrioritySetImpl to create their own type of HostSetImpl. virtual HostSetImplPtr createHostSet(uint32_t priority, diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index af06fa6b4c89c..65b2314a7cd37 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1240,7 +1240,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { cluster1->priority_set_.updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, - {}); + 200); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); EXPECT_CALL(*cluster1, initializePhase()).Times(0); diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h index 8e4cfd71589db..ce36debb3d35b 100644 --- a/test/mocks/upstream/mocks.h +++ b/test/mocks/upstream/mocks.h @@ -88,6 +88,7 @@ class MockPrioritySet : public PrioritySet { MOCK_CONST_METHOD1(addMemberUpdateCb, Common::CallbackHandle*(MemberUpdateCb callback)); MOCK_CONST_METHOD1(addPriorityUpdateCb, Common::CallbackHandle*(PriorityUpdateCb callback)); MOCK_CONST_METHOD0(hostSetsPerPriority, const std::vector&()); + MOCK_CONST_METHOD0(empty, bool()); MOCK_METHOD0(hostSetsPerPriority, std::vector&()); MOCK_METHOD6(updateHosts, void(uint32_t priority, UpdateHostsParams&& update_hosts_params, LocalityWeightsConstSharedPtr locality_weights, From 19fe1f44ec1e5c13e93af93265d07e1e8ab36d80 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Sat, 23 Feb 2019 10:04:31 +0530 Subject: [PATCH 05/14] add test for tls update verification Signed-off-by: Rama Chavali --- .../common/upstream/cluster_manager_impl.cc | 2 + .../upstream/cluster_manager_impl_test.cc | 71 +++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index def7b50a7b008..23f186203e46d 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -495,6 +495,8 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust active_cluster_entry.cluster_->prioritySet().hostSetsPerPriority(); for (size_t priority = 0; priority < active_host_sets.size(); ++priority) { const auto& active_host_set = active_host_sets[priority]; + // TODO(ramaraochavali): Can we skip these copies by exporting out const shared_ptr from + // HostSet? HostVectorConstSharedPtr hosts_copy(new HostVector(active_host_set->hosts())); HostsPerLocalityConstSharedPtr hosts_per_locality_copy = active_host_set->hostsPerLocality().clone(); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 65b2314a7cd37..d28710d721a6b 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1286,6 +1286,77 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); } +// Validates that TLS updates are triggered correctly when warming cluster has empty hosts. +TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHostsTriggersTlsUpdatesCorrectly) { + createWithLocalClusterUpdate(); + + InSequence s; + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::shared_ptr cluster1(new NiceMock()); + + // Set up the HostSet with 1 healthy, 1 degraded and 1 unhealthy for active cluster. + HostSharedPtr host1 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + host1->healthFlagSet(HostImpl::HealthFlag::DEGRADED_ACTIVE_HC); + HostSharedPtr host2 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + host2->healthFlagSet(HostImpl::HealthFlag::FAILED_ACTIVE_HC); + HostSharedPtr host3 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + + HostVector hosts{host1, host2, host3}; + auto hosts_ptr = std::make_shared(hosts); + + cluster1->priority_set_.updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, + 200); + + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(*cluster1, initializePhase()).Times(0); + EXPECT_CALL(*cluster1, initialize(_)); + + // Validate that TLS updates are triggered correctly. + EXPECT_CALL(local_cluster_update_, post(_, _, _)) + .WillOnce(Invoke([](uint32_t priority, const HostVector& hosts_added, + const HostVector& hosts_removed) -> void { + EXPECT_EQ(0, priority); + EXPECT_EQ(3, hosts_added.size()); + EXPECT_EQ(0, hosts_removed.size()); + })); + + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "", + dummyWarmingCb)); + EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); + cluster1->initialize_callback_(); + + EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); + + // Now trigger warming of this cluster with no hosts. + auto update_cluster = defaultStaticCluster("fake_cluster"); + update_cluster.mutable_per_connection_buffer_limit_bytes()->set_value(12345); + + std::shared_ptr cluster2(new NiceMock()); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(*cluster2, initializePhase()).Times(0); + + // Validate that TLS updates are triggered correctly after warming. + EXPECT_CALL(local_cluster_update_, post(_, _, _)) + .WillOnce(Invoke([](uint32_t priority, const HostVector& hosts_added, + const HostVector& hosts_removed) -> void { + EXPECT_EQ(0, priority); + EXPECT_EQ(3, hosts_added.size()); + EXPECT_EQ(0, hosts_removed.size()); + })); + + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "", dummyWarmingCb)); + cluster2->initialize_callback_(); + + EXPECT_EQ(cluster2->info_, cluster_manager_->get("fake_cluster")->info()); + + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); +} + TEST_F(ClusterManagerImplTest, addOrUpdateClusterStaticExists) { const std::string json = fmt::sprintf("{%s}", clustersJson({defaultStaticClusterJson("some_cluster")})); From c3bbf6868967058ffc09e4634eec659cd4e8b1fe Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 25 Feb 2019 09:52:04 +0530 Subject: [PATCH 06/14] wip Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 9 +++++++++ source/common/upstream/eds.cc | 1 + source/common/upstream/upstream_impl.h | 3 ++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 23f186203e46d..688a1862e52cd 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -487,6 +487,15 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second; + std::cout<<"before get"<<"\n"; + ClusterImplBase* cluster_impl_base = dynamic_cast(warming_cluster_entry.cluster_.get()); + std::cout<<"after get"<<"\n"; + if (cluster_impl_base) { + std::cout<<"cluster impl base is bot null" <<"\n"; + } else { + std::cout<<"cluster impl base is null" <<"\n"; + } + std::cout<<"upate empty ::"<update_empty_<<"\n"; if (!active_cluster_entry.cluster_->prioritySet().empty() && warming_cluster_entry.cluster_->prioritySet().empty()) { ENVOY_LOG(debug, "copying host set from active cluster {} to warming cluster", diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index f1895f37a88ad..fa1209726ba9a 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -47,6 +47,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: if (resources.empty()) { ENVOY_LOG(debug, "Missing ClusterLoadAssignment for {} in onConfigUpdate()", cluster_name_); info_->stats().update_empty_.inc(); + update_empty_ = true; onPreInitComplete(); return; } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index aadd603c21b3d..81dcd37050c25 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -415,7 +415,7 @@ class PrioritySetImpl : public PrioritySet { bool empty() const override { for (auto const& host_set : host_sets_) { - if (host_set->hosts().size() > 0) { + if (host_set->hosts().empty()) { return false; } } @@ -620,6 +620,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable callback) override; + bool update_empty_{false}; protected: ClusterImplBase(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, From 398f37a52a38a0ce50f527a36de6e12b9a0d4334 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 25 Feb 2019 11:50:09 +0530 Subject: [PATCH 07/14] handle empty host EDS update Signed-off-by: Rama Chavali --- include/envoy/upstream/upstream.h | 5 ++ .../common/upstream/cluster_manager_impl.cc | 12 +-- source/common/upstream/eds.cc | 7 +- .../upstream/health_discovery_service.h | 1 + source/common/upstream/logical_dns_cluster.cc | 2 +- source/common/upstream/original_dst_cluster.h | 2 +- source/common/upstream/upstream_impl.cc | 8 +- source/common/upstream/upstream_impl.h | 9 ++- .../upstream/cluster_manager_impl_test.cc | 80 ++++++++++++++++++- test/mocks/upstream/mocks.h | 1 + 10 files changed, 101 insertions(+), 26 deletions(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 1834af5f7fbf1..43ffb1d5d6d03 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -776,6 +776,11 @@ class Cluster { * @return the const PrioritySet for the cluster. */ virtual const PrioritySet& prioritySet() const PURE; + + /** + * @return true, if this cluster is being initialized by empty config update. + */ + virtual bool isBeingInitializedByEmptyConfigUpdate() const PURE; }; typedef std::shared_ptr ClusterSharedPtr; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index c95a5d1748b2f..a2550a1af9848 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -487,16 +487,8 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second; - std::cout<<"before get"<<"\n"; - ClusterImplBase* cluster_impl_base = dynamic_cast(warming_cluster_entry.cluster_.get()); - std::cout<<"after get"<<"\n"; - if (cluster_impl_base) { - std::cout<<"cluster impl base is bot null" <<"\n"; - } else { - std::cout<<"cluster impl base is null" <<"\n"; - } - std::cout<<"upate empty ::"<update_empty_<<"\n"; - if (!active_cluster_entry.cluster_->prioritySet().empty() && + if (warming_cluster_entry.cluster_->isBeingInitializedByEmptyConfigUpdate() && + !active_cluster_entry.cluster_->prioritySet().empty() && warming_cluster_entry.cluster_->prioritySet().empty()) { ENVOY_LOG(debug, "copying host set from active cluster {} to warming cluster", cluster_name); diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index fa1209726ba9a..e647a7a7f2886 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -47,8 +47,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: if (resources.empty()) { ENVOY_LOG(debug, "Missing ClusterLoadAssignment for {} in onConfigUpdate()", cluster_name_); info_->stats().update_empty_.inc(); - update_empty_ = true; - onPreInitComplete(); + onPreInitComplete(true); return; } if (resources.size() != 1) { @@ -121,7 +120,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std:: // If we didn't setup to initialize when our first round of health checking is complete, just // do it now. - onPreInitComplete(); + onPreInitComplete(false); } bool EdsClusterImpl::updateHostsPerLocality( @@ -164,7 +163,7 @@ bool EdsClusterImpl::updateHostsPerLocality( void EdsClusterImpl::onConfigUpdateFailed(const EnvoyException* e) { UNREFERENCED_PARAMETER(e); // We need to allow server startup to continue, even if we have a bad config. - onPreInitComplete(); + onPreInitComplete(false); } } // namespace Upstream diff --git a/source/common/upstream/health_discovery_service.h b/source/common/upstream/health_discovery_service.h index cd1b5f412b34f..1aa2af2ae108a 100644 --- a/source/common/upstream/health_discovery_service.h +++ b/source/common/upstream/health_discovery_service.h @@ -55,6 +55,7 @@ class HdsCluster : public Cluster, Logger::Loggable { Outlier::Detector* outlierDetector() override { return outlier_detector_.get(); } const Outlier::Detector* outlierDetector() const override { return outlier_detector_.get(); } void initialize(std::function callback) override; + bool isBeingInitializedByEmptyConfigUpdate() const override { return false; } // Creates and starts healthcheckers to its endpoints void startHealthchecks(AccessLog::AccessLogManager& access_log_manager, Runtime::Loader& runtime, diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index c5bcc8bbb67df..c8ade9c0f8708 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -134,7 +134,7 @@ void LogicalDnsCluster::startResolve() { } } - onPreInitComplete(); + onPreInitComplete(false); resolve_timer_->enableTimer(dns_refresh_rate_ms_); }); } diff --git a/source/common/upstream/original_dst_cluster.h b/source/common/upstream/original_dst_cluster.h index b88f18d57be29..3ed3774f45347 100644 --- a/source/common/upstream/original_dst_cluster.h +++ b/source/common/upstream/original_dst_cluster.h @@ -109,7 +109,7 @@ class OriginalDstCluster : public ClusterImplBase { void cleanup(); // ClusterImplBase - void startPreInit() override { onPreInitComplete(); } + void startPreInit() override { onPreInitComplete(false); } Event::Dispatcher& dispatcher_; const std::chrono::milliseconds cleanup_interval_ms_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 25bb107e13707..1e3a540041456 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -741,13 +741,13 @@ void ClusterImplBase::initialize(std::function callback) { startPreInit(); } -void ClusterImplBase::onPreInitComplete() { +void ClusterImplBase::onPreInitComplete(const bool empty_update) { // Protect against multiple calls. if (initialization_started_) { return; } initialization_started_ = true; - + empty_update_ = empty_update; ENVOY_LOG(debug, "initializing secondary cluster {} completed", info()->name()); init_manager_.initialize([this]() { onInitDone(); }); } @@ -1059,7 +1059,7 @@ void StaticClusterImpl::startPreInit() { } priority_state_manager_.reset(); - onPreInitComplete(); + onPreInitComplete(false); } bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, @@ -1372,7 +1372,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // multiple DNS names, this will return initialized after a single DNS resolution // completes. This is not perfect but is easier to code and unclear if the extra // complexity is needed so will start with this. - parent_.onPreInitComplete(); + parent_.onPreInitComplete(false); resolve_timer_->enableTimer(parent_.dns_refresh_rate_ms_); }); } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 81dcd37050c25..45afcfe1f77a9 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -415,7 +415,7 @@ class PrioritySetImpl : public PrioritySet { bool empty() const override { for (auto const& host_set : host_sets_) { - if (host_set->hosts().empty()) { + if (!host_set->hosts().empty()) { return false; } } @@ -587,6 +587,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable callback) override; - bool update_empty_{false}; protected: ClusterImplBase(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, @@ -637,8 +637,10 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable initialization_complete_callback_; uint64_t pending_initialize_health_checks_{}; + bool empty_update_{}; }; /** diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index d28710d721a6b..98ef5696b2f40 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1212,7 +1212,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { // This test validates that if warming cluster's initialization is triggered with empty hosts, it // does not clear the active cluster hosts. Regression to test to validate the behaviour observed in // https://github.com/envoyproxy/envoy/issues/5168. -TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { +TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdate) { const std::string json = R"EOF( { "clusters": [] @@ -1254,13 +1254,14 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); - // Now trigger warming of this cluster with no hosts. + // Now trigger warming of this cluster with empty config update. auto update_cluster = defaultStaticCluster("fake_cluster"); update_cluster.mutable_per_connection_buffer_limit_bytes()->set_value(12345); std::shared_ptr cluster2(new NiceMock()); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); + EXPECT_CALL(*cluster2, isBeingInitializedByEmptyConfigUpdate()).WillOnce(Return(true)); EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "", dummyWarmingCb)); checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 1 /*warming*/); cluster2->initialize_callback_(); @@ -1286,8 +1287,80 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); } +TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { + const std::string json = R"EOF( + { + "clusters": [] + } + )EOF"; + + create(parseBootstrapFromJson(json)); + + InSequence s; + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::shared_ptr cluster1(new NiceMock()); + + // Set up the HostSet with 1 healthy, 1 degraded and 1 unhealthy for active cluster. + HostSharedPtr host1 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + host1->healthFlagSet(HostImpl::HealthFlag::DEGRADED_ACTIVE_HC); + HostSharedPtr host2 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + host2->healthFlagSet(HostImpl::HealthFlag::FAILED_ACTIVE_HC); + HostSharedPtr host3 = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80"); + + HostVector hosts{host1, host2, host3}; + auto hosts_ptr = std::make_shared(hosts); + + cluster1->priority_set_.updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, + 200); + + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster1)); + EXPECT_CALL(*cluster1, initializePhase()).Times(0); + EXPECT_CALL(*cluster1, initialize(_)); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "", + dummyWarmingCb)); + checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); + EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); + cluster1->initialize_callback_(); + + EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); + checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); + + // Now trigger warming of the again with empty hosts. This validates the intentional empty + // hosts sent by management server are processed. auto update_cluster = + // defaultStaticCluster("fake_cluster"); + auto update_cluster = defaultStaticCluster("fake_cluster"); + update_cluster.mutable_per_connection_buffer_limit_bytes()->set_value(12345); + + std::shared_ptr cluster2(new NiceMock()); + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); + EXPECT_CALL(*cluster2, initializePhase()).Times(0); + EXPECT_CALL(*cluster2, isBeingInitializedByEmptyConfigUpdate()).WillOnce(Return(false)); + EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "", dummyWarmingCb)); + checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 1 /*warming*/); + cluster2->initialize_callback_(); + + checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); + + EXPECT_EQ(cluster2->info_, cluster_manager_->get("fake_cluster")->info()); + EXPECT_EQ(1UL, cluster_manager_->clusters().size()); + + // Validate that TLS cluster has empty priority set. + auto* tls_cluster = cluster_manager_->get(cluster2->info_->name()); + + EXPECT_EQ(true, tls_cluster->prioritySet().empty()); + + factory_.tls_.shutdownThread(); + + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); +} + // Validates that TLS updates are triggered correctly when warming cluster has empty hosts. -TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHostsTriggersTlsUpdatesCorrectly) { +TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdateTriggersTlsUpdatesCorrectly) { createWithLocalClusterUpdate(); InSequence s; @@ -1338,6 +1411,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHostsTriggersTlsUpdatesCor std::shared_ptr cluster2(new NiceMock()); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); + EXPECT_CALL(*cluster2, isBeingInitializedByEmptyConfigUpdate()).WillOnce(Return(true)); // Validate that TLS updates are triggered correctly after warming. EXPECT_CALL(local_cluster_update_, post(_, _, _)) diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h index ce36debb3d35b..a363d7203f7e6 100644 --- a/test/mocks/upstream/mocks.h +++ b/test/mocks/upstream/mocks.h @@ -160,6 +160,7 @@ class MockCluster : public Cluster { MOCK_METHOD1(initialize, void(std::function callback)); MOCK_CONST_METHOD0(initializePhase, InitializePhase()); MOCK_CONST_METHOD0(sourceAddress, const Network::Address::InstanceConstSharedPtr&()); + MOCK_CONST_METHOD0(isBeingInitializedByEmptyConfigUpdate, bool()); std::shared_ptr info_{new NiceMock()}; std::function initialize_callback_; From f0c1da955c09ec63d32edfdee43eaae4045dd416 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 25 Feb 2019 12:05:44 +0530 Subject: [PATCH 08/14] simplify condition and docs Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index a2550a1af9848..fc8e292b82c83 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -479,17 +479,15 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // Otherwise, applyUpdates() will fire with a dangling cluster reference. updates_map_.erase(cluster_name); - // If the active cluster has hosts in priority set and the warming cluster does not have them, - // means that onConfigUpdate was triggered by an EDS update that had no references to this - // cluster. In such cases, copy the active cluster priority set to the warming cluster to - // prevent the hosts from being cleared after warming. See + // If onConfigUpdate was triggered by an EDS update that had no references to this + // cluster and active cluster has some hosts, copy the active cluster priority set to the + // warming cluster to prevent the hosts from being cleared after warming. See // https://github.com/envoyproxy/envoy/issues/5168 for more context. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second; if (warming_cluster_entry.cluster_->isBeingInitializedByEmptyConfigUpdate() && - !active_cluster_entry.cluster_->prioritySet().empty() && - warming_cluster_entry.cluster_->prioritySet().empty()) { + !active_cluster_entry.cluster_->prioritySet().empty()) { ENVOY_LOG(debug, "copying host set from active cluster {} to warming cluster", cluster_name); const auto& active_host_sets = From 95dc0c42fcc6fd16447caa9aced79d0855086d69 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 25 Feb 2019 12:07:06 +0530 Subject: [PATCH 09/14] simplify condition and docs Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index fc8e292b82c83..6c7dfbf9f1739 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -481,8 +481,8 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // If onConfigUpdate was triggered by an EDS update that had no references to this // cluster and active cluster has some hosts, copy the active cluster priority set to the - // warming cluster to prevent the hosts from being cleared after warming. See - // https://github.com/envoyproxy/envoy/issues/5168 for more context. + // warming cluster to prevent the hosts from being cleared after warming. + // See https://github.com/envoyproxy/envoy/issues/5168 for more context. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second; From c932fc825ca560bc3b00ba6e608ae06244568dfc Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 25 Feb 2019 12:17:32 +0530 Subject: [PATCH 10/14] test docs Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 2 +- test/common/upstream/cluster_manager_impl_test.cc | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 6c7dfbf9f1739..db53e0f6bfcac 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -481,7 +481,7 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // If onConfigUpdate was triggered by an EDS update that had no references to this // cluster and active cluster has some hosts, copy the active cluster priority set to the - // warming cluster to prevent the hosts from being cleared after warming. + // warming cluster to prevent the hosts from being cleared after warming. // See https://github.com/envoyproxy/envoy/issues/5168 for more context. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 98ef5696b2f40..689857016f675 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1209,9 +1209,9 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(callbacks.get())); } -// This test validates that if warming cluster's initialization is triggered with empty hosts, it -// does not clear the active cluster hosts. Regression to test to validate the behaviour observed in -// https://github.com/envoyproxy/envoy/issues/5168. +// This test validates that if warming cluster's initialization is triggered via empty config +// update, it does not clear the active cluster hosts. Regression to test to validate the behaviour +// observed in https://github.com/envoyproxy/envoy/issues/5168. TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdate) { const std::string json = R"EOF( { @@ -1287,6 +1287,8 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdate) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); } +// Validates the behaviour when warming cluster's initialization is triggered with empty hosts EDS +// update. TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { const std::string json = R"EOF( { @@ -1329,9 +1331,8 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); - // Now trigger warming of the again with empty hosts. This validates the intentional empty - // hosts sent by management server are processed. auto update_cluster = - // defaultStaticCluster("fake_cluster"); + // Now trigger warming of the cluster with empty hosts. This validates the intentional empty + // hosts update sent by management server is processed correctly. auto update_cluster = defaultStaticCluster("fake_cluster"); update_cluster.mutable_per_connection_buffer_limit_bytes()->set_value(12345); @@ -1359,7 +1360,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); } -// Validates that TLS updates are triggered correctly when warming cluster has empty hosts. +// Validates that TLS updates are triggered correctly when warming cluster is initialized with empty config update. TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdateTriggersTlsUpdatesCorrectly) { createWithLocalClusterUpdate(); From c72f8d4d9f26fc0b7ad63956e804e966da624ea4 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 25 Feb 2019 12:25:46 +0530 Subject: [PATCH 11/14] format Signed-off-by: Rama Chavali --- test/common/upstream/cluster_manager_impl_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 689857016f675..eb356fca07b12 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1360,7 +1360,8 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster2.get())); } -// Validates that TLS updates are triggered correctly when warming cluster is initialized with empty config update. +// Validates that TLS updates are triggered correctly when warming cluster is initialized with empty +// config update. TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdateTriggersTlsUpdatesCorrectly) { createWithLocalClusterUpdate(); From f92ad648d59b39fea37692f930e6f1e3a4bcbe6b Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Tue, 26 Feb 2019 10:12:35 +0530 Subject: [PATCH 12/14] rename method and add more docs Signed-off-by: Rama Chavali --- include/envoy/upstream/upstream.h | 4 ++-- source/common/upstream/cluster_manager_impl.cc | 15 +++++++++++---- source/common/upstream/health_discovery_service.h | 2 +- source/common/upstream/upstream_impl.h | 2 +- test/common/upstream/cluster_manager_impl_test.cc | 6 +++--- test/mocks/upstream/mocks.h | 2 +- 6 files changed, 19 insertions(+), 12 deletions(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 43ffb1d5d6d03..1eb310f1df0a5 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -778,9 +778,9 @@ class Cluster { virtual const PrioritySet& prioritySet() const PURE; /** - * @return true, if this cluster is being initialized by empty config update. + * @return true, if this cluster is initialized by empty config update. */ - virtual bool isBeingInitializedByEmptyConfigUpdate() const PURE; + virtual bool initializedByEmptyConfig() const PURE; }; typedef std::shared_ptr ClusterSharedPtr; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index db53e0f6bfcac..bf5c34f789883 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -479,14 +479,21 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // Otherwise, applyUpdates() will fire with a dangling cluster reference. updates_map_.erase(cluster_name); - // If onConfigUpdate was triggered by an EDS update that had no references to this - // cluster and active cluster has some hosts, copy the active cluster priority set to the - // warming cluster to prevent the hosts from being cleared after warming. + // If management server sends a EDS response, for any other cluster grpc_mux_impl calls + // onConfigUpdate on this cluster with empty resources. + // See + // https://github.com/envoyproxy/envoy/blob/master/source/common/config/grpc_mux_impl.cc#L161 + // for more details. If the cluster is in fully initialized state, that would just increment + // update_empty stat. However, if the cluster is in warming state the initialization call back + // would be triggered and warming cluster would not have any hosts. So if onConfigUpdate was + // triggered by an EDS update that had no references to this cluster and active cluster has + // some hosts, copy the active cluster priority set to the warming cluster to prevent the + // hosts from being cleared after warming. // See https://github.com/envoyproxy/envoy/issues/5168 for more context. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second; - if (warming_cluster_entry.cluster_->isBeingInitializedByEmptyConfigUpdate() && + if (warming_cluster_entry.cluster_->initializedByEmptyConfig() && !active_cluster_entry.cluster_->prioritySet().empty()) { ENVOY_LOG(debug, "copying host set from active cluster {} to warming cluster", cluster_name); diff --git a/source/common/upstream/health_discovery_service.h b/source/common/upstream/health_discovery_service.h index 1aa2af2ae108a..3a7a9b505084f 100644 --- a/source/common/upstream/health_discovery_service.h +++ b/source/common/upstream/health_discovery_service.h @@ -55,7 +55,7 @@ class HdsCluster : public Cluster, Logger::Loggable { Outlier::Detector* outlierDetector() override { return outlier_detector_.get(); } const Outlier::Detector* outlierDetector() const override { return outlier_detector_.get(); } void initialize(std::function callback) override; - bool isBeingInitializedByEmptyConfigUpdate() const override { return false; } + bool initializedByEmptyConfig() const override { return false; } // Creates and starts healthcheckers to its endpoints void startHealthchecks(AccessLog::AccessLogManager& access_log_manager, Runtime::Loader& runtime, diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 45afcfe1f77a9..f11bc0bfb31c9 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -587,7 +587,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable cluster2(new NiceMock()); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); - EXPECT_CALL(*cluster2, isBeingInitializedByEmptyConfigUpdate()).WillOnce(Return(true)); + EXPECT_CALL(*cluster2, initializedByEmptyConfig()).WillOnce(Return(true)); EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "", dummyWarmingCb)); checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 1 /*warming*/); cluster2->initialize_callback_(); @@ -1339,7 +1339,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyHosts) { std::shared_ptr cluster2(new NiceMock()); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); - EXPECT_CALL(*cluster2, isBeingInitializedByEmptyConfigUpdate()).WillOnce(Return(false)); + EXPECT_CALL(*cluster2, initializedByEmptyConfig()).WillOnce(Return(false)); EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(update_cluster, "", dummyWarmingCb)); checkStats(1 /*added*/, 1 /*modified*/, 0 /*removed*/, 1 /*active*/, 1 /*warming*/); cluster2->initialize_callback_(); @@ -1413,7 +1413,7 @@ TEST_F(ClusterManagerImplTest, WarmingClusterWithEmptyConfigUpdateTriggersTlsUpd std::shared_ptr cluster2(new NiceMock()); EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)).WillOnce(Return(cluster2)); EXPECT_CALL(*cluster2, initializePhase()).Times(0); - EXPECT_CALL(*cluster2, isBeingInitializedByEmptyConfigUpdate()).WillOnce(Return(true)); + EXPECT_CALL(*cluster2, initializedByEmptyConfig()).WillOnce(Return(true)); // Validate that TLS updates are triggered correctly after warming. EXPECT_CALL(local_cluster_update_, post(_, _, _)) diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h index a363d7203f7e6..e9a67886840f3 100644 --- a/test/mocks/upstream/mocks.h +++ b/test/mocks/upstream/mocks.h @@ -160,7 +160,7 @@ class MockCluster : public Cluster { MOCK_METHOD1(initialize, void(std::function callback)); MOCK_CONST_METHOD0(initializePhase, InitializePhase()); MOCK_CONST_METHOD0(sourceAddress, const Network::Address::InstanceConstSharedPtr&()); - MOCK_CONST_METHOD0(isBeingInitializedByEmptyConfigUpdate, bool()); + MOCK_CONST_METHOD0(initializedByEmptyConfig, bool()); std::shared_ptr info_{new NiceMock()}; std::function initialize_callback_; From 899b12631df8900a469072792716acc197b3567d Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Tue, 26 Feb 2019 23:23:57 +0530 Subject: [PATCH 13/14] add link to xds protocol clause Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index bf5c34f789883..102529929374c 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -490,6 +490,9 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // some hosts, copy the active cluster priority set to the warming cluster to prevent the // hosts from being cleared after warming. // See https://github.com/envoyproxy/envoy/issues/5168 for more context. + // This also ensures that we adhere to the clause "When a requested resource is missing in a RDS + // or EDS update, Envoy will retain the last known value for this resource." as documented in + // https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second; From e968017b092e6318493c546a0dd1bf169d1595b4 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Tue, 26 Feb 2019 23:29:53 +0530 Subject: [PATCH 14/14] formatting Signed-off-by: Rama Chavali --- source/common/upstream/cluster_manager_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 102529929374c..a4883ddb746f1 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -490,9 +490,9 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust // some hosts, copy the active cluster priority set to the warming cluster to prevent the // hosts from being cleared after warming. // See https://github.com/envoyproxy/envoy/issues/5168 for more context. - // This also ensures that we adhere to the clause "When a requested resource is missing in a RDS - // or EDS update, Envoy will retain the last known value for this resource." as documented in - // https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md. + // This also ensures that we adhere to the clause "When a requested resource is missing in a + // RDS or EDS update, Envoy will retain the last known value for this resource." as documented + // in https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md. const auto active_it = active_clusters_.find(cluster_name); if (active_it != active_clusters_.end()) { const auto& active_cluster_entry = *active_it->second;