drain manager: Make probabilistic drain configurable #11403

auni53 · 2020-06-02T14:09:02Z

Add DrainStrategy enum to Options with Graceful and Immediate
Disable probabilistic drain in DrainManager if DrainStrategy == Immediate
Add integration tests

Risk Level: Low.
Testing: Integration tests, verify that the race condition from #11240 does not occur if the probabilistic drain is disabled.

Signed-off-by: Auni Ahsan <[email protected]>

auni53 · 2020-06-02T14:11:49Z

@mattklein123 wanted to get your thoughts on this before I add the docs and release notes. I added a bool that guards the probabilistic drain behaviour, but I've got two thoughts:

Instead of bool drainIncrementally() I was thinking about enum drainStrategy(), where we can define different drain strategies in the server options header. There'd just be two right now, but this would make it easier to put more of the overall server shutdown sequence into the drainmanager in the future.
It's a little tiresome having to change so many codepaths every time we want to add a new option to the integration server config. Is it possible to modify the server optionsimpl directly? If not, I'll probably add a simple struct DrainManagerOptions to pass in the three drain manager configs.

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123 · 2020-06-02T15:08:50Z

I was thinking about enum drainStrategy(), where we can define different drain strategies in the server options header. There'd just be two right now, but this would make it easier to put more of the overall server shutdown sequence into the drainmanager in the future.

+1 agreed this seems more future proof.

It's a little tiresome having to change so many codepaths every time we want to add a new option to the integration server config. Is it possible to modify the server optionsimpl directly? If not, I'll probably add a simple struct DrainManagerOptions to pass in the three drain manager configs.

For integration tests it seems reasonable to allow direct access, but I will defer to @alyssawilk on that (it might already be possible).

Signed-off-by: Auni Ahsan <[email protected]>

alyssawilk · 2020-06-04T13:38:49Z

I don't mind allowing direct access, but please be mindful of thread safety issues - I think accessing it after the server starts running would be problematic.

auni53 · 2020-06-04T15:18:10Z

I don't mind allowing direct access, but please be mindful of thread safety issues - I think accessing it after the server starts running would be problematic.

I decided this was too complicated for this PR and just plumbed the other variable. I'll maybe try to get this to work on the side.

Signed-off-by: Auni Ahsan <[email protected]>

alyssawilk · 2020-06-04T17:11:24Z

I'm going to assume this is still not ready for review as CI is red. Ping when it's ready for a pass!

Signed-off-by: Auni Ahsan <[email protected]>

Signed-off-by: Shikugawa <[email protected]> Signed-off-by: Auni Ahsan <[email protected]>

Moving the choice of http or tcp connection pool from the router to the generic connection pool. This will allow pluggable connection pools to choose to do HTTP or TCP on their own, as well as getting rid of a bunch of ugly variant logic. Risk Level: medium (router refactor, ideally no-op) Testing: existing test pass Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Auni Ahsan <[email protected]>

…nvoyproxy#11126) Commit Message: grpc-json: preserve http request method in `x-envoy-original-method` header so that applications have access to it. Additional Description: The grpc-json transcoder currently forwards HTTP path to applications via "x-envoy-original-path" header. We would find it useful if it also forwarded the HTTP method. Risk Level: Low Testing: Updated grpc-json-transcoder unit tests Docs Changes: Added docs Release Notes: Added release notes Signed-off-by: Phillip Huang <[email protected]> Signed-off-by: Auni Ahsan <[email protected]>

Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Auni Ahsan <[email protected]>

Signed-off-by: Auni Ahsan <[email protected]>

auni53 · 2020-06-07T16:59:16Z

OK @alyssawilk this is green.

alyssawilk

Looks good! just a few comments from my end.

alyssawilk · 2020-06-08T13:41:09Z

source/server/options_impl.h

  void setParentShutdownTime(std::chrono::seconds parent_shutdown_time) {
    parent_shutdown_time_ = parent_shutdown_time;
  }
+  void setDrainStrategy(Server::DrainStrategy drain_strategy) { drain_strategy_ = drain_strategy; }
+  void setLogLevel(spdlog::level::level_enum log_level) { log_level_ = log_level; }


Why move these?

Haha, the diff makes it look weird, but I was trying to move setParentShutdownTime(std::chrono::seconds) in order to group all of the drain-related setters together.

alyssawilk · 2020-06-08T13:46:35Z

