server: make the --disable-hot-restart flag functional by jmarantz · Pull Request #2679 · envoyproxy/envoy

jmarantz · 2018-02-26T23:32:28Z

Description:
The --disable-hot-restart flag was added to the binary in #2576 by @tonya11en but not hooked up (at my request). Now that the main_common refactor is in, it's easier to hook it up. This also offers the opportunity for some minor code simplifications.

Risk Level: Medium -- this affects the startup path.

Testing:
//test/...
added a new subtest to hotrestart_test.sh to cover --disable-hot-restart. I'd appreciate suggestions though on how to cover the functionality more thoroughly...how should I detect from a test that hot restart is disabled?

Release Notes: TBD: I'm not sure if release notes were added already in #2576 -- if not they can be added during this PR review.

*Fixes #2029 -- actually #2576 claims to fix that but I don't think it really does, since in that PR the switch is a no-op.

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

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

alyssawilk · 2018-02-27T14:20:24Z

@junr03 @ccaraman would one of you take first pass? This is a good one to have on Lyft's radar as it' deploys :-)

ccaraman

LGTM - This is a really nice clean up!

ccaraman · 2018-02-28T22:32:13Z

test/exe/main_common_test.cc

@@ -25,14 +25,25 @@



Can't comment on line 22. The ifdef for ENVOY_HOT_RESTART isn't needed anymore!

See @mattklein123's comment in #2029

"@tonya11en I would probably leave the compile flag as some people want the ability to just compile out the functionality entirely. I think doing a CLI flag alongside would be pretty straightforward."

I'm fine letting Matt's opinion stand, but we're not actually compiling out the functionality, we're compiling out the ability to configure the functionality. I think it's cleaner to just have people turn it off but I'll totally merge this as a huge improvement in readability as it stands :-)

I thought in the old code we actually used the bazel switch to not compile the full hot restart implementation and only compile the NOP implementation. This might not be the case. I don't feel strongly about this either way. If people think it's better to get rid of the compile option we can do that. I suspect almost no one is using that option and as long as we have a clear release note (please check) it's probably OK. If we do that please remember to cleanup the bazel README docs about the option also.

Ooh, indeed, I was grepping source/... only and missed it in bazel/envoy_build_system.bzl

I was commenting on the lines 22-24 not being required in this file because the header "#include "server/hot_restart_impl.h"" isn't used in this file.

Good catch Carmen! Done.

RE removing the ifdef -- if no one thinks we need this ifdef anymore I'll remove it in a future PR. I think before we do that we just need to first make sure everyone who doesn't want hot-restart sets the new flag (after this is checked in).

ccaraman · 2018-02-28T22:36:46Z

test/exe/main_common_test.cc

+                            "/test/config/integration/google_com_proxy_port_0.v2.yaml";
+  const char* argv[] = {"envoy-static",          "-c",   config_file.c_str(), "--base-id", "2",
+                        "--disable-hot-restart", nullptr};
+  MainCommon main_common(ARRAY_SIZE(argv) - 1, const_cast<char**>(argv));


nit/curious - would wrapping these in EXPECT_NO_THROW make the test clearer that it is expected that these construct with no issue? (Context for my question - I was doing a quick look for EXPECT_... and got lost for a few seconds. :) )

seems reasonable. I guess if it threw otherwise it would stop all the tests in the file?

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

…RESTART is disabled. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

alyssawilk

Code otherwise LGTM, once you have the tests fixed up

…throw in CI. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…sts on the same machine. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-03-01T21:37:36Z

test/exe/main_common_test.cc

+  MainCommonTest()
+      : config_file_(Envoy::TestEnvironment::getCheckedEnvVar("TEST_RUNDIR") +
+                     "/test/config/integration/google_com_proxy_port_0.v2.yaml"),
+        random_string_(fmt::format("{}", random_generator_.random() % 1024)),


It's probably worth pointing out here that (a) I refactored the test so it has a test-class, to share the argv prep code, which became more complex when I added randomized base-IDs.

I needed to add randomized base-IDs to get tests to pass consistently under CI; the theory being that CI was instantiating more than one test at a time on a machine, and things were conflicting. I wasn't sure what the range of 'base-id' was so I made it 0-1023. Comments welcome.

I think this comment would be useful in the file itself.

Indeed, good call. I added a class comment and also factored out the base-id string generation. Also after chatting with @htuch about why this was needed, I decided getpid() was the critical component of the ID, so I combined it in. Looking at the base-id impl, we have a full uint32_t to play with, but I decided to give more of the bits to the pid() in my choice of prime.

…e're doing it. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

ccaraman

LGTM - one tiny nit

ccaraman · 2018-03-02T19:28:57Z

test/exe/main_common_test.cc

