server: add an API for registering for notifications for server instance life… by eziskind · Pull Request #6254 · envoyproxy/envoy

eziskind · 2019-03-11T20:23:20Z

…cycle events

Description: add an API to allow components and extensions to register for server instance lifecycle events (such as startup and shutdown).
Risk Level: low
Testing: unit tests
Docs Changes:
Release Notes:
Fixes #2802

Signed-off-by: Elisha Ziskind eziskind@google.com

…cycle events Signed-off-by: Elisha Ziskind <eziskind@google.com>

Signed-off-by: Elisha Ziskind <eziskind@google.com>

include/envoy/server/lifecycle_notifier.h

dnoe · 2019-03-12T21:28:43Z

include/envoy/server/lifecycle_notifier.h

+     * The server instance is being shutdown and the dispatcher is about the exit.
+     */
+    ShutdownExit
+  };


This API restricts us from distinguishing between normal and abnormal shutdowns in the future. Do you think it's useful to build that capability in? It's unclear from the PR description what the motivation is for this change so I can't answer that without more context.

We can easily add additional values to the Stages enum to represent certain failure cases. Also it should be easy to extend the API to pass a parameter in the StageCallback function so that we can pass things like an error code or other state to the listeners. I'd rather wait till we have a more concrete use case for that though and keep things simple for now.

The bug for this (#2802) has some more context about the motivation for this change. In my particular use case I have a filter that needs to be given a chance to flush cached state when envoy is shutting down.

Sounds good to me, thanks for expanding the comment.

dnoe · 2019-03-12T21:30:55Z

source/server/server.h

  Http::ContextImpl http_context_;
  std::unique_ptr<Memory::HeapShrinker> heap_shrinker_;
+  std::thread::id main_thread_id_;
+  std::unordered_map<Stage, std::vector<StageCallback>> stage_callbacks_;


absl::flat_hash_map? Or perhaps more pragmatically, just use a vector since the possible stages are all known at compile time, and there are only two.

From a quick search it seems like std::unordered_map is much more common than absl::flat_hash_map in the envoy code base. The Stage enum values are strongly typed so to use them as indexes into a vector I think I'd need to do a static_cast to int which seems somewhat ugly so using a map seems better.

Most of the code in Envoy by lines was written before absl::flat_hash_map was open sourced, so I suspect that's the main reason why it's more common. I think it's still a good practice to use flat_hash_map where possible in new code, so I'd use it here.

I think it's good as a map rather than a vector, your reasoning makes total sense. This isn't performance critical so if it's more readable this way that's the right solution.

source/server/server.h

Signed-off-by: Elisha Ziskind <eziskind@google.com>

eziskind · 2019-03-13T15:27:42Z

/retest

repokitteh-read-only · 2019-03-13T15:27:49Z

🔨 rebuilding ci/circleci: api (failed build)

🐱

Caused by: a #6254 (comment) was created by @eziskind.

see: more, trace.

Signed-off-by: Elisha Ziskind <eziskind@google.com>

dnoe

LGTM - I'd consider using flat_hash_map, up to you.

dnoe · 2019-03-13T20:25:36Z

source/server/server.h

  Http::ContextImpl http_context_;
  std::unique_ptr<Memory::HeapShrinker> heap_shrinker_;
+  std::thread::id main_thread_id_;
+  std::unordered_map<Stage, std::vector<StageCallback>> stage_callbacks_;


Most of the code in Envoy by lines was written before absl::flat_hash_map was open sourced, so I suspect that's the main reason why it's more common. I think it's still a good practice to use flat_hash_map where possible in new code, so I'd use it here.

I think it's good as a map rather than a vector, your reasoning makes total sense. This isn't performance critical so if it's more readable this way that's the right solution.

dnoe · 2019-03-13T20:26:07Z

include/envoy/server/lifecycle_notifier.h

+     * The server instance is being shutdown and the dispatcher is about the exit.
+     */
+    ShutdownExit
+  };


Sounds good to me, thanks for expanding the comment.

