router: retry overloaded requests (#10847)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
envoyproxy · Apr 22, 2020 · ae0a45a · ae0a45a
1 parent 25ccc5f
commit ae0a45a
Show file tree

Hide file tree

Showing 7 changed files with 11 additions and 18 deletions.
diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/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
-^^^^^^^^^^^^^^^^^^
-
-If this header is set by upstream, Envoy will not retry. Currently the value of the header is not
-looked at, only its presence.
-
 .. _config_http_filters_router_x-envoy-ratelimited:
 
 x-envoy-ratelimited

diff --git a/docs/root/intro/arch_overview/http/http_routing.rst b/docs/root/intro/arch_overview/http/http_routing.rst
@@ -122,8 +122,10 @@ headers <config_http_filters_router_headers_consumed>`. The following configurat
   :ref:`retry priority <envoy_api_field_route.RetryPolicy.retry_priority>` can be
   configured to adjust the priority load used when selecting a priority for retries.
 
-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 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 to avoid retry storms.
 
 .. _arch_overview_http_routing_hedging:
 

diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -21,6 +21,7 @@ Changes
   to set :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header in response even if
   tracing is not forced.
 * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`.
+* router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded <config_http_filters_router_x-envoy-overloaded_set>`.
 * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager
   can now have a separate :ref:`tracing provider <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.Tracing.provider>`.
 * upstream: fixed a bug where Envoy would panic when receiving a GRPC SERVICE_UNKNOWN status on the health check.

diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h
@@ -325,7 +325,6 @@ class HeaderEntry {
   HEADER_FUNC(Etag)                                                                                \
   HEADER_FUNC(EnvoyDegraded)                                                                       \
   HEADER_FUNC(EnvoyImmediateHealthCheckFail)                                                       \
-  HEADER_FUNC(EnvoyOverloaded)                                                                     \
   HEADER_FUNC(EnvoyRateLimited)                                                                    \
   HEADER_FUNC(EnvoyUpstreamCanary)                                                                 \
   HEADER_FUNC(EnvoyUpstreamHealthCheckedCluster)                                                   \

diff --git a/source/common/router/retry_state_impl.cc b/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) {
-    return false;
-  }
-
   // We never retry if the request is rate limited.
   if (response_headers.EnvoyRateLimited() != nullptr) {
     return false;

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -516,7 +516,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
         Http::Code::ServiceUnavailable, "maintenance mode",
         [modify_headers, this](Http::ResponseHeaderMap& headers) {
           if (!config_.suppress_envoy_headers_) {
-            headers.setReferenceEnvoyOverloaded(Http::Headers::get().EnvoyOverloadedValues.True);
+            headers.addReference(Http::Headers::get().EnvoyOverloaded,
+                                 Http::Headers::get().EnvoyOverloadedValues.True);
           }
           // Note: append_cluster_info does not respect suppress_envoy_headers.
           modify_headers(headers);
@@ -1007,7 +1008,8 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_
         code, body,
         [dropped, this](Http::ResponseHeaderMap& headers) {
           if (dropped && !config_.suppress_envoy_headers_) {
-            headers.setReferenceEnvoyOverloaded(Http::Headers::get().EnvoyOverloadedValues.True);
+            headers.addReference(Http::Headers::get().EnvoyOverloaded,
+                                 Http::Headers::get().EnvoyOverloadedValues.True);
           }
           modify_headers_(headers);
         },

diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc
@@ -203,7 +203,8 @@ TEST_F(RouterRetryStateImplTest, Policy5xxRemote503Overloaded) {
 
   Http::TestResponseHeaderMapImpl response_headers{{":status", "503"},
                                                    {"x-envoy-overloaded", "true"}};
-  EXPECT_EQ(RetryStatus::No, state_->shouldRetryHeaders(response_headers, callback_));
+  expectTimerCreateAndEnable();
+  EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers, callback_));
 }
 
 TEST_F(RouterRetryStateImplTest, PolicyResourceExhaustedRemoteRateLimited) {