HTTP2 Proactive GOAWAY on Drain - Preamble #17026

murray-stripe · 2021-06-17T15:47:00Z

Commit Message:

The initial set of changes (broken up to reduce PR/change size) to
enable proactive GOAWAY signals on drain events. These changes
(currently a no-op) open the way for a push-based notification model to
the Http::ConnectionManagerImpl class (or any other interested party).
Allowing a move away from the pull-based model that currently exists,
which causes issues such as #14350.

The primary change here is to introduce the idea of a "drain tree" which
allows the drain-manager to be structured with parent and child
managers. Each manager may drain independent of it's parent and all
drain actions cascade downward through the tree. The parent <-> child
relationship is thread-safe.

In addition, drain events can now be registered via callbacks to be
invoked when draining starts. This is a non-thread-safe operation.

A new thread-safe callback manager utility has been written to support
the parent <-> child drain-manager relationship.

Additional test coverage has been added for the drain-manager and
thread-safe callback-manager, as well as updates to necessary mock
classes.

Because the callback registration impacts the DrainDecision interface,
necessary no-op implementations have been addedto various classes
implementing this interface. The final versions will be wired up with
correct behavior in subsequent PRs that take advantage of the new
functionality.

Additional Description: This PR was created at the advisement of @antoniovicente in relation to #16201 as an effort to reduce the amount of change being reviewed in a single PR. The content of this PR is meant to be a no-op with #16201 enacting the change (and will be updated once this is merged to reduce it's scope).

Risk Level: Low. Changes included should be no-op.
Testing: Additional test coverage included.
Docs Changes: None.
Release Notes: None.
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

The initial set of changes (broken up to reduce PR/change size) to enable proactive GOAWAY signals on drain events. These changes (currently a no-op) open the way for a push-based notification model to the Http::ConnectionManagerImpl class (or any other interested party). Aloowing a move _away_ from the pull-based model that currently exists, which causes issues such as envoyproxy#14350. The primary change here is to introduce the idea of a "drain tree" which allows the drain-manager to be structured with parent and child managers. Each manager may drain independent of it's parent and all drain actions cascade downward through the tree. The parent <-> child relationship is thread-safe. In addition, drain events can now be registered via callbacks to be invoked when draining starts. This is a non-thread-safe operation. A new thread-safe callback manager utility has been written to support the parent <-> child drain-manager relationship. Additional test coverage has been added for the drain-manager and thread-safe callback-manager, as well as updates to necessary mock classes. Because the callback registration impacts the DrainDecision interface, necessary no-op implementations have been addedto various classes implementing this interface. The final versions will be wired up with correct behavior in subsequent PRs that take advantage of the new functionality. Signed-off-by: John Murray <murray@stripe.com>

source/common/common/callback_impl.cc

Signed-off-by: John Murray <murray@stripe.com>

murray-stripe · 2021-06-17T20:10:41Z

startDrainSequence can only be called once on each DrainManagerImpl as enforced by ASSERTs. It seems to me that PerFilterChainFactoryContextImpl and InstanceImpl::drainListeners() can both invoke startDrainSequence, which would result in startDrainSequence being called twice on the same object: once via child relations on the global drain impl owned by the server instance and again directly via the filter chain manager.

This was a super good callout. I had anticipated for double-calling in the order of 1. direct, 2. parent; but not the other way around. I have updated the code to allow for multiple invocations and simply handling multiple on-drain-complete callbacks. I've added additional test cases to validate this behavior.

murray-stripe · 2021-06-18T13:28:00Z

/retest

repokitteh-read-only · 2021-06-18T13:28:04Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-18T18:00:18Z

The MacOS tests keep being cancelled (I'm guessing it's timing out), but are otherwise passing up until that point. This is ready for review.

source/common/common/callback_impl.cc

source/common/common/callback_impl.h

Signed-off-by: John Murray <murray@stripe.com>

murray-stripe · 2021-06-22T14:46:45Z

/retest

repokitteh-read-only · 2021-06-22T14:46:49Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-22T14:51:18Z

@antoniovicente I have incorporated your feedback and this is ready for review.

antoniovicente

Almost there.

/wait

source/server/drain_manager_impl.cc

source/common/common/callback_impl.cc

source/common/common/callback_impl.h

Signed-off-by: John Murray <murray@stripe.com>

murray-stripe · 2021-06-22T21:59:56Z

@antoniovicente I've addressed your feedback. Really liking how things have simplified. 😄

antoniovicente

The change looks great. Thanks for this refactor in preparation for H2 drain propagation.

/assign-from @envoyproxy/senior-maintainers for a final approval.

repokitteh-read-only · 2021-06-22T22:22:38Z

@envoyproxy/senior-maintainers assignee is @zuercher
for assignee is @None
a assignee is @None
final assignee is @None
approval. assignee is @None

🐱

Caused by: a #17026 (review) was submitted by @antoniovicente.

see: more, trace.

antoniovicente · 2021-06-23T02:28:44Z

retrying macos timeout

/retest

repokitteh-read-only · 2021-06-23T02:28:47Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @antoniovicente.

see: more, trace.

