test: add tests for corner-cases around sending requests before run() starts or after run() ends. #4114

jmarantz · 2018-08-11T17:14:00Z

Signed-off-by: Joshua Marantz jmarantz@google.com

Description: There was concern around wanting to understand exactly what happens if an adminRequest gets issued prior run() starting, or right after run() ends.

Risk Level: none (just new tests)
Testing: //test/exe:main_common_test
Docs Changes: n/a
Release Notes: n/a

…s or after run() ends. Also adds some test infrastructure to make it easier to work with condvar/mutex combos. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…s destroyed. Signed-off-by: Joshua Marantz <jmarantz@google.com>

… tests. Many of the MainCommon tests do not require multiple threads and sync-points, so this factors out a new test-class to encapsulate these. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 · 2018-08-12T05:44:17Z

@dnoe PTAL

Signed-off-by: Joshua Marantz <jmarantz@google.com>

dnoe · 2018-08-13T20:57:16Z

test/exe/main_common_test.cc

+    addArg("--disable-hot-restart");
+  }
+
+  // Runs a an admin request specified in path, blocking until completion, and


"Runs a an admin" -> "Runs an admin request"

dnoe · 2018-08-13T21:01:20Z

test/exe/main_common_test.cc

+  }
+
+  // Conditionally pauses at a critical point in the Envoy thread waiting, waiting
+  // the for the test thread to try something. The test thread can then call resume_.Notify()


Extra the snuck in. And I can't tell if the repetition of waiting is intention. If it is, SGTM :)

dnoe · 2018-08-13T21:07:31Z

test/exe/main_common_test.cc

+
+  // The admin handler can't be called until after we let run() go.
+  EXPECT_FALSE(admin_handler_was_called);
+


We could add EXPECT_THAT(out, IsEmpty()) too, right?

dnoe · 2018-08-13T21:10:24Z

test/exe/main_common_test.cc

+  adminRequest("/quitquitquit", "POST");
+  EXPECT_TRUE(waitForEnvoyToExit());
+  EXPECT_TRUE(admin_handler_was_called);
+  EXPECT_THAT(out, HasSubstr("filesystem.reopen_failed"));


Seems like a weird thing to look for in the output. What's trying to be reopened here and what's failed? Can you add a comment that points to the bit that logs that line?

I was just checking for any stat. Adding comment.

dnoe · 2018-08-13T21:12:00Z

test/exe/main_common_test.cc

+// Class to track whether an object has been destroyed, which it does by bumping an atomic.
+class DestroyCounter {
+public:
+  explicit DestroyCounter(std::atomic<uint64_t>& destroy_count) : destroy_count_(destroy_count) {}


Taking and storing a reference like this is definitely not the norm, although it seems intended here since there's no accessor. Can you add a comment about the ownership semantics here?

Added comment.

dnoe · 2018-08-13T21:13:31Z

test/exe/main_common_test.cc

+  pause_point_.WaitForNotification(); // run() finished, but main_common_ still exists.
+
+  // Admin requests will not work, but will never complete. The lambda itself will be
+  // destroyed on thread exit, which we'll track with an object that counts destructor calls.


neat approach!

Signed-off-by: Joshua Marantz <jmarantz@google.com>

dnoe · 2018-08-13T22:50:41Z

Changes look good. Any idea what's up with the ASAN failure?

jmarantz · 2018-08-13T22:54:31Z

asan failure seems to be just a CI timeout, but I'm not sure. This change is only a unit test so it seems unlikely to affect a different test :)

FAIL: @envoy//test/integration:hds_integration_test (see /build/tmp/_bazel_bazel/400406edc57d332f0b9b805d2b8e33a1/execroot/envoy/bazel-out/k8-dbg/testlogs/external/envoy/test/integration/hds_integration_test/test.log)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-08-13T23:21:16Z

Trying out new 'git kick-ci' from slack

Signed-off-by: Joshua Marantz <jmarantz@google.com>

dnoe · 2018-08-14T13:26:44Z

I think ipv6_tests failure is a flake unrelated to this change too, so maybe you get to try kick-ci again:

HDS integration test failed with this:

[2018-08-14 01:19:45.812][11202][critical][assert] test/integration/fake_upstream.h:302] assert failure: parented_ || allow_unexpected_disconnects_. Details: An queued upstream connection was torn down without being associated with a fake connection. Either manage the connection via waitForRawConnection() or waitForHttpConnection(), or set_allow_unexpected_disconnects(true).

Looks like #4135

And the echo integration test timed out (maybe CI problem?)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

jmarantz added 4 commits August 11, 2018 13:10

Add tests for corner-cases around sending requests before run() start…

5c275e9

…s or after run() ends. Also adds some test infrastructure to make it easier to work with condvar/mutex combos. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add tracker to detect that an unexecuted lambda for an adminRequest i…

277b0ac

…s destroyed. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Factor out test infrastructure which is only needed for admin-request…

2e8c905

… tests. Many of the MainCommon tests do not require multiple threads and sync-points, so this factors out a new test-class to encapsulate these. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Make a simpler destroy-counter by exploiting std::make_shared.

d187ac2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 assigned dnoe Aug 12, 2018

jmarantz added 2 commits August 12, 2018 16:09

Switch to absl::Notification, which covers SyncPoint.

173d4c6

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into post-across-main-common

4d76443

Signed-off-by: Joshua Marantz <jmarantz@google.com>

dnoe reviewed Aug 13, 2018

View reviewed changes

Address review comments.

4dfafe8

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Kick CI

c3c6547

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~Add tests for corner-cases around sending requests before run() starts or after run() ends.~~ test: add tests for corner-cases around sending requests before run() starts or after run() ends. Aug 13, 2018

Kick CI

69cd153

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Kick CI

9af3567

Signed-off-by: Joshua Marantz <jmarantz@google.com>

dnoe approved these changes Aug 14, 2018

View reviewed changes

mattklein123 approved these changes Aug 14, 2018

View reviewed changes

mattklein123 merged commit da6194b into envoyproxy:master Aug 14, 2018

jmarantz deleted the post-across-main-common branch August 14, 2018 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add tests for corner-cases around sending requests before run() starts or after run() ends. #4114

test: add tests for corner-cases around sending requests before run() starts or after run() ends. #4114

jmarantz commented Aug 11, 2018 •

edited

Loading

mattklein123 commented Aug 12, 2018

dnoe Aug 13, 2018

jmarantz Aug 13, 2018

dnoe Aug 13, 2018

jmarantz Aug 13, 2018

dnoe Aug 13, 2018

jmarantz Aug 13, 2018

dnoe Aug 13, 2018

jmarantz Aug 13, 2018

dnoe Aug 13, 2018

jmarantz Aug 13, 2018

dnoe Aug 13, 2018

jmarantz Aug 13, 2018

dnoe commented Aug 13, 2018

jmarantz commented Aug 13, 2018

jmarantz commented Aug 13, 2018

dnoe commented Aug 14, 2018


		// The admin handler can't be called until after we let run() go.
		EXPECT_FALSE(admin_handler_was_called);

test: add tests for corner-cases around sending requests before run() starts or after run() ends. #4114

test: add tests for corner-cases around sending requests before run() starts or after run() ends. #4114

Conversation

jmarantz commented Aug 11, 2018 • edited Loading

mattklein123 commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe commented Aug 13, 2018

jmarantz commented Aug 13, 2018

jmarantz commented Aug 13, 2018

dnoe commented Aug 14, 2018

jmarantz commented Aug 11, 2018 •

edited

Loading