overload: scale transport socket connect timeout #13800

akonradi · 2020-10-28T17:08:25Z

Commit Message: Scale transport socket connect timeout in response to load
Additional Description:
Add support for scaling the transport socket connect timeout with load.

Risk Level: low
Testing: added tests and ran affected tests
Docs Changes: none
Release Notes: none
Platform Specific Features: none
Fixes: #11426

/assign @KBaichoo

Splitting these will prevent circular dependencies when the Dispatcher references thread-local overload state. Signed-off-by: Alex Konradi <akonradi@google.com>

Thread the ThreadLocalOverloadState down into the ConnectionHandlerImpl and ConnectionImpl and use it to create the timer for the transport socket handshake timeout. Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only · 2020-10-28T17:08:34Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13800 was opened by akonradi.

see: more, trace.

KBaichoo

LGTM; most of it was refactoring / interface changes.

KBaichoo · 2020-10-28T21:34:32Z

source/server/overload_manager_impl.cc

@@ -143,6 +142,8 @@ OverloadTimerType parseTimerType(
  switch (config_timer_type) {
  case Config::HTTP_DOWNSTREAM_CONNECTION_IDLE:
    return OverloadTimerType::HttpDownstreamIdleConnectionTimeout;
+  case Config::TRANSPORT_SOCKET_CONNECT:
+    return OverloadTimerType::HttpDownstreamIdleConnectionTimeout;


think you meant TransportSocketConnectTimeout?

That's embarrassing, and that there wasn't a test that caught it is problematic. I've added a test to verify that the correct scale factors are applied for the different timer types.

yanavlasov · 2020-10-29T00:45:18Z

format check failed

Add tests to verify that the correct scale factors are being applied based on the overload manager action config. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi

Fixed formatting too.

akonradi · 2020-10-29T15:46:31Z

source/server/overload_manager_impl.cc

@@ -143,6 +142,8 @@ OverloadTimerType parseTimerType(
  switch (config_timer_type) {
  case Config::HTTP_DOWNSTREAM_CONNECTION_IDLE:
    return OverloadTimerType::HttpDownstreamIdleConnectionTimeout;
+  case Config::TRANSPORT_SOCKET_CONNECT:
+    return OverloadTimerType::HttpDownstreamIdleConnectionTimeout;


That's embarrassing, and that there wasn't a test that caught it is problematic. I've added a test to verify that the correct scale factors are applied for the different timer types.

This addresses the clang-tidy issue Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente

Great to see this happening. First set of comments related just to PR size and unrelated looking changes.

include/envoy/event/scaled_range_timer_manager.h

antoniovicente · 2020-11-03T23:25:57Z

include/envoy/event/scaled_range_timer_manager.h

+  // Test-only functions that need access to the internals of
+  // ScaledTimerMinimum. These are declared here but only defined in tests.
+  friend bool operator==(const ScaledTimerMinimum&, const ScaledTimerMinimum&);
+  friend std::ostream& operator<<(std::ostream&, const ScaledTimerMinimum&);


Would it make sense to implement these functions outside tests?

Only if we want to write these as output anywhere. We don't need to yet, and this avoids (a tiny bit of) binary bloat.

It seems very unusual to implement these functions in a different library. Consider the case of "ForTest" methods. We tend to implement those in the same library as the main object despite they only being used in tests. I think that modern linkers are capable of removing unused code from the resulting binaries, but don't know if they do so by default.

I'm sold, the confusion isn't worth the tiny bit of size gains. Moved these definitions inline.

antoniovicente · 2020-11-03T23:42:50Z

include/envoy/server/overload/overload_manager.h

-#include "envoy/event/timer.h"
-#include "envoy/thread_local/thread_local.h"
+#include "envoy/event/dispatcher.h"
+#include "envoy/server/overload/thread_local_overload_state.h"


The refactor to add a directory for overload manager components in the server subdirectory and thread_local_object.h adds a lot of noise to this review. Worth doing it as a separate change instead? Also see comments about forward declaring instead.

Example:

envoy/include/envoy/network/connection.h

Line 19 in db756af

class Dispatcher;

I'm happy to do the refactor in a separate PR. I don't have strong opinions about forward declarations vs #includes other than an inclination to follow the style guide.

antoniovicente · 2020-11-03T23:56:46Z

source/common/network/connection_impl.cc


 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,
+                                          [this] { onTransportSocketConnectTimeout(); });


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.

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.

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.

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.

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.

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.

Yeah I think that makes sense.

I started working on this. It's doable but introduces some weirdness; see #14401 for more.

Made a fresh attempt at this in #14679 which feels reasonably clean.

antoniovicente · 2020-11-04T00:14:59Z

include/envoy/thread_local/thread_local.h

@@ -6,30 +6,13 @@

 #include "envoy/common/pure.h"
 #include "envoy/event/dispatcher.h"
+#include "envoy/thread_local/thread_local_object.h"
+


I think that the dispatcher could be forward declared and dispatcher interface library depedency removed as a dependency if that would make some of these changes easier.

include/envoy/event/dispatcher.h

antoniovicente · 2020-11-04T00:19:17Z

source/common/event/dispatcher_impl.cc

@@ -9,6 +9,7 @@
 #include "envoy/api/api.h"
 #include "envoy/network/listen_socket.h"
 #include "envoy/network/listener.h"
+#include "envoy/server/overload/overload_manager.h"


Is this include needed? It seems that envoy/server/overload/thread_local_overload_state.h would be sufficient.

Neither is needed since thread_local_overload_state.h is included in the interface header.

source/common/event/dispatcher_impl.h

htuch · 2020-11-09T20:45:05Z

/lgtm api

akonradi

Happy to do the refactoring in a separate PR, or to talk about making the thread-local overload state accessible via the dispatcher.

include/envoy/event/dispatcher.h

include/envoy/event/scaled_range_timer_manager.h

akonradi · 2020-11-09T20:55:21Z

include/envoy/event/scaled_range_timer_manager.h

+  // Test-only functions that need access to the internals of
+  // ScaledTimerMinimum. These are declared here but only defined in tests.
+  friend bool operator==(const ScaledTimerMinimum&, const ScaledTimerMinimum&);
+  friend std::ostream& operator<<(std::ostream&, const ScaledTimerMinimum&);


Only if we want to write these as output anywhere. We don't need to yet, and this avoids (a tiny bit of) binary bloat.

akonradi · 2020-11-09T21:38:22Z

include/envoy/server/overload/overload_manager.h

-#include "envoy/event/timer.h"
-#include "envoy/thread_local/thread_local.h"
+#include "envoy/event/dispatcher.h"
+#include "envoy/server/overload/thread_local_overload_state.h"


I'm happy to do the refactor in a separate PR. I don't have strong opinions about forward declarations vs #includes other than an inclination to follow the style guide.

antoniovicente · 2020-11-09T22:14:49Z

Happy to do the refactoring in a separate PR, or to talk about making the thread-local overload state accessible via the dispatcher.

See #13800 (comment)

If you decide to expose this via dispatcher instead, I suggest you add a createRangeTimer method to the dispatcher interface instead of providing a way to access thread locals via the dispatcher interface.

akonradi · 2020-11-12T16:17:19Z

As discussed offline, I think I'd prefer keeping the scaled timer accessible via the OverloadManager, not the Dispatcher, since the scaling factor is determined by the OM. To that end, I've sent #13999 to do the refactoring requested above separate from this PR.

…-scaled-timeout Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente · 2020-11-13T07:33:02Z

api/envoy/config/overload/v3/overload.proto

@@ -93,6 +93,11 @@ message ScaleTimersOverloadActionConfig {
    // Adjusts the idle timer for downstream HTTP connections that takes effect when there are no active streams.
    // This affects the value of :ref:`RouteAction.idle_timeout <envoy_v3_api_field_config.route.v3.RouteAction.idle_timeout>`.
    HTTP_DOWNSTREAM_CONNECTION_IDLE = 1;
+
+    // Adjusts the timer for how long downstream clients have to finish transport-level negotiations before the connection is closed.


nit: This line seem very long. Can you rerun format?

Fixed manually since the formatter doesn't seem to touch proto comments.

antoniovicente · 2020-11-13T07:38:31Z

include/envoy/event/scaled_range_timer_manager.h

+  // Test-only functions that need access to the internals of
+  // ScaledTimerMinimum. These are declared here but only defined in tests.
+  friend bool operator==(const ScaledTimerMinimum&, const ScaledTimerMinimum&);
+  friend std::ostream& operator<<(std::ostream&, const ScaledTimerMinimum&);


It seems very unusual to implement these functions in a different library. Consider the case of "ForTest" methods. We tend to implement those in the same library as the main object despite they only being used in tests. I think that modern linkers are capable of removing unused code from the resulting binaries, but don't know if they do so by default.

antoniovicente · 2020-11-13T07:47:40Z

test/common/network/connection_impl_test.cc

-    return ConnectionMocks{std::move(dispatcher), timer, std::move(transport_socket), file_event,
-                           &file_ready_cb_};
+    return ConnectionMocks{std::move(dispatcher),       {},         timer,
+                           std::move(transport_socket), file_event, &file_ready_cb_};


nit: unusual looking formatting, can you rerun format?

Reran format.

Seems that the {}'s around the arguments are confusing the formatter, ugh.

nit: will explicitly constructing Server::MockThreadLocalOverloadState() make formatter happy?

antoniovicente · 2020-11-13T07:56:32Z

test/server/overload_manager_impl_test.cc

  )YAML";

+// These are the timer types according to the reduced timeouts config above.
+constexpr std::pair<OverloadTimerType, Event::ScaledTimerMinimum> kReducedTimeoutsMinimums[]{


nit: Please move this list of cases inside the relevant test case to avoid having a large separation between these definitions and the test body.

This list matches exactly what kReducedTimeoutsConfig produces, so I wanted to keep them close together. Please push back if that's not a good enough reason.

I don't know which of the two separations is more problematic. I would consider having both in the test case so everything is in 1 place, or move OverloadManagerImplTest.TimerTypesProduceCorrectMinimums so it is just below this list definition.

Moved the test case; that's a good option.

akonradi · 2020-12-11T16:07:46Z

I'm trying to figure out what integrating the thread-local overload state into the dispatcher would look like. Should I consider that a blocker for getting this PR merged?

antoniovicente · 2020-12-11T19:04:20Z

I'm trying to figure out what integrating the thread-local overload state into the dispatcher would look like. Should I consider that a blocker for getting this PR merged?

I defer to @ggreenway 's opinion.

Seems like you accumulated some merge conflicts as your other PRs which modify some of the same files got merged in.

ggreenway · 2020-12-15T16:47:03Z

I'm trying to figure out what integrating the thread-local overload state into the dispatcher would look like. Should I consider that a blocker for getting this PR merged?

Yes, I'd like to have that sorted out before we merge this.

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2021-01-20T18:45:04Z

I've merged main. This is now using Dispatcher::createScaledTimer, which obviates the need for plumbing around the ThreadLocalOverloadState.

akonradi · 2021-01-20T23:03:56Z

/retest

repokitteh-read-only · 2021-01-20T23:04:00Z

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

🐱

Caused by: a #13800 (comment) was created by @akonradi.

see: more, trace.

antoniovicente

Looks great otherwise.

@ggreenway could you take a look?

api/envoy/config/overload/v3/overload.proto

…caled-timeout Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

ggreenway

This looks great, aside from one nit.

include/envoy/event/scaled_timer.h

…caled-timeout Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2021-01-25T15:07:41Z

/retest

repokitteh-read-only · 2021-01-25T15:07:46Z

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

🐱

Caused by: a #13800 (comment) was created by @akonradi.

see: more, trace.

akonradi · 2021-01-25T20:26:24Z

@envoyproxy/api-shepherds for the new timer type

akonradi added 2 commits October 28, 2020 13:04

Split thread_local_object.h and overload_manager.h

abcdb84

Splitting these will prevent circular dependencies when the Dispatcher references thread-local overload state. Signed-off-by: Alex Konradi <akonradi@google.com>

Scale transport socket handshake timeout

f5b8d56

Thread the ThreadLocalOverloadState down into the ConnectionHandlerImpl and ConnectionImpl and use it to create the timer for the transport socket handshake timeout. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi requested review from alyssawilk, asraa, lizan, mattklein123 and PiotrSikora as code owners October 28, 2020 17:08

repokitteh-read-only bot assigned KBaichoo Oct 28, 2020

repokitteh-read-only bot added the api label Oct 28, 2020

KBaichoo previously approved these changes Oct 28, 2020

View reviewed changes

Add tests for different scaled timer types

7d28234

Add tests to verify that the correct scale factors are being applied based on the overload manager action config. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed KBaichoo’s stale review via 7d28234 October 29, 2020 15:45

akonradi commented Oct 29, 2020

View reviewed changes

akonradi added 2 commits November 3, 2020 11:18

Change PrintTo to operator<<

5d0d600

This addresses the clang-tidy issue Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into tls-scaled-timeout

76cd8c4

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente self-assigned this Nov 3, 2020

antoniovicente reviewed Nov 4, 2020

View reviewed changes

antoniovicente added the waiting:any label Nov 7, 2020

repokitteh-read-only bot removed api waiting:any labels Nov 9, 2020

akonradi commented Nov 9, 2020

View reviewed changes

akonradi mentioned this pull request Nov 12, 2020

server: split thread_local_object.h and overload_manager.h #13999

Merged

Merge branch 'master' of https://github.com/envoyproxy/envoy into tls…

2f169bf

…-scaled-timeout Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi requested a review from ggreenway as a code owner November 12, 2020 20:51

antoniovicente previously approved these changes Nov 13, 2020

View reviewed changes

akonradi mentioned this pull request Dec 14, 2020

Refactor Envoy's Dispatcher abstraction #14401

Closed

akonradi mentioned this pull request Dec 16, 2020

[tls] timeout for TLS handshakes #11426

Closed

akonradi mentioned this pull request Dec 29, 2020

overload: remove createScaledTimer with OverloadTimerType #14536

Closed

akonradi mentioned this pull request Jan 12, 2021

overload: create scaled timers via the dispatcher #14679

Merged

Base automatically changed from master to main January 15, 2021 23:01

Merge remote-tracking branch 'upstream/main' into tls-scaled-timeout

ee93040

Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only bot added the api label Jan 20, 2021

Fix formatting

ddad3e5

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Jan 20, 2021

View reviewed changes

api/envoy/config/overload/v3/overload.proto Show resolved Hide resolved

akonradi mentioned this pull request Jan 21, 2021

docs: cover timeout scaling in timeout FAQ #14775

Open

akonradi added 2 commits January 21, 2021 11:04

Merge branch 'main' of https://github.com/envoyproxy/envoy into tls-s…

9be6e8f

…caled-timeout Signed-off-by: Alex Konradi <akonradi@google.com>

Document new scaled timer in release notes

475253b

Signed-off-by: Alex Konradi <akonradi@google.com>

ggreenway requested changes Jan 21, 2021

View reviewed changes

include/envoy/event/scaled_timer.h Outdated Show resolved Hide resolved

akonradi added 2 commits January 22, 2021 11:08

Merge branch 'main' of https://github.com/envoyproxy/envoy into tls-s…

2f2ed9f

…caled-timeout Signed-off-by: Alex Konradi <akonradi@google.com>

Remove unused operator<< methods

6ad33cf

Signed-off-by: Alex Konradi <akonradi@google.com>

ggreenway approved these changes Jan 22, 2021

View reviewed changes

lizan approved these changes Jan 25, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jan 25, 2021

lizan merged commit 8814014 into envoyproxy:main Jan 25, 2021

overload: scale transport socket connect timeout #13800

overload: scale transport socket connect timeout #13800

Conversation

akonradi commented Oct 28, 2020 • edited by antoniovicente Loading

repokitteh-read-only bot commented Oct 28, 2020

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Oct 29, 2020

akonradi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Nov 9, 2020

akonradi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente commented Nov 9, 2020

akonradi commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akonradi commented Dec 11, 2020

antoniovicente commented Dec 11, 2020

ggreenway commented Dec 15, 2020

akonradi commented Jan 20, 2021

akonradi commented Jan 20, 2021

repokitteh-read-only bot commented Jan 20, 2021

antoniovicente left a comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

akonradi commented Jan 25, 2021

repokitteh-read-only bot commented Jan 25, 2021

akonradi commented Jan 25, 2021

akonradi commented Oct 28, 2020 •

edited by antoniovicente

Loading

antoniovicente Nov 4, 2020 •

edited

Loading