server: factor out MainCommon as a class, with a run method by jmarantz · Pull Request #2568 · envoyproxy/envoy

jmarantz · 2018-02-09T13:54:26Z

Description:
This is step 2 in the plan to improve startup performance. It makes it possible instantiate the full server stack without running the event loop, so the instantiation can be timed. This also simplifies the code required in main(), though a legacy path is retained allowing existing alternate main()s to continue to work.

Risk Level: Medium: this has been extensively tested but deserves scrutiny as it is part of the startup path.

Testing:
//test/... including sanitizers.
Added tests for the new MainCommon instantiation, as well as the legacy main_common() where some of the work was delegated to main().

This is another step toward addressing #2373

… in main_common.cc. This sets the stage for another PR which will use the factored out MainCommon to performance test initialization without entering a main loop. Part of this PR is where we factor out all the initialization done prior to running the server loop into MainCommon's ctor. The listener loop is started by a run() method on the class. The orgaqnization is a little more complex than ideal because we want to, for one round of import, support the existing calling pattern of main_common, which leaves some of the initialization in main(). Another part of this CL is to fix a SEGV in the Server destruction process, if you ever instantiate a Server without running the listener loop and terminating via a signal or quitquitquit. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

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

mattklein123 · 2018-02-09T18:09:43Z

@dnoe can you take a pass on this?

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

jmarantz · 2018-02-09T20:27:05Z

I'm iterating a little bit on some issues with main_common_test.cc across CI which I understand. The rest of the system should be stable.

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

My theory is that main_common's signal handlers are affecting the death-tests when linked into signals_test, which appears to happen during coverage tests, but not when tests are run individually. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…G_COVERAGE didn't seem to work. Signed-off-by: Joshua Marantz <jmarantz@google.com>

… test during CI coverage. Also removes a filegroup for the sample config now that I added one that uses port 0. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-02-10T15:42:48Z

TWIMC: I have managed to get this through CI by #if-ing out main_common_test, which works fine in all scenarios except for the coverage tests. My hypothesis is that when MainCommon instantiates and destructing SignalAction, and that sequence alters the global signal handler state so that signals_test can no longer run properly. In coverage test, because reasons, all the _test.cc files are linked together so we don't get a new process boundary.

I also attempted to use #ifndef ENVOY_CONFIG_COVERAGE which made my coverage tests kind of run test/run_envoy_bazel_coverage.sh locally, but they timed out and didn't give me any data, so I'm not sure if that resolved the issue. Committing that didn't work -- CI still failed in coverage/SignalsTest the same way. I then tried committing "#if 0" and that worked. Now I'm trying #if !ENVOY_CONFIG_COVERAGE.

I'd like to be able to include main_common_test in coverage. I suspect that requires diving deep into signal_action.h to understand why it isn't restoring state fully, which feels too deep for this PR.

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

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

htuch

Overall approach looks good.

htuch · 2018-02-11T12:44:30Z

configs/google_com_proxy_port_0.json