murray-stripe · 2021-06-23T13:48:24Z

macos still timing out (seems to be the case most of the time)

/retest

repokitteh-read-only · 2021-06-23T13:48:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

zuercher

This looks good to me. I found some small issues, in comments and had one question about a test. None of these are blocking.

envoy/server/drain_manager.h

test/server/drain_manager_impl_test.cc

test/common/common/callback_impl_test.cc

Signed-off-by: John Murray <murray@stripe.com>

murray-stripe · 2021-06-23T22:10:52Z

@zuercher Thank you for the review! I have addressed your feedback.

murray-stripe · 2021-06-24T14:06:19Z

/retest

repokitteh-read-only · 2021-06-24T14:06:23Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-24T15:12:14Z

/retest

repokitteh-read-only · 2021-06-24T15:12:17Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-24T22:52:01Z

/retest

repokitteh-read-only · 2021-06-24T22:52:05Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-25T13:37:10Z

/retest

repokitteh-read-only · 2021-06-25T13:37:14Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-25T14:27:53Z

gah, keep getting some failures in the coverage build for //test/extensions/filters/http/kill_request:kill_request_filter_integration_test. Seems to be pretty flakey. will give one more re-test and then merge in main if one more doesn't work.

/retest

repokitteh-read-only · 2021-06-25T14:27:56Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17026 (comment) was created by @murray-stripe.

see: more, trace.

murray-stripe · 2021-06-25T15:20:56Z

@zuercher This is ready for your review/merge again.

zuercher

Thanks!

mattklein123

Thanks very cool stuff. A few post-submit drive by comments for your next PR. Thank you @murray-stripe!

source/server/drain_manager_impl.h

envoy/network/drain_decision.h

alyssawilk · 2021-06-29T17:43:38Z

Hey @murray-stripe could you take a look at
https://storage.googleapis.com/envoy-postsubmit/main/coverage/source/common/common/callback_impl.cc.gcov.html and do a follow-up with some extra unit testing? I think this PR has caused some flakes with falling under our coverage bar. I'm addressing with #17190 but it'd be great to add another test and bump the number back up.

Thanks!

The initial set of changes (broken up to reduce PR/change size) to enable proactive GOAWAY signals on drain events. These changes (currently a no-op) open the way for a push-based notification model to the Http::ConnectionManagerImpl class (or any other interested party). Aloowing a move _away_ from the pull-based model that currently exists, which causes issues such as envoyproxy#14350. The primary change here is to introduce the idea of a "drain tree" which allows the drain-manager to be structured with parent and child managers. Each manager may drain independent of it's parent and all drain actions cascade downward through the tree. The parent <-> child relationship is thread-safe. In addition, drain events can now be registered via callbacks to be invoked when draining starts. This is a non-thread-safe operation. A new thread-safe callback manager utility has been written to support the parent <-> child drain-manager relationship. Additional test coverage has been added for the drain-manager and thread-safe callback-manager, as well as updates to necessary mock classes. Because the callback registration impacts the DrainDecision interface, necessary no-op implementations have been addedto various classes implementing this interface. The final versions will be wired up with correct behavior in subsequent PRs that take advantage of the new functionality. Risk Level: Low. Changes included should be no-op. Testing: Additional test coverage included. Docs Changes: n/a Release Notes: n/a Signed-off-by: John Murray <murray@stripe.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

The initial set of changes (broken up to reduce PR/change size) to enable proactive GOAWAY signals on drain events. These changes (currently a no-op) open the way for a push-based notification model to the Http::ConnectionManagerImpl class (or any other interested party). Aloowing a move _away_ from the pull-based model that currently exists, which causes issues such as envoyproxy#14350. The primary change here is to introduce the idea of a "drain tree" which allows the drain-manager to be structured with parent and child managers. Each manager may drain independent of it's parent and all drain actions cascade downward through the tree. The parent <-> child relationship is thread-safe. In addition, drain events can now be registered via callbacks to be invoked when draining starts. This is a non-thread-safe operation. A new thread-safe callback manager utility has been written to support the parent <-> child drain-manager relationship. Additional test coverage has been added for the drain-manager and thread-safe callback-manager, as well as updates to necessary mock classes. Because the callback registration impacts the DrainDecision interface, necessary no-op implementations have been addedto various classes implementing this interface. The final versions will be wired up with correct behavior in subsequent PRs that take advantage of the new functionality. Risk Level: Low. Changes included should be no-op. Testing: Additional test coverage included. Docs Changes: n/a Release Notes: n/a Signed-off-by: John Murray <murray@stripe.com>

murray-stripe commented Jun 17, 2021

View reviewed changes

source/common/common/callback_impl.cc Outdated Show resolved Hide resolved

murray-stripe mentioned this pull request Jun 17, 2021

HTTP2 proactive GOAWAY on drain-start #16201

Closed

antoniovicente self-assigned this Jun 17, 2021

handle multiple invocations of 'startDrainSequence' + tests

d738e86

Signed-off-by: John Murray <murray@stripe.com>

antoniovicente reviewed Jun 18, 2021

View reviewed changes