test/server/drain_manager_impl_test.cc

-  EXPECT_CALL(server_, healthCheckFailed()).Times(0); // Listener check will short-circuit
-  EXPECT_FALSE(drain_manager.drainClose());
-}
+INSTANTIATE_TEST_SUITE_P(DrainGradually, DrainManagerImplTest, testing::Bool());


Is DrainGradually the correct name for this? Perhaps DrainStrategies?

test/integration/integration.h

alyssawilk · 2020-06-08T13:49:46Z

source/server/options_impl.cc

-      signal_handling_enabled_(true), mutex_tracing_enabled_(false), cpuset_threads_(false),
-      fake_symbol_table_enabled_(false) {}
+      parent_shutdown_time_(900), drain_strategy_(Server::DrainStrategy::Gradual),
+      mode_(Server::Mode::Serve), hot_restart_disabled_(false), signal_handling_enabled_(true),


How are non-google users to configure this option?

Assuming you're saying this config path is only used by Google?

I was sticking to YAGNI, don't add further complications to the config plane unless someone actually wants it. I'd be happy to add CLI options for this similar to the --drain-time-s, though I'd prefer to do that in a separate PR.

It's pretty envoy-standard to land the configuration, release notes, and code changes in a single PR - I don't think it'll make this one too large to review.

Done I think, I looked at similar PRs to try to make sure I updated the right spot everywhere, but I'm not sure if I missed anything or if the language is correct.

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only · 2020-06-09T18:43:16Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11403 was synchronize by auni53.

see: more, trace.

Signed-off-by: Auni Ahsan <[email protected]>

jmarantz · 2020-06-09T21:49:01Z

/azp run

azure-pipelines · 2020-06-09T21:49:12Z

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s).

api/envoy/admin/v3/server_info.proto

alyssawilk

LGTM from a code perspective. Will defer to @htuch on API

htuch · 2020-06-10T23:46:01Z

/lgtm api

Add DrainStrategy enum to Options with Graceful and Immediate Disable probabilistic drain in DrainManager if DrainStrategy == Immediate Add integration tests Risk Level: Low. Testing: Integration tests, verify that the race condition from envoyproxy#11240 does not occur if the probabilistic drain is disabled. Signed-off-by: Auni Ahsan <[email protected]> Signed-off-by: yashwant121 <[email protected]>

Add DrainStrategy enum to Options with Graceful and Immediate Disable probabilistic drain in DrainManager if DrainStrategy == Immediate Add integration tests Risk Level: Low. Testing: Integration tests, verify that the race condition from envoyproxy#11240 does not occur if the probabilistic drain is disabled. Signed-off-by: Auni Ahsan <[email protected]>

Add DrainStrategy enum to Options with Graceful and Immediate Disable probabilistic drain in DrainManager if DrainStrategy == Immediate Add integration tests Risk Level: Low. Testing: Integration tests, verify that the race condition from envoyproxy#11240 does not occur if the probabilistic drain is disabled. Signed-off-by: Auni Ahsan <[email protected]> Signed-off-by: yashwant121 <[email protected]>

auni53 added 3 commits June 2, 2020 13:50

Add server option for drain_incrementally

cc60a44

Signed-off-by: Auni Ahsan <[email protected]>

Add drainIncrementally() to drain manager

0d055ca

Signed-off-by: Auni Ahsan <[email protected]>

add integration test

a2609f7

Signed-off-by: Auni Ahsan <[email protected]>

auni53 added 4 commits June 2, 2020 14:15

Fix format

c74e082

Signed-off-by: Auni Ahsan <[email protected]>

add headerparsertest

daa37c1

Signed-off-by: Auni Ahsan <[email protected]>

restore header formatter

98011c9

Signed-off-by: Auni Ahsan <[email protected]>

Merge branch 'formatter2'

bb1e965

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123 assigned alyssawilk Jun 2, 2020

alyssawilk added the waiting label Jun 2, 2020

auni53 added 4 commits June 2, 2020 23:47

enum wip

701b923

Signed-off-by: Auni Ahsan <[email protected]>

Add drainStrategy

f46b5e0

Signed-off-by: Auni Ahsan <[email protected]>

remove header parser change

6a57035

Signed-off-by: Auni Ahsan <[email protected]>

fix formatting

a54c074

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 3, 2020

auni53 marked this pull request as ready for review June 3, 2020 21:47

Merge remote-tracking branch 'envoy/master'

