A31: xDS Timeout Support and Config Selector Design #192

dfawley · 2020-07-13T21:16:16Z

This proposal includes details for implementing support for xDS RouteActions
which configure RPC timeouts, retries, etc.

This proposal includes details for implementing support for xDS RouteActions which configure RPC timeouts, retries, etc.

markdroth

Thanks for writing this up, Doug!

Please let me know if you have any questions about any of this.

A31-xds-routeaction-support.md

dfawley

Thanks for the review! Comments addressed inline.

A31-xds-routeaction-support.md

markdroth

This looks really good! The only remaining significant issue is the one about improving the diagram.

A31-xds-routeaction-support.md

dfawley

All comments addressed. PTAL

A31-xds-routeaction-support.md

markdroth

This looks great! Just a couple of minor issues remaining.

A31-xds-routeaction-support.md

markdroth

Text looks great! Please add the SVG file before merging. Thanks!

markdroth

Actually, wait -- I just remembered that we still want to document the timeout behavior differences from Envoy w.r.t. when the timeout starts counting.

ejona86

This looks good to me, except for the discussion about timeouts that needs to be resolved. I don't believe there would be any need for me to re-review after that is resolved, so approving.

A31-xds-routeaction-support.md

dfawley · 2020-09-16T23:46:25Z

@markdroth @ejona86 Updated to account for xDS API changes; PTAL & thanks!

markdroth

This looks great! Just a few minor comments.

A31-xds-routeaction-support.md

dfawley

Apparently my last round of comments (3) did not send yesterday. So this update contains both sets; sorry for the confusion.

ALSO: I have updated the title of the gRFC as I believe we agreed upon previously. I'll rename the file (and PR title) to match before submitting, but I don't want to do that yet for the sake of the diffs and your email threading.

A31-xds-routeaction-support.md

markdroth

Just a few more minor comments.

markdroth · 2020-09-17T17:30:51Z

A31-xds-routeaction-support.md

-[`max_stream_duration.grpc_timeout_offset`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L789).
+Only some xDS timeout-related settings will be supported:
+
+- [`RouteActions.max_stream_duration.max_stream_duration`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L775)


s/RouteActions/RouteAction/

Same thing throughout.

markdroth · 2020-09-17T17:34:13Z

A31-xds-routeaction-support.md

-[`timeout`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L952)
-and
-[`max_stream_duration.grpc_timeout_offset`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L789).
+Only some xDS timeout-related settings will be supported:


It might be a good idea to start this section by explaining that Envoy has two separate and indepenent timeouts, one that starts from the client's half-close (controlled by the timeout field) and another that starts from the start of the stream (controlled by the max_stream_duration field). The latter matches gRPC timeout semantics, so that's the one we're supporting.

markdroth · 2020-09-17T17:36:21Z

A31-xds-routeaction-support.md

