gRPC retries, assuming gRPC status code is in the returned headers by alyssawilk · Pull Request #1067 · envoyproxy/envoy

alyssawilk · 2017-06-08T19:55:43Z

for issue #721

rshriram

This is cool! Could you add the retry policies to docs as well?

alyssawilk · 2017-06-08T20:25:57Z

Oops. Good call! router_filter.rst looks like the obvious place to add to, but if there's another one I'm missing please let me know

…

On Thu, Jun 8, 2017 at 4:04 PM, Shriram Rajagopalan < ***@***.***> wrote: ***@***.**** commented on this pull request. This is cool! Could you add the retry policies to docs as well? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1067 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFvQ0gd4pTUFeWQNaN1ZRxXAWIH4dBks5sCFPCgaJpZM4N0i_9> .

htuch · 2017-06-08T20:54:56Z

source/common/grpc/common.h


 class Common {
 public:
+  enum GrpcStatus {


It would be helpful if this could be moved to include/envoy/grpc/status.h, since I would like to use this in interface files for the bidi client I'm working on.

gRPC status code are identical with protobuf status code, so just re-use it might be enough.

+1 to what @htuch said. I would like to re-use those for fault injection as well. Do we also have HTTP2 specific error codes in a similar place (in addition to HTTP1)? All I see are HTTP1 codes in include/envoy/http/codes.h

I'm generally a fan of decoupling protobuf from gRPC. As we know from the other stuff going on with grpc-web, there's not necessarily a direct correspondence between gRPC and protobuf.

lizan · 2017-06-08T21:12:29Z

include/envoy/router/router.h

+  static const uint32_t RETRY_ON_CONNECT_FAILURE         = 0x2;
+  static const uint32_t RETRY_ON_RETRIABLE_4XX           = 0x4;
+  static const uint32_t RETRY_ON_REFUSED_STREAM          = 0x8;
+  static const uint32_t RETRY_ON_GRPC_CANCELLED          = 0x16;


0x10, 0x20, 0x40

facepalm Done :-P

Don't see this in the updated diff yet.

lizan · 2017-06-08T21:15:38Z

source/common/grpc/common.h


 class Common {
 public:
+  enum GrpcStatus {


gRPC status code are identical with protobuf status code, so just re-use it might be enough.

lizan · 2017-06-08T21:20:08Z

docs/configuration/http_filters/router_filter.rst

+only supported for gRPC status codes in response headers.  gRPC status codes in trailers will not trigger
+retry logic.One or more policies can be specified  using a ',' delimited list. The supported policies are:
+
+cancelled


Cancelled are returned typically when the call are cancelled by the caller, not sure a proxy should retry.

http://www.grpc.io/docs/guides/concepts.html#cancelling-rpcs

Would that be worth taking up with @twoism over in #721 ? If you two work out a good set I'm happy to update the patch to whatever you agree on.

mattklein123 · 2017-06-08T22:27:10Z

source/common/router/retry_state_impl.cc


  // We short circuit here and do not both with an allocation if there is no chance we will retry.
-  if (request_headers.EnvoyRetryOn() || route_policy.retryOn()) {
+  if (request_headers.EnvoyRetryOn() || request_headers.EnvoyRetryGrpcOn() || route_policy.retryOn()) {


quick comment (not full review), can we also add inline route support as is done in route_policy.retryOn() ?

Rather than extending this if block with more conditions (gRPC, HTTP2 error codes, etc.), would it make sense to abstract them under EnvoyRetryOn() ?

I think request_headers.EnvoyRetryOn() returning true if there were a literal header x-envoy-grpc-retry-on would be confusing as all generally header functions map cleanly to the header specified. Most other code points could be covered under the general "retry" header if folks think that's cleaner.

Working on route policy now. I'm inclined to add the gRPC strings to the existing retry_on field rather than have retry_on and grpc_retry_on (and num_retries and num_grpc_retries) in router config. If anyone has concerns about HTTP vs gRPC collisions and prefers splitting them out, let me know!

htuch

LGTM

htuch · 2017-06-09T14:22:32Z

docs/configuration/http_filters/router_filter.rst

+Setting this header on egress requests will cause Envoy to attempt to retry failed requests (number of
+retries defaults to 1, and is configured as x-envoy-grpc-retry is, above).  gRPC retries are currently
+only supported for gRPC status codes in response headers.  gRPC status codes in trailers will not trigger
+retry logic.One or more policies can be specified  using a ',' delimited list. The supported policies are:


Nit: logic. One.

htuch · 2017-06-09T14:23:17Z

include/envoy/router/router.h

+  static const uint32_t RETRY_ON_CONNECT_FAILURE         = 0x2;
+  static const uint32_t RETRY_ON_RETRIABLE_4XX           = 0x4;
+  static const uint32_t RETRY_ON_REFUSED_STREAM          = 0x8;
+  static const uint32_t RETRY_ON_GRPC_CANCELLED          = 0x16;


Don't see this in the updated diff yet.

htuch · 2017-06-09T14:36:33Z

test/common/router/router_test.cc

+  HttpTestUtility::addDefaultHeaders(headers);
+  router_.decodeHeaders(headers, true);
+
+  // 5xx response.


htuch · 2017-06-09T14:38:06Z

test/integration/integration.cc

+         Http::StreamEncoder* request_encoder;
+         request_encoder =
+             &codec_client->startRequest(Http::TestHeaderMapImpl{{":method", "POST"},
+                                                                 {":path", "/test/long/url"},


Nit: gRPC looks more like /service/method

htuch · 2017-06-09T14:39:39Z

test/integration/integration.cc

+                                                                 {"x-envoy-retry-grpc-on", "cancelled"}},
+                                         *response);
+         codec_client->sendData(*request_encoder, 1024, false);
+         codec_client->sendTrailers(*request_encoder, request_trailers);


Nit: gRPC requests don't have trailers.

htuch · 2017-06-09T14:44:49Z

test/integration/integration.cc

+         request->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {"grpc-status", "1"}}, false);
+       },
+       [&]() -> void {
+         if (fake_upstreams_[0]->httpType() == FakeHttpConnection::Type::HTTP1) {


Should we just exclude this from HTTP1 upstream tests? In general you can't proxy gRPC to arbitrary H1 backends I think (certainly not bidi streaming, also you need to get grpc-status into trailers, etc.)

I thought it was worth testing because it is possible to translate gRPC to HTTP/1.1, and the retry logic doesn't check the connection type before checking for gRPC status codes. Basically if the code might be exercised for HTTP responses which include gRPC status codes for some bizarre reason, might as well test it.

…further cleanup

mattklein123 · 2017-06-09T15:31:11Z

@alyssawilk before I forget please sign CLA: https://oss.lyft.com/cla/#/signature (CLA bot has been resourced!)

htuch · 2017-06-09T17:12:14Z

source/common/grpc/common.cc


 const std::string Common::GRPC_CONTENT_TYPE{"application/grpc"};

+Status::GrpcStatus Common::getGrpcStatus(const Http::HeaderMap& trailers) {


Could have Common::validateTrailers() make use of this to do its gRPC status parsing.

Sure. It's a minor behavior change for out-of-bounds error codes. If we care we could split returns from getGrpcStatus into "missing" status and "out of bounds" status but either way we'll throw an exception so I assume it's moot.

…further cleanup

lizan · 2017-06-09T19:48:25Z

include/envoy/grpc/status.h

+
+    // This is a non-GRPC error code, indicating the status code in gRPC headers
+    // was either invalid or missing.
+    InvalidCode = 16,


If you decided copy this but not reusing protobuf (which is fine), let's not use 16 here, maybe -1?

https://github.com/grpc/grpc/blob/master/include/grpc%2B%2B/impl/codegen/status_code_enum.h

…HeaderOnlyError

lizan · 2017-06-10T00:46:31Z

source/common/grpc/common.cc

    return Status::GrpcStatus::InvalidCode;
  }
-  if (grpc_status_code > Status::GrpcStatus::InvalidCode) {
+  if (grpc_status_code > Status::GrpcStatus::DataLoss) {


Unauthenticated = 16 is the largest one

htuch

LGTM, some minor nits and ready to ship.

htuch · 2017-06-12T13:39:28Z

include/envoy/grpc/BUILD

 )
+
+envoy_cc_library(
+    name = "status_interface",


Nit: Could just call this "status", since it's not as much an interface as an independent interface-level library (we do something similar for optional).

htuch · 2017-06-12T13:39:59Z

include/envoy/grpc/status.h

+    // Permission was denied for the RPC.
+    PermissionDenied = 7,
+    // The RPC does not have required credentials for the RPC to succeed.
+    Unauthenticated = 16,


Can you reorder this so that this is after DataLoss?

htuch · 2017-06-12T13:43:16Z

source/common/grpc/common.cc

+
+  uint64_t grpc_status_code;
+  if (!grpc_status_header || grpc_status_header->value().empty()) {
+    return Status::GrpcStatus::MissingStatus;


I'm inclined to make this an optional return type, since having a GrpcStatus that indicates status missing is somewhat self contradictory.

htuch · 2017-06-12T14:47:13Z

source/common/grpc/common.cc

+    return Optional<Status::GrpcStatus>();
+  }
+  if (!StringUtil::atoul(grpc_status_header->value().c_str(), grpc_status_code) ||
+      grpc_status_code > Status::GrpcStatus::DataLoss) {


This still needs updating.

mattklein123

looks great. few small question/comments.

mattklein123 · 2017-06-12T15:00:19Z

docs/configuration/http_filters/router_filter.rst

+x-envoy-grpc-retry-on
+^^^^^^^^^^^^^^^^^^^^^
+Setting this header on egress requests will cause Envoy to attempt to retry failed requests (number of
+retries defaults to 1, and is configured as x-envoy-grpc-retry is, above).  gRPC retries are currently


"x-envoy-grpc-retry" I think is a typo? (Not sure what you meant here)

mattklein123 · 2017-06-12T15:03:49Z

docs/configuration/http_filters/router_filter.rst

+resource-exhausted
+  Envoy will attempt a retry if the gRPC status code in the response headers is "resource-exhausted" (8)
+
+As with x-envoy-grpc-retry, the number of retries can be controlled via the


mattklein123 · 2017-06-12T15:07:05Z

docs/configuration/http_conn_man/route_config/route.rst

 retry_on
  *(required, string)* specifies the conditions under which retry takes place. These are the same
-  conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on`.
+  conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on` and


It seems a bit odd to me that in the route, we stick HTTP/gRPC retry policies into a single field, but we have 2 different headers. I don't really feel strongly about this, but thought I would mention it in case anyone else has any strong opinions.

htuch · 2017-06-12T19:30:34Z

Hopefully we can get a merge on this today if there isn't anything major left, my PR (#1057) needs this before I can move it from non-WIP status for final review.

mattklein123 · 2017-06-12T19:34:15Z

LGTM, I still think we might want to think about how things are configured between headers and routes but we can change this in a follow up before 1.4.0 is released if we want.

…#1067) Description: scope destruction should happen ahead of destroying main_common_. Additionally while cross thread destruction is acceptable, this PR also moves destruction to happen in the context of the main thread. Risk Level: med - resolve crash Testing: repro in alpha builds of the lyft app. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** Garbage was checked in accidentally via a doc fix PR #1030, so this commit removes it Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

gRPC retries, assuming status code in returned headers

66f18e9

rshriram reviewed Jun 8, 2017

View reviewed changes

gRPC retries, now with (partial) documentation

9a3f9a6

htuch reviewed Jun 8, 2017

View reviewed changes

lizan reviewed Jun 8, 2017

View reviewed changes

mattklein123 reviewed Jun 8, 2017

View reviewed changes

minor changes

11281eb

htuch reviewed Jun 9, 2017

View reviewed changes

alyssawilk added 5 commits June 9, 2017 11:11

allowing gRPC retries to be configured using current router configs, …

05c6dba

…further cleanup

gRPC retries, assuming status code in returned headers

8fb9bbe

gRPC retries, now with (partial) documentation

e46dbc1

minor changes

7ed0e35

allowing gRPC retries to be configured using current router configs, …

2775929

…further cleanup

htuch reviewed Jun 9, 2017

View reviewed changes

alyssawilk added 14 commits June 9, 2017 13:31

doc fix and clang_cleanup

9fec1d8

clang_cleanup

6f613d8

clang_cleanup

872fad0

Merge branch 'grpc' of github.com:alyssawilk/envoy into grpc

a5aac27

gRPC retries, assuming status code in returned headers

0130379

gRPC retries, now with (partial) documentation

a4e13ff

minor changes

ab6c25a

allowing gRPC retries to be configured using current router configs, …

bc4f8de

…further cleanup

gRPC retries, assuming status code in returned headers

65566c9

gRPC retries, now with (partial) documentation

e5e7f53

minor changes

39c9606

allowing gRPC retries to be configured using current router configs, …

f0cfd1c

…further cleanup

doc fix and clang_cleanup

b0643f2

trying to come up with clean diffs

d8a7a28

alyssawilk force-pushed the grpc branch from d8a7a28 to b0643f2 Compare June 9, 2017 19:08

reusing getGrpcStatus in trailer validation

3d33288

htuch mentioned this pull request Jun 9, 2017

gRPC streaming client. #1054

Merged

lizan reviewed Jun 9, 2017

View reviewed changes

Making gRPC status errors negative, reusing getGrpcStatus in checkFor…

bbba9a8

…HeaderOnlyError

lizan reviewed Jun 10, 2017

View reviewed changes

Merge remote-tracking branch 'refs/remotes/origin/grpc' into grpc

3dec86d

htuch reviewed Jun 12, 2017

View reviewed changes

alyssawilk added 2 commits June 12, 2017 10:35

Merge remote-tracking branch 'upstream/master' into grpc

1dcc41e

Making getGrpcStatus optional

36aae46

htuch reviewed Jun 12, 2017

View reviewed changes

allowing all gRPC codes

5df0a7e

htuch previously approved these changes Jun 12, 2017

View reviewed changes

mattklein123 reviewed Jun 12, 2017

View reviewed changes

hopefully clarifying docs

6692aef

alyssawilk dismissed htuch’s stale review via 6692aef June 12, 2017 15:46

mattklein123 approved these changes Jun 12, 2017

View reviewed changes

htuch merged commit 7828e38 into envoyproxy:master Jun 12, 2017

alyssawilk deleted the grpc branch June 15, 2017 19:39

mathetake added a commit that referenced this pull request Mar 3, 2026

chore: removes garbage checked in (#1067)

fb1f875

**Description** Garbage was checked in accidentally via a doc fix PR #1030, so this commit removes it Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>


		const std::string Common::GRPC_CONTENT_TYPE{"application/grpc"};

		Status::GrpcStatus Common::getGrpcStatus(const Http::HeaderMap& trailers) {

Conversation

alyssawilk commented Jun 8, 2017

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 8, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

htuch 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!

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 Jun 9, 2017

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!

htuch 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!

rshriram Jun 9, 2017 •

edited

Loading

alyssawilk Jun 9, 2017 •

edited

Loading

mattklein123 Jun 12, 2017 •

edited

Loading