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

xds: parse timeout from RDS responses #7257

Merged
merged 5 commits into from
Jul 31, 2020
Merged

xds: parse timeout from RDS responses #7257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e2acd8
Parse timeout from RDS responses.
voidzcy Jul 28, 2020
138f6b4
Set timeout to infinite if maxGrpcTimeout is set to 0
voidzcy Jul 29, 2020
0cfc73b
Delete comment
voidzcy Jul 29, 2020
4c416aa
Add case for timeout disabled.
voidzcy Jul 29, 2020
910790d
Use max value instead of null.
voidzcy Jul 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 26 additions & 4 deletions xds/src/main/java/io/grpc/xds/EnvoyProtoData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.protobuf.NullValue;
import com.google.protobuf.Struct;
import com.google.protobuf.Value;
import com.google.protobuf.util.Durations;
import com.google.re2j.Pattern;
import com.google.re2j.PatternSyntaxException;
import io.envoyproxy.envoy.type.v3.FractionalPercent;
Expand All @@ -40,6 +41,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -1067,18 +1069,30 @@ static StructOrError<HeaderMatcher> convertEnvoyProtoHeaderMatcher(

/** See corresponding Envoy proto message {@link io.envoyproxy.envoy.api.v2.route.RouteAction}. */
static final class RouteAction {
// Specifies the upstream timeout for the route, which spans between the point at which
// the entire downstream request (i.e., end-of-stream) has been processed and when the
Copy link
Member

Choose a reason for hiding this comment

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

The "downstream end-of-stream has been processed" in envoy API doc is about the stream from client to the envoy proxy, which more or less corresponds to onComplete() of client's request StreamObserver in proxyless case (Because the local xds plugin itself is the "envoy proxy".). I think it shouldn't be stated as "end-of-stream has been processed" for the proxyless case, in which the stream connects to the backend directly. cc @dfawley @ejona86

Copy link
Member

Choose a reason for hiding this comment

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

I also just noticed that by the description of this envoy API doc, in bidi-streaming or client-streaming case, the timeout may not be converted to grpc deadline. (Although max_grpc_timeout can always be converted to grpc deadline)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted the comment if you don't like it (although I don't think there is any problem stating in that way as the description is from the perspective of a Route, "downstream" still makes senses from an implementation's perspective).

Envoy's API doc also mentions that a value of 0 for timeout will disable the route's timeout. Added that case as well.

Copy link
Member

Choose a reason for hiding this comment

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

Deleted the comment if you don't like it (although I don't think there is any problem stating in that way as the description is from the perspective of a Route, "downstream" still makes senses from an implementation's perspective).

The timeout java field is a combination of timeout envoy api field and max_grpc_timeout envoy api field (The latter, if specified, overrides the former). And I noticed that the two envoy api fields measure different time span for grpc requests, especially for client-streaming case. The latter definitely does not measure the span from "downstream EOS"

Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice this detail before (timeout starts counting after the client sends its end-stream). This behavior might be difficult to implement -- at least we can't do it in Go for unary RPCs without some changes in the channel.

@markdroth do you think we'll need to match Envoy behavior here, or could we treat it as a known difference and call it out in the design?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's really necessary to mimic Envoy's behavior in this regard. I think it's fine for us to just use this to control our existing timeout semantics, just as if timeout were set via the service config.

We should call this out in the gRFC, though.

Copy link
Member

Choose a reason for hiding this comment

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

A preexisting management server may set the HTTP timeout for only 500ms, with grpc_max_timeout unset. This works perfectly for long client streaming with envoy proxy. But it will fail all client streaming requests from a proxyless grpc client.

Copy link
Member

Choose a reason for hiding this comment

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

How about we don't support HTTP timeout for client/bidi streaming, only max_grpc_timeout is supported for them?

// upstream response has been completely processed.
private final long timeoutNano;
// Exactly one of the following fields is non-null.
@Nullable
private final String cluster;
@Nullable
private final List<ClusterWeight> weightedClusters;

@VisibleForTesting
RouteAction(@Nullable String cluster, @Nullable List<ClusterWeight> weightedClusters) {
RouteAction(
long timeoutNano,
@Nullable String cluster,
@Nullable List<ClusterWeight> weightedClusters) {
this.timeoutNano = timeoutNano;
this.cluster = cluster;
this.weightedClusters = weightedClusters;
}

long getTimeoutNano() {
return timeoutNano;
}

@Nullable
String getCluster() {
return cluster;
Expand All @@ -1098,18 +1112,20 @@ public boolean equals(Object o) {
return false;
}
RouteAction that = (RouteAction) o;
return Objects.equals(cluster, that.cluster)
return Objects.equals(timeoutNano, that.timeoutNano)
&& Objects.equals(cluster, that.cluster)
&& Objects.equals(weightedClusters, that.weightedClusters);
}

@Override
public int hashCode() {
return Objects.hash(cluster, weightedClusters);
return Objects.hash(timeoutNano, cluster, weightedClusters);
}

@Override
public String toString() {
ToStringHelper toStringHelper = MoreObjects.toStringHelper(this);
toStringHelper.add("timeout", timeoutNano + "ns");
if (cluster != null) {
toStringHelper.add("cluster", cluster);
}
Expand Down Expand Up @@ -1146,7 +1162,13 @@ static StructOrError<RouteAction> fromEnvoyProtoRouteAction(
return StructOrError.fromError(
"Unknown cluster specifier: " + proto.getClusterSpecifierCase());
}
return StructOrError.fromStruct(new RouteAction(cluster, weightedClusters));
long timeoutNano = TimeUnit.SECONDS.toNanos(15L); // default 15s
if (proto.hasMaxGrpcTimeout()) {
timeoutNano = Durations.toNanos(proto.getMaxGrpcTimeout());
Copy link
Member

Choose a reason for hiding this comment

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

If configured as 0, the maximum allowed timeout for gRPC requests is infinity.
https://github.com/envoyproxy/envoy/blob/c694e470dbc9a1dd24b296d117330f0e5becb2c9/api/envoy/api/v2/route/route_components.proto#L964

} else if (proto.hasTimeout()) {
timeoutNano = Durations.toNanos(proto.getTimeout());
}
return StructOrError.fromStruct(new RouteAction(timeoutNano, cluster, weightedClusters));
}
}

Expand Down
10 changes: 9 additions & 1 deletion xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.protobuf.Struct;
import com.google.protobuf.UInt32Value;
import com.google.protobuf.Value;
import com.google.protobuf.util.Durations;
import com.google.re2j.Pattern;
import io.envoyproxy.envoy.config.core.v3.RuntimeFractionalPercent;
import io.envoyproxy.envoy.config.route.v3.QueryParameterMatcher;
Expand All @@ -44,6 +45,7 @@
import io.grpc.xds.RouteMatch.PathMatcher;
import java.util.Arrays;
import java.util.Collections;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -206,7 +208,7 @@ public void convertRoute() {
new Route(
new RouteMatch(new PathMatcher("/service/method", null, null),
Collections.<HeaderMatcher>emptyList(), null),
new RouteAction("cluster-foo", null)));
new RouteAction(TimeUnit.SECONDS.toNanos(15L), "cluster-foo", null)));

