overload: tcp connection refusal overload action by akonradi · Pull Request #13311 · envoyproxy/envoy

akonradi · 2020-09-29T15:15:27Z

Commit Message: Add an overload action to reject (accept and then close) TCP connections
Additional Description:
Add an action "envoy.overload_actions.reject_incoming_connections" that will reject
incoming connections when Envoy is overloaded. It will randomly choose whether to
reject connections based on the overload factor, with expected behavior at the ends
of the range (reject no connections at 0, reject all at saturation).
Risk Level: low
Testing: ran unit tests
Docs Changes: documented new overload action
Release Notes: documented new overload action

This will allow creating an overload action that rejects (accepts the connection and then closes it) some fraction of connections in response to overload conditions. Signed-off-by: Alex Konradi <akonradi@google.com>

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

akonradi · 2020-09-29T15:16:11Z

I still need to add documentation, but I wanted to get the code parts in shape first.

alyssawilk

Looks great overall - thanks for picking this up!

alyssawilk · 2020-09-30T19:22:41Z

source/server/connection_handler_impl.cc

 }

+void ConnectionHandlerImpl::setListenerRejectFraction(float reject_fraction) {
+  disable_listeners_ = false;


seems strange to change this boolean without resuming listening.
Also if new listeners are added won't they be added listening but without a rejection fraction?

Thanks, this was a bad copy-paste on my part. Fixed in the latest commit, and added tests.

alyssawilk · 2020-09-30T19:23:07Z

include/envoy/server/overload_manager.h

+      return true;
+    }
+
+    return random_generator.random() <


maybe comment why this is overflow-safe?

alyssawilk · 2020-09-30T19:23:27Z

include/envoy/server/overload_manager.h