+- [`RouteActions.max_stream_duration.grpc_max_timeout`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L781)
+- [`max_stream_duration`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/core/v3/protocol.proto#L95) from the [`HttpConnectionManager.common_http_options`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L289) as a default if `RouteActions.max_stream_duration.max_stream_duration` is unset.
+
+Notably, gRPC will not support


How about making this a bulleted list, just like the one above where you list the fields that we do support? That way, we can discuss each of these fields individually.

markdroth · 2020-09-17T17:38:36Z

A31-xds-routeaction-support.md

+If `RouteActions.max_stream_duration.grpc_max_timeout` is present, then
+`RouteActions.max_stream_duration.max_stream_duration` is ignored and
+`RouteActions.max_stream_duration.grpc_max_timeout` limits the maximum timeout
+for RPCs on this route instead.  A value of 0 indicates the application's


I guess this means that setting either max_stream_duration to 0 or grpc_max_timeout to 0 will effectively not specify any default timeout.

Should we include rows in the table below for max_stream_duration set to 0?

Missed this somehow.

I'm actually not sure how max_stream_duration = 0 would/should behave. Theoretically it should timeout instantly since there is no need for "0" to represent "infinite" AFAICT.

Follow-up: looks like "0" disables the timer.

Ah, you're right -- I was thinking it was an int, so unset was the same as 0. But the field is actually a google.protobuf.Duration, so I think you're right.

Oh, looks like you changed it to say that 0 is unlimited. Please check what Envoy does -- we should be consistent with it here.

dfawley

All comments addressed, thanks!

dfawley · 2020-09-17T21:44:27Z

A31-xds-routeaction-support.md

-[`max_stream_duration.grpc_timeout_offset`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L789).
+Only some xDS timeout-related settings will be supported:
+
+- [`RouteActions.max_stream_duration.max_stream_duration`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L775)


dfawley · 2020-09-17T21:44:34Z

A31-xds-routeaction-support.md

+- [`RouteActions.max_stream_duration.grpc_max_timeout`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L781)
+- [`max_stream_duration`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/core/v3/protocol.proto#L95) from the [`HttpConnectionManager.common_http_options`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L289) as a default if `RouteActions.max_stream_duration.max_stream_duration` is unset.
+
+Notably, gRPC will not support


dfawley · 2020-09-17T21:48:43Z

A31-xds-routeaction-support.md

-[`timeout`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L952)
-and
-[`max_stream_duration.grpc_timeout_offset`](https://github.com/envoyproxy/envoy/blob/6f2ad057f8655e2688a195337f7520dbd77015fa/api/envoy/config/route/v3/route_components.proto#L789).
+Only some xDS timeout-related settings will be supported:


markdroth

This looks great!

dfawley · 2020-09-18T15:40:53Z

Thanks for the review!

A31: xDS RouteActions support

a7a632b

This proposal includes details for implementing support for xDS RouteActions which configure RPC timeouts, retries, etc.

dfawley requested review from ejona86 and markdroth July 13, 2020 21:16

dfawley assigned ejona86 and markdroth Jul 13, 2020

add discussion thread link

b2fe83a

markdroth reviewed Jul 13, 2020

View reviewed changes

review comments

3ae918c

dfawley commented Jul 14, 2020

View reviewed changes

markdroth reviewed Jul 14, 2020

View reviewed changes

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

dfawley added 2 commits July 14, 2020 16:00

clarify supported fields section

273096c

replace code tags with backticks

cf84dfc

markdroth reviewed Jul 15, 2020

View reviewed changes

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

markdroth mentioned this pull request Jul 20, 2020

A32: gRPC xDS circuit breaking #200

Merged

lidizheng reviewed Jul 21, 2020

View reviewed changes

A31-xds-routeaction-support.md Show resolved Hide resolved

update graphic; address review comments

9de6a65

dfawley commented Jul 29, 2020

View reviewed changes

A31-xds-routeaction-support.md Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

voidzcy reviewed Jul 29, 2020

View reviewed changes

A31-xds-routeaction-support.md Show resolved Hide resolved

markdroth reviewed Jul 29, 2020

View reviewed changes

A31-xds-routeaction-support.md Show resolved Hide resolved

A31-xds-routeaction-support.md Show resolved Hide resolved

add note about timeout=0 indicating an infinte deadline

f638f1d

markdroth approved these changes Jul 30, 2020

View reviewed changes

markdroth requested changes Jul 30, 2020

View reviewed changes

add svg; add note about timeout behavior difference

01e8e58

ejona86 approved these changes Jul 30, 2020

View reviewed changes

A31-xds-routeaction-support.md Show resolved Hide resolved

dfawley added 2 commits September 16, 2020 16:39

update for recent xds API changes

faf14d8

add parameters for Config Selector function

a1ec45f

markdroth reviewed Sep 17, 2020

View reviewed changes

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

A31-xds-routeaction-support.md Outdated Show resolved Hide resolved

final touches

3617c2a

dfawley commented Sep 17, 2020

View reviewed changes

markdroth reviewed Sep 17, 2020

View reviewed changes

RouteAction(s), bullets, and intros. Oh my!

135a4f3

dfawley commented Sep 17, 2020

View reviewed changes

markdroth approved these changes Sep 17, 2020

View reviewed changes

dfawley added 2 commits September 17, 2020 15:02

add examples where max_stream_duration=0

1f311e7

rename A31 file

d674155

dfawley changed the title ~~A31: xDS RouteActions Support~~ A31: xDS Timeout Support and Config Selector Design Sep 18, 2020

dfawley merged commit 212dd81 into grpc:master Sep 18, 2020

dfawley deleted the route_actions branch September 18, 2020 15:41

A31: xDS Timeout Support and Config Selector Design #192

A31: xDS Timeout Support and Config Selector Design #192

Uh oh!

Conversation

dfawley commented Jul 13, 2020

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfawley commented Sep 16, 2020

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!