From 1db18d297c5e3ca33ede1836172960b8ca218d12 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 31 Aug 2021 00:33:02 +0000 Subject: [PATCH 1/7] per filter chain scope Signed-off-by: Yuchen Dai --- source/server/filter_chain_manager_impl.cc | 8 ++++---- source/server/filter_chain_manager_impl.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/source/server/filter_chain_manager_impl.cc b/source/server/filter_chain_manager_impl.cc index f1a74a70e78cd..e250903c933e2 100644 --- a/source/server/filter_chain_manager_impl.cc +++ b/source/server/filter_chain_manager_impl.cc @@ -31,7 +31,9 @@ Network::Address::InstanceConstSharedPtr fakeAddress() { PerFilterChainFactoryContextImpl::PerFilterChainFactoryContextImpl( Configuration::FactoryContext& parent_context, Init::Manager& init_manager) - : parent_context_(parent_context), init_manager_(init_manager) {} + : parent_context_(parent_context), + filter_chain_scope_(parent_context_.listenerScope().createScope("")), + init_manager_(init_manager) {} bool PerFilterChainFactoryContextImpl::drainClose() const { return is_draining_.load() || parent_context_.drainDecision().drainClose(); @@ -135,9 +137,7 @@ PerFilterChainFactoryContextImpl::getTransportSocketFactoryContext() const { return parent_context_.getTransportSocketFactoryContext(); } -Stats::Scope& PerFilterChainFactoryContextImpl::listenerScope() { - return parent_context_.listenerScope(); -} +Stats::Scope& PerFilterChainFactoryContextImpl::listenerScope() { return *filter_chain_scope_; } bool PerFilterChainFactoryContextImpl::isQuicListener() const { return parent_context_.isQuicListener(); diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h index 8218e89777127..c6121f021021f 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -87,6 +87,8 @@ class PerFilterChainFactoryContextImpl : public Configuration::FilterChainFactor private: Configuration::FactoryContext& parent_context_; + // filter_chain_scope_ has the exact prefix as listener owners scope. + Stats::ScopePtr filter_chain_scope_; Init::Manager& init_manager_; std::atomic is_draining_{false}; }; From e3ed57c7392de50fda69c4fd741d9893c7b9e946 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 31 Aug 2021 09:15:58 +0000 Subject: [PATCH 2/7] add test and fix scope() Signed-off-by: Yuchen Dai --- source/server/filter_chain_manager_impl.cc | 4 +- source/server/filter_chain_manager_impl.h | 2 + test/integration/server.h | 6 +++ test/integration/server_stats.h | 9 +++++ test/integration/xds_integration_test.cc | 43 +++++++++++++++------- test/test_common/utility.cc | 20 ++++++++++ test/test_common/utility.h | 13 +++++++ 7 files changed, 81 insertions(+), 16 deletions(-) diff --git a/source/server/filter_chain_manager_impl.cc b/source/server/filter_chain_manager_impl.cc index e250903c933e2..01e63097f2c63 100644 --- a/source/server/filter_chain_manager_impl.cc +++ b/source/server/filter_chain_manager_impl.cc @@ -31,7 +31,7 @@ Network::Address::InstanceConstSharedPtr fakeAddress() { PerFilterChainFactoryContextImpl::PerFilterChainFactoryContextImpl( Configuration::FactoryContext& parent_context, Init::Manager& init_manager) - : parent_context_(parent_context), + : parent_context_(parent_context), scope_(parent_context_.scope().createScope("")), filter_chain_scope_(parent_context_.listenerScope().createScope("")), init_manager_(init_manager) {} @@ -103,7 +103,7 @@ Envoy::Runtime::Loader& PerFilterChainFactoryContextImpl::runtime() { return parent_context_.runtime(); } -Stats::Scope& PerFilterChainFactoryContextImpl::scope() { return parent_context_.scope(); } +Stats::Scope& PerFilterChainFactoryContextImpl::scope() { return *scope_; } Singleton::Manager& PerFilterChainFactoryContextImpl::singletonManager() { return parent_context_.singletonManager(); diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h index c6121f021021f..421f63fefe322 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -87,6 +87,8 @@ class PerFilterChainFactoryContextImpl : public Configuration::FilterChainFactor private: Configuration::FactoryContext& parent_context_; + // The scope that has empty prefix. + Stats::ScopePtr scope_; // filter_chain_scope_ has the exact prefix as listener owners scope. Stats::ScopePtr filter_chain_scope_; Init::Manager& init_manager_; diff --git a/test/integration/server.h b/test/integration/server.h index 94d5f79504cbe..f501e01cb10b7 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -462,6 +462,12 @@ class IntegrationTestServer : public Logger::Loggable, notifyingStatsAllocator().waitForCounterExists(name); } + void waitForGaugeDestroyed( + const std::string& name, + std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) override { + ASSERT_TRUE(TestUtility::waitForGaugeDestroyed(statStore(), name, time_system_, timeout)); + } + void waitUntilHistogramHasSamples( const std::string& name, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) override { diff --git a/test/integration/server_stats.h b/test/integration/server_stats.h index 66cb7e07e7d27..336bbba6b1c37 100644 --- a/test/integration/server_stats.h +++ b/test/integration/server_stats.h @@ -66,6 +66,15 @@ class IntegrationTestServerStats { waitForGaugeEq(const std::string& name, uint64_t value, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) PURE; + /** + * Wait for a gauge to be destroyed. Note that MockStatStore does not destroy stat. + * @param name gauge name. + * @param timeout amount of time to wait before asserting false, or 0 for no timeout. + */ + virtual void + waitForGaugeDestroyed(const std::string& name, + std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) PURE; + /** * Counter lookup. This is not thread safe, since we don't get a consistent * snapshot, uses counters() instead for this behavior. diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc index 7a11e6a156470..1869b1096ca27 100644 --- a/test/integration/xds_integration_test.cc +++ b/test/integration/xds_integration_test.cc @@ -291,7 +291,9 @@ class LdsInplaceUpdateHttpIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: - LdsInplaceUpdateHttpIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) {} + LdsInplaceUpdateHttpIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) { + use_real_stats_ = true; + } void inplaceInitialize(bool add_default_filter_chain = false) { autonomous_upstream_ = true; @@ -301,6 +303,9 @@ class LdsInplaceUpdateHttpIntegrationTest std::string tls_inspector_config = ConfigHelper::tlsInspectorFilter(); config_helper_.addListenerFilter(tls_inspector_config); config_helper_.addSslConfig(); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { hcm.mutable_stat_prefix()->assign("hcm0"); }); config_helper_.addConfigModifier([this, add_default_filter_chain]( envoy::config::bootstrap::v3::Bootstrap& bootstrap) { if (!use_default_balancer_) { @@ -335,6 +340,7 @@ class LdsInplaceUpdateHttpIntegrationTest ->mutable_routes(0) ->mutable_route() ->set_cluster("cluster_1"); + hcm_config.mutable_stat_prefix()->assign("hcm1"); config_blob->PackFrom(hcm_config); bootstrap.mutable_static_resources()->mutable_clusters()->Add()->MergeFrom( *bootstrap.mutable_static_resources()->mutable_clusters(0)); @@ -381,7 +387,7 @@ class LdsInplaceUpdateHttpIntegrationTest } } - void expectConnenctionServed(std::string alpn = "alpn0") { + void expectConnectionServed(std::string alpn = "alpn0") { auto codec_client_after_config_update = createHttpCodec(alpn); expectResponseHeaderConnectionClose(*codec_client_after_config_update, false); codec_client_after_config_update->close(); @@ -395,7 +401,7 @@ class LdsInplaceUpdateHttpIntegrationTest }; // Verify that http response on filter chain 1 and default filter chain have "Connection: close" -// header when these 2 filter chains are deleted during the listener update. +// header when these 2 filter chains are deleted during the listener update. TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigDeletingFilterChain) { inplaceInitialize(/*add_default_filter_chain=*/true); @@ -403,12 +409,6 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigDeletingFilterChain) { auto codec_client_0 = createHttpCodec("alpn0"); auto codec_client_default = createHttpCodec("alpndefault"); - Cleanup cleanup([c1 = codec_client_1.get(), c0 = codec_client_0.get(), - c_default = codec_client_default.get()]() { - c1->close(); - c0->close(); - c_default->close(); - }); ConfigHelper new_config_helper( version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); new_config_helper.addConfigModifier( @@ -422,12 +422,20 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigDeletingFilterChain) { test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1); test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); + test_server_->waitForGaugeGe("http.hcm0.downstream_cx_active", 1); + test_server_->waitForGaugeGe("http.hcm1.downstream_cx_active", 1); + expectResponseHeaderConnectionClose(*codec_client_1, true); expectResponseHeaderConnectionClose(*codec_client_default, true); test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 0); expectResponseHeaderConnectionClose(*codec_client_0, false); - expectConnenctionServed(); + expectConnectionServed(); + + codec_client_1->close(); + test_server_->waitForGaugeDestroyed("http.hcm1.downstream_cx_active"); + codec_client_0->close(); + codec_client_default->close(); } // Verify that http clients of filter chain 0 survives if new listener config adds new filter @@ -438,15 +446,19 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigAddingFilterChain) { auto codec_client_0 = createHttpCodec("alpn0"); Cleanup cleanup0([c0 = codec_client_0.get()]() { c0->close(); }); + test_server_->waitForGaugeGe("http.hcm0.downstream_cx_active", 1); + ConfigHelper new_config_helper( version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); new_config_helper.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); - listener->mutable_filter_chains()->Add()->MergeFrom(*listener->mutable_filter_chains(1)); + // Note that HCM2 copies the stats prefix from HCM0 + listener->mutable_filter_chains()->Add()->MergeFrom(*listener->mutable_filter_chains(0)); *listener->mutable_filter_chains(2) ->mutable_filter_chain_match() ->mutable_application_protocols(0) = "alpn2"; + auto default_filter_chain = bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_default_filter_chain(); default_filter_chain->MergeFrom(*listener->mutable_filter_chains(1)); @@ -458,6 +470,9 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigAddingFilterChain) { auto codec_client_2 = createHttpCodec("alpn2"); auto codec_client_default = createHttpCodec("alpndefault"); + // 1 connection from filter chain 0 and 1 connection from filter chain 2. + test_server_->waitForGaugeGe("http.hcm0.downstream_cx_active", 2); + Cleanup cleanup2([c2 = codec_client_2.get(), c_default = codec_client_default.get()]() { c2->close(); c_default->close(); @@ -465,7 +480,7 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigAddingFilterChain) { expectResponseHeaderConnectionClose(*codec_client_2, false); expectResponseHeaderConnectionClose(*codec_client_default, false); expectResponseHeaderConnectionClose(*codec_client_0, false); - expectConnenctionServed(); + expectConnectionServed(); } // Verify that http clients of default filter chain is drained and recreated if the default filter @@ -493,7 +508,7 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigUpdatingDefaultFilterCha Cleanup cleanup2([c_default_v3 = codec_client_default_v3.get()]() { c_default_v3->close(); }); expectResponseHeaderConnectionClose(*codec_client_default, true); expectResponseHeaderConnectionClose(*codec_client_default_v3, false); - expectConnenctionServed(); + expectConnectionServed(); } // Verify that balancer is inherited. Test only default balancer because ExactConnectionBalancer @@ -515,7 +530,7 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, OverlappingFilterChainServesNewConne new_config_helper.setLds("1"); test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1); expectResponseHeaderConnectionClose(*codec_client_0, false); - expectConnenctionServed(); + expectConnectionServed(); } // Verify default filter chain update is filter chain only update. diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index fe0db0bcf7ee8..2400e29585a8d 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -238,6 +238,26 @@ AssertionResult TestUtility::waitForGaugeEq(Stats::Store& store, const std::stri return AssertionSuccess(); } +AssertionResult TestUtility::waitForGaugeDestroyed(Stats::Store& store, const std::string& name, + Event::TestTimeSystem& time_system, + std::chrono::milliseconds timeout) { + Event::TestTimeSystem::RealTimeBound bound(timeout); + while (findGauge(store, name) != nullptr) { + time_system.advanceTimeWait(std::chrono::milliseconds(10)); + if (timeout != std::chrono::milliseconds::zero() && !bound.withinBound()) { + std::string current_value; + if (findGauge(store, name)) { + current_value = absl::StrCat(findGauge(store, name)->value()); + } else { + current_value = "nil"; + } + return AssertionFailure() << fmt::format( + "timed out waiting for {} destroyed, current value {}", name, current_value); + } + } + return AssertionSuccess(); +} + AssertionResult TestUtility::waitUntilHistogramHasSamples(Stats::Store& store, const std::string& name, Event::TestTimeSystem& time_system, diff --git a/test/test_common/utility.h b/test/test_common/utility.h index c355b50643394..38552f9a14b3a 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -277,6 +277,19 @@ class TestUtility { Event::TestTimeSystem& time_system, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()); + /** + * Wait for a gauge to be destroyed. + * @param store supplies the stats store. + * @param name gauge name. + * @param time_system the time system to use for waiting. + * @param timeout the maximum time to wait before timing out, or 0 for no timeout. + * @return AssertionSuccess() if the gauge was == to the value within the timeout, else + * AssertionFailure(). + */ + static AssertionResult + waitForGaugeDestroyed(Stats::Store& store, const std::string& name, + Event::TestTimeSystem& time_system, + std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()); /** * Wait for a histogram to have samples. * @param store supplies the stats store. From fe7b123dd31ed6940c50deec20c7d0dc7e6d513d Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 31 Aug 2021 09:54:49 +0000 Subject: [PATCH 3/7] add release log Signed-off-by: Yuchen Dai --- docs/root/version_history/current.rst | 1 + test/test_common/utility.h | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 71c41c24694b5..a19cfedc3e1a0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -55,6 +55,7 @@ Minor Behavior Changes to false. As part of this change, the use of reuse_port for TCP listeners on both macOS and Windows has been disabled due to suboptimal behavior. See the field documentation for more information. +* listener: destroy per network filter chain stats when a network filter chain is removed during the listener in place update. Bug Fixes --------- diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 38552f9a14b3a..a9dd44ccfe2c9 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -290,6 +290,7 @@ class TestUtility { waitForGaugeDestroyed(Stats::Store& store, const std::string& name, Event::TestTimeSystem& time_system, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()); + /** * Wait for a histogram to have samples. * @param store supplies the stats store. From 11459f9a19131922419316e1fb954f62396ab90c Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 1 Sep 2021 16:39:00 +0000 Subject: [PATCH 4/7] avoid too many findGauge Signed-off-by: Yuchen Dai --- source/server/filter_chain_manager_impl.h | 2 +- test/test_common/utility.cc | 13 +++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h index 421f63fefe322..3919aeb81073d 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -89,7 +89,7 @@ class PerFilterChainFactoryContextImpl : public Configuration::FilterChainFactor Configuration::FactoryContext& parent_context_; // The scope that has empty prefix. Stats::ScopePtr scope_; - // filter_chain_scope_ has the exact prefix as listener owners scope. + // filter_chain_scope_ has the same prefix as listener owners scope. Stats::ScopePtr filter_chain_scope_; Init::Manager& init_manager_; std::atomic is_draining_{false}; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 2400e29585a8d..15363fb3cbd17 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -242,18 +242,15 @@ AssertionResult TestUtility::waitForGaugeDestroyed(Stats::Store& store, const st Event::TestTimeSystem& time_system, std::chrono::milliseconds timeout) { Event::TestTimeSystem::RealTimeBound bound(timeout); - while (findGauge(store, name) != nullptr) { + Stats::GaugeSharedPtr gauge = findGauge(store, name); + while (gauge != nullptr) { time_system.advanceTimeWait(std::chrono::milliseconds(10)); if (timeout != std::chrono::milliseconds::zero() && !bound.withinBound()) { - std::string current_value; - if (findGauge(store, name)) { - current_value = absl::StrCat(findGauge(store, name)->value()); - } else { - current_value = "nil"; - } return AssertionFailure() << fmt::format( - "timed out waiting for {} destroyed, current value {}", name, current_value); + "timed out waiting for {} destroyed, current value {}", name, + absl::StrCat(gauge->value())); } + gauge = findGauge(store, name); } return AssertionSuccess(); } From a69db690a0c401a613d721743d2254b61a290549 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 1 Sep 2021 19:17:42 +0000 Subject: [PATCH 5/7] remove timeout arg Signed-off-by: Yuchen Dai --- test/integration/server.h | 6 ++---- test/integration/server_stats.h | 5 +---- test/integration/xds_integration_test.cc | 4 +--- test/test_common/utility.cc | 12 ++---------- test/test_common/utility.h | 7 ++----- 5 files changed, 8 insertions(+), 26 deletions(-) diff --git a/test/integration/server.h b/test/integration/server.h index f501e01cb10b7..f9404e63a5995 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -462,10 +462,8 @@ class IntegrationTestServer : public Logger::Loggable, notifyingStatsAllocator().waitForCounterExists(name); } - void waitForGaugeDestroyed( - const std::string& name, - std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) override { - ASSERT_TRUE(TestUtility::waitForGaugeDestroyed(statStore(), name, time_system_, timeout)); + void waitForGaugeDestroyed(const std::string& name) override { + ASSERT_TRUE(TestUtility::waitForGaugeDestroyed(statStore(), name, time_system_)); } void waitUntilHistogramHasSamples( diff --git a/test/integration/server_stats.h b/test/integration/server_stats.h index 336bbba6b1c37..d4520d3456db5 100644 --- a/test/integration/server_stats.h +++ b/test/integration/server_stats.h @@ -69,11 +69,8 @@ class IntegrationTestServerStats { /** * Wait for a gauge to be destroyed. Note that MockStatStore does not destroy stat. * @param name gauge name. - * @param timeout amount of time to wait before asserting false, or 0 for no timeout. */ - virtual void - waitForGaugeDestroyed(const std::string& name, - std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) PURE; + virtual void waitForGaugeDestroyed(const std::string& name) PURE; /** * Counter lookup. This is not thread safe, since we don't get a consistent diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc index 1869b1096ca27..8e866132782b7 100644 --- a/test/integration/xds_integration_test.cc +++ b/test/integration/xds_integration_test.cc @@ -291,9 +291,7 @@ class LdsInplaceUpdateHttpIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: - LdsInplaceUpdateHttpIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) { - use_real_stats_ = true; - } + LdsInplaceUpdateHttpIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) {} void inplaceInitialize(bool add_default_filter_chain = false) { autonomous_upstream_ = true; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 15363fb3cbd17..964333d24fe4e 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -239,18 +239,10 @@ AssertionResult TestUtility::waitForGaugeEq(Stats::Store& store, const std::stri } AssertionResult TestUtility::waitForGaugeDestroyed(Stats::Store& store, const std::string& name, - Event::TestTimeSystem& time_system, - std::chrono::milliseconds timeout) { - Event::TestTimeSystem::RealTimeBound bound(timeout); + Event::TestTimeSystem& time_system) { Stats::GaugeSharedPtr gauge = findGauge(store, name); - while (gauge != nullptr) { + while (findGauge(store, name) == nullptr) { time_system.advanceTimeWait(std::chrono::milliseconds(10)); - if (timeout != std::chrono::milliseconds::zero() && !bound.withinBound()) { - return AssertionFailure() << fmt::format( - "timed out waiting for {} destroyed, current value {}", name, - absl::StrCat(gauge->value())); - } - gauge = findGauge(store, name); } return AssertionSuccess(); } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index a9dd44ccfe2c9..e7fa952614c92 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -282,14 +282,11 @@ class TestUtility { * @param store supplies the stats store. * @param name gauge name. * @param time_system the time system to use for waiting. - * @param timeout the maximum time to wait before timing out, or 0 for no timeout. * @return AssertionSuccess() if the gauge was == to the value within the timeout, else * AssertionFailure(). */ - static AssertionResult - waitForGaugeDestroyed(Stats::Store& store, const std::string& name, - Event::TestTimeSystem& time_system, - std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()); + static AssertionResult waitForGaugeDestroyed(Stats::Store& store, const std::string& name, + Event::TestTimeSystem& time_system); /** * Wait for a histogram to have samples. From 6db5721e60a876554ec66bd24ba263440932b683 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 1 Sep 2021 20:50:45 +0000 Subject: [PATCH 6/7] add TODO Signed-off-by: Yuchen Dai --- test/integration/server.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/server.h b/test/integration/server.h index f9404e63a5995..109743e551b23 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -462,6 +462,7 @@ class IntegrationTestServer : public Logger::Loggable, notifyingStatsAllocator().waitForCounterExists(name); } + // TODO(#17956): Add Gauge type to NotifyingAllocator and adopt it in this method. void waitForGaugeDestroyed(const std::string& name) override { ASSERT_TRUE(TestUtility::waitForGaugeDestroyed(statStore(), name, time_system_)); } From 08b6fa37f1ce3d1b4aa2cb184d41c58023d45590 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 1 Sep 2021 21:31:27 +0000 Subject: [PATCH 7/7] remove unused var Signed-off-by: Yuchen Dai --- test/test_common/utility.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 964333d24fe4e..c2f68cc000c6d 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -240,7 +240,6 @@ AssertionResult TestUtility::waitForGaugeEq(Stats::Store& store, const std::stri AssertionResult TestUtility::waitForGaugeDestroyed(Stats::Store& store, const std::string& name, Event::TestTimeSystem& time_system) { - Stats::GaugeSharedPtr gauge = findGauge(store, name); while (findGauge(store, name) == nullptr) { time_system.advanceTimeWait(std::chrono::milliseconds(10)); }