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 4 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
5 changes: 3 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,9 @@ 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 configure
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonya11en I will adjust the doc.

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

:ref:`maximum active retries circuit breaker <arch_overview_circuit_break>` 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 @@ -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>`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

* 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
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
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