From a90f2da4ef2c76617e3274bb36a3ab2f63e3a77d Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Mon, 13 Sep 2021 08:31:00 -0700 Subject: [PATCH] xds: populate envoy RetryPolicy with no retryOn to resolver (#8511) Envoy RetryPolicy with empty retryOn should not be ignored as no retry config when selecting Route config. Therefore, if xDS update for a route contains a RetryPolicy that has no RetryOn value that we support, but the virtual host config does, xds client should choose the Envoy RetryPolicy from the route (even with no RetryOn), rather than choosing the one from virtual host, and try to convert it into grpc RetryPolicy, and end up with no retry. --- xds/src/main/java/io/grpc/xds/ClientXdsClient.java | 11 ++++------- xds/src/main/java/io/grpc/xds/XdsNameResolver.java | 5 +++-- .../java/io/grpc/xds/ClientXdsClientDataTest.java | 3 ++- .../test/java/io/grpc/xds/XdsNameResolverTest.java | 14 +++++++++++++- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java index d099fb72db1..451470f8ad2 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -1272,13 +1272,10 @@ private static StructOrError parseRetryPolicy( retryableStatusCodesBuilder.add(code); } List retryableStatusCodes = retryableStatusCodesBuilder.build(); - if (!retryableStatusCodes.isEmpty()) { - return StructOrError.fromStruct( - RetryPolicy.create( - maxAttempts, retryableStatusCodes, initialBackoff, maxBackoff, - /* perAttemptRecvTimeout= */ null)); - } - return null; + return StructOrError.fromStruct( + RetryPolicy.create( + maxAttempts, retryableStatusCodes, initialBackoff, maxBackoff, + /* perAttemptRecvTimeout= */ null)); } @VisibleForTesting diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 3ae6346c158..34b6a415200 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -182,13 +182,14 @@ public void shutdown() { @VisibleForTesting static Map generateServiceConfigWithMethodConfig( @Nullable Long timeoutNano, @Nullable RetryPolicy retryPolicy) { - if (timeoutNano == null && retryPolicy == null) { + if (timeoutNano == null + && (retryPolicy == null || retryPolicy.retryableStatusCodes().isEmpty())) { return Collections.emptyMap(); } ImmutableMap.Builder methodConfig = ImmutableMap.builder(); methodConfig.put( "name", Collections.singletonList(Collections.emptyMap())); - if (retryPolicy != null) { + if (retryPolicy != null && !retryPolicy.retryableStatusCodes().isEmpty()) { ImmutableMap.Builder rawRetryPolicy = ImmutableMap.builder(); rawRetryPolicy.put("maxAttempts", (double) retryPolicy.maxAttempts()); rawRetryPolicy.put("initialBackoff", Durations.toString(retryPolicy.initialBackoff())); diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java index 5bd57c2f16b..53d8388427b 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java @@ -547,7 +547,8 @@ public void parseRouteAction_withRetryPolicy() { .setRetryPolicy(builder.build()) .build(); struct = ClientXdsClient.parseRouteAction(proto, filterRegistry, false); - assertThat(struct.getStruct().retryPolicy()).isNull(); + assertThat(struct.getStruct().retryPolicy()).isNotNull(); + assertThat(struct.getStruct().retryPolicy().retryableStatusCodes()).isEmpty(); // base_interval unset builder diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index cb2b4481fad..8b604fbe6ab 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -989,6 +989,8 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException { RetryPolicy retryPolicy = RetryPolicy.create( 4, ImmutableList.of(Code.UNAVAILABLE, Code.CANCELLED), Durations.fromMillis(100), Durations.fromMillis(200), null); + RetryPolicy retryPolicyWithEmptyStatusCodes = RetryPolicy.create( + 4, ImmutableList.of(), Durations.fromMillis(100), Durations.fromMillis(200), null); // timeout only String expectedServiceConfigJson = "{\n" @@ -1002,6 +1004,11 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException { assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(timeoutNano, null)) .isEqualTo(expectedServiceConfig); + // timeout and retry with empty retriable status codes + assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig( + timeoutNano, retryPolicyWithEmptyStatusCodes)) + .isEqualTo(expectedServiceConfig); + // retry only expectedServiceConfigJson = "{\n" + " \"methodConfig\": [{\n" @@ -1022,6 +1029,7 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException { assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(null, retryPolicy)) .isEqualTo(expectedServiceConfig); + // timeout and retry expectedServiceConfigJson = "{\n" + " \"methodConfig\": [{\n" @@ -1044,12 +1052,16 @@ public void generateServiceConfig_forPerMethodConfig() throws IOException { .isEqualTo(expectedServiceConfig); // no timeout and no retry - // timeout and retry expectedServiceConfigJson = "{}"; expectedServiceConfig = (Map) JsonParser.parse(expectedServiceConfigJson); assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig(null, null)) .isEqualTo(expectedServiceConfig); + + // retry with emtry retriable status codes only + assertThat(XdsNameResolver.generateServiceConfigWithMethodConfig( + null, retryPolicyWithEmptyStatusCodes)) + .isEqualTo(expectedServiceConfig); } @Test