drain manager: Refactor DrainManagerImpl with simulated time by aunu53 · Pull Request #11138 · envoyproxy/envoy

aunu53 · 2020-05-11T12:39:41Z

drain_manager: Refactor DrainManagerImpl with simulated time
* Replace recurring timer in DrainManagerImpl with a single timer that can be tested via simulated time.
* Clarify the behaviour where probability of drain close increases as the deadline is approached, and add mocks on the random() call to test this
* Rely on the system scheduler rather than tick time
* Had to remove the "drain tick #x" trace log
* Remove overly explicit includes
* Add TODO for integration tests on this via a real time system, once TestDrainManager is removed (that's a separate thread of work)

Additional Description: Preparatory work for enhancing graceful drain functionality.
Risk Level: Medium. This is changing a core dependency (via the server) that hasn't been touched in a while, but the behaviour should be a noop.
Testing: I added a lot more testing coverage, and some aspects of this (the probabilistic draining) weren't being tested at all prior.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-12T19:41:18Z

/retest

repokitteh-read-only · 2020-05-12T19:41:24Z

🔨 rebuilding ci/circleci: api (failed build)

🐱

Caused by: a #11138 (comment) was created by @auni53.

see: more, trace.

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz · 2020-05-13T12:11:06Z

/wait

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-13T15:45:06Z

Sorry for the format presubmit fails...I thought I had it setup to automatically lint before pushing, trying to fix that.

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz · 2020-05-13T18:00:29Z

/wait

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-14T15:38:10Z

/retest

repokitteh-read-only · 2020-05-14T15:38:16Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11138 (comment) was created by @auni53.

see: more, trace.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-14T17:10:47Z

I don't understand why these presubmits are failing...unable to reproduce yet, and my local bazel //test/... is fine. Going to merge from master and push up again.

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz

basically looks good modulo one last naming nit.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-15T18:00:35Z

Fix the naming nit, is this good to go?

jmarantz · 2020-05-15T18:05:57Z

@envoyproxy/senior-maintainers

mattklein123

Nice, thanks. LGTM with one comment.

/wait

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-20T21:02:44Z

(I'm waiting to resolve #11240 before introducing more changes.)

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-28T14:18:22Z

Fixed the ternary, added an expected_delay to the mock expect, and put up #11345 which addresses the tangential race condition. Hope this is good to go!

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-05-28T20:22:42Z

/retest

repokitteh-read-only · 2020-05-28T20:22:45Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11138 (comment) was created by @auni53.

see: more, trace.

jmarantz · 2020-05-29T12:47:42Z

+  // P(return true) = elapsed time / drain timeout
+  const auto remaining_time =
+      std::chrono::duration_cast<std::chrono::seconds>(drain_deadline_ - current_time);
+  const auto elapsed_time = server_.options().drainTime() - remaining_time;


is there a guarantee that drainTime() >= current_time ? Or should we do a compare first? I don't totally understand the surrounding draining architecture, but it looks like drainClose() can be called by something not controlled by the timer, so current_time_ may just be later. In that case maybe we should skip the subtraction underflow and compare to random and just return true?

Assuming you mean drain_deadline_ (the computed deadline time point) and not drainTime() (the server configuration)? This case is tested for at drain_manager_impl_test.cc:83. If current_time > drain_deadline, the probability of drain close is 100%,

sorry I meant checking server_.options().drainTime() >= remaining_time

or is that guaranteed to be true due to the wait remaining_time is calculated?

(psuedo variables)

drain_deadline = drain_timeout + previous_time
remaining_time = drain_deadline - current_time
remaining_time = drain_timeout + previous_time - current_time
current_time >= previous time, therefore remaining_time <= drain_timeout

Math checks out to me!

awesome. add assert then?

Not sure if it's the exact ASSERT you wanted but

drain >= remaining
drain - remaining >= 0
elapsed = drain - remaining
elapsed >= 0

so I added ASSERT(elapsed_time >= 0);

probably not, if elapsed_time is ultimately a uint_something it would be true by defintion.

I meant asserting the subtraction is non-underflowing.

ASSERT(server_.options().drainTime() >= remaining_time);

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2020-06-01T15:12:39Z

@mattklein123 is this good to merge?

mattklein123

Thanks!

mattklein123 · 2020-06-01T15:18:10Z

 namespace {

-class DrainManagerImplTest : public testing::Test {
+constexpr int DrainTimeSeconds(600);


nit: just use std::chrono::seconds here. Feel free to do this change in one of your other PRs.

Auni Ahsan added 9 commits May 6, 2020 14:50

drain refactor work

8dde7b9

Signed-off-by: Auni Ahsan <auni@google.com>

wip

0787ca1

Signed-off-by: Auni Ahsan <auni@google.com>

Add simtime and deadline

82be974

Signed-off-by: Auni Ahsan <auni@google.com>

add test

5eb72a9

Signed-off-by: Auni Ahsan <auni@google.com>

Move all draining logic over to scheduler

c94a942

Signed-off-by: Auni Ahsan <auni@google.com>

Merge branch 'master' into drain

902480d

Signed-off-by: Auni Ahsan <auni@google.com>

add todo

2d7cbd2

Signed-off-by: Auni Ahsan <auni@google.com>

fix format

df8e6b4

Signed-off-by: Auni Ahsan <auni@google.com>

Fix a dumb typo

a6b1680

Signed-off-by: Auni Ahsan <auni@google.com>

dio assigned alyssawilk May 12, 2020

alyssawilk assigned jmarantz May 12, 2020

Replace nullptr cb with an empty lambda

aaf9926

Signed-off-by: Auni Ahsan <auni@google.com>

mattklein123 reviewed May 12, 2020

View reviewed changes

Comment thread source/server/drain_manager_impl.cc

Fix format

25c2adf

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz reviewed May 12, 2020

View reviewed changes

Comment thread source/server/drain_manager_impl.cc Outdated

Comment thread source/server/drain_manager_impl.cc Outdated

Comment thread source/server/drain_manager_impl.cc Outdated

Remove callback as a member variable and test coverage for exceeded time

799b37e

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only Bot added the waiting label May 13, 2020

Skip random drain close if deadline exceeded

8186456

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only Bot removed the waiting label May 13, 2020

Fix format

9edeb0a

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only Bot added the waiting label May 13, 2020

Add assert on empty function

93ed112

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only Bot removed the waiting label May 14, 2020

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

863d955

Signed-off-by: Auni Ahsan <auni@google.com>

Remove nullptr call in protocol integration test

c627418

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz reviewed May 14, 2020

View reviewed changes

Comment thread test/server/drain_manager_impl_test.cc Outdated

Auni Ahsan added 2 commits May 15, 2020 01:06

Fix constant name

badd8e0

Signed-off-by: Auni Ahsan <auni@google.com>

Actually it's DrainTimeSeconds

9497cd0

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz previously approved these changes May 15, 2020

View reviewed changes

mattklein123 requested changes May 16, 2020

View reviewed changes

Comment thread source/server/drain_manager_impl.cc Outdated

repokitteh-read-only Bot added the waiting label May 16, 2020

mattklein123 self-assigned this May 16, 2020

Remove extraneous ternary

6687933

Signed-off-by: Auni Ahsan <auni@google.com>

Auni Ahsan added 2 commits May 28, 2020 13:42

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

c810618

Signed-off-by: Auni Ahsan <auni@google.com>

Expect a time in the mock call

b225f21

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 dismissed jmarantz’s stale review via b225f21 May 28, 2020 14:16

repokitteh-read-only Bot removed the waiting label May 28, 2020

aunu53 requested a review from mattklein123 May 28, 2020 14:20

Clean up expected delay initialization

3edaebe

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz reviewed May 29, 2020

View reviewed changes

assert on elapsed time computation

09bdb27

Signed-off-by: Auni Ahsan <auni@google.com>

mattklein123 approved these changes Jun 1, 2020

View reviewed changes

mattklein123 merged commit c6642ba into envoyproxy:master Jun 1, 2020

aunu53 deleted the drain branch June 1, 2020 15:25

Conversation

aunu53 commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aunu53 commented May 12, 2020

Uh oh!

repokitteh-read-only Bot commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented May 13, 2020

Uh oh!

aunu53 commented May 13, 2020

Uh oh!

jmarantz commented May 13, 2020

Uh oh!

aunu53 commented May 14, 2020

Uh oh!

repokitteh-read-only Bot commented May 14, 2020

Uh oh!

aunu53 commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aunu53 commented May 15, 2020

Uh oh!

jmarantz commented May 15, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aunu53 commented May 20, 2020

Uh oh!

aunu53 commented May 28, 2020

Uh oh!

aunu53 commented May 28, 2020

Uh oh!

repokitteh-read-only Bot commented May 28, 2020

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!

jmarantz May 29, 2020 • 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!

aunu53 commented Jun 1, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aunu53 commented May 11, 2020 •

edited

Loading

aunu53 commented May 14, 2020 •

edited

Loading

jmarantz May 29, 2020 •

edited

Loading