Skip to content

Commit

Permalink
xDS retry policy should be considered present even if there are no su…
Browse files Browse the repository at this point in the history
…pported policies in retry_on (grpc#27315)

* Ensure that per route retry policy (even when there are no supported
retry_on statuses) still takes precedence over virtual host level retry
policy.

Added a test to guard this case.

* Taking care of code review comments and removing unnecessary block
  • Loading branch information
donnadionne authored Sep 14, 2021
1 parent ac9e521 commit 10f2180
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ grpc_error_handle XdsResolver::XdsConfigSelector::CreateMethodConfig(
RefCountedPtr<ServiceConfig>* method_config) {
std::vector<std::string> fields;
// Set retry policy if any.
if (route.retry_policy.has_value()) {
if (route.retry_policy.has_value() && !route.retry_policy->retry_on.Empty()) {
std::vector<std::string> retry_parts;
retry_parts.push_back(absl::StrFormat(
"\"retryPolicy\": {\n"
Expand Down
4 changes: 0 additions & 4 deletions src/core/ext/xds/xds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1552,10 +1552,6 @@ grpc_error_handle RetryPolicyParse(
}
}
}
// TODO(donnadionne): when we add support for per_try_timeout, we will need to
// return a policy if per_try_timeout is set even if retry_on specified no
// supported policies.
if (retry_to_return.retry_on.Empty()) return GRPC_ERROR_NONE;
const google_protobuf_UInt32Value* num_retries =
envoy_config_route_v3_RetryPolicy_num_retries(retry_policy);
if (num_retries != nullptr) {
Expand Down
33 changes: 33 additions & 0 deletions test/cpp/end2end/xds_end2end_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5894,6 +5894,39 @@ TEST_P(LdsRdsTest, XdsRetryPolicyUnsupportedStatusCode) {
EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
}

TEST_P(LdsRdsTest,
XdsRetryPolicyUnsupportedStatusCodeWithVirtualHostLevelRetry) {
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 with no supported retry_on
// statuses.
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");
retry_policy->mutable_num_retries()->set_value(kNumRetries);
// Construct a virtual host level retry policy with supported statuses.
auto* virtual_host_retry_policy =
new_route_config.mutable_virtual_hosts(0)->mutable_retry_policy();
virtual_host_retry_policy->set_retry_on(
"cancelled,deadline-exceeded,internal,resource-exhausted,unavailable");
virtual_host_retry_policy->mutable_num_retries()->set_value(kNumRetries);
SetRouteConfiguration(0, new_route_config);
// We expect no retry.
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());
}

TEST_P(LdsRdsTest, XdsRetryPolicyInvalidNumRetriesZero) {
SetNextResolution({});
SetNextResolutionForLbChannelAllBalancers();
Expand Down

0 comments on commit 10f2180

Please sign in to comment.