dnoe · 2019-03-13T20:31:38Z

@mattklein123 for senior review

mattklein123

Thanks LGTM with 1 question.

mattklein123 · 2019-03-13T20:54:36Z

source/server/server.cc

-  shutdown_ = true;
-  restarter_.terminateParent();
-  dispatcher_->exit();
+  dispatcher_->post([this] {


I'm pretty sure this method must have always run on the main thread due to the terminateParent() call, right? Why is the post now necessary?

Yup, you're right. shutdown is called from the admin quitquitquit handler and I wasn't sure if that was always run on the main thread but integration tests which exercise this code path still pass without this change so I'll revert it.

Signed-off-by: Elisha Ziskind <eziskind@google.com>

mattklein123

Thank you!

* master: (59 commits) http fault: add response rate limit injection (envoyproxy#6267) xds: introduce initial_fetch_timeout option to limit initialization time (envoyproxy#6048) test: fix cpuset-threads tests (envoyproxy#6278) server: add an API for registering for notifications for server instance life… (envoyproxy#6254) remove remains of TestBase (envoyproxy#6286) dubbo_proxy: Implement the routing of Dubbo requests (envoyproxy#5973) Revert "stats: add new BoolIndicator stat type (envoyproxy#5813)" (envoyproxy#6280) runtime: codifying runtime guarded features (envoyproxy#6134) mysql_filter: fix integration test flakes (envoyproxy#6272) tls: update BoringSSL to debed9a4 (3683). (envoyproxy#6273) rewrite buffer implementation to eliminate evbuffer dependency (envoyproxy#5441) Remove the dependency from TimeSystem to libevent by using the Event::Scheduler abstraction as a delegate. (envoyproxy#6240) fuzz: fix use of literal in default initialization. (envoyproxy#6268) http: add HCM functionality required for rate limiting (envoyproxy#6242) Disable mysql_integration_test until it is deflaked. (envoyproxy#6250) test: use ipv6_only IPv6 addresses in custom cluster integration tests. (envoyproxy#6260) tracing: If parent span is propagated with empty string, it causes th… (envoyproxy#6263) upstream: fix oss-fuzz issue envoyproxy#11095. (envoyproxy#6220) Wire up panic mode subset to receive updates (envoyproxy#6221) docs: clarify xds docs with warming information (envoyproxy#6236) ...

eziskind added 3 commits March 11, 2019 16:12

Add an API for registering for notifications for server instance life…

0fcf494

…cycle events Signed-off-by: Elisha Ziskind <eziskind@google.com>

fix spelling

b296a21

Signed-off-by: Elisha Ziskind <eziskind@google.com>

fix format

393779b

Signed-off-by: Elisha Ziskind <eziskind@google.com>

dnoe self-assigned this Mar 12, 2019

dnoe reviewed Mar 12, 2019

View reviewed changes

Add const qualifier to main_thread_id_ field

056855d

Signed-off-by: Elisha Ziskind <eziskind@google.com>

Update comments

fc2c5a7

Signed-off-by: Elisha Ziskind <eziskind@google.com>

dnoe previously approved these changes Mar 13, 2019

View reviewed changes

dnoe assigned mattklein123 Mar 13, 2019

mattklein123 reviewed Mar 13, 2019

View reviewed changes

Revert change to InstanceImpl::shutdown

f4f46df

Signed-off-by: Elisha Ziskind <eziskind@google.com>

eziskind dismissed dnoe’s stale review via f4f46df March 14, 2019 00:37

mattklein123 approved these changes Mar 14, 2019

View reviewed changes

mattklein123 merged commit b4ac13c into envoyproxy:master Mar 14, 2019

eziskind deleted the lifecycle branch March 14, 2019 16:19

Conversation

eziskind commented Mar 11, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eziskind commented Mar 13, 2019

Uh oh!

repokitteh-read-only bot commented Mar 13, 2019

Uh oh!

dnoe left a comment

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!

dnoe commented Mar 13, 2019

Uh oh!

mattklein123 left a comment

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants