Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add overload action for closing new streams #4126

Merged
merged 9 commits into from
Sep 14, 2018
Merged

Add overload action for closing new streams #4126

merged 9 commits into from
Sep 14, 2018

Conversation

eziskind
Copy link
Contributor

Add an overload action in the http connection manager to immediately close new streams in case of envoy overload (issue #373).

Signed-off-by: Elisha Ziskind eziskind@google.com

@eziskind
Copy link
Contributor Author

Test failures seem unrelated.
asan failure - @envoy//test/integration:hds_integration_test FAILED in 4.7s
tsan failure - envoy//test/integration:echo_integration_test TIMEOUT in 315.0s

@mattklein123 mattklein123 self-assigned this Aug 14, 2018
mattklein123
mattklein123 reviewed Aug 14, 2018
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super cool. A few high level comments to get started.

source/common/http/conn_manager_impl.cc Outdated
ActiveStream* stream = streams_.begin()->get();

if (overload_stop_accepting_streams_ == Server::OverloadActionState::Active) {
read_callbacks_->connection().dispatcher().post([this, stream] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the post here was because doing a direct response from within the newStream callback does not work? (Not surprising). Ultimately, it would be nice if the newStream() callback could do the direct response immediately without a dispatcher post, but it seems fine to indicate that via comment/TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just tried the unit tests with a direct response and it worked fine so I'll send an update to do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely convinced this will work in the real server but I'm not sure. Can you look into adding an integration test? I think for this type of feature it would be great to have one. I'm thinking you could develop an overload plugin just for integration tests that you can completely control and it probably wouldn't be that hard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will look into adding an integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an integration test for this. Also moved this code that drops new requests (by doing a sendLocalReply) to after envoy has decoded the client headers, rather than when we create the stream which is too early to close when using http2.

source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Aug 22, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 22, 2018
@htuch htuch added the no stalebot Disables stalebot from closing an issue label Aug 22, 2018
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 22, 2018
@eziskind
Copy link
Contributor Author

Was OOO last week, but will get back to working on the integration test this week.

@eziskind
Add overload action for closing new streams
73bbf9f 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Remove stale code from an incorrect merge
59fbce3 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Fix HCM fuzz test
ac41903 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
zuercher
zuercher reviewed Sep 14, 2018
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One take-it-or-leave-it thought and question. Otherwise, looks good to me.

source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved
source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved
@eziskind
Address review feedback and fix integration test in release build
bc69b1f 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Use ASSERT_TRUE for tests
c4edcda 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Kick CI
7684435 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
htuch
htuch suggested changes Sep 14, 2018
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; some nits and then can ship.

source/common/http/conn_manager_impl.cc Show resolved Hide resolved
test/common/http/conn_manager_impl_test.cc Outdated Show resolved Hide resolved
include/envoy/server/overload_manager.h Outdated Show resolved Hide resolved
test/integration/overload_integration_test.cc Outdated Show resolved Hide resolved
@htuch htuch self-assigned this Sep 14, 2018
@eziskind
Address more review feedback
cddab27 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Remove all .Times(1) from HCM unit tests and add some comments
c130798 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
fix format
cb43f4f 
Signed-off-by: Elisha Ziskind <eziskind@google.com>
htuch
htuch approved these changes Sep 14, 2018
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@zuercher zuercher merged commit 35ec541 into envoyproxy:master Sep 14, 2018
@eziskind eziskind deleted the overloadhcm branch September 14, 2018 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@zuercher zuercher zuercher approved these changes

@htuch htuch htuch approved these changes

@lizan lizan Awaiting requested review from lizan

Assignees

@mattklein123 mattklein123

@htuch htuch

Labels
no stalebot Disables stalebot from closing an issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eziskind @zuercher @mattklein123 @htuch