From b597c6d14ec5a2528095b281ee225d7a2b6e951d Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Wed, 18 Mar 2026 21:25:20 +0000 Subject: [PATCH] stats: refactor forEachHostMetric to avoid using clusters() Signed-off-by: Adi Suissa-Peleg --- envoy/upstream/cluster_manager.h | 8 ++++++++ source/common/upstream/cluster_manager_impl.h | 7 +++++++ source/common/upstream/host_utility.cc | 8 ++++---- test/mocks/upstream/cluster_manager.cc | 12 ++++++++++++ test/mocks/upstream/cluster_manager.h | 1 + test/server/admin/stats_handler_speed_test.cc | 6 ++++++ test/server/server_stats_flush_benchmark_test.cc | 1 + 7 files changed, 39 insertions(+), 4 deletions(-) diff --git a/envoy/upstream/cluster_manager.h b/envoy/upstream/cluster_manager.h index 887482b700d93..72af71f9085e3 100644 --- a/envoy/upstream/cluster_manager.h +++ b/envoy/upstream/cluster_manager.h @@ -340,6 +340,14 @@ class ClusterManager { */ virtual ClusterInfoMaps clusters() const PURE; + /** + * Iterates over all active clusters and invokes the callback for each. + * @param cb the callback to invoke for each active cluster. + * + * NOTE: This method is only thread safe on the main thread. It should not be called elsewhere. + */ + virtual void forEachActiveCluster(std::function cb) const PURE; + /** * Receives a cluster name and returns an active cluster (if found). * @param cluster_name the name of the cluster. diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 4469b677d021a..75c71314c3189 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -272,6 +272,13 @@ class ClusterManagerImpl : public ClusterManager, return clusters_maps; } + void forEachActiveCluster(std::function cb) const override { + ASSERT_IS_MAIN_OR_TEST_THREAD(); + for (const auto& [unused_name, cluster_data] : active_clusters_) { + cb(*cluster_data->cluster_); + } + } + OptRef getActiveCluster(const std::string& cluster_name) const override { ASSERT_IS_MAIN_OR_TEST_THREAD(); if (const auto& it = active_clusters_.find(cluster_name); it != active_clusters_.end()) { diff --git a/source/common/upstream/host_utility.cc b/source/common/upstream/host_utility.cc index 94c4bf51cb145..5a1f6ea242b84 100644 --- a/source/common/upstream/host_utility.cc +++ b/source/common/upstream/host_utility.cc @@ -175,15 +175,15 @@ void HostUtility::forEachHostMetric( const ClusterManager& cluster_manager, const std::function& counter_cb, const std::function& gauge_cb) { - for (const auto& [unused_name, cluster_ref] : cluster_manager.clusters().active_clusters_) { - Upstream::ClusterInfoConstSharedPtr cluster_info = cluster_ref.get().info(); + cluster_manager.forEachActiveCluster([&](const Cluster& cluster) { + Upstream::ClusterInfoConstSharedPtr cluster_info = cluster.info(); if (cluster_info->perEndpointStatsEnabled()) { const std::string cluster_name = Stats::Utility::sanitizeStatsName(cluster_info->observabilityName()); const Stats::TagVector& fixed_tags = cluster_info->statsScope().store().fixedTags(); - for (auto& host_set : cluster_ref.get().prioritySet().hostSetsPerPriority()) { + for (auto& host_set : cluster.prioritySet().hostSetsPerPriority()) { for (auto& host : host_set->hosts()) { Stats::TagVector tags; @@ -234,7 +234,7 @@ void HostUtility::forEachHostMetric( } } } - } + }); } } // namespace Upstream diff --git a/test/mocks/upstream/cluster_manager.cc b/test/mocks/upstream/cluster_manager.cc index 7be5b8e9af46f..f74e1f49a8e33 100644 --- a/test/mocks/upstream/cluster_manager.cc +++ b/test/mocks/upstream/cluster_manager.cc @@ -32,6 +32,12 @@ MockClusterManager::MockClusterManager() OptRef, ProtobufMessage::ValidationVisitor&) { return MockOdCdsApiHandle::create(); })); ON_CALL(*this, addOrUpdateCluster(_, _, _)).WillByDefault(Return(false)); + ON_CALL(*this, forEachActiveCluster(_)) + .WillByDefault(Invoke([this](std::function cb) { + for (const auto& [unused_name, cluster_ref] : clusters().active_clusters_) { + cb(cluster_ref.get()); + } + })); ON_CALL(*this, hasActiveClusters()).WillByDefault(Return(false)); } @@ -56,6 +62,12 @@ void MockClusterManager::initializeClusters(const std::vector& acti } ON_CALL(*this, clusters()).WillByDefault(Return(info_map)); + ON_CALL(*this, forEachActiveCluster(_)) + .WillByDefault(Invoke([this](std::function cb) { + for (const auto& [unused_name, cluster] : active_clusters_) { + cb(*cluster); + } + })); ON_CALL(*this, getActiveCluster(_)) .WillByDefault(Invoke([this](const std::string& cluster_name) -> OptRef { if (const auto& it = active_clusters_.find(cluster_name); it != active_clusters_.end()) { diff --git a/test/mocks/upstream/cluster_manager.h b/test/mocks/upstream/cluster_manager.h index f3bde539c9a2f..f9b4717704ac1 100644 --- a/test/mocks/upstream/cluster_manager.h +++ b/test/mocks/upstream/cluster_manager.h @@ -43,6 +43,7 @@ class MockClusterManager : public ClusterManager { MOCK_METHOD(absl::Status, initializeSecondaryClusters, (const envoy::config::bootstrap::v3::Bootstrap& bootstrap)); MOCK_METHOD(ClusterInfoMaps, clusters, (), (const)); + MOCK_METHOD(void, forEachActiveCluster, (std::function), (const)); MOCK_METHOD(OptRef, getActiveCluster, (const std::string& cluster_name), (const)); MOCK_METHOD(OptRef, getActiveOrWarmingCluster, (const std::string& cluster_name), (const)); diff --git a/test/server/admin/stats_handler_speed_test.cc b/test/server/admin/stats_handler_speed_test.cc index 92d5ad33bbfdc..6ccaffdfe98dd 100644 --- a/test/server/admin/stats_handler_speed_test.cc +++ b/test/server/admin/stats_handler_speed_test.cc @@ -22,6 +22,12 @@ class FastMockClusterManager : public testing::StrictMock cb) const override { + for (const auto& [unused_name, cluster_ref] : clusters_.active_clusters_) { + cb(cluster_ref.get()); + } + } + ClusterInfoMaps clusters_; std::vector> clusters_storage_; Stats::TestUtil::TestStore store_; diff --git a/test/server/server_stats_flush_benchmark_test.cc b/test/server/server_stats_flush_benchmark_test.cc index 45bf76a3a7b58..47bafb6040c56 100644 --- a/test/server/server_stats_flush_benchmark_test.cc +++ b/test/server/server_stats_flush_benchmark_test.cc @@ -24,6 +24,7 @@ namespace Envoy { class FastMockClusterManager : public testing::StrictMock { public: ClusterInfoMaps clusters() const override { return ClusterInfoMaps{}; } + void forEachActiveCluster(std::function) const override {} }; class TestSinkPredicates : public Stats::SinkPredicates {