Skip to content
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.

Sign up for GitHub

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

Jump to bottom

router: retry overloaded requests #10847

Merged
merged 10 commits into from
Apr 22, 2020
Merged

router: retry overloaded requests #10847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3801e21
retry overload requests
ramaraochavali Apr 19, 2020
a1bf01b
Merge remote-tracking branch 'upstream/master' into fix/retry_overload
ramaraochavali Apr 19, 2020
d22b56e
retry overload requests
ramaraochavali Apr 20, 2020
acf6885
format
ramaraochavali Apr 20, 2020
6bb3c6a
fix version history and correct docs
ramaraochavali Apr 21, 2020
e648a0d
Merge remote-tracking branch 'upstream/master' into fix/retry_overload
ramaraochavali Apr 21, 2020
69fc2da
resolve conflicts
ramaraochavali Apr 21, 2020
899d4a6
fix docs
ramaraochavali Apr 21, 2020
556ae19
kick ci
ramaraochavali Apr 21, 2020
998f8ed
review comments
ramaraochavali Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions docs/root/configuration/http/http_filters/router_filter.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

^^^^^^^^^^^^^^^^^^

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
Expand Down
6 changes: 4 additions & 2 deletions docs/root/intro/arch_overview/http/http_routing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Changes
Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false.
* network filters: added a :ref:`postgres proxy filter <config_network_filters_postgres_proxy>`.
* 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>`.

Expand Down
1 change: 0 additions & 1 deletion include/envoy/http/header_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
4 changes: 0 additions & 4 deletions source/common/router/retry_state_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,6 @@ RetryStatus RetryStateImpl::shouldHedgeRetryPerTryTimeout(DoRetryCallback callba
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return false;
}

// We never retry if the request is rate limited.
if (response_headers.EnvoyRateLimited() != nullptr) {
return false;
Expand Down
6 changes: 4 additions & 2 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,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);
Expand Down Expand Up @@ -999,7 +1000,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);
},
Expand Down
3 changes: 2 additions & 1 deletion test/common/router/retry_state_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down