diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index aa360d1db3ced..dc731e6038b3b 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -67,20 +66,12 @@ void ClusterManagerInitHelper::addCluster(ClusterManagerCluster& cm_cluster) { Cluster& cluster = cm_cluster.cluster(); if (cluster.initializePhase() == Cluster::InitializePhase::Primary) { // Remove the previous cluster before the cluster object is destroyed. - primary_init_clusters_.remove_if( - [name_to_remove = cluster.info()->name()](ClusterManagerCluster* cluster_iter) { - return cluster_iter->cluster().info()->name() == name_to_remove; - }); - primary_init_clusters_.push_back(&cm_cluster); + primary_init_clusters_.insert_or_assign(cm_cluster.cluster().info()->name(), &cm_cluster); cluster.initialize(initialize_cb); } else { ASSERT(cluster.initializePhase() == Cluster::InitializePhase::Secondary); // Remove the previous cluster before the cluster object is destroyed. - secondary_init_clusters_.remove_if( - [name_to_remove = cluster.info()->name()](ClusterManagerCluster* cluster_iter) { - return cluster_iter->cluster().info()->name() == name_to_remove; - }); - secondary_init_clusters_.push_back(&cm_cluster); + secondary_init_clusters_.insert_or_assign(cm_cluster.cluster().info()->name(), &cm_cluster); if (started_secondary_initialize_) { // This can happen if we get a second CDS update that adds new clusters after we have // already started secondary init. In this case, just immediately initialize. @@ -105,17 +96,22 @@ void ClusterManagerInitHelper::removeCluster(ClusterManagerCluster& cluster) { // There is a remote edge case where we can remove a cluster via CDS that has not yet been // initialized. When called via the remove cluster API this code catches that case. - std::list* cluster_list; + absl::flat_hash_map* cluster_map; if (cluster.cluster().initializePhase() == Cluster::InitializePhase::Primary) { - cluster_list = &primary_init_clusters_; + cluster_map = &primary_init_clusters_; } else { ASSERT(cluster.cluster().initializePhase() == Cluster::InitializePhase::Secondary); - cluster_list = &secondary_init_clusters_; + cluster_map = &secondary_init_clusters_; } // It is possible that the cluster we are removing has already been initialized, and is not - // present in the initializer list. If so, this is fine. - cluster_list->remove(&cluster); + // present in the initializer map. If so, this is fine as a CDS update may happen for a + // cluster with the same name. See the case "UpdateAlreadyInitialized" of the + // target //test/common/upstream:cluster_manager_impl_test. + auto iter = cluster_map->find(cluster.cluster().info()->name()); + if (iter != cluster_map->end() && iter->second == &cluster) { + cluster_map->erase(iter); + } ENVOY_LOG(debug, "cm init: init complete: cluster={} primary={} secondary={}", cluster.cluster().info()->name(), primary_init_clusters_.size(), secondary_init_clusters_.size()); @@ -124,13 +120,13 @@ void ClusterManagerInitHelper::removeCluster(ClusterManagerCluster& cluster) { void ClusterManagerInitHelper::initializeSecondaryClusters() { started_secondary_initialize_ = true; - // Cluster::initialize() method can modify the list of secondary_init_clusters_ to remove + // Cluster::initialize() method can modify the map of secondary_init_clusters_ to remove // the item currently being initialized, so we eschew range-based-for and do this complicated // dance to increment the iterator before calling initialize. for (auto iter = secondary_init_clusters_.begin(); iter != secondary_init_clusters_.end();) { - ClusterManagerCluster* cluster = *iter; + ClusterManagerCluster* cluster = iter->second; + ENVOY_LOG(debug, "initializing secondary cluster {}", iter->first); ++iter; - ENVOY_LOG(debug, "initializing secondary cluster {}", cluster->cluster().info()->name()); cluster->cluster().initialize([cluster, this] { onClusterInit(*cluster); }); } } diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 655616cf37e12..5f7fa6cdceedc 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -188,8 +188,8 @@ class ClusterManagerInitHelper : Logger::Loggable { CdsApi* cds_{}; ClusterManager::PrimaryClustersReadyCallback primary_clusters_initialized_callback_; ClusterManager::InitializationCompleteCallback initialized_callback_; - std::list primary_init_clusters_; - std::list secondary_init_clusters_; + absl::flat_hash_map primary_init_clusters_; + absl::flat_hash_map secondary_init_clusters_; State state_{State::Loading}; bool started_secondary_initialize_{}; }; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 08b3d01968e8c..cd8e167b29a78 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3318,6 +3318,13 @@ TEST_F(ClusterManagerInitHelperTest, UpdateAlreadyInitialized) { init_helper_.startInitializingSecondaryClusters(); } +TEST_F(ClusterManagerInitHelperTest, RemoveUnknown) { + InSequence s; + + NiceMock cluster; + init_helper_.removeCluster(cluster); +} + // If secondary clusters initialization triggered outside of CdsApiImpl::onConfigUpdate()'s // callback flows, sending ClusterLoadAssignment should not be paused before calling // ClusterManagerInitHelper::maybeFinishInitialize(). This case tests that