stats: prevent unused counters from leaking across hot restart by fredlas · Pull Request #6850 · envoyproxy/envoy

fredlas · 2019-05-07T22:22:44Z

During hot restart, stores counters imported from the parent in a temporary scope. When hot restart concludes, that scope is dropped. The result is that any counter the child accessed (independently of the hot restart merging) will be preserved by the separate reference that access created, but any counter not so accessed will be dropped. This brings the hot restart counter behavior back to how it was before shared memory was removed in #5910.

Risk Level: low
Testing: updated test/common/stats/stat_merger_test.cc.

More fully fixes #4974.

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-05-07T22:23:49Z

/assign jmarantz
/assign mattklein123

… passed anyways Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <fredlas@google.com>

mattklein123

Awesome, thanks for tackling. A few comments to get started.

/wait

mattklein123 · 2019-05-07T23:34:46Z

include/envoy/stats/store.h

+  /**
+   * @return whether any known counter exists with this name.
+   */
+  virtual bool counterExists(const std::string& counter_name) PURE;


cc @ahedberg for intersection with #6712. Can we intersect with the APIs being added there so we don't duplicate?

This one is test only, so it will definitely be best to just get rid of counterExists, and use those ones in here. That will be a very easy switch; left a todo.

Would've called this counterExistsForTests, but probably not worth it at this point. Maybe follow-up with a rename?

Also note in the doc that this will take a lock.

source/common/stats/stat_merger.h

tools/spelling_dictionary.txt

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-05-08T19:33:39Z

Ran all tests this time. All the CI should be fixed.

Signed-off-by: Fred Douglas <fredlas@google.com>

mattklein123

Thanks, this looks great, assuming @ahedberg is OK with this merging first? @jmarantz can you take a quick pass?

mattklein123 · 2019-05-08T20:07:45Z

@fredlas ASAN failure looks legit.

ahedberg · 2019-05-08T20:10:00Z

Thanks, this looks great, assuming @ahedberg is OK with this merging first? @jmarantz can you take a quick pass?

Yup, SGTM. Fred's TODO SGTM too.

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-05-08T20:51:22Z

Haha sorry, changed my mind. Turns out the state transitions don't 100% make sense to switch to an enum. Just adding the 3rd bool is super simple, so I have now added that to this PR.

mattklein123 · 2019-05-08T22:27:17Z

/retest

repokitteh-read-only · 2019-05-08T22:27:21Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #6850 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

Very nice. Will give @jmarantz a chance to take a look.

source/common/stats/stat_merger.h

jmarantz

Nice work!

I'm OK with merging this with my nits addressed in a follow-up, but I'll let Matt make the call?

jmarantz · 2019-05-09T02:55:10Z

include/envoy/stats/store.h

+  /**
+   * @return whether any known counter exists with this name.
+   */
+  virtual bool counterExists(const std::string& counter_name) PURE;


Would've called this counterExistsForTests, but probably not worth it at this point. Maybe follow-up with a rename?

Also note in the doc that this will take a lock.

source/common/stats/isolated_store_impl.h

source/common/stats/thread_local_store.cc

test/common/stats/stat_merger_test.cc

mattklein123 · 2019-05-09T16:04:57Z

Fine for me to wait for @jmarantz comments. They all seem like great things to do.

/wait

fredlas · 2019-05-09T17:12:14Z

All of that code is going to be deleted when #6712 merges. I don't think it's worth perfecting. If #6712 is going to take a while, maybe splitting out just the getCounter part of it to be submitted separately would be the way to go.

mattklein123 · 2019-05-09T17:35:58Z

All of that code is going to be deleted when #6712 merges. I don't think it's worth perfecting. If #6712 is going to take a while, maybe splitting out just the getCounter part of it to be submitted separately would be the way to go.

I agree, but why add it in the first place given that we don't know when the other PR will merge? Per @jmarantz we can just have a method directly on the ThreadLocalStore that does what we need and is marked for testing?

jmarantz · 2019-05-09T17:46:43Z

Moreover it occurred to me after reflecting on the code that these new functions aren't needed, as there is already TestUtility::findCounter() and findGauge, declared in test/test_common/utility.h

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-05-09T17:57:51Z

Nice find, thanks! Switched to those.

Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz

nice!

jmarantz · 2019-05-09T18:11:27Z

test/common/stats/stat_merger_test.cc

+  }
+  // We accessed 0 and 1 above, but not 2. Now that StatMerger has been destroyed,
+  // 2 should be gone.
+  EXPECT_TRUE(TestUtility::findCounter(store, "newcounter0"));


I'd probably write it as EXPECT_TRUE(TestUtility::findCounter(store, "newcounter0") != nullptr) or maybe EXPECT_NE(TestUtility::findCounter(store, "newcounter0"), nullptr) if that compiles, but meh :)

mattklein123

Awesome

