From 73d654a931a6b94c54cd42830a52e33434dbe44b Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 7 Aug 2018 17:31:12 +0700 Subject: [PATCH 01/17] health check: update address when needed Signed-off-by: Dhi Aurrahman --- include/envoy/upstream/host_description.h | 7 ++++- source/common/upstream/upstream_impl.cc | 7 +++++ source/common/upstream/upstream_impl.h | 4 +++ test/common/upstream/eds_test.cc | 32 +++++++++++++++++++++++ 4 files changed, 49 insertions(+), 1 deletion(-) diff --git a/include/envoy/upstream/host_description.h b/include/envoy/upstream/host_description.h index 9f5eb67f7ba20..1ccd0009f8c68 100644 --- a/include/envoy/upstream/host_description.h +++ b/include/envoy/upstream/host_description.h @@ -108,9 +108,14 @@ class HostDescription { * @return the address used to health check the host. */ virtual Network::Address::InstanceConstSharedPtr healthCheckAddress() const PURE; + + /** + * Sets the address used to health check the host. This noop by default. + */ + virtual void setHealthCheckAddress(Network::Address::InstanceConstSharedPtr){}; }; typedef std::shared_ptr HostDescriptionConstSharedPtr; -} // Upstream +} // namespace Upstream } // namespace Envoy diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 83117cf59c5fb..ffc3bfdddac52 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -903,6 +903,13 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, hosts_changed = true; } + // Did the health check address change? + if (*(*i)->healthCheckAddress() != *host->healthCheckAddress()) { + // If the health check address is updated, set the current host's health check address + // using the updated one. + (*i)->setHealthCheckAddress(host->healthCheckAddress()); + } + (*i)->weight(host->weight()); final_hosts.push_back(*i); i = current_hosts.erase(i); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index cbd158034fc7a..5a25558284672 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -121,6 +121,10 @@ class HostDescriptionImpl : virtual public HostDescription { Network::Address::InstanceConstSharedPtr healthCheckAddress() const override { return health_check_address_; } + void + setHealthCheckAddress(Network::Address::InstanceConstSharedPtr health_check_address) override { + health_check_address_ = health_check_address; + } const envoy::api::v2::core::Locality& locality() const override { return locality_; } protected: diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 33dd33578df34..8d7498373c152 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1018,6 +1018,7 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { EXPECT_TRUE(initialized); EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + uint32_t host_index = 0; { auto& first_hosts_per_locality = cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality(); @@ -1044,13 +1045,44 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { EXPECT_EQ(60, second_locality_weights[0]); EXPECT_EQ(2, second_hosts_per_locality.get()[1].size()); EXPECT_EQ(40, second_locality_weights[1]); + + // Find the index of host that has health check port value equals to 1000 (i.e. the first + // endpoint). + const auto& hosts = first_hosts_per_locality.get()[1]; + for (uint32_t i = 0; i < hosts.size(); ++i) { + if (hosts[i]->healthCheckAddress()->ip()->port() == 1000) { + host_index = i; + break; + } + } } + // Update the health check port value of the first endpoint. + const uint32_t updated_health_check_port = 8000; + cluster_load_assignment->mutable_endpoints(0) + ->mutable_lb_endpoints(0) + ->mutable_endpoint() + ->mutable_health_check_config() + ->set_port_value(updated_health_check_port); + // This should noop (regression test for earlier bug where we would still // rebuild). VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value()); + { + auto& first_hosts_per_locality = + cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality(); + EXPECT_EQ(2, first_hosts_per_locality.get().size()); + EXPECT_EQ(1, first_hosts_per_locality.get()[0].size()); + + // Check if the health check port value is updated. + const auto& hosts = first_hosts_per_locality.get()[1]; + const auto& current_health_check_port = hosts[host_index]->healthCheckAddress()->ip()->port(); + EXPECT_NE(port, current_health_check_port); + EXPECT_EQ(updated_health_check_port, current_health_check_port); + } + // Adjust locality weights, validate that we observe an update. cluster_load_assignment->mutable_endpoints(0)->mutable_load_balancing_weight()->set_value(60); cluster_load_assignment->mutable_endpoints(1)->mutable_load_balancing_weight()->set_value(40); From 1fb38829842e8472da26fc8a0ffab0d55ce887e8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 8 Aug 2018 06:06:45 +0700 Subject: [PATCH 02/17] Rebuild when health check config changes Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 16 +++++------- test/common/upstream/eds_test.cc | 34 +------------------------ 2 files changed, 8 insertions(+), 42 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 65a6c65790697..cf9879e428d74 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -864,9 +864,11 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, bool found = false; for (auto i = current_hosts.begin(); i != current_hosts.end();) { + const bool health_check_changed = + health_checker_ != nullptr && *(*i)->healthCheckAddress() != *host->healthCheckAddress(); // If we find a host matched based on address, we keep it. However we do change weight inline // so do that here. - if (*(*i)->address() == *host->address()) { + if (*(*i)->address() == *host->address() && !health_check_changed) { if (host->weight() > max_host_weight) { max_host_weight = host->weight(); } @@ -904,18 +906,14 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, hosts_changed = true; } - // Did the health check address change? - if (*(*i)->healthCheckAddress() != *host->healthCheckAddress()) { - // If the health check address is updated, set the current host's health check address - // using the updated one. - (*i)->setHealthCheckAddress(host->healthCheckAddress()); - } - (*i)->weight(host->weight()); final_hosts.push_back(*i); i = current_hosts.erase(i); found = true; } else { + if (health_check_changed) { + hosts_changed = true; + } i++; } } @@ -938,7 +936,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); // If there are removed hosts, check to see if we should only delete if unhealthy. - if (!current_hosts.empty() && dont_remove_healthy_hosts) { + if (!current_hosts.empty() && dont_remove_healthy_hosts && !hosts_changed) { for (auto i = current_hosts.begin(); i != current_hosts.end();) { if (!(*i)->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)) { if ((*i)->weight() > max_host_weight) { diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index eb6bb0ccd88cd..422248b16f2ab 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1019,7 +1019,6 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { EXPECT_TRUE(initialized); EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); - uint32_t host_index = 0; { auto& first_hosts_per_locality = cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality(); @@ -1046,44 +1045,13 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { EXPECT_EQ(60, second_locality_weights[0]); EXPECT_EQ(2, second_hosts_per_locality.get()[1].size()); EXPECT_EQ(40, second_locality_weights[1]); - - // Find the index of host that has health check port value equals to 1000 (i.e. the first - // endpoint). - const auto& hosts = first_hosts_per_locality.get()[1]; - for (uint32_t i = 0; i < hosts.size(); ++i) { - if (hosts[i]->healthCheckAddress()->ip()->port() == 1000) { - host_index = i; - break; - } - } } - // Update the health check port value of the first endpoint. - const uint32_t updated_health_check_port = 8000; - cluster_load_assignment->mutable_endpoints(0) - ->mutable_lb_endpoints(0) - ->mutable_endpoint() - ->mutable_health_check_config() - ->set_port_value(updated_health_check_port); - // This should noop (regression test for earlier bug where we would still // rebuild). VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value()); - { - auto& first_hosts_per_locality = - cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality(); - EXPECT_EQ(2, first_hosts_per_locality.get().size()); - EXPECT_EQ(1, first_hosts_per_locality.get()[0].size()); - - // Check if the health check port value is updated. - const auto& hosts = first_hosts_per_locality.get()[1]; - const auto& current_health_check_port = hosts[host_index]->healthCheckAddress()->ip()->port(); - EXPECT_NE(port, current_health_check_port); - EXPECT_EQ(updated_health_check_port, current_health_check_port); - } - // Adjust locality weights, validate that we observe an update. cluster_load_assignment->mutable_endpoints(0)->mutable_load_balancing_weight()->set_value(60); cluster_load_assignment->mutable_endpoints(1)->mutable_load_balancing_weight()->set_value(40); @@ -1110,4 +1078,4 @@ TEST_F(EdsTest, MalformedIP) { } } // namespace Upstream -} // namespace Envoy +} // namespace Envoy \ No newline at end of file From 8e257c1180d0346c5ca50b3788cf60cf9ad9e8ad Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 8 Aug 2018 06:25:17 +0700 Subject: [PATCH 03/17] Revert adding mutable health check address Signed-off-by: Dhi Aurrahman --- include/envoy/upstream/host_description.h | 7 +------ source/common/upstream/upstream_impl.h | 4 ---- test/common/upstream/eds_test.cc | 2 +- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/include/envoy/upstream/host_description.h b/include/envoy/upstream/host_description.h index 1ccd0009f8c68..9f5eb67f7ba20 100644 --- a/include/envoy/upstream/host_description.h +++ b/include/envoy/upstream/host_description.h @@ -108,14 +108,9 @@ class HostDescription { * @return the address used to health check the host. */ virtual Network::Address::InstanceConstSharedPtr healthCheckAddress() const PURE; - - /** - * Sets the address used to health check the host. This noop by default. - */ - virtual void setHealthCheckAddress(Network::Address::InstanceConstSharedPtr){}; }; typedef std::shared_ptr HostDescriptionConstSharedPtr; -} // namespace Upstream +} // Upstream } // namespace Envoy diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index fba7cfdde8331..3ef1281ab5fdd 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -122,10 +122,6 @@ class HostDescriptionImpl : virtual public HostDescription { Network::Address::InstanceConstSharedPtr healthCheckAddress() const override { return health_check_address_; } - void - setHealthCheckAddress(Network::Address::InstanceConstSharedPtr health_check_address) override { - health_check_address_ = health_check_address; - } const envoy::api::v2::core::Locality& locality() const override { return locality_; } protected: diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 422248b16f2ab..09f64217386f5 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1078,4 +1078,4 @@ TEST_F(EdsTest, MalformedIP) { } } // namespace Upstream -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From 10446cc4defd94a188c8de8bd28a6c66e22668b7 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 8 Aug 2018 10:06:44 +0700 Subject: [PATCH 04/17] Add test for post config update cluster rebuild Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 5 +- test/common/upstream/eds_test.cc | 93 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index cf9879e428d74..52f568926f898 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -864,11 +864,12 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, bool found = false; for (auto i = current_hosts.begin(); i != current_hosts.end();) { + const bool address_matched = *(*i)->address() == *host->address(); const bool health_check_changed = health_checker_ != nullptr && *(*i)->healthCheckAddress() != *host->healthCheckAddress(); // If we find a host matched based on address, we keep it. However we do change weight inline // so do that here. - if (*(*i)->address() == *host->address() && !health_check_changed) { + if (address_matched && !health_check_changed) { if (host->weight() > max_host_weight) { max_host_weight = host->weight(); } @@ -911,7 +912,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, i = current_hosts.erase(i); found = true; } else { - if (health_check_changed) { + if (address_matched && health_check_changed) { hosts_changed = true; } i++; diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 09f64217386f5..4baf1cc403d66 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1059,6 +1059,99 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value()); } +TEST_F(EdsTest, EndpointUpdateHealthCheckConfig) { + resetCluster(R"EOF( + name: name + connect_timeout: 0.25s + type: EDS + lb_policy: ROUND_ROBIN + drain_connections_on_host_removal: true + eds_cluster_config: + service_name: fare + eds_config: + api_config_source: + cluster_names: + - eds + refresh_delay: 1s + )EOF"); + + auto health_checker = std::make_shared(); + EXPECT_CALL(*health_checker, start()); + EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); + cluster_->setHealthChecker(health_checker); + + Protobuf::RepeatedPtrField resources; + auto* cluster_load_assignment = resources.Add(); + cluster_load_assignment->set_cluster_name("fare"); + + auto add_endpoint = [cluster_load_assignment](int port) { + auto* endpoints = cluster_load_assignment->add_endpoints(); + + auto* socket_address = endpoints->add_lb_endpoints() + ->mutable_endpoint() + ->mutable_address() + ->mutable_socket_address(); + socket_address->set_address("1.2.3.4"); + socket_address->set_port_value(port); + }; + + auto update_health_check_port = [cluster_load_assignment](const uint32_t index, + const uint32_t port) { + cluster_load_assignment->mutable_endpoints(index) + ->mutable_lb_endpoints(0) + ->mutable_endpoint() + ->mutable_health_check_config() + ->set_port_value(port); + }; + + add_endpoint(80); + add_endpoint(81); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + // Make sure the custer is rebuilt. + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 2); + + EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + // Mark the hosts as healthy + hosts[0]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + hosts[1]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + } + + const uint32_t new_health_check_port = 8000; + update_health_check_port(0, new_health_check_port); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 2); + // Make sure the first endpoint health check port is updated. + EXPECT_EQ(new_health_check_port, hosts[0]->healthCheckAddress()->ip()->port()); + + EXPECT_NE(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); + } + + update_health_check_port(1, new_health_check_port); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 2); + EXPECT_EQ(new_health_check_port, hosts[0]->healthCheckAddress()->ip()->port()); + + // Make sure the second endpoint health check port is updated. + EXPECT_EQ(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); + } +} + // Throw on adding a new resource with an invalid endpoint (since the given address is invalid). TEST_F(EdsTest, MalformedIP) { Protobuf::RepeatedPtrField resources; From 96bc0c204c7c63d092627f8fe089ccff7cdd83c3 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 8 Aug 2018 13:40:04 +0700 Subject: [PATCH 05/17] Add tests for !drain_connections_on_host_removal Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 9 +- test/common/upstream/eds_test.cc | 115 ++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 52f568926f898..f45e729dc9b15 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -867,8 +867,8 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool address_matched = *(*i)->address() == *host->address(); const bool health_check_changed = health_checker_ != nullptr && *(*i)->healthCheckAddress() != *host->healthCheckAddress(); - // If we find a host matched based on address, we keep it. However we do change weight inline - // so do that here. + // If we find a host matched based on address and the health check address is not changed, we + // keep it. However we do change weight inline so do that here. if (address_matched && !health_check_changed) { if (host->weight() > max_host_weight) { max_host_weight = host->weight(); @@ -912,9 +912,6 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, i = current_hosts.erase(i); found = true; } else { - if (address_matched && health_check_changed) { - hosts_changed = true; - } i++; } } @@ -937,7 +934,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, const bool dont_remove_healthy_hosts = health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval(); // If there are removed hosts, check to see if we should only delete if unhealthy. - if (!current_hosts.empty() && dont_remove_healthy_hosts && !hosts_changed) { + if (!current_hosts.empty() && dont_remove_healthy_hosts) { for (auto i = current_hosts.begin(); i != current_hosts.end();) { if (!(*i)->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)) { if ((*i)->weight() > max_host_weight) { diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 4baf1cc403d66..0ea93b1f70cb2 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1060,6 +1060,119 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { } TEST_F(EdsTest, EndpointUpdateHealthCheckConfig) { + resetCluster(R"EOF( + name: name + connect_timeout: 0.25s + type: EDS + lb_policy: ROUND_ROBIN + eds_cluster_config: + service_name: fare + eds_config: + api_config_source: + cluster_names: + - eds + refresh_delay: 1s + )EOF"); + + auto health_checker = std::make_shared(); + EXPECT_CALL(*health_checker, start()); + EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); + cluster_->setHealthChecker(health_checker); + + Protobuf::RepeatedPtrField resources; + auto* cluster_load_assignment = resources.Add(); + cluster_load_assignment->set_cluster_name("fare"); + + auto add_endpoint = [cluster_load_assignment](int port) { + auto* endpoints = cluster_load_assignment->add_endpoints(); + + auto* socket_address = endpoints->add_lb_endpoints() + ->mutable_endpoint() + ->mutable_address() + ->mutable_socket_address(); + socket_address->set_address("1.2.3.4"); + socket_address->set_port_value(port); + }; + + auto update_health_check_port = [cluster_load_assignment](const uint32_t index, + const uint32_t port) { + cluster_load_assignment->mutable_endpoints(index) + ->mutable_lb_endpoints(0) + ->mutable_endpoint() + ->mutable_health_check_config() + ->set_port_value(port); + }; + + add_endpoint(80); + add_endpoint(81); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + // Make sure the custer is rebuilt. + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 2); + + EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + + // Mark the hosts as healthy + hosts[0]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + hosts[1]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + } + + const uint32_t new_health_check_port = 8000; + update_health_check_port(0, new_health_check_port); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 3); + // Make sure the first endpoint health check port is updated. + EXPECT_EQ(new_health_check_port, hosts[0]->healthCheckAddress()->ip()->port()); + + EXPECT_NE(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); + EXPECT_NE(new_health_check_port, hosts[2]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(81, hosts[1]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(80, hosts[2]->healthCheckAddress()->ip()->port()); + + EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + + // The old hosts still active. The health checker continues to do health checking to these + // hosts, until they are removed. + EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_FALSE(hosts[2]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + } + + update_health_check_port(1, new_health_check_port); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 4); + EXPECT_EQ(new_health_check_port, hosts[0]->healthCheckAddress()->ip()->port()); + + // Make sure the second endpoint health check port is updated. + EXPECT_EQ(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); + + EXPECT_EQ(81, hosts[2]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(80, hosts[3]->healthCheckAddress()->ip()->port()); + + EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + + // The old hosts still active. + EXPECT_FALSE(hosts[2]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_FALSE(hosts[3]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + } +} + +TEST_F(EdsTest, EndpointUpdateHealthCheckConfigWithDrainConnectionsOnRemoval) { resetCluster(R"EOF( name: name connect_timeout: 0.25s @@ -1130,6 +1243,8 @@ TEST_F(EdsTest, EndpointUpdateHealthCheckConfig) { { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + // Since drain_connections_on_host_removal is set to true, the old hosts are removed + // immediately. EXPECT_EQ(hosts.size(), 2); // Make sure the first endpoint health check port is updated. EXPECT_EQ(new_health_check_port, hosts[0]->healthCheckAddress()->ip()->port()); From 99943d7ae1a8a27c5fc1aa214959fd642299a950 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 15 Aug 2018 09:04:24 +0700 Subject: [PATCH 06/17] Bring back if health_check_changed check Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5a94e7859f8c1..0115c26c83226 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -899,8 +899,16 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( } auto existing_host = all_hosts_.find(host->address()->asString()); + const bool existing_host_found = existing_host != all_hosts_.end(); - if (existing_host != all_hosts_.end()) { + // When an existing host is found, check if the health check address of that host is requested + // to be changed. This condition matters if the cluster active health checker is activated. If + // true, we need to rebuild. + const bool health_check_changed = + existing_host_found && health_checker_ != nullptr && + *existing_host->second->healthCheckAddress() != *host->healthCheckAddress(); + + if (existing_host_found && !health_check_changed) { existing_hosts_for_current_priority.emplace(existing_host->first); // If we find a host matched based on address and the health check address is not changed, we // keep it. However we do change weight inline so do that here. From 110d3397e8f1c3af693ae19b63866d09a3702e99 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 15 Aug 2018 09:10:46 +0700 Subject: [PATCH 07/17] Remove unused lines Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 0115c26c83226..ab07fa742348b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -951,15 +951,6 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( hosts_changed = true; } - // Did health check configuration change? - const bool health_check_changed = - health_checker_ != nullptr && - existing_host->second->healthCheckAddress() != host->healthCheckAddress(); - if (health_check_changed) { - // If health check configuration changed, we need to rebuild. - hosts_changed = true; - } - existing_host->second->weight(host->weight()); final_hosts.push_back(existing_host->second); updated_hosts[existing_host->second->address()->asString()] = existing_host->second; From 458d53e8bb95e95acab7f624f21dd5c9a802e737 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 15 Aug 2018 09:13:27 +0700 Subject: [PATCH 08/17] Remove bad merge lines Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index ab07fa742348b..542a25933ec2f 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -910,8 +910,6 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( if (existing_host_found && !health_check_changed) { existing_hosts_for_current_priority.emplace(existing_host->first); - // If we find a host matched based on address and the health check address is not changed, we - // keep it. However we do change weight inline so do that here. // If we find a host matched based on address, we keep it. However we do change weight inline // so do that here. if (host->weight() > max_host_weight) { From f87cf0e1fd83669aa9430fbf607aa6990a9d234f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 15 Aug 2018 17:35:17 +0700 Subject: [PATCH 09/17] Update comment Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 542a25933ec2f..171c67c2eb088 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -901,11 +901,11 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( auto existing_host = all_hosts_.find(host->address()->asString()); const bool existing_host_found = existing_host != all_hosts_.end(); - // When an existing host is found, check if the health check address of that host is requested - // to be changed. This condition matters if the cluster active health checker is activated. If - // true, we need to rebuild. + // If we have found a new host matched, based on address, to an existing one: check if the + // health check address of that host is different. If it is, we need to rebuild. Note that this + // checking matters only if the cluster's active health checker is activated. const bool health_check_changed = - existing_host_found && health_checker_ != nullptr && + health_checker_ != nullptr && existing_host_found && *existing_host->second->healthCheckAddress() != *host->healthCheckAddress(); if (existing_host_found && !health_check_changed) { From a940f154906024a8c9ad2c49e47162e83a0290ad Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 08:00:17 +0700 Subject: [PATCH 10/17] review: rename and add more comments Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 171c67c2eb088..243bc95cb86b3 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -898,17 +898,22 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( continue; } + // To match a new host with an existing host means comparing their addresses. auto existing_host = all_hosts_.find(host->address()->asString()); const bool existing_host_found = existing_host != all_hosts_.end(); - // If we have found a new host matched, based on address, to an existing one: check if the - // health check address of that host is different. If it is, we need to rebuild. Note that this - // checking matters only if the cluster's active health checker is activated. - const bool health_check_changed = + // Check if in-place host update should be skipped, i.e. when the following criteria are met + // (currently there is only one criterion, but we might add more in the future): + // - The cluster health checker is activated and a new host is matched with the existing one, + // but the health check address is different. + const bool no_inplace_host_update = health_checker_ != nullptr && existing_host_found && *existing_host->second->healthCheckAddress() != *host->healthCheckAddress(); - if (existing_host_found && !health_check_changed) { + // When there is a match and we decided to do in-place update, we potentially updating the + // host's health check flag and metadata. Afterwards, the host is pushed back into the + // final_hosts, i.e. hosts that should be preserved in the current priority. + if (existing_host_found && !no_inplace_host_update) { existing_hosts_for_current_priority.emplace(existing_host->first); // If we find a host matched based on address, we keep it. However we do change weight inline // so do that here. From 72f31f42314b331b76993075af9dcac0868cb559 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 08:00:41 +0700 Subject: [PATCH 11/17] review: refactor eds test Signed-off-by: Dhi Aurrahman --- test/common/upstream/eds_test.cc | 238 ++++++++++++------------------- 1 file changed, 94 insertions(+), 144 deletions(-) diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 354ae8beea6cc..3ad7fda62a001 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -75,6 +75,89 @@ class EdsTest : public testing::Test { NiceMock local_info_; }; +class EdsWithHealthCheckUpdateTest : public EdsTest { +protected: + EdsWithHealthCheckUpdateTest() {} + + // Build the initial cluster with some endpoints. + void initializeCluster(const std::vector endpoint_ports, + const bool drain_connections_on_host_removal) { + resetCluster(drain_connections_on_host_removal); + + auto health_checker = std::make_shared(); + EXPECT_CALL(*health_checker, start()); + EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); + cluster_->setHealthChecker(health_checker); + + cluster_load_assignment_ = resources_.Add(); + cluster_load_assignment_->set_cluster_name("fare"); + + for (const auto& port : endpoint_ports) { + addEndpoint(port); + } + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources_, "")); + + // Make sure the cluster is rebuilt. + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); + EXPECT_EQ(hosts.size(), 2); + + EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + + // Mark the hosts as healthy + hosts[0]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + hosts[1]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + } + } + + void resetCluster(const bool drain_connections_on_host_removal) { + const std::string config = R"EOF( + name: name + connect_timeout: 0.25s + type: EDS + lb_policy: ROUND_ROBIN + drain_connections_on_host_removal: {} + eds_cluster_config: + service_name: fare + eds_config: + api_config_source: + cluster_names: + - eds + refresh_delay: 1s + )EOF"; + EdsTest::resetCluster(fmt::format(config, drain_connections_on_host_removal)); + } + + void addEndpoint(const uint32_t port) { + auto* endpoints = cluster_load_assignment_->add_endpoints(); + auto* socket_address = endpoints->add_lb_endpoints() + ->mutable_endpoint() + ->mutable_address() + ->mutable_socket_address(); + socket_address->set_address("1.2.3.4"); + socket_address->set_port_value(port); + } + + void updateHealthCheckPort(const uint32_t index, const uint32_t port) { + cluster_load_assignment_->mutable_endpoints(index) + ->mutable_lb_endpoints(0) + ->mutable_endpoint() + ->mutable_health_check_config() + ->set_port_value(port); + + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources_, "")); + + // Always rebuild if health check config is changed. + EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + } + + Protobuf::RepeatedPtrField resources_; + envoy::api::v2::ClusterLoadAssignment* cluster_load_assignment_; +}; + // Negative test for protoc-gen-validate constraints. TEST_F(EdsTest, ValidateFail) { Protobuf::RepeatedPtrField resources; @@ -1152,75 +1235,12 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value()); } -TEST_F(EdsTest, EndpointUpdateHealthCheckConfig) { - resetCluster(R"EOF( - name: name - connect_timeout: 0.25s - type: EDS - lb_policy: ROUND_ROBIN - eds_cluster_config: - service_name: fare - eds_config: - api_config_source: - cluster_names: - - eds - refresh_delay: 1s - )EOF"); - - auto health_checker = std::make_shared(); - EXPECT_CALL(*health_checker, start()); - EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); - cluster_->setHealthChecker(health_checker); - - Protobuf::RepeatedPtrField resources; - auto* cluster_load_assignment = resources.Add(); - cluster_load_assignment->set_cluster_name("fare"); - - auto add_endpoint = [cluster_load_assignment](int port) { - auto* endpoints = cluster_load_assignment->add_endpoints(); - - auto* socket_address = endpoints->add_lb_endpoints() - ->mutable_endpoint() - ->mutable_address() - ->mutable_socket_address(); - socket_address->set_address("1.2.3.4"); - socket_address->set_port_value(port); - }; - - auto update_health_check_port = [cluster_load_assignment](const uint32_t index, - const uint32_t port) { - cluster_load_assignment->mutable_endpoints(index) - ->mutable_lb_endpoints(0) - ->mutable_endpoint() - ->mutable_health_check_config() - ->set_port_value(port); - }; - - add_endpoint(80); - add_endpoint(81); - - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); - // Make sure the custer is rebuilt. - EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); - - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); - EXPECT_EQ(hosts.size(), 2); - - EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - - // Mark the hosts as healthy - hosts[0]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); - hosts[1]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); - } +TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { + initializeCluster({80, 81}, false); const uint32_t new_health_check_port = 8000; - update_health_check_port(0, new_health_check_port); - - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); - EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + updateHealthCheckPort(0, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 3); @@ -1234,17 +1254,13 @@ TEST_F(EdsTest, EndpointUpdateHealthCheckConfig) { EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - // The old hosts still active. The health checker continues to do health checking to these + // The old hosts are still active. The health checker continues to do health checking to these // hosts, until they are removed. EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); EXPECT_FALSE(hosts[2]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); } - update_health_check_port(1, new_health_check_port); - - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); - EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); - + updateHealthCheckPort(1, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 4); @@ -1259,81 +1275,19 @@ TEST_F(EdsTest, EndpointUpdateHealthCheckConfig) { EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - // The old hosts still active. + // The old hosts are still active. EXPECT_FALSE(hosts[2]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); EXPECT_FALSE(hosts[3]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); } } -TEST_F(EdsTest, EndpointUpdateHealthCheckConfigWithDrainConnectionsOnRemoval) { - resetCluster(R"EOF( - name: name - connect_timeout: 0.25s - type: EDS - lb_policy: ROUND_ROBIN - drain_connections_on_host_removal: true - eds_cluster_config: - service_name: fare - eds_config: - api_config_source: - cluster_names: - - eds - refresh_delay: 1s - )EOF"); - - auto health_checker = std::make_shared(); - EXPECT_CALL(*health_checker, start()); - EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); - cluster_->setHealthChecker(health_checker); - - Protobuf::RepeatedPtrField resources; - auto* cluster_load_assignment = resources.Add(); - cluster_load_assignment->set_cluster_name("fare"); - - auto add_endpoint = [cluster_load_assignment](int port) { - auto* endpoints = cluster_load_assignment->add_endpoints(); - - auto* socket_address = endpoints->add_lb_endpoints() - ->mutable_endpoint() - ->mutable_address() - ->mutable_socket_address(); - socket_address->set_address("1.2.3.4"); - socket_address->set_port_value(port); - }; - - auto update_health_check_port = [cluster_load_assignment](const uint32_t index, - const uint32_t port) { - cluster_load_assignment->mutable_endpoints(index) - ->mutable_lb_endpoints(0) - ->mutable_endpoint() - ->mutable_health_check_config() - ->set_port_value(port); - }; - - add_endpoint(80); - add_endpoint(81); - - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); - // Make sure the custer is rebuilt. - EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); - - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); - EXPECT_EQ(hosts.size(), 2); - - EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - // Mark the hosts as healthy - hosts[0]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); - hosts[1]->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); - } +TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfigWithDrainConnectionsOnRemoval) { + // Set the cluster to drain connections on host removal. + initializeCluster({80, 81}, true); const uint32_t new_health_check_port = 8000; - update_health_check_port(0, new_health_check_port); - - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); - EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); + updateHealthCheckPort(0, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); // Since drain_connections_on_host_removal is set to true, the old hosts are removed @@ -1345,11 +1299,7 @@ TEST_F(EdsTest, EndpointUpdateHealthCheckConfigWithDrainConnectionsOnRemoval) { EXPECT_NE(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); } - update_health_check_port(1, new_health_check_port); - - VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); - EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value()); - + updateHealthCheckPort(1, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 2); From 038d1146ebaa09b3907fa1665e75d57183d0b0d8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 08:03:10 +0700 Subject: [PATCH 12/17] Remove newlines Signed-off-by: Dhi Aurrahman --- test/common/upstream/eds_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 3ad7fda62a001..9914170a9775a 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -1239,7 +1239,6 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { initializeCluster({80, 81}, false); const uint32_t new_health_check_port = 8000; - updateHealthCheckPort(0, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); @@ -1286,7 +1285,6 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfigWithDrainCon initializeCluster({80, 81}, true); const uint32_t new_health_check_port = 8000; - updateHealthCheckPort(0, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); From 7d7aea09ae4cb2d400a4e8b1c04ca8422f0667bd Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 09:04:07 +0700 Subject: [PATCH 13/17] Rename some functions and vars Signed-off-by: Dhi Aurrahman --- test/common/upstream/eds_test.cc | 33 ++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index e6b6fef72699f..a9db413e6afa8 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -141,7 +141,7 @@ class EdsWithHealthCheckUpdateTest : public EdsTest { socket_address->set_port_value(port); } - void updateHealthCheckPort(const uint32_t index, const uint32_t port) { + void updateEndpointHealthCheckPortAtIndex(const uint32_t index, const uint32_t port) { cluster_load_assignment_->mutable_endpoints(index) ->mutable_lb_endpoints(0) ->mutable_endpoint() @@ -1236,10 +1236,13 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) { } TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { - initializeCluster({80, 81}, false); - + const std::vector endpoint_ports = {80, 81}; const uint32_t new_health_check_port = 8000; - updateHealthCheckPort(0, new_health_check_port); + + // Initialize the cluster with two endpoints without draining connections on host removal. + initializeCluster(endpoint_ports, false); + + updateEndpointHealthCheckPortAtIndex(0, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 3); @@ -1248,8 +1251,8 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { EXPECT_NE(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); EXPECT_NE(new_health_check_port, hosts[2]->healthCheckAddress()->ip()->port()); - EXPECT_EQ(81, hosts[1]->healthCheckAddress()->ip()->port()); - EXPECT_EQ(80, hosts[2]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(endpoint_ports[1], hosts[1]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(endpoint_ports[0], hosts[2]->healthCheckAddress()->ip()->port()); EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); @@ -1259,7 +1262,7 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { EXPECT_FALSE(hosts[2]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); } - updateHealthCheckPort(1, new_health_check_port); + updateEndpointHealthCheckPortAtIndex(1, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 4); @@ -1268,8 +1271,8 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { // Make sure the second endpoint health check port is updated. EXPECT_EQ(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); - EXPECT_EQ(81, hosts[2]->healthCheckAddress()->ip()->port()); - EXPECT_EQ(80, hosts[3]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(endpoint_ports[1], hosts[2]->healthCheckAddress()->ip()->port()); + EXPECT_EQ(endpoint_ports[0], hosts[3]->healthCheckAddress()->ip()->port()); EXPECT_TRUE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); EXPECT_TRUE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); @@ -1281,11 +1284,13 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfig) { } TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfigWithDrainConnectionsOnRemoval) { - // Set the cluster to drain connections on host removal. - initializeCluster({80, 81}, true); - + const std::vector endpoint_ports = {80, 81}; const uint32_t new_health_check_port = 8000; - updateHealthCheckPort(0, new_health_check_port); + + // Initialize the cluster with two endpoints with draining connections on host removal. + initializeCluster(endpoint_ports, true); + + updateEndpointHealthCheckPortAtIndex(0, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); // Since drain_connections_on_host_removal is set to true, the old hosts are removed @@ -1297,7 +1302,7 @@ TEST_F(EdsWithHealthCheckUpdateTest, EndpointUpdateHealthCheckConfigWithDrainCon EXPECT_NE(new_health_check_port, hosts[1]->healthCheckAddress()->ip()->port()); } - updateHealthCheckPort(1, new_health_check_port); + updateEndpointHealthCheckPortAtIndex(1, new_health_check_port); { auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 2); From 0f8d6f9300b6a2cfc59a31e47ea7dbb13dcc142b Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 09:09:49 +0700 Subject: [PATCH 14/17] Kick CI Signed-off-by: Dhi Aurrahman From 295a8cb43ff277e9ba5fb4d887002534a65ca112 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 09:09:58 +0700 Subject: [PATCH 15/17] Kick CI Signed-off-by: Dhi Aurrahman From 9dd4941d237c579c207fd081d1602cf72e216340 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 09:29:04 +0700 Subject: [PATCH 16/17] Use 'skip' instead of 'no'? Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 2b45019e29641..3184a9ebe6cdd 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -913,14 +913,14 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( // (currently there is only one criterion, but we might add more in the future): // - The cluster health checker is activated and a new host is matched with the existing one, // but the health check address is different. - const bool no_inplace_host_update = + const bool skip_inplace_host_update = health_checker_ != nullptr && existing_host_found && *existing_host->second->healthCheckAddress() != *host->healthCheckAddress(); // When there is a match and we decided to do in-place update, we potentially updating the // host's health check flag and metadata. Afterwards, the host is pushed back into the // final_hosts, i.e. hosts that should be preserved in the current priority. - if (existing_host_found && !no_inplace_host_update) { + if (existing_host_found && !skip_inplace_host_update) { existing_hosts_for_current_priority.emplace(existing_host->first); // If we find a host matched based on address, we keep it. However we do change weight inline // so do that here. From 274f9da8c4bf13aac5c8a3f581d3ca073745d2ed Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 Aug 2018 13:14:06 +0700 Subject: [PATCH 17/17] review: updating -> update Signed-off-by: Dhi Aurrahman --- source/common/upstream/upstream_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 3184a9ebe6cdd..1f191c007f73b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -917,9 +917,9 @@ bool BaseDynamicClusterImpl::updateDynamicHostList( health_checker_ != nullptr && existing_host_found && *existing_host->second->healthCheckAddress() != *host->healthCheckAddress(); - // When there is a match and we decided to do in-place update, we potentially updating the - // host's health check flag and metadata. Afterwards, the host is pushed back into the - // final_hosts, i.e. hosts that should be preserved in the current priority. + // When there is a match and we decided to do in-place update, we potentially update the host's + // health check flag and metadata. Afterwards, the host is pushed back into the final_hosts, + // i.e. hosts that should be preserved in the current priority. if (existing_host_found && !skip_inplace_host_update) { existing_hosts_for_current_priority.emplace(existing_host->first); // If we find a host matched based on address, we keep it. However we do change weight inline