From a871846eaffefe6b0f1bad4128eda8ec8c1c8875 Mon Sep 17 00:00:00 2001 From: Shulin Jia Date: Fri, 23 Aug 2024 12:02:58 -0700 Subject: [PATCH] addressing comments Signed-off-by: Shulin Jia --- changelogs/current.yaml | 6 +- source/common/upstream/upstream_impl.cc | 12 +- test/extensions/clusters/eds/eds_test.cc | 166 ++++++++--------------- 3 files changed, 65 insertions(+), 119 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index cdb6a8e8fae35..aaf8670b2b39a 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -2,9 +2,6 @@ date: Pending behavior_changes: # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* -- area: upstream - change: | - Use previously calculated healthcheck when endpoints move priority levels repeatedly. - area: tracing change: | Removed support for (long deprecated) opentracing. See `issue 27401 @@ -72,6 +69,9 @@ minor_behavior_changes: bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* +- area: upstream + change: | + Use previously calculated healthcheck when endpoints move priority levels repeatedly. - area: dns change: | The DNS filter no longer returns FORMERR if a message has an ID of 0. diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index c301e111e169d..0d6e09a648a80 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -988,16 +988,14 @@ void MainPrioritySetImpl::updateCrossPriorityHostMap(uint32_t priority, } for (const auto& host : hosts_removed) { - auto existing_host = mutable_cross_priority_host_map_->find(addressToString(host->address())); - + const auto host_address = addressToString(host->address()); + const auto existing_host = mutable_cross_priority_host_map_->find(host_address); if (existing_host != mutable_cross_priority_host_map_->end()) { - // an earlier priority added this host, which indicates that the host moved from + // An earlier priority added this host, which indicates that the host moved from // a higher value priority to a lower value priority. We should not remove it from the // cross priority map. - if (existing_host->second->priority() < priority) { - continue; - } else { - mutable_cross_priority_host_map_->erase(addressToString(host->address())); + if (existing_host->second->priority() >= priority) { + mutable_cross_priority_host_map_->erase(host_address); } } } diff --git a/test/extensions/clusters/eds/eds_test.cc b/test/extensions/clusters/eds/eds_test.cc index 576bf964f2164..67e9cbb356d4e 100644 --- a/test/extensions/clusters/eds/eds_test.cc +++ b/test/extensions/clusters/eds/eds_test.cc @@ -1403,132 +1403,80 @@ TEST_F(EdsTest, EndpointMovedToNewPriority) { EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); } +} - // 1 -> 2 - // Moves all the endpoints to priority 1. - cluster_load_assignment.clear_endpoints(); - add_endpoint(80, 2); - add_endpoint(81, 2); - - doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - - { - // Priority 0 and 1 should now be empty. - auto& hosts1 = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); - EXPECT_EQ(hosts1.size(), 0); - auto& hosts2 = cluster_->prioritySet().hostSetsPerPriority()[1]->hosts(); - EXPECT_EQ(hosts2.size(), 0); - } - - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[2]->hosts(); - EXPECT_EQ(hosts.size(), 2); - - // The endpoints were healthy, so moving them around should preserve that. - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - } - - // 2 -> 3 - cluster_load_assignment.clear_endpoints(); - add_endpoint(80, 3); - add_endpoint(81, 3); - - doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - - { - // Priority 0, 1, and 2 should now be empty. - auto& hosts1 = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); - EXPECT_EQ(hosts1.size(), 0); - auto& hosts2 = cluster_->prioritySet().hostSetsPerPriority()[1]->hosts(); - EXPECT_EQ(hosts2.size(), 0); - auto& hosts3 = cluster_->prioritySet().hostSetsPerPriority()[2]->hosts(); - EXPECT_EQ(hosts3.size(), 0); - } - - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[3]->hosts(); - EXPECT_EQ(hosts.size(), 2); - - // The endpoints were healthy, so moving them around should preserve that. - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - } - - // 3 -> 2 - cluster_load_assignment.clear_endpoints(); - add_endpoint(80, 2); - add_endpoint(81, 2); - - doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); +// Verifies that if a endpoint is moved to a new priority multiple times, the health +// check value is preserved. +TEST_F(EdsTest, EndpointMovedToNewPriorityRepeated) { + envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment; + cluster_load_assignment.set_cluster_name("fare"); + resetCluster(); - { - // Priority 3 should now be empty. - auto& hosts1 = cluster_->prioritySet().hostSetsPerPriority()[3]->hosts(); - EXPECT_EQ(hosts1.size(), 0); - } + auto health_checker = std::make_shared(); + EXPECT_CALL(*health_checker, start()); + EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)).Times(2); + cluster_->setHealthChecker(health_checker); - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[2]->hosts(); - EXPECT_EQ(hosts.size(), 2); + auto add_endpoint = [&cluster_load_assignment](int port, int priority) { + auto* endpoints = cluster_load_assignment.add_endpoints(); + endpoints->set_priority(priority); - // The endpoints were healthy, so moving them around should preserve that. - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - } + 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); + }; - // 2 -> 1 - cluster_load_assignment.clear_endpoints(); - add_endpoint(80, 1); - add_endpoint(81, 1); + add_endpoint(80, 0); + add_endpoint(81, 0); doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); { - // Priority 2 should now be empty. - auto& hosts1 = cluster_->prioritySet().hostSetsPerPriority()[2]->hosts(); - EXPECT_EQ(hosts1.size(), 0); - } - - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[1]->hosts(); + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); EXPECT_EQ(hosts.size(), 2); - // The endpoints were healthy, so moving them around should preserve that. - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); + // Mark the hosts as healthy + for (auto& host : hosts) { + EXPECT_TRUE(host->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + host->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC); + host->healthFlagClear(Host::HealthFlag::PENDING_ACTIVE_HC); + } } - // 1 -> 0 - cluster_load_assignment.clear_endpoints(); - add_endpoint(80, 0); - add_endpoint(81, 0); + std::vector priority_levels = {1, 2, 3, 2, 1}; + for (uint32_t priority : priority_levels) { + cluster_load_assignment.clear_endpoints(); + add_endpoint(80, priority); + add_endpoint(81, priority); - doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); + doOnConfigUpdateVerifyNoThrow(cluster_load_assignment); - { - // Priority 1 should now be empty. - auto& hosts1 = cluster_->prioritySet().hostSetsPerPriority()[1]->hosts(); - EXPECT_EQ(hosts1.size(), 0); - } + { + for (uint32_t i = 0; i < cluster_->prioritySet().hostSetsPerPriority().size(); i++) { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[i]->hosts(); + if (i == priority) { + // Priorities equal to this one should have the endpoints with port 80 and 81 + EXPECT_EQ(hosts.size(), 2); + } else { + // Priorities not equal to this one should now be empty. + EXPECT_EQ(hosts.size(), 0); + } + } + } - { - auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts(); - EXPECT_EQ(hosts.size(), 2); + // Confirm that as priority level change, successful healthcheck status was preserved + { + auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[priority]->hosts(); - // The endpoints were healthy, so moving them around should preserve that. - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); - EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); + // The endpoints were healthy, so moving them around should preserve that. + EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_FALSE(hosts[0]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); + EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC)); + EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL)); + } } }