io.envoyproxy.envoy.config.route.v3.Route unsupportedProto =
io.envoyproxy.envoy.config.route.v3.Route.newBuilder()
Expand Down Expand Up @@ -400,6 +402,8 @@ public void convertRouteAction() {
.build();
StructOrError<RouteAction> struct1 = RouteAction.fromEnvoyProtoRouteAction(proto1);
assertThat(struct1.getErrorDetail()).isNull();
assertThat(struct1.getStruct().getTimeoutNano())
.isEqualTo(TimeUnit.SECONDS.toNanos(15L)); // default value
assertThat(struct1.getStruct().getCluster()).isEqualTo("cluster-foo");
assertThat(struct1.getStruct().getWeightedCluster()).isNull();

Expand All @@ -414,6 +418,8 @@ public void convertRouteAction() {
// cluster_specifier = weighted_cluster
io.envoyproxy.envoy.config.route.v3.RouteAction proto3 =
io.envoyproxy.envoy.config.route.v3.RouteAction.newBuilder()
.setMaxGrpcTimeout(Durations.fromSeconds(6L))
.setTimeout(Durations.fromMicros(20L))
.setWeightedClusters(
WeightedCluster.newBuilder()
.addClusters(
Expand All @@ -424,6 +430,8 @@ public void convertRouteAction() {
.build();
StructOrError<RouteAction> struct3 = RouteAction.fromEnvoyProtoRouteAction(proto3);
assertThat(struct3.getErrorDetail()).isNull();
assertThat(struct3.getStruct().getTimeoutNano())
.isEqualTo(TimeUnit.SECONDS.toNanos(6L));
assertThat(struct3.getStruct().getCluster()).isNull();
assertThat(struct3.getStruct().getWeightedCluster())
.containsExactly(new ClusterWeight("cluster-baz", 100));
Expand Down
9 changes: 6 additions & 3 deletions xds/src/test/java/io/grpc/xds/XdsClientImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,14 +748,16 @@ public void resolveVirtualHostWithPathMatchingInRdsResponse() {
new io.grpc.xds.RouteMatch(
/* prefix= */ null,
/* path= */ "/service1/method1"),
new EnvoyProtoData.RouteAction("cl1.googleapis.com", null)));
new EnvoyProtoData.RouteAction(
TimeUnit.SECONDS.toNanos(15L), "cl1.googleapis.com", null)));
assertThat(routes.get(1)).isEqualTo(
new EnvoyProtoData.Route(
// path match with weighted cluster route
new io.grpc.xds.RouteMatch(
/* prefix= */ null,
/* path= */ "/service2/method2"),
new EnvoyProtoData.RouteAction(
TimeUnit.SECONDS.toNanos(15L),
null,
ImmutableList.of(
new EnvoyProtoData.ClusterWeight("cl21.googleapis.com", 30),
Expand All @@ -767,15 +769,16 @@ public void resolveVirtualHostWithPathMatchingInRdsResponse() {
new io.grpc.xds.RouteMatch(
/* prefix= */ "/service1/",
/* path= */ null),
new EnvoyProtoData.RouteAction("cl1.googleapis.com", null)));
new EnvoyProtoData.RouteAction(
TimeUnit.SECONDS.toNanos(15L), "cl1.googleapis.com", null)));
assertThat(routes.get(3)).isEqualTo(
new EnvoyProtoData.Route(
// default match with cluster route
new io.grpc.xds.RouteMatch(
/* prefix= */ "",
/* path= */ null),
new EnvoyProtoData.RouteAction(
"cluster.googleapis.com", null)));
TimeUnit.SECONDS.toNanos(15L), "cluster.googleapis.com", null)));
}

/**
Expand Down
5 changes: 3 additions & 2 deletions xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public void generateXdsRoutingRawConfig() {
new RouteMatch(
new PathMatcher(null, "", null), Collections.<HeaderMatcher>emptyList(),
new FractionMatcher(10, 20)),
new RouteAction("cluster-foo", null));
new RouteAction(15L, "cluster-foo", null));
Route r2 =
new Route(
new RouteMatch(
Expand All @@ -92,6 +92,7 @@ public void generateXdsRoutingRawConfig() {
new HeaderMatcher(":scheme", "https", null, null, null, null, null, false)),
null),
new RouteAction(
15L,
null,
Arrays.asList(
new ClusterWeight("cluster-foo", 20),
Expand Down Expand Up @@ -134,7 +135,7 @@ public void generateXdsRoutingRawConfig_allowDuplicateMatchers() {
new RouteMatch(
new PathMatcher("/service/method", null, null),
Collections.<HeaderMatcher>emptyList(), null),
new RouteAction("cluster-foo", null));
new RouteAction(15L, "cluster-foo", null));

Map<String, ?> config =
XdsNameResolver.generateXdsRoutingRawConfig(Arrays.asList(route, route));
Expand Down