diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5c781c9bd70a..cd93a394f518 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -823,6 +823,8 @@ HostMapConstSharedPtr MainPrioritySetImpl::crossPriorityHostMap() const { if (mutable_cross_priority_host_map_ != nullptr) { const_cross_priority_host_map_ = std::move(mutable_cross_priority_host_map_); ASSERT(mutable_cross_priority_host_map_ == nullptr); + ENVOY_LOG(debug, "cross_priority host map, moving mutable to const, len: {}", + const_cross_priority_host_map_->size()); } return const_cross_priority_host_map_; } @@ -839,14 +841,18 @@ void MainPrioritySetImpl::updateCrossPriorityHostMap(const HostVector& hosts_add if (mutable_cross_priority_host_map_ == nullptr) { // Copy old read only host map to mutable host map. mutable_cross_priority_host_map_ = std::make_shared(*const_cross_priority_host_map_); + ENVOY_LOG(debug, "cross_priority host map, copying from const, len: {}", + const_cross_priority_host_map_->size()); } for (const auto& host : hosts_removed) { mutable_cross_priority_host_map_->erase(addressToString(host->address())); + ENVOY_LOG(debug, "cross_priority host map, removing: {}", addressToString(host->address())); } for (const auto& host : hosts_added) { mutable_cross_priority_host_map_->insert({addressToString(host->address()), host}); + ENVOY_LOG(debug, "cross_priority host map, adding: {}", addressToString(host->address())); } } diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc index 1200a49052da..4dae5d8f6758 100644 --- a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc +++ b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc @@ -98,14 +98,10 @@ Cluster::~Cluster() { void Cluster::startPreInit() { // If we are attaching to a pre-populated cache we need to initialize our hosts. - std::unique_ptr hosts_added; dns_cache_->iterateHostMap( [&](absl::string_view host, const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& info) { - addOrUpdateHost(host, info, hosts_added); + addOrUpdateHost(host, info); }); - if (hosts_added) { - updatePriorityState(*hosts_added, {}); - } onPreInitComplete(); } @@ -237,9 +233,11 @@ bool Cluster::ClusterInfo::checkIdle() { void Cluster::addOrUpdateHost( absl::string_view host, - const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info, - std::unique_ptr& hosts_added) { - Upstream::LogicalHostSharedPtr emplaced_host; + const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) { + Upstream::HostVector hosts_added, hosts_removed; + Upstream::LogicalHostSharedPtr new_host = std::make_shared( + info(), std::string{host}, host_info->address(), host_info->addressList(), + dummy_locality_lb_endpoint_, dummy_lb_endpoint_, nullptr, time_source_); { absl::WriterMutexLock lock{&host_map_lock_}; @@ -251,7 +249,6 @@ void Cluster::addOrUpdateHost( // future. const auto host_map_it = host_map_.find(host); if (host_map_it != host_map_.end()) { - // If we only have an address change, we can do that swap inline without any other updates. // The appropriate R/W locking is in place to allow this. The details of this locking are: // - Hosts are not thread local, they are global. // - We take a read lock when reading the address and a write lock when changing it. @@ -267,44 +264,31 @@ void Cluster::addOrUpdateHost( // semantics, meaning the cache would expose multiple addresses and the // cluster would create multiple logical hosts based on those addresses. // We will leave this is a follow up depending on need. - ASSERT(host_info == host_map_it->second.shared_host_info_); ASSERT(host_map_it->second.shared_host_info_->address() != host_map_it->second.logical_host_->address()); - ENVOY_LOG(debug, "updating dfproxy cluster host address '{}'", host); - host_map_it->second.logical_host_->setNewAddresses( - host_info->address(), host_info->addressList(), dummy_lb_endpoint_); - return; - } - ENVOY_LOG(debug, "adding new dfproxy cluster host '{}'", host); + // remove the old host + hosts_removed.emplace_back(host_map_it->second.logical_host_); + ENVOY_LOG(debug, "updating dfproxy cluster host address '{}'", host); + host_map_.erase(host_map_it); + host_map_.try_emplace(host, host_info, new_host); - emplaced_host = host_map_ - .try_emplace(host, host_info, - std::make_shared( - info(), std::string{host}, host_info->address(), - host_info->addressList(), dummy_locality_lb_endpoint_, - dummy_lb_endpoint_, nullptr, time_source_)) - .first->second.logical_host_; + } else { + ENVOY_LOG(debug, "adding new dfproxy cluster host '{}'", host); + host_map_.try_emplace(host, host_info, new_host); + } + hosts_added.emplace_back(new_host); } - ASSERT(emplaced_host); - if (hosts_added == nullptr) { - hosts_added = std::make_unique(); - } - hosts_added->emplace_back(emplaced_host); + ASSERT(hosts_added.size() > 0); + updatePriorityState(hosts_added, hosts_removed); } void Cluster::onDnsHostAddOrUpdate( const std::string& host, const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) { - ENVOY_LOG(debug, "Adding host info for {}", host); - - std::unique_ptr hosts_added; - addOrUpdateHost(host, host_info, hosts_added); - if (hosts_added != nullptr) { - ASSERT(!hosts_added->empty()); - updatePriorityState(*hosts_added, {}); - } + ENVOY_LOG(debug, "Adding/Updating host info for {}", host); + addOrUpdateHost(host, host_info); } void Cluster::updatePriorityState(const Upstream::HostVector& hosts_added, diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.h b/source/extensions/clusters/dynamic_forward_proxy/cluster.h index f974474b3e90..82e62c01711d 100644 --- a/source/extensions/clusters/dynamic_forward_proxy/cluster.h +++ b/source/extensions/clusters/dynamic_forward_proxy/cluster.h @@ -161,8 +161,7 @@ class Cluster : public Upstream::BaseDynamicClusterImpl, void addOrUpdateHost(absl::string_view host, - const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info, - std::unique_ptr& hosts_added) + const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) ABSL_LOCKS_EXCLUDED(host_map_lock_); void updatePriorityState(const Upstream::HostVector& hosts_added, diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc index 75f48aadfdf7..7143bebb6da3 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -86,7 +86,7 @@ class ClusterTest : public testing::Test, } })); if (!existing_hosts.empty()) { - EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(existing_hosts.size()), SizeIs(0))); + EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(1), SizeIs(0))).Times(existing_hosts.size()); } cluster_->initialize([] {}); } @@ -257,6 +257,7 @@ TEST_F(ClusterTest, BasicFlow) { // After changing the address, LB will immediately resolve the new address with a refresh. updateTestHostAddress("host1:0", "2.3.4.5"); + EXPECT_CALL(*this, onMemberUpdateCb(SizeIs(1), SizeIs(1))); update_callbacks_->onDnsHostAddOrUpdate("host1:0", host_map_["host1:0"]); EXPECT_EQ(1UL, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ("2.3.4.5:0",