server: wait workers to start before draining parent. #14319

caitong93 · 2020-12-08T11:50:55Z

Signed-off-by: Tong Cai [email protected]

Commit Message: server: wait workers to start before draining parent.

Additional Description:

Add test.
The initialization process takes longer. What's the impact ?
Manual test pass. Inject 5s delay in socket() call, keep sending traffic to Envoy during hot restarts. Everything seems good.

Risk Level: medium
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]

Fixes #14295

Signed-off-by: Tong Cai <[email protected]>

caitong93 · 2020-12-08T13:20:02Z

Trying to fix. It seems more complex than I thought, and i will spent sometime to figure out the test logic of hot restart.

mattklein123 · 2020-12-08T17:45:10Z

At a high level this looks correct, so let me know if you have any questions or want me to do a further review.

/wait

Signed-off-by: Tong Cai <[email protected]>

caitong93 · 2020-12-10T14:34:52Z

@mattklein123

Basically ready for review.

caitong93 · 2020-12-10T14:41:34Z

The new stage WorkerStarted is added for testing. I'm still trying to add code to check restarter_.drainParentListeners will not be called before workers started.(there is a deadlock problem to solve)

Signed-off-by: Tong Cai <[email protected]>

caitong93 · 2020-12-11T14:50:33Z

Updated. Tests pass.

Signed-off-by: Tong Cai <[email protected]>

caitong93 · 2020-12-12T08:19:30Z

test/server/server_test.cc

    server_ = nullptr;
    thread_local_ = nullptr;
  });

  started.WaitForNotification();
  EXPECT_TRUE(startup);
  EXPECT_FALSE(shutdown);
-  EXPECT_TRUE(TestUtility::findGauge(stats_store_, "server.state")->used());


Remove server.state check here because it's non deterministic.

mattklein123

At a high level this looks mostly correct but I'm confused about why some of the changes were made. Thank you!

/wait

mattklein123 · 2020-12-13T21:58:50Z

source/server/server.h

+  // startup_ is true means Startup notifications have been called.
+  bool startup_{};


Move this up into the variables section. Also startup_lifecycle_event_raised_ or something like that?

mattklein123 · 2020-12-13T22:00:04Z

source/server/server.cc

+                                      if (!startup_) {
+                                        notifyCallbacksForStage(Stage::Startup);
+                                        startup_ = true;
+                                      }


Can you explain these changes? It's not clear to my why they were made. Please add more comments both here and below if they are necessary.

This ensures Startup notifications to be sent first. Otherwise at LifecycleNotifications test , the notification order will be PostInit, WorkerStarted, Startup(because in static configuration, post_init_cb will be called immediately , before main thread dispatcher start), and deadlock will happen.(because we block callback at WorkerStarted stage)

mattklein123 · 2020-12-13T22:00:57Z

include/envoy/server/lifecycle_notifier.h

+    /**
+     * All workers have started.
+     */
+    WorkerStarted,


This is not used? If this is needed WorkersStarted

It's used here.

I see. My preference would be to not make prod changes like this just for tests. If you need synchronization hooks can you use https://github.com/envoyproxy/envoy/blob/master/source/server/listener_hooks.h instead? Thank you.

/wait

mattklein123 · 2020-12-13T22:01:36Z

test/server/listener_manager_impl_test.h

