Skip to content

listener: create per network filter chain stats scope #17931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 7 commits into from
Sep 2, 2021
Merged

listener: create per network filter chain stats scope #17931

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1db18d2
per filter chain scope
lambdai Aug 31, 2021
e3ed57c
add test and fix scope()
lambdai Aug 31, 2021
fe7b123
add release log
lambdai Aug 31, 2021
11459f9
avoid too many findGauge
lambdai Sep 1, 2021
a69db69
remove timeout arg
lambdai Sep 1, 2021
6db5721
add TODO
lambdai Sep 1, 2021
08b6fa3
remove unused var
lambdai Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
---------
Expand Down
10 changes: 5 additions & 5 deletions source/server/filter_chain_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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), scope_(parent_context_.scope().createScope("")),
filter_chain_scope_(parent_context_.listenerScope().createScope("")),
init_manager_(init_manager) {}

bool PerFilterChainFactoryContextImpl::drainClose() const {
return is_draining_.load() || parent_context_.drainDecision().drainClose();
Expand Down Expand Up @@ -101,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();
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 4 additions & 0 deletions source/server/filter_chain_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ 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.
Comment thread
lambdai marked this conversation as resolved.
Outdated
Stats::ScopePtr filter_chain_scope_;
Init::Manager& init_manager_;
std::atomic<bool> is_draining_{false};
};
Expand Down
6 changes: 6 additions & 0 deletions test/integration/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,12 @@ class IntegrationTestServer : public Logger::Loggable<Logger::Id::testing>,
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 {
Expand Down
9 changes: 9 additions & 0 deletions test/integration/server_stats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
43 changes: 29 additions & 14 deletions test/integration/xds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,9 @@ class LdsInplaceUpdateHttpIntegrationTest
: public testing::TestWithParam<Network::Address::IpVersion>,
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;
Expand All @@ -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_) {
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();
Expand All @@ -395,20 +401,14 @@ 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);

auto codec_client_1 = createHttpCodec("alpn1");
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(
Expand All @@ -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
Expand All @@ -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));
Expand All @@ -458,14 +470,17 @@ 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();
});
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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
20 changes: 20 additions & 0 deletions test/test_common/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment thread
lambdai marked this conversation as resolved.
Outdated
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()) {

@jmarantz jmarantz Sep 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a much better pattern for waiting for things to happen with stats. See NotifyingAllocatorImpl::waitForCounterExists. It uses a condvar rather than a spin-loop, which in my experience is inherently flaky.

It looks like in the context where you are calling this, you can use NotifyingAllocatorImpl so this should be a straightforward extension.

@lambdai lambdai Sep 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think NotifyingAllocatorImpl is not available when I use real_stats

    RELEASE_ASSERT(ret != nullptr,
                   "notifyingStatsAllocator() is not created when real_stats is true");

I choose real_stats only because real_stats support stat destroy.

Am I understanding it correctly?

@jmarantz jmarantz Sep 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea. Did you just see that in the code or did you get that failure during a test?

@jmarantz jmarantz Sep 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just took another look. At this level, your impl parallels another one. I have general flakiness concerns about this so it'd be better if we didn't need it.

However in test/integration/server.h you are implementing a virtual method that calls this one. However if you look right above that, in the impl of waitForCounterExists, you can see it using the notifyingStatsAllocator(), which is the better way to go.

It's possible the condvars are hooked up in that for counters and not gauges (I haven't checked) but it should be straightforward to replicate for gauges.

@lambdai lambdai Sep 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see the failure when I use real stats. Reason here
https://github.com/envoyproxy/envoy/blob/main/test/integration/server.cc#L206-L213

Below is what I call waitForCounterExist b/c waitForGauge{Exist,Destroyed} is not implemented yet.

[2021-09-01 17:28:52.512][12][critical][assert] [./test/integration/server.h:590] assert failure: ret != nullptr. Details: notifyingStatsAllocator() is not created when real_stats is true

@jmarantz jmarantz Sep 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That's just a bool arg to IntegrationTestServerImpl. Switch that bool to false so you can do these tests without risking flakes?

@jmarantz jmarantz Sep 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default is actually false, so I guess we need to figure out first why someone decided to set it to true.

@lambdai lambdai Sep 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's me in this PR :(

I jump to integration test from unit test because the mocked store doesn't support query for destroy. Maybe the non-real stats support query for destroy? Let me give it a try.

@lambdai lambdai Sep 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, confirmed real stats is not needed.

The notified version of waitForGaugeDestroyed need a bunch notification for Gauge while currently only Counter type support the notification way. Can I (or anyone else) add the notification for Gauge in another PR?

std::string current_value;
if (findGauge(store, name)) {
current_value = absl::StrCat(findGauge(store, name)->value());
} else {
Comment thread
lambdai marked this conversation as resolved.
Outdated
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,
Expand Down
14 changes: 14 additions & 0 deletions test/test_common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,20 @@ 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.
Expand Down