source/common/common/callback_impl.cc Outdated Show resolved Hide resolved

source/common/common/callback_impl.h Outdated Show resolved Hide resolved

antoniovicente added the waiting:any label Jun 18, 2021

repokitteh-read-only bot removed the waiting:any label Jun 21, 2021

antoniovicente added the waiting:any label Jun 21, 2021

remove DrainManagerSharedPtr and ThreadSafeCallbackHandlePtr

d24e121

Signed-off-by: John Murray <murray@stripe.com>

repokitteh-read-only bot removed the waiting:any label Jun 21, 2021

antoniovicente reviewed Jun 22, 2021

View reviewed changes

source/server/drain_manager_impl.cc Outdated Show resolved Hide resolved

source/common/common/callback_impl.cc Outdated Show resolved Hide resolved

source/common/common/callback_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 22, 2021

PR Feedback

e5bf0ab

Signed-off-by: John Murray <murray@stripe.com>

repokitteh-read-only bot removed the waiting label Jun 22, 2021

antoniovicente previously approved these changes Jun 22, 2021

View reviewed changes

repokitteh-read-only bot assigned zuercher Jun 22, 2021

zuercher previously approved these changes Jun 23, 2021

View reviewed changes

Typo's + test update

2de1367

Signed-off-by: John Murray <murray@stripe.com>

murray-stripe dismissed stale reviews from zuercher and antoniovicente via 2de1367 June 23, 2021 22:09

zuercher approved these changes Jun 28, 2021

View reviewed changes

zuercher merged commit 96dd735 into envoyproxy:main Jun 28, 2021

mattklein123 reviewed Jun 28, 2021

View reviewed changes

source/server/drain_manager_impl.h Show resolved Hide resolved

envoy/network/drain_decision.h Show resolved Hide resolved

alyssawilk mentioned this pull request Jun 29, 2021

coverage: fixing flake #17190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP2 Proactive GOAWAY on Drain - Preamble #17026

HTTP2 Proactive GOAWAY on Drain - Preamble #17026

murray-stripe commented Jun 17, 2021

murray-stripe commented Jun 17, 2021

murray-stripe commented Jun 18, 2021

repokitteh-read-only bot commented Jun 18, 2021

murray-stripe commented Jun 18, 2021

murray-stripe commented Jun 22, 2021

repokitteh-read-only bot commented Jun 22, 2021

murray-stripe commented Jun 22, 2021

antoniovicente left a comment

murray-stripe commented Jun 22, 2021

antoniovicente left a comment

repokitteh-read-only bot commented Jun 22, 2021

antoniovicente commented Jun 23, 2021

repokitteh-read-only bot commented Jun 23, 2021

murray-stripe commented Jun 23, 2021

repokitteh-read-only bot commented Jun 23, 2021

zuercher left a comment

murray-stripe commented Jun 23, 2021

murray-stripe commented Jun 24, 2021

repokitteh-read-only bot commented Jun 24, 2021

murray-stripe commented Jun 24, 2021

repokitteh-read-only bot commented Jun 24, 2021

murray-stripe commented Jun 24, 2021

repokitteh-read-only bot commented Jun 24, 2021

murray-stripe commented Jun 25, 2021

repokitteh-read-only bot commented Jun 25, 2021

murray-stripe commented Jun 25, 2021

repokitteh-read-only bot commented Jun 25, 2021

murray-stripe commented Jun 25, 2021

zuercher left a comment

mattklein123 left a comment

alyssawilk commented Jun 29, 2021

HTTP2 Proactive GOAWAY on Drain - Preamble #17026

HTTP2 Proactive GOAWAY on Drain - Preamble #17026

Conversation

murray-stripe commented Jun 17, 2021

murray-stripe commented Jun 17, 2021

murray-stripe commented Jun 18, 2021

repokitteh-read-only bot commented Jun 18, 2021

murray-stripe commented Jun 18, 2021

murray-stripe commented Jun 22, 2021

repokitteh-read-only bot commented Jun 22, 2021

murray-stripe commented Jun 22, 2021

antoniovicente left a comment

Choose a reason for hiding this comment

murray-stripe commented Jun 22, 2021

antoniovicente left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jun 22, 2021

antoniovicente commented Jun 23, 2021

repokitteh-read-only bot commented Jun 23, 2021

murray-stripe commented Jun 23, 2021

repokitteh-read-only bot commented Jun 23, 2021

zuercher left a comment

Choose a reason for hiding this comment

murray-stripe commented Jun 23, 2021

murray-stripe commented Jun 24, 2021

repokitteh-read-only bot commented Jun 24, 2021

murray-stripe commented Jun 24, 2021

repokitteh-read-only bot commented Jun 24, 2021

murray-stripe commented Jun 24, 2021

repokitteh-read-only bot commented Jun 24, 2021

murray-stripe commented Jun 25, 2021

repokitteh-read-only bot commented Jun 25, 2021

murray-stripe commented Jun 25, 2021

repokitteh-read-only bot commented Jun 25, 2021

murray-stripe commented Jun 25, 2021

zuercher left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk commented Jun 29, 2021