@@ -0,0 +1,48 @@
+{


Best practice is to write new example configs in YAML; this the preferred human readable config representation.

htuch · 2018-02-11T12:45:14Z

configs/google_com_proxy_port_0.json

+    }],
+    "admin": {
+        "access_log_path": "/tmp/admin_access.log",
+        "address": "tcp://127.0.0.1:0"


Is there a reason we're using a v1 config here? In general, unless verifying v1 regression, we're trying to turn down v1 and only add new stuff as v2.

I was being lazy and copied the first plausible config I found. Done.

htuch · 2018-02-11T12:47:21Z

source/exe/BUILD

        "//source/server/config_validation:server_lib",
-    ],
+    ] + select({
+        "//bazel:disable_signal_trace": [],


Did you test with --define signal_trace=disabled? We don't have any CI tests to catch these variants of Bazel build, so when moving things around it's probably a good idea to manually verify.

Great idea. Doing so now. Actually, why is this a compile-time decision rather than an option? Then it would be easier to test without rebuilding the entire tree. Same with HotRestart, though that's addressed in #2568.

Some of these options are compile-time as we want to squash dependencies (e.g. Google gRPC) or demonstrate that site-local replacement or build time removal of features is possible (e.g. we replace during import on the Google side). Hot restart is probably a better contender to be fully dynamic, as it doesn't introduce any deps.

There's a wider conversation happening right now in #2069. Basically, we need some way to provide greater build time flexibility to allow very minimal Envoy binary deployments while also support kitchen sink builds with dynamic configuration. The former might be interesting in resource constrained embedded or container environments, the latter on a traditional fat server.

I wasn't arguing against any compile-time switches, but it wasn't obvious what dependency this one had. I was thinking that if we were compiling on Windows and the APIs didn't exist, we'd just #if them out based on platform support as opposed to config, but keep the class API above that consistent.

Yeah, the signal_trace deps are the Backtrace library, an external dependency.

htuch · 2018-02-11T12:49:18Z

source/exe/main_common.cc

-  }
-};
+Server::DrainManagerPtr ProdComponentFactory::createDrainManager(Server::Instance& server) {
+  return Server::DrainManagerPtr{


Nit: std::make_unique

done (this code was kind of just moved, but fixed anyway).

htuch · 2018-02-11T12:54:46Z

source/server/server.cc

+
+void InstanceImpl::terminate() {
+  {
+    std::unique_lock<std::mutex> lock(terminate_mutex_);


Why is this mutex protected? If we get here and we're not on the main thread, I think we're going to have a bad day, as most of the state that is touched below is local to the main thread.

I didn't find a race, but I noticed a similar bool in ThreadLocalImpl::shutdown_ that was declared atomic. I was then going to make shutdown_ atomic but felt like the equivalent set-if-not-already-and-run-code flow was easier to read as a mutex.

I probably need to do some more debugging of a live server to understand which structures are live in which threads. Will investigate.

I think shutdown is atomic as it's used for cross-thread communication, between main and workers. I think InstanceImp::terminate is only used on the main thread.

htuch · 2018-02-11T12:55:53Z

source/exe/main_common.h

+#endif
+
+  std::unique_ptr<Envoy::OptionsImpl> options_;
+  MainCommonBase base_;


Curious why membership vs. inheritance was chosen here.

I needed to compute the options_ before I could instantiate the Base, which takes Options& as a param for now, due to the legacy main_common. Hopefully this can all melt away once we remove our main_common call at Google.

htuch · 2018-02-11T12:57:48Z

source/exe/main_common.cc

+#else
+    MainCommonBase main_common(options, false);
+#endif
+    return main_common.run() ? 0 : 1;


I'm a slight fan of EXIT_SUCCESS and EXIT_FAILURE as per POSIX, but I realize this was like this before.

Agreed. I had an irrational desire to make the diffs less. But that failed anyway. Fixing.

htuch · 2018-02-11T13:04:30Z

source/exe/main_common.cc

-  Server::ProdComponentFactory component_factory;
-  auto local_address = Network::Utility::getLocalAddress(options.localAddressIpVersion());
-  switch (options.mode()) {
+bool MainCommonBase::run() {


Should we have a better name for this? This capture historically what was in the common main, but I think logically it's about overall server setup, teardown and run. We already have a Server::Instance, so maybe Server::Cli, Server::Launcher, etc.?

I'm OK renaming it but that would also lose any kind of a/b correlation in the review. AFAIK there's no way to tell git that a file was renamed; it either figures it out or it doesn't, and mostly it doesn't :)

Can this be a follow-up?

Sure. Either later or in a final pass before merging, either works.

Thanks; I'll put this in a follow-up.

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

Although it seems to work to declare main() as taking const char** argv, I wonder if there may be portability issues with that, and I decided to leave it at char**. Testing this is annoying because string literals are const. Signed-off-by: Joshua Marantz <jmarantz@google.com>

junr03 · 2018-02-12T23:20:43Z

@jmarantz I can smoke test this at Lyft. Please let me know here that the change is ready to be plopped in a machine. If it is, I can do this tomorrow 02/13

htuch

LGTM modulo comments remaining and canary.

htuch · 2018-02-13T13:07:25Z

include/envoy/stats/stats.h

+
+  /**
+   * @return RawStatData* a raw stat data block for a given stat name or nullptr if there is no more
+   *         memory available for stats. The allocator may return a reference counted data location


Technically, I think s/may return/should return/. We have an outstanding issue related to this at #2453

htuch · 2018-02-13T13:08:52Z

source/exe/BUILD

        "//source/server/config_validation:server_lib",
-    ],
+    ] + select({
+        "//bazel:disable_signal_trace": [],


Yeah, the signal_trace deps are the Backtrace library, an external dependency.

htuch · 2018-02-13T13:12:14Z

test/exe/main_common_test.cc

+namespace Envoy {
+
+TEST(MainCommon, ConstructDestruct) {
+  // TODO(jmarantz): I think we may need to hack the config file to pick an unused port.


Why isn't port zero binding sufficient?

htuch · 2018-02-13T13:14:58Z

test/config/integration/google_com_proxy_port_0.v2.yaml

+    dns_lookup_family: V4_ONLY
+    lb_policy: ROUND_ROBIN
+    hosts: [{ socket_address: { address: google.com, port_value: 443 }}]
+    tls_context: { sni: www.google.com }


You should be able to significantly reduce this down, since you don't need any TLS or DNS lookup, HCM etc. in a server startup example. See https://github.com/envoyproxy/envoy/blob/master/test/config/integration/server_ads.yaml for a very minimal example.

dnoe

@kmyerson Please take a look at this to make sure there aren't any obvious problems with our integration.

@jmarantz You might want to try doing a Google import and test to do our equivalent of the Lyft smoke test. There've been tricky problems around lifecycle before.

dnoe · 2018-02-13T14:31:52Z

source/exe/main.cc

-  return Envoy::main_common(*options);
+
+  // Run the server listener loop outside try/catch blocks, so that unexpected exceptions
+  // show up as a core-dumps for easier diagnostis.


typo: diagnostics

kmyerson · 2018-02-13T15:02:57Z

LGTM. It'll be a bit of work to switch our integration to this new main path, but no obvious problems.

I attempted to use server_ads.yaml but it triggerd an unexpected exception in the yaml library. I'm adding instrumentation to track that in a different PR. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-02-13T18:08:44Z

Addressed comments from @htuch
@kmyerson existing calls to main_common() should work, and that is at least somewhat tested in main_common_test.cc. The intent is for us to migrate off that legacy path after integrating this, and then the legacy main_common() support can be deleted.

@junr03 can you kick off a smoke-test? Thanks!

junr03 · 2018-02-13T18:55:13Z

on it

junr03 · 2018-02-13T20:58:17Z

@jmarantz I put the binary on two of our machines (staging, canary) in our edge cluster. I did a full stop/start and a hot restart, and in both cases things look ok; server came up with no hiccups, serving traffic without any noticeable aberrations

jmarantz · 2018-02-13T22:45:06Z

Thanks all!

htuch · 2018-02-13T22:45:14Z

@junr03 @jmarantz Merged. We will roll back if there is any major breakage reported in next 48h.

…nvoyproxy#2568)" This reverts commit deffec6, which was causing CI to flake today in TSAN runs. The proximate root cause is the change in envoyproxy#2568, which appears not to correctly setup logging in --mode=validate. This in turn leads to some log related races that I don't fully grok. Rolling back to unblock CI, we should make sure hotrestart_test passes TSAN with high confidence before rolling forward again. Signed-off-by: Harvey Tuch <htuch@google.com>

…2568)" (#2613) This reverts commit deffec6, which was causing CI to flake today in TSAN runs. The proximate root cause is the change in #2568, which appears not to correctly setup logging in --mode=validate. This in turn leads to some log related races that I don't fully grok. Rolling back to unblock CI, we should make sure hotrestart_test passes TSAN with high confidence before rolling forward again. Signed-off-by: Harvey Tuch <htuch@google.com>

…method (envoyproxy#2568)" (envoyproxy#2613)" This reverts commit 034135f. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ation (#2623) Description: Reverts #2613 the revert of #2568 Fixed the underlying issue which was that logging was not initialized during validation, so it ran lockless. This caused intermittent tsan errors. Although #2619 makes the failure immediate and consistent by asserting that logging has been initialized prior to spawning any threads. Risk Level: Medium -- the earlier #2568 caused intermittent tsan errors which we believe to be fixed, but #2568 was medium-risk in the first place. Release Notes: N/A

…gging before validation (envoyproxy#2623)" This reverts commit 89b1ea6.

Description: Converts EM's responseFlags to Cronet's API Errors Additional Description: Note that this change does not include ERR_NETWORK_CHANGED, ERR_INTERNET_DISCONNECT and QUIC_PROTOCOL_FAILED as they aren't implemented yet Risk Level: Low Testing: Test included Docs Changes: n/a Release Notes: n/a Signed-off-by: Chidera Olibie <colibie@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

jmarantz added 13 commits February 6, 2018 16:33

Formatting fixups.

d796786

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

Get all tests working with minimal changes.

622e5d2

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

formatting fixups

6a87f69

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

streamline the validation flow.

de78e95

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

Merge branch 'master' into main-common-as-class

3e5e52e

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

mutex-protect Server::InstanceImpl::terminate_

53f3ec0

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

formatting fixups

b9d284b

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

Fixes problems found after after retesting new main_common with old m…

2ff4f4a

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

fix struct/class conflict which I saw only in CI

b28fa8e

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

add missing override.

ccd38ad

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

Use TEST_RUNDIR rather than TEST_SRCDIR.

4d2bc12

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

Specify envoy-standard return comment.

4bd2da7

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

mattklein123 assigned dnoe Feb 9, 2018

jmarantz added 2 commits February 9, 2018 14:56

Test the legacy-main past using an old copy of main() in a test.

c84e9b4

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

formatting fixup.

8e0262a

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

Just get a writable char* out of std::string by taking a pointer to c…

928deb7

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

mattklein123 mentioned this pull request Feb 9, 2018

Add cli flag '--disable-hot-restart'. #2576

Merged

jmarantz added 3 commits February 9, 2018 22:14

Try using #if 0 to disable a test for coverage, as ifndef ENVOY_CONFI…

1763e71

…G_COVERAGE didn't seem to work. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Try using #if !ENVOY_CONFIG_COVERAGE rather than #ifndef to skip this…

2ba3207

… test during CI coverage. Also removes a filegroup for the sample config now that I added one that uses port 0. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits February 10, 2018 20:06

Define ENVOY_CONFIG_COVERAGE as a -D flag in addition to a bazel sett…

a9237fe

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

Fix the --cxxopt syntax.

57ef61f

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

htuch self-assigned this Feb 11, 2018

htuch suggested changes Feb 11, 2018

View reviewed changes

Raise coverage timeout to 1000s and make test output streamed.

3b469a4

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

jmarantz mentioned this pull request Feb 11, 2018

MainCommonTest (pending) breaks SignalsTest when run in same binary in coverage tests. #2580

Closed

htuch suggested changes Feb 13, 2018

View reviewed changes

dnoe reviewed Feb 13, 2018

View reviewed changes

Address comments from hutch.

1e29195

I attempted to use server_ads.yaml but it triggerd an unexpected exception in the yaml library. I'm adding instrumentation to track that in a different PR. Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch approved these changes Feb 13, 2018

View reviewed changes

htuch merged commit deffec6 into envoyproxy:master Feb 13, 2018

jmarantz deleted the main-common-as-class branch February 13, 2018 22:45

htuch mentioned this pull request Feb 15, 2018

Revert "server: factor out MainCommon as a class, with a run method (… #2613

Merged

jmarantz mentioned this pull request Feb 15, 2018

stats: add perf-annotation regexes used in stats tag extraction #2615

Merged

This was referenced Feb 15, 2018

WIP server: resolve tsan failure due to main_common refactor #2616

Closed

logging: Assert fail on pthread without log lock #2619

Merged

jmarantz mentioned this pull request Feb 15, 2018

server: un-revert #2568 with a fix to initialize logging before validation #2623

Merged

lita pushed a commit to lita/envoy that referenced this pull request Feb 15, 2018

Revert "server: un-revert envoyproxy#2568 with a fix to initialize lo…

def4128

…gging before validation (envoyproxy#2623)" This reverts commit 89b1ea6.

cmluciano mentioned this pull request Feb 16, 2018

TSAN error in SignalAction::sigHandler #2561

Closed

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

add golang lint (envoyproxy#2568)

40b1c7d

Conversation

jmarantz commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Feb 9, 2018

Uh oh!

jmarantz commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Feb 10, 2018

Uh oh!

htuch 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Feb 12, 2018

Uh oh!

htuch 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmyerson commented Feb 13, 2018

Uh oh!

jmarantz commented Feb 13, 2018

Uh oh!

jmarantz commented Feb 9, 2018 •

edited

Loading

jmarantz commented Feb 9, 2018 •

edited

Loading

junr03 commented Feb 13, 2018 •

edited

Loading