diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index 1a750ae3fa4..7676346d233 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -137,17 +137,6 @@ bool XdsSecurityEnabled() { return parse_succeeded && parsed_value; } -// TODO(donnadionne): Check to see if retry policy is enabled, this will be -// removed once retry functionality is fully integration-tested and enabled by -// default. -bool XdsRetryEnabled() { - char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); - bool parsed_value; - bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); - gpr_free(value); - return parse_succeeded && parsed_value; -} - // // XdsApi::Route::HashPolicy // @@ -1839,15 +1828,13 @@ grpc_error_handle RouteActionParse(const EncodingContext& context, } } // Get retry policy - if (XdsRetryEnabled()) { - const envoy_config_route_v3_RetryPolicy* retry_policy = - envoy_config_route_v3_RouteAction_retry_policy(route_action); - if (retry_policy != nullptr) { - absl::optional retry; - grpc_error_handle error = RetryPolicyParse(context, retry_policy, &retry); - if (error != GRPC_ERROR_NONE) return error; - route->retry_policy = retry; - } + const envoy_config_route_v3_RetryPolicy* retry_policy = + envoy_config_route_v3_RouteAction_retry_policy(route_action); + if (retry_policy != nullptr) { + absl::optional retry; + grpc_error_handle error = RetryPolicyParse(context, retry_policy, &retry); + if (error != GRPC_ERROR_NONE) return error; + route->retry_policy = retry; } return GRPC_ERROR_NONE; } @@ -1898,12 +1885,10 @@ grpc_error_handle RouteConfigParse( absl::optional virtual_host_retry_policy; const envoy_config_route_v3_RetryPolicy* retry_policy = envoy_config_route_v3_VirtualHost_retry_policy(virtual_hosts[i]); - if (XdsRetryEnabled()) { - if (retry_policy != nullptr) { - grpc_error_handle error = - RetryPolicyParse(context, retry_policy, &virtual_host_retry_policy); - if (error != GRPC_ERROR_NONE) return error; - } + if (retry_policy != nullptr) { + grpc_error_handle error = + RetryPolicyParse(context, retry_policy, &virtual_host_retry_policy); + if (error != GRPC_ERROR_NONE) return error; } // Parse routes. size_t num_routes; @@ -1939,8 +1924,7 @@ grpc_error_handle RouteConfigParse( error = RouteActionParse(context, routes[j], &route, &ignore_route); if (error != GRPC_ERROR_NONE) return error; if (ignore_route) continue; - if (XdsRetryEnabled() && route.retry_policy == absl::nullopt && - retry_policy != nullptr) { + if (route.retry_policy == absl::nullopt && retry_policy != nullptr) { route.retry_policy = virtual_host_retry_policy; } if (context.use_v3) { diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 3b4be9db752..89070960fed 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -5650,7 +5650,6 @@ TEST_P(LdsRdsTest, XdsRoutingWithOnlyApplicationTimeout) { } TEST_P(LdsRdsTest, XdsRetryPolicyNumRetries) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); const size_t kNumRetries = 3; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -5711,11 +5710,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyNumRetries) { StatusCode::UNAUTHENTICATED)) .set_expected_error_code(StatusCode::UNAUTHENTICATED)); EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); } TEST_P(LdsRdsTest, XdsRetryPolicyAtVirtualHostLevel) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); const size_t kNumRetries = 3; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -5739,11 +5736,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyAtVirtualHostLevel) { StatusCode::DEADLINE_EXCEEDED)) .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED)); EXPECT_EQ(kNumRetries + 1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); } TEST_P(LdsRdsTest, XdsRetryPolicyLongBackOff) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); // Set num retries to 3, but due to longer back off, we expect only 1 retry // will take place. const size_t kNumRetries = 3; @@ -5777,11 +5772,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyLongBackOff) { StatusCode::CANCELLED)) .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED)); EXPECT_EQ(1 + 1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); } TEST_P(LdsRdsTest, XdsRetryPolicyMaxBackOff) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); // Set num retries to 3, but due to longer back off, we expect only 2 retry // will take place, while the 2nd one will obey the max backoff. const size_t kNumRetries = 3; @@ -5822,11 +5815,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyMaxBackOff) { StatusCode::CANCELLED)) .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED)); EXPECT_EQ(2 + 1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); } TEST_P(LdsRdsTest, XdsRetryPolicyUnsupportedStatusCode) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); const size_t kNumRetries = 3; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -5849,11 +5840,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyUnsupportedStatusCode) { StatusCode::DEADLINE_EXCEEDED)) .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED)); EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); } TEST_P(LdsRdsTest, XdsRetryPolicyInvalidNumRetriesZero) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); // Populate new EDS resources. @@ -5876,11 +5865,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyInvalidNumRetriesZero) { response_state.error_message, ::testing::HasSubstr( "RouteAction RetryPolicy num_retries set to invalid value 0.")); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); } TEST_P(LdsRdsTest, XdsRetryPolicyRetryBackOffMissingBaseInterval) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true"); SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); // Populate new EDS resources. @@ -5907,70 +5894,6 @@ TEST_P(LdsRdsTest, XdsRetryPolicyRetryBackOffMissingBaseInterval) { response_state.error_message, ::testing::HasSubstr( "RouteAction RetryPolicy RetryBackoff missing base interval.")); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"); -} - -TEST_P(LdsRdsTest, XdsRetryPolicyDisabled) { - const size_t kNumRetries = 3; - SetNextResolution({}); - SetNextResolutionForLbChannelAllBalancers(); - // Populate new EDS resources. - AdsServiceImpl::EdsResourceArgs args({ - {"locality0", CreateEndpointsForBackends(0, 1)}, - }); - balancers_[0]->ads_service()->SetEdsResource(BuildEdsResource(args)); - // Construct route config to set retry policy. - RouteConfiguration new_route_config = default_route_config_; - auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); - auto* retry_policy = route1->mutable_route()->mutable_retry_policy(); - retry_policy->set_retry_on( - "5xx,cancelled,deadline-exceeded,internal,resource-exhausted," - "unavailable"); - retry_policy->mutable_num_retries()->set_value(kNumRetries); - SetRouteConfiguration(0, new_route_config); - // Ensure we don't retry on supported statuses. - CheckRpcSendFailure( - CheckRpcSendFailureOptions() - .set_rpc_options( - RpcOptions().set_server_expected_error(StatusCode::CANCELLED)) - .set_expected_error_code(StatusCode::CANCELLED)); - EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - ResetBackendCounters(); - CheckRpcSendFailure( - CheckRpcSendFailureOptions() - .set_rpc_options(RpcOptions().set_server_expected_error( - StatusCode::DEADLINE_EXCEEDED)) - .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED)); - EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - ResetBackendCounters(); - CheckRpcSendFailure( - CheckRpcSendFailureOptions() - .set_rpc_options( - RpcOptions().set_server_expected_error(StatusCode::INTERNAL)) - .set_expected_error_code(StatusCode::INTERNAL)); - EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - ResetBackendCounters(); - CheckRpcSendFailure( - CheckRpcSendFailureOptions() - .set_rpc_options(RpcOptions().set_server_expected_error( - StatusCode::RESOURCE_EXHAUSTED)) - .set_expected_error_code(StatusCode::RESOURCE_EXHAUSTED)); - EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - ResetBackendCounters(); - CheckRpcSendFailure( - CheckRpcSendFailureOptions() - .set_rpc_options( - RpcOptions().set_server_expected_error(StatusCode::UNAVAILABLE)) - .set_expected_error_code(StatusCode::UNAVAILABLE)); - EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); - ResetBackendCounters(); - // Ensure we don't retry on an unsupported status. - CheckRpcSendFailure( - CheckRpcSendFailureOptions() - .set_rpc_options(RpcOptions().set_server_expected_error( - StatusCode::UNAUTHENTICATED)) - .set_expected_error_code(StatusCode::UNAUTHENTICATED)); - EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); } TEST_P(LdsRdsTest, XdsRoutingHeadersMatching) {