* master: (88 commits) upstream: Null-deref on TCP health checker if setsockopt fails (envoyproxy#6793) ci: switch macOS CI to azure pipelines (envoyproxy#6889) os syscalls lib: break apart syscalls used for hot restart (envoyproxy#6880) Kafka codec: precompute request size before serialization, so we do n… (envoyproxy#6862) upstream: move static and strict_dns clusters to dedicated files (envoyproxy#6886) Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) (envoyproxy#6784) fix explicit constructor in copy-initialization (envoyproxy#6884) stats: use tag iterator rather than constructing the tag-array and searching that. (envoyproxy#6853) common: use unscoped build target in generate_version_linkstamp (envoyproxy#6877) Addendum to envoyproxy#6778 (envoyproxy#6882) ci: add minimum Linux build for Azure Pipelines (envoyproxy#6881) grpc: utilities for inter-converting grpc::ByteBuffer and Buffer::Instance. (envoyproxy#6732) upstream: allow excluding hosts from lb calculations until initial health check (envoyproxy#6794) stats: prevent unused counters from leaking across hot restart (envoyproxy#6850) network filters: add `injectDataToFilterChain(data, end_stream)` method to network filter callbacks (envoyproxy#6750) delete things that snuck back in (envoyproxy#6873) config: scoped rds (2b): support delta APIs in ConfigProvider framework (envoyproxy#6781) string == string! (envoyproxy#6868) config: add mssing imports to delta_subscription_state (envoyproxy#6869) protobuf: add missing default case to enum (envoyproxy#6870) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

rgs1 · 2019-05-13T16:49:28Z

@fredlas @jmarantz fwiw, we picked up this change but we are still seeing metrics drop over a hot restart. I'll dig into it a bit more, but wanted to give you an early heads up.

fredlas · 2019-05-13T17:02:37Z

By "drop", do you mean values going to 0, or metrics going from existing to not existing? Either version would be different from the issue solved in this PR, so this discussion should probably move to an issue, but it does sound like something to discuss.

rgs1 · 2019-05-13T17:38:13Z

This is how it looks for cluster.${cluster}.health_check.failure during a hot restart:

I am wondering if the move from shared mem to copying stats over IPC would allow for such a drop & recovery to happen. Given that copying stats over takes time...

cc: @fishcakez

fredlas · 2019-05-13T17:41:58Z

That problem is not related to this PR; we should discuss it elsewhere.

mattklein123 · 2019-05-13T17:45:18Z

@rgs1 I have an idea of why this is happening but per @fredlas it's unrelated to this PR. Can you open a fresh issue and we can discuss there?

rgs1 · 2019-05-13T17:50:36Z

@mattklein123 yeah lets chat here: #6924

@fredlas thanks!

fredlas added 3 commits May 7, 2019 17:42

make hot restart counters temporary by using a scope

e1881aa

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into SCO_scope

6ddb458

Signed-off-by: Fred Douglas <fredlas@google.com>

update test

a0eff7f

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot assigned jmarantz and mattklein123 May 7, 2019

fredlas added 3 commits May 7, 2019 18:26

added statricia and statrick to spellcheck, although statrick somehow…

0dae84f

… passed anyways Signed-off-by: Fred Douglas <fredlas@google.com>

fill in missing counterExists

9f862f8

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into SCO_scope

7ddf146

Signed-off-by: Fred Douglas <fredlas@google.com>

mattklein123 requested changes May 7, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 7, 2019

fredlas added 2 commits May 8, 2019 12:59

snapshot

05d456f

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into SCO_scope

fff5c97

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label May 8, 2019

more TODOs

c3c888a

Signed-off-by: Fred Douglas <fredlas@google.com>

mattklein123 added the waiting label May 8, 2019

fredlas mentioned this pull request May 8, 2019

stats: (NON-SCOPE-BASED APPROACH)prevent unused counters from leaking across hot restart #6783

Closed

fredlas added 2 commits May 8, 2019 15:33

add gaugeExsits to test store thing

7b377c7

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into SCO_scope

b634093

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label May 8, 2019

remove weird autogen files that snuck in

2d572f7

Signed-off-by: Fred Douglas <fredlas@google.com>

mattklein123 previously approved these changes May 8, 2019

View reviewed changes

mattklein123 added the waiting label May 8, 2019

fredlas added 2 commits May 8, 2019 16:27

work around tls thread shutdown

21082d2

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into SCO_scope

c206ae1

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas dismissed mattklein123’s stale review via c206ae1 May 8, 2019 20:27

repokitteh-read-only bot removed the waiting label May 8, 2019

fix TLS never started up threading

95b22ed

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas dismissed mattklein123’s stale review via 95b22ed May 8, 2019 20:51

mattklein123 previously approved these changes May 8, 2019

View reviewed changes

source/common/stats/stat_merger.h Show resolved Hide resolved

jmarantz previously approved these changes May 9, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 9, 2019

fredlas added 2 commits May 9, 2019 13:57

switch to TestUtility::findCounter

a935618

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into SCO_scope

e91f05a

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas dismissed stale reviews from jmarantz and mattklein123 via e91f05a May 9, 2019 17:57

repokitteh-read-only bot removed the waiting label May 9, 2019

dependency cleanup

1958fee

Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz approved these changes May 9, 2019

View reviewed changes

mattklein123 approved these changes May 9, 2019

View reviewed changes

mattklein123 merged commit 6ffa8c1 into envoyproxy:master May 9, 2019

mattklein123 mentioned this pull request May 9, 2019

stats: don't leak obsolete stats across hot restart #6756

Closed

Conversation

fredlas commented May 7, 2019

Uh oh!

fredlas commented May 7, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 May 7, 2019

Choose a reason for hiding this comment

Uh oh!

fredlas May 8, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz May 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fredlas commented May 8, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 8, 2019

Uh oh!

ahedberg commented May 8, 2019

Uh oh!

fredlas commented May 8, 2019

Uh oh!

mattklein123 commented May 8, 2019

Uh oh!

repokitteh-read-only bot commented May 8, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz May 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented May 9, 2019

Uh oh!

fredlas commented May 9, 2019

Uh oh!

mattklein123 commented May 9, 2019

Uh oh!

jmarantz commented May 9, 2019

Uh oh!

fredlas commented May 9, 2019

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz May 9, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 commented May 13, 2019

Uh oh!

fredlas commented May 13, 2019

Uh oh!

rgs1 commented May 13, 2019

Uh oh!

fredlas commented May 13, 2019

Uh oh!

mattklein123 commented May 13, 2019

Uh oh!

rgs1 commented May 13, 2019

Uh oh!

Reviewers