-
Notifications
You must be signed in to change notification settings - Fork 5.5k
overload: scale transport socket connect timeout #13800
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.
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
Changes from 10 commits
abcdb84
f5b8d56
7d28234
5d0d600
76cd8c4
2f169bf
0c1cf92
3376062
927ea02
68e62d4
9d85d8a
29ea543
f52c76e
a5ccd82
18f6022
4ca4b11
ee93040
ddad3e5
9be6e8f
475253b
2f2ed9f
6ad33cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #include "envoy/event/timer.h" | ||
| #include "envoy/network/filter.h" | ||
| #include "envoy/network/socket.h" | ||
| #include "envoy/server/overload/thread_local_overload_state.h" | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/common/empty_string.h" | ||
|
|
@@ -730,19 +731,22 @@ void ConnectionImpl::flushWriteBuffer() { | |
| } | ||
|
|
||
| ServerConnectionImpl::ServerConnectionImpl(Event::Dispatcher& dispatcher, | ||
| Server::ThreadLocalOverloadState& overload_state, | ||
| ConnectionSocketPtr&& socket, | ||
| TransportSocketPtr&& transport_socket, | ||
| StreamInfo::StreamInfo& stream_info, bool connected) | ||
| : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info, | ||
| connected) {} | ||
| connected), | ||
| overload_state_(overload_state) {} | ||
|
|
||
| void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) { | ||
| if (!transport_connect_pending_) { | ||
| return; | ||
| } | ||
| if (transport_socket_connect_timer_ == nullptr) { | ||
| transport_socket_connect_timer_ = | ||
| dispatcher_.createTimer([this] { onTransportSocketConnectTimeout(); }); | ||
| overload_state_.createScaledTimer(Server::OverloadTimerType::TransportSocketConnectTimeout, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a local change that undid changes to this line and fixed all the unused variable compile errors, and noticed that the only test that failed //test/common/network:connection_impl_test due to changes to mock expectations. It would be good to have some e2e integration test coverage for this functionality. This issue also came up in #14155 . We have multiple scaled timers, we should have some generic recipes for how to integration test scaled timer functionality. Possible strawman drive the connection to a certain state, force proxy into overload and verify that the timeout triggers shortly afterwards.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sent #14290 to do just that for the existing scaled timer. |
||
| [this] { onTransportSocketConnectTimeout(); }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should revisit the possibility of having the dispatcher be the one that is aware of the overload state and provide a method to create scaled timers. Dispatcher is plumbed in widely. I can see a counter argument involving our desire to bring overload manager closer to connections as a starting point for adjusting work done on wakeup based on overload state; something like smaller allocations/reads when under memory pressure. So this new plumbing may be more generally useful. But I think the direction we're taking there is to measure memory usage by connection, which actually doesn't require involvement from the overload mananger.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned elsewhere, I'd rather keep scaled timer creation going through the Overload Manager since it is ultimately responsible for determining the scale factor of those timers. We can revisit making the Dispatcher aware of the OM later.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of plumbing the OM widely, could the dispatcher hold a reference to it? I dislike that there are now two very different places to create timers: OM and dispatcher.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a chicken-and-egg problem there since the thread-local overload state requires a reference to the dispatcher. We could work around this, though, if that sounds preferable. It seems like the larger issue is that there is a divide between the dispatcher, which represents a thread worker, and thread-local state, which is managed elsewhere.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dispatcher is also owned by the thread. I wonder if the solution would be to have the dispatcher own the thread overload state instead of having it be part of the thread local storage.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah I think that makes sense.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I started working on this. It's doable but introduces some weirdness; see #14401 for more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made a fresh attempt at this in #14679 which feels reasonably clean. |
||
| } | ||
| transport_socket_connect_timer_->enableTimer(timeout); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.