+
+  // Adds an argument, assuring that argv remains null-terminated.
+  void addArg(const char* arg) {
+    argv_[argv_.size() - 1] = arg;


nit: maybe comment that this doesn't lead to undefined behevior because it is replacing the nullptr?

done; added some asserts too.

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

htuch

LGTM with a couple of quick fix comments.

htuch · 2018-03-02T21:54:58Z

test/exe/main_common_test.cc

+    // Pick a prime number to give more of the 32-bits of entropy to the PID, and the
+    // remainder to the random number.
+    const uint32_t four_digit_prime = 7919;
+    return getpid() * four_digit_prime + random_generator_.random() % four_digit_prime;


This seems overkill; i would either just use the PID or random(), I don't think we're benefiting that much from combining.

htuch · 2018-03-02T21:56:27Z

test/exe/main_common_test.cc

+  std::vector<const char*> argv_;
+};
+
+TEST_F(MainCommonTest, ConstructDestructHotRestartEnabled) {


Please add a single line explanation of what the test is designed to do.

htuch · 2018-03-02T21:56:35Z

test/exe/main_common_test.cc

+  // Adds an argument, assuring that argv remains null-terminated.
+  void addArg(const char* arg) {
+    ASSERT(!argv_.empty());
+    size_t last = argv_.size() - 1;


Nit: const size_t

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

htuch

Thanks.

While we figure out and fix envoyproxy/envoy-mobile#2678, this commit reduces the initial_fetch_timeout on the xDS config sources in the integration tests from 15s to 1s, preventing issues like test timeouts. Signed-off-by: Ali Beyad <abeyad@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

jmarantz added 2 commits February 26, 2018 18:26

Hook up the --disable-hot-restart flag, thereby simplifying MainCommo…

6e9f3df

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

Unit-test both with hot restart enabled and disabled.

7e149ee

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

ggreenway assigned ccaraman Feb 27, 2018

ccaraman previously approved these changes Feb 28, 2018

View reviewed changes

Use EXPECT_NO_THROW to more clearly show the expectation when constru…

d72ac06

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

jmarantz dismissed ccaraman’s stale review via d72ac06 February 28, 2018 22:56

jmarantz closed this Mar 1, 2018

jmarantz reopened this Mar 1, 2018

jmarantz closed this Mar 1, 2018

jmarantz reopened this Mar 1, 2018

jmarantz closed this Mar 1, 2018

jmarantz reopened this Mar 1, 2018

Don't assert-fail if --disable-hot-restart is missing when ENVOY_HOT_…

65d966f

…RESTART is disabled. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz closed this Mar 1, 2018

jmarantz reopened this Mar 1, 2018

Declare hot_restart_enabled as UNREFERENCED_PARAMETER to fix MacOS bu…

d95b085

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

alyssawilk self-assigned this Mar 1, 2018

alyssawilk reviewed Mar 1, 2018

View reviewed changes

jmarantz added 2 commits March 1, 2018 13:59

Use VERBOSE_EXPECT_NO_THROW so I can see what happens when there's a …

a23cd39

…throw in CI. Signed-off-by: Joshua Marantz <jmarantz@google.com>

use randomized base ID to avoid conflicts with multiple concurrent te…

10a3f9d

…sts on the same machine. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Mar 1, 2018

View reviewed changes

jmarantz added 2 commits March 1, 2018 20:05

Incorporate getpid() into the --base-id calculation and comment why w…

1013c91

…e're doing it. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add some more commentary.

b1b8196

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

ccaraman previously approved these changes Mar 2, 2018

View reviewed changes

Add asserts and comments about the invariant argv_[size-1]==nullptr.

d5d9a70

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

jmarantz dismissed ccaraman’s stale review via d5d9a70 March 2, 2018 19:37

jmarantz mentioned this pull request Mar 2, 2018

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

Merged

ccaraman previously approved these changes Mar 2, 2018

View reviewed changes

htuch suggested changes Mar 2, 2018

View reviewed changes

Add comments & const, remove random number and just use getpid() for …

b331880

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

jmarantz dismissed ccaraman’s stale review via b331880 March 2, 2018 22:09

htuch approved these changes Mar 2, 2018

View reviewed changes

htuch merged commit 99d5fe9 into envoyproxy:master Mar 2, 2018

jmarantz deleted the use-hot-restart-disabled-option branch March 3, 2018 01:15

suniltheta mentioned this pull request Nov 29, 2023

disable hot restart capability in Envoy on bootup aws/amazon-ecs-service-connect-agent#61

Merged

Conversation

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

Uh oh!

Uh oh!

alyssawilk commented Feb 27, 2018

Uh oh!

ccaraman left a comment

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmarantz commented Feb 26, 2018 •

edited

Loading

ccaraman Feb 28, 2018 •

edited

Loading