router: retry overloaded requests #10847

ramaraochavali · 2020-04-20T05:11:44Z

Description: Retries overloaded requests
Risk Level: Low
Testing: Added
Docs Changes: Updated
Release Notes: Added

Fixes #10706

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2020-04-20T07:14:08Z

@snowp @mattklein123 PTAL

mattklein123 · 2020-04-20T16:36:47Z

@tonya11en can you take a look at the linked issue and do a first pass over this? I want to make sure we are all in agreement here on the best overall approach.

tonya11en

I think this is a good change and should occur, but we should be aware that this will almost certainly result in a noticeable increase in retry overflows. Retry budgets were implemented to mitigate this problem, but it still might raise an eyebrow for anyone monitoring their systems during CB load shedding events.

Just a few small comments around wording of the release note.

tonya11en · 2020-04-20T20:03:35Z

docs/root/intro/arch_overview/http/http_routing.rst

-Note that retries may be disabled depending on the contents of the :ref:`x-envoy-overloaded
-<config_http_filters_router_x-envoy-overloaded_consumed>`.
+Note that Envoy retries requests when :ref:`x-envoy-overloaded
+<config_http_filters_router_x-envoy-overloaded_set>` is present. It is recommended to configure


This is already set to a default value of 3, so folks should already be protected. I also would mention retry budgets here instead of the max_retries CB threshold. Especially since the max_retries circuit breaker already is enabled by default to a low value. This change will certainly result in a dramatic increase in retry overflows when circuit breaking occurs.

As an aside, I really wonder if we should at some point turn on the budgets by default vs. the hard limit. It's a much better default?

@tonya11en I will adjust the doc.

@mattklein123 Agreed. Retry budgets are better defaults than hard limits. We should consider doing that

tonya11en · 2020-04-20T20:07:55Z

docs/root/version_history/current.rst

@@ -12,6 +12,7 @@ Changes
  Disabled by default and can be enabled via :ref:`enable_upstream_stats <envoy_v3_api_field_extensions.filters.http.grpc_stats.v3.FilterConfig.enable_upstream_stats>`.
 * http: fixed a bug where the upgrade header was not cleared on responses to non-upgrade requests.
  Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false.
+* router: added support for retrying requests when upstream responds with :ref:`x-envoy-overloaded <config_http_filters_router_x-envoy-overloaded_set>`.


I would reword this to be more clear about what this change actually does. Maybe say something like:

router: allow retries by default when encountering x-envoy-overloaded header

As it reads now, it can imply that the old behavior is still intact.

might be good to explicitly mention circuit breakers, readers might not be aware of this header

I updated the doc and mentioned about using circuit breaker as a recommendation in the retry section. Do you think we should add it in the Version History as well?

mattklein123

Few drive by comments on top of Tony's. Thank you!

/wait

mattklein123 · 2020-04-21T00:15:16Z

docs/root/intro/arch_overview/http/http_routing.rst

-Note that retries may be disabled depending on the contents of the :ref:`x-envoy-overloaded
-<config_http_filters_router_x-envoy-overloaded_consumed>`.
+Note that Envoy retries requests when :ref:`x-envoy-overloaded
+<config_http_filters_router_x-envoy-overloaded_set>` is present. It is recommended to configure


As an aside, I really wonder if we should at some point turn on the budgets by default vs. the hard limit. It's a much better default?

mattklein123 · 2020-04-21T00:16:01Z

docs/root/configuration/http/http_filters/router_filter.rst

@@ -274,14 +274,6 @@ for the next health check interval. The host can become healthy again via standa
 checks. See the :ref:`health checking overview <arch_overview_health_checking>` for more
 information.

-.. _config_http_filters_router_x-envoy-overloaded_consumed:
-
-x-envoy-overloaded


Is this header documented anywhere else? I think we still should document it. I think it does still make sense to send it as downstream may want to track it, but we should document somewhere?

I checked earlier- it's explained further down in the same file.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2020-04-21T08:15:42Z

/retest

repokitteh-read-only · 2020-04-21T08:15:45Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #10847 (comment) was created by @ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2020-04-21T10:09:19Z

/retest

repokitteh-read-only · 2020-04-21T10:09:23Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10847 (comment) was created by @ramaraochavali.

see: more, trace.

mattklein123

LGTM with small comments, thanks!

/wait

mattklein123 · 2020-04-21T20:34:39Z

docs/root/intro/arch_overview/http/http_routing.rst

+Note that Envoy retries requests when :ref:`x-envoy-overloaded
+<config_http_filters_router_x-envoy-overloaded_set>` is present. It is recommended to either configure
+:ref:`retry budgets (preferred) <envoy_api_field_cluster.CircuitBreakers.Thresholds.retry_budget>` or set
+:ref:`maximum active retries circuit breaker <arch_overview_circuit_break>` to an appropriate value avoid retry storms.


Suggested change

:ref:`maximum active retries circuit breaker <arch_overview_circuit_break>` to an appropriate value avoid retry storms.

:ref:`maximum active retries circuit breaker <arch_overview_circuit_break>` to an appropriate value to avoid retry storms.

mattklein123 · 2020-04-21T20:35:16Z

