HTTP2 Proactive GOAWAY on Drain - Preamble#17026
Conversation
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>
Signed-off-by: John Murray <murray@stripe.com>
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. |
|
/retest |
|
Retrying Azure Pipelines: |
|
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. |
Signed-off-by: John Murray <murray@stripe.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
@antoniovicente I have incorporated your feedback and this is ready for review. |
antoniovicente
left a comment
There was a problem hiding this comment.
Almost there.
/wait
Signed-off-by: John Murray <murray@stripe.com>
|
@antoniovicente I've addressed your feedback. Really liking how things have simplified. 😄 |
antoniovicente
left a comment
There was a problem hiding this comment.
The change looks great. Thanks for this refactor in preparation for H2 drain propagation.
/assign-from @envoyproxy/senior-maintainers for a final approval.
|
@envoyproxy/senior-maintainers assignee is @zuercher |
|
retrying macos timeout /retest |
|
Retrying Azure Pipelines: |
|
macos still timing out (seems to be the case most of the time) /retest |
|
Retrying Azure Pipelines: |
zuercher
left a comment
There was a problem hiding this comment.
This looks good to me. I found some small issues, in comments and had one question about a test. None of these are blocking.
Signed-off-by: John Murray <murray@stripe.com>
2de1367
|
@zuercher Thank you for the review! I have addressed your feedback. |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
gah, keep getting some failures in the coverage build for /retest |
|
Retrying Azure Pipelines: |
|
@zuercher This is ready for your review/merge again. |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks very cool stuff. A few post-submit drive by comments for your next PR. Thank you @murray-stripe!
|
Hey @murray-stripe could you take a look at 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>
…roxy#17026)" This reverts commit 96dd735. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…roxy#17026)" This reverts commit 96dd735. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…roxy#17026)" This reverts commit 96dd735. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…roxy#17026)" This reverts commit 96dd735. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…rain - Preamble (envoyproxy#17026)" (envoyproxy#37465)" This reverts commit 072831e. Signed-off-by: Yan Avlasov <yavlasov@google.com>
…rain - Preamble (#17026)" (#37465)" (#38333) Commit Message: This reverts commit 072831e. Revert in PR-37465 created problems for boundary proxy for Google partner cloud, as it depends on the reverted internal API `addOnDrainCloseCb`. Boundary proxy is open source and Google cloud partners will be unable to build OSS version, since it depends on OSS Envoy. Putting back the reverted code for now, as it is unclear what exactly can be removed from Envoy at this point. Risk Level: Low Testing: Unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Yan Avlasov <yavlasov@google.com>
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:]