94f798a

Signed-off-by: Auni Ahsan <[email protected]>

Merge remote-tracking branch 'envoy/master'

a4ffcf6

Signed-off-by: Auni Ahsan <[email protected]>

alyssawilk added the waiting label Jun 4, 2020

auni53 and others added 7 commits June 4, 2020 23:31

Merge remote-tracking branch 'envoy/master'

62f8923

Signed-off-by: Auni Ahsan <[email protected]>

add headerparsertest

19e56d8

Signed-off-by: Auni Ahsan <[email protected]>

restore header formatter

8517860

Signed-off-by: Auni Ahsan <[email protected]>

http: initialize HeaderMap from iterator (envoyproxy#11360)

f224710

Signed-off-by: Shikugawa <[email protected]> Signed-off-by: Auni Ahsan <[email protected]>

docs: fixing broken link (envoyproxy#11406)

6fe0fc0

Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 4, 2020

auni53 added 4 commits June 5, 2020 15:47

add assert on disconnect

23f885a

Signed-off-by: Auni Ahsan <[email protected]>

Merge remote-tracking branch 'envoy/master' into incremental

24d4964

Signed-off-by: Auni Ahsan <[email protected]>

Merge branch 'master' into incremental

3ca777b

Signed-off-by: Auni Ahsan <[email protected]>

delete?

b8bfb0d

Signed-off-by: Auni Ahsan <[email protected]>

alyssawilk reviewed Jun 8, 2020

View reviewed changes

auni53 added 5 commits June 8, 2020 15:36

Merge remote-tracking branch 'envoy/master' into incremental

db6114c

review changes

68ca4a5

Signed-off-by: Auni Ahsan <[email protected]>

--drain-strategy cli

e87aa2f

Signed-off-by: Auni Ahsan <[email protected]>

proto sync wip

b76447e

Signed-off-by: Auni Ahsan <[email protected]>

add release note

b4265fd

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot added the api label Jun 9, 2020

auni53 added 2 commits June 9, 2020 18:44

fix format

3fed4f3

Signed-off-by: Auni Ahsan <[email protected]>

Fix braces for clangtidy

16b1ebd

Signed-off-by: Auni Ahsan <[email protected]>

htuch reviewed Jun 9, 2020

View reviewed changes

api/envoy/admin/v3/server_info.proto Show resolved Hide resolved

alyssawilk approved these changes Jun 10, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jun 10, 2020

htuch merged commit 8c7df0f into envoyproxy:master Jun 10, 2020

auni53 deleted the incremental branch June 11, 2020 14:15

twghu mentioned this pull request Mar 16, 2021

Load balance long-lived connections upon auto scale-out or scale-in of Envoy #15283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drain manager: Make probabilistic drain configurable #11403

drain manager: Make probabilistic drain configurable #11403

auni53 commented Jun 2, 2020 •

edited

Loading

auni53 commented Jun 2, 2020

mattklein123 commented Jun 2, 2020

alyssawilk commented Jun 4, 2020

auni53 commented Jun 4, 2020

alyssawilk commented Jun 4, 2020

auni53 commented Jun 7, 2020

alyssawilk left a comment

alyssawilk Jun 8, 2020

auni53 Jun 8, 2020

alyssawilk Jun 8, 2020

auni53 Jun 8, 2020

alyssawilk Jun 8, 2020

auni53 Jun 8, 2020

alyssawilk Jun 9, 2020

auni53 Jun 9, 2020

repokitteh-read-only bot commented Jun 9, 2020

jmarantz commented Jun 9, 2020

azure-pipelines bot commented Jun 9, 2020

alyssawilk left a comment

htuch commented Jun 10, 2020

drain manager: Make probabilistic drain configurable #11403

drain manager: Make probabilistic drain configurable #11403

Conversation

auni53 commented Jun 2, 2020 • edited Loading

auni53 commented Jun 2, 2020

mattklein123 commented Jun 2, 2020

alyssawilk commented Jun 4, 2020

auni53 commented Jun 4, 2020

alyssawilk commented Jun 4, 2020

auni53 commented Jun 7, 2020

alyssawilk 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

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

repokitteh-read-only bot commented Jun 9, 2020

jmarantz commented Jun 9, 2020

azure-pipelines bot commented Jun 9, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

htuch commented Jun 10, 2020

auni53 commented Jun 2, 2020 •

edited

Loading