+   * of the level of saturation.
+   */
+  bool isRandomizedActive(Random::RandomGenerator& random_generator) const {
+    if (isInactive()) {


Is this just for performance, or do the edges not work well with the casts?

This was stolen from #13120, but I assume it's for performance - don't grab a random number if it's not necessary. That's why this PR is still a draft.

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

Address feedback around adding new listeners when the reject fraction is already set, and add tests. Signed-off-by: Alex Konradi <akonradi@google.com>

alyssawilk

I think the structure looks good overall, so want to ping when you have docs and relnotes and such? It wouldn't hurt to have an integration test too. Even if we can only usefully test the case where we're rejecting 100%, we could test accepting, config reload, and rejecting.

alyssawilk · 2020-10-01T13:42:09Z

source/common/network/tcp_listener_impl.cc

    }

-    if (rejectCxOverGlobalLimit()) {
+    if (rejectCxOverGlobalLimit() || reject_fraction_.isRandomizedActive(random_)) {


Once #13305 lands (which I think will before this) I thin it'd be useful to tag why the connection was closed.

I don't think we can tag anything here since this happens before an Envoy connection object is created, so there's nothing to annotate.

alyssawilk · 2020-10-01T13:43:29Z

source/common/network/tcp_listener_impl.cc

+    if (rejectCxOverGlobalLimit() || reject_fraction_.isRandomizedActive(random_)) {
      // The global connection limit has been reached.
      io_handle->close();
      cb_.onReject();


Looks like this increments stats_.downstream_global_cx_overflow_.inc()
Think we should have a separate limit for overflow?
If not maybe at least update the docs to note the stat covers both overflow cases.

I've added a separate stat.

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

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

akonradi · 2020-10-09T21:12:14Z

@alyssawilk I've updated the release notes and documentation, PTAL

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

alyssawilk · 2020-10-12T15:11:13Z

Looks great thanks. Throwing over to Matt for !googler review but in the meantime if you're up for updating the TODOs in PR description that'd be helpful

akonradi · 2020-10-12T15:18:44Z

Thanks, updated TODOs (that I had completely forgotten about)

mattklein123 · 2020-10-12T17:40:10Z

Windows CI failure looks real. Can you take a look?

/wait

Relax the ordering constraints for events that occur asynchronously on different ends of the connection while maintaining requirements for relative ordering on each end. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-10-12T21:34:39Z

Relaxed some of the ordering requirements in the test for things that shouldn't necessarily be synchronized. Looks like the tests are passing now

mattklein123

Very nice! LGTM with small comments. Thank you.

/wait

mattklein123 · 2020-10-12T22:43:32Z

source/common/network/tcp_listener_impl.cc

 void TcpListenerImpl::disable() { file_event_->setEnabled(0); }

+void TcpListenerImpl::setRejectFraction(const float reject_fraction) {
+  reject_fraction_ = reject_fraction;


Can you ASSERT 0 <= reject_fraction <= 1? I think the Bernoulli function correctly handles out of bounds but might as well try to check for obvious config/code bugs. (Bonus points would be to introduce a struct wrapper for the float which would check this at point of assignment, etc. but up to you.)

mattklein123 · 2020-10-12T22:45:49Z

source/server/connection_handler_impl.cc

+    stats_.downstream_global_cx_overflow_.inc();
+    break;
+  case RejectCause::OverloadAction:
+    stats_.downstream_cx_overload_reject_.inc();


Please verify this stat increment in one of your tests.

Added a unit test.

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

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

mattklein123 · 2020-10-13T15:00:35Z

Needs main merge.

/wait

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

akonradi · 2020-10-13T15:25:49Z

Yep, I managed to add new tests in the same place both in this PR and in 13515 🤦

mattklein123

Thanks!

* master: (22 commits) http: using CONNECT_ERROR for HTTP/2 (envoyproxy#13519) listener: respect address.pipe.mode (it didn't work) (envoyproxy#13493) examples: Fix more deprecations/warnings in configs (envoyproxy#13529) overload: tcp connection refusal overload action (envoyproxy#13311) tcp: towards pluggable upstreams (envoyproxy#13331) conn_pool: fixing comments (envoyproxy#13520) Prevent SEGFAULT when disabling listener (envoyproxy#13515) Convert overload manager config literals to YAML (envoyproxy#13518) Fix runtime feature variable name (envoyproxy#13533) dependencies: refactor repository location schema utils, cleanups. (envoyproxy#13452) router: fix an invalid ASSERT when encoding metadata frames in the router. (envoyproxy#13511) http2: Proactively disconnect connections flooded when resetting stream (envoyproxy#13482) ci use azp to sync filter example (envoyproxy#13501) mongo_proxy: support configurable command list for metrics (envoyproxy#13494) http local rate limit: note token bucket is shared (envoyproxy#13525) wasm/extensions: Wasm extension policy. (envoyproxy#13526) http: removing envoy.reloadable_features.http1_flood_protection (envoyproxy#13508) build: update ppc64le CI build status shield (envoyproxy#13521) dependencies: enforce dependency shepherd sign-off via RepoKitteh. (envoyproxy#13522) Add no_traffic_healthy_interval (envoyproxy#13336) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

akonradi added 2 commits September 28, 2020 16:41

Add TcpListenerImpl::setRejectFraction method

b984e0c

This will allow creating an overload action that rejects (accepts the connection and then closes it) some fraction of connections in response to overload conditions. Signed-off-by: Alex Konradi <akonradi@google.com>

Add overload action for rejecting TCP connections

8eda768

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

akonradi marked this pull request as draft September 29, 2020 15:54

mattklein123 assigned alyssawilk Sep 30, 2020

alyssawilk reviewed Sep 30, 2020

View reviewed changes

akonradi added 2 commits September 30, 2020 17:55

Merge commit '52161ce8cec0512de4158c95fdc01c8e39f7938c' into HEAD

cf2edf7

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

Record reject fraction in connection handler

792fd41

Address feedback around adding new listeners when the reject fraction is already set, and add tests. Signed-off-by: Alex Konradi <akonradi@google.com>

alyssawilk reviewed Oct 1, 2020

View reviewed changes

akonradi added 3 commits October 9, 2020 10:47

Merge remote-tracking branch 'upstream/master' into scaled-connection…

fa931af

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

Use separate stats for connection rejection

dbba9e9

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

Add release notes and documentation

6e4525b

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

akonradi marked this pull request as ready for review October 9, 2020 21:12

Add missing period

fdcb8ce

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

alyssawilk previously approved these changes Oct 12, 2020

View reviewed changes

alyssawilk assigned mattklein123 Oct 12, 2020

repokitteh-read-only bot added the waiting label Oct 12, 2020

Relax ordering constraints for events

5b2f333

Relax the ordering constraints for events that occur asynchronously on different ends of the connection while maintaining requirements for relative ordering on each end. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed alyssawilk’s stale review via 5b2f333 October 12, 2020 19:17

repokitteh-read-only bot removed the waiting label Oct 12, 2020

mattklein123 requested changes Oct 12, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 12, 2020

Add ASSERT and stat tests

9cc57ce

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

repokitteh-read-only bot removed the waiting label Oct 13, 2020

Merge remote-tracking branch 'upstream/master' into scaled-connection…

b0abb08

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

repokitteh-read-only bot added the waiting label Oct 13, 2020

Merge remote-tracking branch 'upstream/master' into scaled-connection…

7457331

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

repokitteh-read-only bot removed the waiting label Oct 13, 2020

mattklein123 approved these changes Oct 13, 2020

View reviewed changes

mattklein123 merged commit 5fdf629 into envoyproxy:master Oct 13, 2020

akonradi deleted the scaled-connection-refusal branch October 13, 2020 19:57

Conversation

akonradi commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akonradi commented Sep 29, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akonradi commented Oct 9, 2020

Uh oh!

alyssawilk commented Oct 12, 2020

Uh oh!

akonradi commented Oct 12, 2020

Uh oh!

mattklein123 commented Oct 12, 2020

Uh oh!

akonradi commented Oct 12, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Oct 13, 2020

Uh oh!

akonradi commented Oct 13, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akonradi commented Sep 29, 2020 •

edited

Loading