listener: create per network filter chain stats scope by lambdai · Pull Request #17931 · envoyproxy/envoy

lambdai · 2021-08-31T16:51:44Z

Commit Message:
Maintain per network filter chain stats scope and avoid leaking stats when the owner filter chain is destroyed.

Additional Description:
The solution was discussed in this thread

Risk Level:
Testing: Integration test to adopt real stats that support query for destroyed stat.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fix #17690

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Nice, thanks. LGTM.

@jmarantz can you also take a quick look?

source/server/filter_chain_manager_impl.h

jmarantz · 2021-09-01T12:39:19Z

test/test_common/utility.cc

+  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()) {


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.

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?

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

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.

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

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

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

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.

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?

test/test_common/utility.cc

jmarantz · 2021-09-01T12:45:38Z

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

test/test_common/utility.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz

thanks, looks good.

Out of curiosity, how long do your new tests run with the 10ms spin-loop?

test/integration/server.h

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-01T20:53:24Z

thanks, looks good.

Out of curiosity, how long do your new tests run with the 10ms spin-loop?

450ms - 500ms Is it a spin loop?

jmarantz

I think it might be a spin loop if we are using simulated time in the test. I don't remember.

It's not a good pattern in any case. If a single test method is taking 450ms to run that's quite a long time, which is why I think it's good to switch to the condvar.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-01T21:35:41Z

I think it might be a spin loop if we are using simulated time in the test. I don't remember.

It's not a good pattern in any case. If a single test method is taking 450ms to run that's quite a long time, which is why I think it's good to switch to the condvar.

Yeah, agree. notification/callback is preferred.

I double check the underlying advanceTime is Thread::sleep_for through gdb.

We are not in the best shape for sure, but it's not too bad

lambdai · 2021-09-01T23:13:30Z

/retest

repokitteh-read-only · 2021-09-01T23:13:33Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17931 (comment) was created by @lambdai.

see: more, trace.

lambdai · 2021-09-02T00:31:56Z

/retest

repokitteh-read-only · 2021-09-02T00:32:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17931 (comment) was created by @lambdai.

see: more, trace.

…roxy#17931)" This reverts commit c361b05. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

…" (#18017) This reverts commit c361b05. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

…roxy#17931)" (envoyproxy#18017) This reverts commit c361b05. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai added 3 commits August 31, 2021 05:09

per filter chain scope

1db18d2

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add test and fix scope()

e3ed57c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add release log

fe7b123

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 self-assigned this Aug 31, 2021

mattklein123 previously approved these changes Aug 31, 2021

View reviewed changes

mattklein123 assigned jmarantz Aug 31, 2021

lambdai mentioned this pull request Aug 31, 2021

stats: add per network filter chain scope #17765

Closed

jmarantz reviewed Sep 1, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 1, 2021

avoid too many findGauge

11459f9

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed mattklein123’s stale review via 11459f9 September 1, 2021 16:39

repokitteh-read-only bot removed the waiting label Sep 1, 2021

jmarantz reviewed Sep 1, 2021

View reviewed changes

test/test_common/utility.cc Outdated Show resolved Hide resolved

remove timeout arg

a69db69

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz reviewed Sep 1, 2021

View reviewed changes

test/integration/server.h Show resolved Hide resolved

lambdai mentioned this pull request Sep 1, 2021

rewrite waitForGaugeDestroyed in notification style #17956

Closed

add TODO

6db5721

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz reviewed Sep 1, 2021

View reviewed changes

remove unused var

08b6fa3

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz approved these changes Sep 1, 2021

View reviewed changes

mattklein123 merged commit c361b05 into envoyproxy:main Sep 2, 2021

lambdai deleted the scope branch September 2, 2021 05:07

lambdai added a commit to lambdai/envoy-dai that referenced this pull request Sep 8, 2021

Revert "listener: create per network filter chain stats scope (envoyp…

082b209

…roxy#17931)" This reverts commit c361b05. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 pushed a commit that referenced this pull request Sep 8, 2021

Revert "listener: create per network filter chain stats scope (#17931)…

9f5e9fb

…" (#18017) This reverts commit c361b05. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Sep 10, 2021

stats scope for grpc access log should be envoy wide #18066

Closed

tyxia pushed a commit to tyxia/envoy that referenced this pull request Sep 21, 2021

Revert "listener: create per network filter chain stats scope (envoyp…

2faac11

…roxy#17931)" (envoyproxy#18017) This reverts commit c361b05. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Oct 11, 2021

listener: 2nd attempt create per network filter chain stats scope #18564

Merged

Conversation

lambdai commented Aug 31, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Sep 1, 2021

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Sep 1, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

lambdai commented Sep 1, 2021

Uh oh!

lambdai commented Sep 1, 2021

Uh oh!

repokitteh-read-only bot commented Sep 1, 2021

Uh oh!

lambdai commented Sep 2, 2021

Uh oh!

repokitteh-read-only bot commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmarantz Sep 1, 2021 •

edited

Loading