source/common/router/retry_state_impl.cc

@@ -281,10 +281,6 @@ RetryStatus RetryStateImpl::shouldHedgeRetryPerTryTimeout(DoRetryCallback callba
 }

 bool RetryStateImpl::wouldRetryFromHeaders(const Http::ResponseHeaderMap& response_headers) {
-  if (response_headers.EnvoyOverloaded() != nullptr) {


Can you remove this from the O(1) header map? I think we don't need this anymore as the header can be added by string reference where it is added currently?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123

Thanks!

Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: pengg <pengg@google.com>

Signed-off-by: Spencer Lewis <slewis@squareup.com> * master: (46 commits) allow specifying the API version of bootstrap from the command line (envoyproxy#10803) config: adding connect matcher (unused) (envoyproxy#10894) Add missing dependency on `assert.h` (envoyproxy#10918) Lower heap and disk space used by kafka tests (envoyproxy#10915) [tools] handle commits merged without PR in deprecated script (envoyproxy#10723) tools: including working tree in modified_since_last_github_commit.sh diff. (envoyproxy#10911) rocketmq_proxy: implement rocketmq proxy [docs] PR template to include commit message (envoyproxy#10900) docs: breaking long word to stop content overflow. (envoyproxy#10880) Delete legacy connection pool code. (envoyproxy#10881) wasm: clarify how configuration is passed (envoyproxy#10782) issue template: clarify security/crash reporting (envoyproxy#10885) api/faq: add entry on incremental xDS. (envoyproxy#10876) router: retry overloaded requests (envoyproxy#10847) Remove inclusion of pthread.h, not needed for linux compilation (envoyproxy#10895) request_id: Add option to always set request id in response (envoyproxy#10808) xray: Use correct types for segment document output (envoyproxy#10834) router: fixing a watermark bug for streaming retries (envoyproxy#10866) http: auditing Path() calls for safety with Pathless CONNECT (envoyproxy#10851) Remove hardcoded type urls Part.2 (envoyproxy#10848) ...

ramaraochavali added 4 commits April 19, 2020 09:13

retry overload requests

3801e21

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge remote-tracking branch 'upstream/master' into fix/retry_overload

a1bf01b

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

retry overload requests

d22b56e

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

format

acf6885

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 assigned mattklein123 and tonya11en Apr 20, 2020

mattklein123 assigned snowp Apr 20, 2020

tonya11en suggested changes Apr 20, 2020

View reviewed changes

mattklein123 reviewed Apr 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 21, 2020

fix version history and correct docs

6bb3c6a

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Apr 21, 2020

ramaraochavali added 3 commits April 21, 2020 11:37

Merge remote-tracking branch 'upstream/master' into fix/retry_overload

e648a0d

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

resolve conflicts

69fc2da

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix docs

899d4a6

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

kick ci

556ae19

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

tonya11en previously approved these changes Apr 21, 2020

View reviewed changes

mattklein123 requested changes Apr 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 21, 2020

review comments

998f8ed

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali dismissed tonya11en’s stale review via 998f8ed April 22, 2020 05:26

repokitteh-read-only bot removed the waiting label Apr 22, 2020

mattklein123 approved these changes Apr 22, 2020

View reviewed changes

mattklein123 merged commit ae0a45a into envoyproxy:master Apr 22, 2020

penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020

router: retry overloaded requests (envoyproxy#10847)

bac607e

Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: pengg <pengg@google.com>

ramaraochavali deleted the fix/retry_overload branch April 23, 2020 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router: retry overloaded requests #10847

router: retry overloaded requests #10847

ramaraochavali commented Apr 20, 2020

ramaraochavali commented Apr 20, 2020

mattklein123 commented Apr 20, 2020

tonya11en left a comment

tonya11en Apr 20, 2020

mattklein123 Apr 21, 2020

ramaraochavali Apr 21, 2020

tonya11en Apr 20, 2020

snowp Apr 21, 2020

ramaraochavali Apr 21, 2020

mattklein123 left a comment

mattklein123 Apr 21, 2020

mattklein123 Apr 21, 2020

tonya11en Apr 21, 2020

ramaraochavali commented Apr 21, 2020

repokitteh-read-only bot commented Apr 21, 2020

ramaraochavali commented Apr 21, 2020

repokitteh-read-only bot commented Apr 21, 2020

mattklein123 left a comment

mattklein123 Apr 21, 2020

mattklein123 Apr 21, 2020

mattklein123 left a comment

	:ref:`maximum active retries circuit breaker <arch_overview_circuit_break>` to an appropriate value avoid retry storms.
	:ref:`maximum active retries circuit breaker <arch_overview_circuit_break>` to an appropriate value to avoid retry storms.

router: retry overloaded requests #10847

router: retry overloaded requests #10847

Conversation

ramaraochavali commented Apr 20, 2020

ramaraochavali commented Apr 20, 2020

mattklein123 commented Apr 20, 2020

tonya11en 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

mattklein123 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

ramaraochavali commented Apr 21, 2020

repokitteh-read-only bot commented Apr 21, 2020

ramaraochavali commented Apr 21, 2020

repokitteh-read-only bot commented Apr 21, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment