From ef9aba8482c01e13d35282c5e502bc222f8db93e Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Wed, 27 Mar 2019 00:23:15 -0400 Subject: [PATCH 1/2] upstream: fix bug causing crashes during priority host moves This fixes a bug where hosts that were moved between priorities would not be included in the hosts_added vector, resulting in crashes if the same host was moved multiple times when used with active health checking: if a host was moved between priorities twice, it would first get removed from the health checker, then on the second move the health checker would crash as it would attempt to remove a host it didn't know about. We fix this by explicitly adding the existing host to the list of added hosts iff the host was previously in a different priority. Uncovering this bug lead to the discovery of a bug in the batch updating done during EDS: std::set_difference assumes that the provided ranges are both *sorted*, which is not generally true during this update flow. This meant that the filtering of hosts that were added/removed did not work correctly, and would produce inconsistent result dependent on the ordering of the host pointers in the unordered_map. We fix this by using a standard for loop instead of std::set_difference. Not only is this more correct, it should also be faster for large sets as it performs the filtering in O(n) instead of O(n^2). Signed-off-by: Snow Pettersen --- source/common/upstream/upstream_impl.cc | 8 ++++++-- test/common/upstream/eds_test.cc | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index ef436d8a084c7..bec672542e134 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -202,8 +202,11 @@ HostVector filterHosts(const std::unordered_set& hosts, HostVector net_hosts; net_hosts.reserve(hosts.size()); - std::set_difference(hosts.begin(), hosts.end(), excluded_hosts.begin(), excluded_hosts.end(), - std::inserter(net_hosts, net_hosts.begin())); + for (const auto& host : hosts) { + if (excluded_hosts.find(host) == excluded_hosts.end()) { + net_hosts.emplace_back(host); + } + } return net_hosts; } @@ -1138,6 +1141,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, // Did the priority change? if (host->priority() != existing_host->second->priority()) { existing_host->second->priority(host->priority()); + hosts_added_to_current_priority.emplace_back(existing_host->second); } existing_host->second->weight(host->weight()); diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index bb22655fda823..ea1802c88283c 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -680,6 +680,12 @@ TEST_F(EdsTest, EndpointMoved) { add_endpoint(81, 0); add_endpoint(80, 1); + // Verify that no hosts gets added or removed to/from the PrioritySet. + cluster_->prioritySet().addMemberUpdateCb([&](const auto& added, const auto& removed) { + EXPECT_TRUE(added.empty()); + EXPECT_TRUE(removed.empty()); + }); + VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, "")); { From 3a1ae99e04229c4c271cef241c2981419858fbba Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Wed, 27 Mar 2019 06:04:59 -0400 Subject: [PATCH 2/2] format Signed-off-by: Snow Pettersen --- test/common/upstream/eds_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index ea1802c88283c..c3e640e637144 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -682,9 +682,9 @@ TEST_F(EdsTest, EndpointMoved) { // Verify that no hosts gets added or removed to/from the PrioritySet. cluster_->prioritySet().addMemberUpdateCb([&](const auto& added, const auto& removed) { - EXPECT_TRUE(added.empty()); - EXPECT_TRUE(removed.empty()); - }); + EXPECT_TRUE(added.empty()); + EXPECT_TRUE(removed.empty()); + }); VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, ""));