@@ -296,6 +296,7 @@ class ListenerManagerImplTest : public testing::Test {
  std::unique_ptr<Network::MockConnectionSocket> socket_;
  uint64_t listener_tag_{1};
  bool enable_dispatcher_stats_{false};
+  std::function<void()> callback_;


This should be a mock and actually verify correct calls.

Signed-off-by: Tong Cai <[email protected]>

mattklein123 · 2020-12-16T20:12:49Z

Please merge main and check format.

/wait

Signed-off-by: Tong Cai <[email protected]>

caitong93 · 2020-12-17T18:12:33Z

@mattklein123 Updated, PTAL.

mattklein123

Thanks this LGTM. Just a few test questions.

/wait-any

mattklein123 · 2020-12-17T19:31:05Z

test/server/server_test.cc

@@ -423,6 +440,37 @@ TEST_P(ServerInstanceImplTest, LifecycleNotifications) {
  server_thread->join();
 }

+TEST_P(ServerInstanceImplTest, DrainParentListenerAfterWorkersStarted) {


Can you run this with --runs_per_test=1000 to make sure it doesn't flake?

Done.

Target //test/server:server_test up-to-date: bazel-bin/test/server/server_test INFO: Elapsed time: 4724.860s, Critical Path: 22.74s INFO: 1001 processes: 1 internal, 1000 processwrapper-sandbox. INFO: Build completed successfully, 1001 total actions //test/server:server_test PASSED in 21.8s Stats over 1000 runs: max = 21.8s, min = 18.5s, avg = 18.8s, dev = 0.3s

mattklein123 · 2020-12-17T19:33:25Z

source/server/server.cc

+    if (isShutdown()) {
+      return;
+    }


Do you have test coverage of this case? (Put an ASSERT in there and see if it's hit or look at coverage report)

Added test for this in the new commit.

Signed-off-by: Tong Cai <[email protected]>

mattklein123 · 2020-12-18T19:46:50Z

/retest

repokitteh-read-only · 2020-12-18T19:46:53Z

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

🐱

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

see: more, trace.

mattklein123 · 2020-12-19T00:18:39Z

/retest

repokitteh-read-only · 2020-12-19T00:18:43Z

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

🐱

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

see: more, trace.

caitong93 · 2020-12-19T01:20:59Z

/retest

repokitteh-read-only · 2020-12-19T01:21:04Z

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

🐱

Caused by: a #14319 (comment) was created by @caitong93.

see: more, trace.

caitong93 · 2020-12-19T04:26:19Z

Seems not like a network problem, will check what cause CI to fail when I got time.

mattklein123 · 2020-12-19T23:14:19Z

It's an unrelated OSX issue. Will just merge.

caitong93 · 2020-12-20T02:50:00Z

Thanks for the review!

* master: (30 commits) Deflaked: Guarddog_impl_test (envoyproxy#14475) [fuzz] add fuzz tests for hpack encoding and decoding (envoyproxy#13315) [filters] Prevent a filter from sending local reply and continue (envoyproxy#14416) oauth2: improving coverage (envoyproxy#14479) owners: Change dio email address (envoyproxy#14498) macos build: Fix ninja install (envoyproxy#14495) http: use OptRef helper to reduce some boilerplate (envoyproxy#14361) doc: update test/integration/README.md (envoyproxy#14485) server: wait workers to start before draining parent. (envoyproxy#14319) api: relax inline_string length limitation in DataSource (envoyproxy#14461) oauth: properly stop filter chain when a response was sent (envoyproxy#14476) listener: deprecate use_proxy_proto (envoyproxy#14406) deps: update cel and remove a patch (envoyproxy#14473) preconnect: rename: (envoyproxy#14474) coverage: ratcheting limits (envoyproxy#14472) grpc mux: fix sending node again after stream is reset (envoyproxy#14080) [test] Replace printers_include with printers_lib. (envoyproxy#14442) tcp: nodelay in the new pool (envoyproxy#14453) test: replace mock_methodn macros with mock_method (envoyproxy#14450) tcp: extending tcp integration test (envoyproxy#14451) ... Signed-off-by: Michael Puncel <[email protected]>

server: wait workers to start before draining parent.

0f79a67

Signed-off-by: Tong Cai <[email protected]>

mattklein123 self-assigned this Dec 8, 2020

repokitteh-read-only bot added the waiting label Dec 8, 2020

server: fix integration test

3f09b9d

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 9, 2020

mattklein123 added the waiting label Dec 9, 2020

server: fix server_test

7627986

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 10, 2020

server: add WorkerStarted stage

e6e3c90

Signed-off-by: Tong Cai <[email protected]>

caitong93 marked this pull request as ready for review December 10, 2020 14:35

mattklein123 added the waiting label Dec 10, 2020

server: fix tests

f84c3f8

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 11, 2020

mattklein123 added the waiting label Dec 11, 2020

server: fix flakey test

871c415

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 12, 2020

caitong93 commented Dec 12, 2020

View reviewed changes

mattklein123 requested changes Dec 13, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 13, 2020

Resolve comments.

1143ecc

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 14, 2020

Fix listener_manager_impl_test.

50c3946

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot added the waiting label Dec 14, 2020

Revert some changes and use ListenerHooks for testing.

9c942c2

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 16, 2020

caitong93 added 2 commits December 17, 2020 00:21

Merge remote-tracking branch 'origin/master' into wait-worker-init

c93ab4f

Signed-off-by: Tong Cai <[email protected]>

Fix compile.

c6b8a32

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot added the waiting label Dec 16, 2020

caitong93 added 2 commits December 17, 2020 21:17

Fix typo.

afc4d8b

Signed-off-by: Tong Cai <[email protected]>

Merge remote-tracking branch 'origin/master' into wait-worker-init

46f9047

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 17, 2020

Fix format.

22f482f

Signed-off-by: Tong Cai <[email protected]>

mattklein123 reviewed Dec 17, 2020

View reviewed changes

repokitteh-read-only bot added the waiting:any label Dec 17, 2020

server: test the server shutdown before workers start.

bad5177

Signed-off-by: Tong Cai <[email protected]>

repokitteh-read-only bot removed the waiting:any label Dec 18, 2020

mattklein123 approved these changes Dec 19, 2020

View reviewed changes

mattklein123 merged commit 867b9e2 into envoyproxy:master Dec 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: wait workers to start before draining parent. #14319

server: wait workers to start before draining parent. #14319

caitong93 commented Dec 8, 2020 •

edited

Loading

caitong93 commented Dec 8, 2020

mattklein123 commented Dec 8, 2020

caitong93 commented Dec 10, 2020

caitong93 commented Dec 10, 2020 •

edited

Loading

caitong93 commented Dec 11, 2020 •

edited

Loading

caitong93 Dec 12, 2020

mattklein123 left a comment

mattklein123 Dec 13, 2020

mattklein123 Dec 13, 2020

caitong93 Dec 14, 2020 •

edited

Loading

mattklein123 Dec 13, 2020

caitong93 Dec 14, 2020

mattklein123 Dec 14, 2020

mattklein123 Dec 13, 2020

mattklein123 commented Dec 16, 2020

caitong93 commented Dec 17, 2020

mattklein123 left a comment

mattklein123 Dec 17, 2020

caitong93 Dec 18, 2020

mattklein123 Dec 17, 2020

caitong93 Dec 18, 2020

mattklein123 commented Dec 18, 2020

repokitteh-read-only bot commented Dec 18, 2020

mattklein123 commented Dec 19, 2020

repokitteh-read-only bot commented Dec 19, 2020

caitong93 commented Dec 19, 2020

repokitteh-read-only bot commented Dec 19, 2020

caitong93 commented Dec 19, 2020

mattklein123 commented Dec 19, 2020

caitong93 commented Dec 20, 2020

		// startup_ is true means Startup notifications have been called.
		bool startup_{};

server: wait workers to start before draining parent. #14319

server: wait workers to start before draining parent. #14319

Conversation

caitong93 commented Dec 8, 2020 • edited Loading

caitong93 commented Dec 8, 2020

mattklein123 commented Dec 8, 2020

caitong93 commented Dec 10, 2020

caitong93 commented Dec 10, 2020 • edited Loading

caitong93 commented Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitong93 Dec 14, 2020 • edited Loading

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

mattklein123 commented Dec 16, 2020

caitong93 commented Dec 17, 2020

mattklein123 left a 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

mattklein123 commented Dec 18, 2020

repokitteh-read-only bot commented Dec 18, 2020

mattklein123 commented Dec 19, 2020

repokitteh-read-only bot commented Dec 19, 2020

caitong93 commented Dec 19, 2020

repokitteh-read-only bot commented Dec 19, 2020

caitong93 commented Dec 19, 2020

mattklein123 commented Dec 19, 2020

caitong93 commented Dec 20, 2020

caitong93 commented Dec 8, 2020 •

edited

Loading

caitong93 commented Dec 10, 2020 •

edited

Loading

caitong93 commented Dec 11, 2020 •

edited

Loading

caitong93 Dec 14, 2020 •

edited

Loading