diff --git a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc index c1b5a23a14b..2b6ab6096a2 100644 --- a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc +++ b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc @@ -667,6 +667,7 @@ void OutlierDetectionLb::UpdateLocked(UpdateArgs args) { // Update child policy. UpdateArgs update_args; update_args.addresses = std::move(args.addresses); + update_args.resolution_note = std::move(args.resolution_note); update_args.config = config_->child_policy(); // Update the policy. update_args.args = std::move(args.args); diff --git a/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc b/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc index 778de9c4560..97640431145 100644 --- a/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc +++ b/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc @@ -285,6 +285,7 @@ class PriorityLb : public LoadBalancingPolicy { ChannelArgs args_; RefCountedPtr config_; absl::StatusOr addresses_; + std::string resolution_note_; // Internal state. bool shutting_down_ = false; @@ -367,6 +368,7 @@ void PriorityLb::UpdateLocked(UpdateArgs args) { args_ = std::move(args.args); // Update addresses. addresses_ = MakeHierarchicalAddressMap(args.addresses); + resolution_note_ = std::move(args.resolution_note); // Check all existing children against the new config. update_in_progress_ = true; for (const auto& p : children_) { @@ -786,6 +788,7 @@ void PriorityLb::ChildPriority::UpdateLocked( } else { update_args.addresses = priority_policy_->addresses_.status(); } + update_args.resolution_note = priority_policy_->resolution_note_; update_args.args = priority_policy_->args_; // Update the policy. if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_priority_trace)) { diff --git a/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc b/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc index 7feb8e50989..cfc0feb840e 100644 --- a/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc +++ b/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc @@ -150,6 +150,7 @@ class WeightedTargetLb : public LoadBalancingPolicy { void UpdateLocked(const WeightedTargetLbConfig::ChildConfig& config, absl::StatusOr addresses, + const std::string& resolution_note, const ChannelArgs& args); void ResetBackoffLocked(); void DeactivateLocked(); @@ -333,7 +334,8 @@ void WeightedTargetLb::UpdateLocked(UpdateArgs args) { } else { addresses = address_map.status(); } - target->UpdateLocked(config, std::move(addresses), args.args); + target->UpdateLocked(config, std::move(addresses), args.resolution_note, + args.args); } update_in_progress_ = false; if (config_->target_map().empty()) { @@ -552,7 +554,8 @@ WeightedTargetLb::WeightedChild::CreateChildPolicyLocked( void WeightedTargetLb::WeightedChild::UpdateLocked( const WeightedTargetLbConfig::ChildConfig& config, - absl::StatusOr addresses, const ChannelArgs& args) { + absl::StatusOr addresses, + const std::string& resolution_note, const ChannelArgs& args) { if (weighted_target_policy_->shutting_down_) return; // Update child weight. weight_ = config.weight; @@ -573,6 +576,7 @@ void WeightedTargetLb::WeightedChild::UpdateLocked( UpdateArgs update_args; update_args.config = config.config; update_args.addresses = std::move(addresses); + update_args.resolution_note = resolution_note; update_args.args = args; // Update the policy. if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_weighted_target_trace)) { diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc index 593eab151cc..f164992d78d 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc @@ -266,6 +266,7 @@ class XdsClusterImplLb : public LoadBalancingPolicy { OrphanablePtr CreateChildPolicyLocked( const ChannelArgs& args); void UpdateChildPolicyLocked(absl::StatusOr addresses, + std::string resolution_note, const ChannelArgs& args); void MaybeUpdatePickerLocked(); @@ -519,7 +520,8 @@ void XdsClusterImplLb::UpdateLocked(UpdateArgs args) { MaybeUpdatePickerLocked(); } // Update child policy. - UpdateChildPolicyLocked(std::move(args.addresses), args.args); + UpdateChildPolicyLocked(std::move(args.addresses), + std::move(args.resolution_note), args.args); } void XdsClusterImplLb::MaybeUpdatePickerLocked() { @@ -576,7 +578,8 @@ OrphanablePtr XdsClusterImplLb::CreateChildPolicyLocked( } void XdsClusterImplLb::UpdateChildPolicyLocked( - absl::StatusOr addresses, const ChannelArgs& args) { + absl::StatusOr addresses, std::string resolution_note, + const ChannelArgs& args) { // Create policy if needed. if (child_policy_ == nullptr) { child_policy_ = CreateChildPolicyLocked(args); @@ -584,6 +587,7 @@ void XdsClusterImplLb::UpdateChildPolicyLocked( // Construct update args. UpdateArgs update_args; update_args.addresses = std::move(addresses); + update_args.resolution_note = std::move(resolution_note); update_args.config = config_->child_policy(); update_args.args = args.Set(GRPC_ARG_XDS_CLUSTER_NAME, config_->cluster_name()); diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc index 983cfc1f3ef..dc9439b96c0 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc @@ -31,6 +31,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -224,16 +225,44 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { // in methods of this class rather than in lambdas to work around an MSVC // bug. void OnResourceChangedHelper(XdsEndpointResource update) { + std::string resolution_note; + if (update.priorities.empty()) { + resolution_note = absl::StrCat( + "EDS resource ", discovery_mechanism_->GetEdsResourceName(), + " contains no localities"); + } else { + std::set empty_localities; + for (const auto& priority : update.priorities) { + for (const auto& p : priority.localities) { + if (p.second.endpoints.empty()) { + empty_localities.insert(p.first->AsHumanReadableString()); + } + } + } + if (!empty_localities.empty()) { + resolution_note = absl::StrCat( + "EDS resource ", discovery_mechanism_->GetEdsResourceName(), + " contains empty localities: [", + absl::StrJoin(empty_localities, "; "), "]"); + } + } discovery_mechanism_->parent()->OnEndpointChanged( - discovery_mechanism_->index(), std::move(update)); + discovery_mechanism_->index(), std::move(update), + std::move(resolution_note)); } void OnErrorHelper(absl::Status status) { - discovery_mechanism_->parent()->OnError(discovery_mechanism_->index(), - status); + discovery_mechanism_->parent()->OnError( + discovery_mechanism_->index(), + absl::StrCat("EDS watcher error for resource ", + discovery_mechanism_->GetEdsResourceName(), " (", + status.ToString(), ")")); } void OnResourceDoesNotExistHelper() { discovery_mechanism_->parent()->OnResourceDoesNotExist( - discovery_mechanism_->index()); + discovery_mechanism_->index(), + absl::StrCat("EDS resource ", + discovery_mechanism_->GetEdsResourceName(), + " does not exist")); } RefCountedPtr discovery_mechanism_; }; @@ -288,6 +317,11 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { // access protected member in base class. friend class ResolverResultHandler; + absl::string_view GetDnsHostname() const { + auto& config = parent()->config_->discovery_mechanisms()[index()]; + return config.dns_hostname; + } + OrphanablePtr resolver_; }; @@ -295,6 +329,8 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { OrphanablePtr discovery_mechanism; // Most recent update reported by the discovery mechanism. absl::optional latest_update; + // Last resolution note reported by the discovery mechanism, if any. + std::string resolution_note; // State used to retain child policy names for priority policy. std::vector priority_child_numbers; size_t next_available_child_number = 0; @@ -335,9 +371,10 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { void ShutdownLocked() override; - void OnEndpointChanged(size_t index, XdsEndpointResource update); - void OnError(size_t index, absl::Status status); - void OnResourceDoesNotExist(size_t index); + void OnEndpointChanged(size_t index, XdsEndpointResource update, + std::string resolution_note); + void OnError(size_t index, std::string resolution_note); + void OnResourceDoesNotExist(size_t index, std::string resolution_note); void MaybeDestroyChildPolicyLocked(); @@ -345,6 +382,7 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { OrphanablePtr CreateChildPolicyLocked( const ChannelArgs& args); ServerAddressList CreateChildPolicyAddressesLocked(); + std::string CreateChildPolicyResolutionNoteLocked(); RefCountedPtr CreateChildPolicyConfigLocked(); ChannelArgs CreateChildPolicyArgsLocked(const ChannelArgs& args_in); @@ -440,17 +478,16 @@ void XdsClusterResolverLb::EdsDiscoveryMechanism::Orphan() { // void XdsClusterResolverLb::LogicalDNSDiscoveryMechanism::Start() { - std::string target = - parent()->config_->discovery_mechanisms()[index()].dns_hostname; + std::string target; ChannelArgs args = parent()->args_; auto* fake_resolver_response_generator = args.GetPointer( GRPC_ARG_XDS_LOGICAL_DNS_CLUSTER_FAKE_RESOLVER_RESPONSE_GENERATOR); if (fake_resolver_response_generator != nullptr) { - target = absl::StrCat("fake:", target); + target = absl::StrCat("fake:", GetDnsHostname()); args = args.SetObject(fake_resolver_response_generator->Ref()); } else { - target = absl::StrCat("dns:", target); + target = absl::StrCat("dns:", GetDnsHostname()); } resolver_ = CoreConfiguration::Get().resolver_registry().CreateResolver( target.c_str(), args, parent()->interested_parties(), @@ -458,7 +495,9 @@ void XdsClusterResolverLb::LogicalDNSDiscoveryMechanism::Start() { absl::make_unique( Ref(DEBUG_LOCATION, "LogicalDNSDiscoveryMechanism"))); if (resolver_ == nullptr) { - parent()->OnResourceDoesNotExist(index()); + parent()->OnResourceDoesNotExist( + index(), + absl::StrCat("error creating DNS resolver for ", GetDnsHostname())); return; } resolver_->StartLocked(); @@ -488,14 +527,18 @@ void XdsClusterResolverLb::LogicalDNSDiscoveryMechanism::Orphan() { void XdsClusterResolverLb::LogicalDNSDiscoveryMechanism::ResolverResultHandler:: ReportResult(Resolver::Result result) { + XdsClusterResolverLb* lb_policy = discovery_mechanism_->parent(); + size_t index = discovery_mechanism_->index(); if (!result.addresses.ok()) { - discovery_mechanism_->parent()->OnError(discovery_mechanism_->index(), - result.addresses.status()); + if (result.resolution_note.empty()) { + result.resolution_note = absl::StrCat( + "DNS resolution failed for ", discovery_mechanism_->GetDnsHostname(), + " (", result.addresses.status().ToString(), ")"); + } + lb_policy->OnError(index, result.resolution_note); return; } // Convert resolver result to EDS update. - // TODO(roth): Figure out a way to pass resolution_note through to the - // child policy. XdsEndpointResource update; XdsEndpointResource::Priority::Locality locality; locality.name = MakeRefCounted("", "", ""); @@ -504,8 +547,8 @@ void XdsClusterResolverLb::LogicalDNSDiscoveryMechanism::ResolverResultHandler:: XdsEndpointResource::Priority priority; priority.localities.emplace(locality.name.get(), std::move(locality)); update.priorities.emplace_back(std::move(priority)); - discovery_mechanism_->parent()->OnEndpointChanged( - discovery_mechanism_->index(), std::move(update)); + lb_policy->OnEndpointChanged(index, std::move(update), + std::move(result.resolution_note)); } // @@ -615,13 +658,14 @@ void XdsClusterResolverLb::ExitIdleLocked() { } void XdsClusterResolverLb::OnEndpointChanged(size_t index, - XdsEndpointResource update) { + XdsEndpointResource update, + std::string resolution_note) { if (shutting_down_) return; if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_xds_cluster_resolver_trace)) { gpr_log(GPR_INFO, "[xds_cluster_resolver_lb %p] Received update from xds client" - " for discovery mechanism %" PRIuPTR "", - this, index); + " for discovery mechanism %" PRIuPTR " (resolution_note=\"%s\")", + this, index, resolution_note.c_str()); } DiscoveryMechanismEntry& discovery_entry = discovery_mechanisms_[index]; // We need at least one priority for each discovery mechanism, just so that we @@ -694,6 +738,7 @@ void XdsClusterResolverLb::OnEndpointChanged(size_t index, } // Save update. discovery_entry.latest_update = std::move(update); + discovery_entry.resolution_note = std::move(resolution_note); discovery_entry.priority_child_numbers = std::move(priority_child_numbers); // If any discovery mechanism has not received its first update, // wait until that happens before creating the child policy. @@ -711,27 +756,28 @@ void XdsClusterResolverLb::OnEndpointChanged(size_t index, UpdateChildPolicyLocked(); } -void XdsClusterResolverLb::OnError(size_t index, absl::Status status) { +void XdsClusterResolverLb::OnError(size_t index, std::string resolution_note) { gpr_log(GPR_ERROR, "[xds_cluster_resolver_lb %p] discovery mechanism %" PRIuPTR - " xds watcher reported error: %s", - this, index, status.ToString().c_str()); + " reported error: %s", + this, index, resolution_note.c_str()); if (shutting_down_) return; if (!discovery_mechanisms_[index].latest_update.has_value()) { - // Call OnEndpointChanged with an empty update just like - // OnResourceDoesNotExist. - OnEndpointChanged(index, XdsEndpointResource()); + // Call OnEndpointChanged() with an empty update just like + // OnResourceDoesNotExist(). + OnEndpointChanged(index, XdsEndpointResource(), std::move(resolution_note)); } } -void XdsClusterResolverLb::OnResourceDoesNotExist(size_t index) { +void XdsClusterResolverLb::OnResourceDoesNotExist(size_t index, + std::string resolution_note) { gpr_log(GPR_ERROR, "[xds_cluster_resolver_lb %p] discovery mechanism %" PRIuPTR - " resource does not exist", - this, index); + " resource does not exist: %s", + this, index, resolution_note.c_str()); if (shutting_down_) return; - // Call OnEndpointChanged with an empty update. - OnEndpointChanged(index, XdsEndpointResource()); + // Call OnEndpointChanged() with an empty update. + OnEndpointChanged(index, XdsEndpointResource(), std::move(resolution_note)); } // @@ -780,6 +826,16 @@ ServerAddressList XdsClusterResolverLb::CreateChildPolicyAddressesLocked() { return addresses; } +std::string XdsClusterResolverLb::CreateChildPolicyResolutionNoteLocked() { + std::vector resolution_notes; + for (const auto& discovery_entry : discovery_mechanisms_) { + if (!discovery_entry.resolution_note.empty()) { + resolution_notes.push_back(discovery_entry.resolution_note); + } + } + return absl::StrJoin(resolution_notes, "; "); +} + RefCountedPtr XdsClusterResolverLb::CreateChildPolicyConfigLocked() { Json::Object priority_children; @@ -948,6 +1004,7 @@ void XdsClusterResolverLb::UpdateChildPolicyLocked() { update_args.config = CreateChildPolicyConfigLocked(); if (update_args.config == nullptr) return; update_args.addresses = CreateChildPolicyAddressesLocked(); + update_args.resolution_note = CreateChildPolicyResolutionNoteLocked(); update_args.args = CreateChildPolicyArgsLocked(args_); if (child_policy_ == nullptr) { child_policy_ = CreateChildPolicyLocked(update_args.args); diff --git a/test/cpp/end2end/xds/xds_cluster_end2end_test.cc b/test/cpp/end2end/xds/xds_cluster_end2end_test.cc index 582e0fbff76..140740b596e 100644 --- a/test/cpp/end2end/xds/xds_cluster_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_cluster_end2end_test.cc @@ -486,27 +486,50 @@ TEST_P(EdsTest, SameBackendListedMultipleTimes) { backends_[0]->backend_service()->request_count()); } -// Tests that RPCs will be blocked until a non-empty serverlist is received. -TEST_P(EdsTest, InitiallyEmptyServerlist) { +TEST_P(EdsTest, OneLocalityWithNoEndpoints) { CreateAndStartBackends(1); - // First response is an empty serverlist. + // Initial EDS resource has one locality with no endpoints. EdsResourceArgs::Locality empty_locality("locality0", {}); EdsResourceArgs args({std::move(empty_locality)}); balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); // RPCs should fail. constexpr char kErrorMessage[] = - // TODO(roth): Improve this error message as part of - // https://github.com/grpc/grpc/issues/22883. - "empty address list: "; + "empty address list: EDS resource eds_service_name contains empty " + "localities: \\[\\{region=\"xds_default_locality_region\", " + "zone=\"xds_default_locality_zone\", sub_zone=\"locality0\"\\}\\]"; CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, kErrorMessage); - // Send non-empty serverlist. + // Send EDS resource that has an endpoint. args = EdsResourceArgs({{"locality0", CreateEndpointsForBackends()}}); balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); // RPCs should eventually succeed. WaitForAllBackends(DEBUG_LOCATION, 0, 1, [&](const RpcResult& result) { if (!result.status.ok()) { EXPECT_EQ(result.status.error_code(), StatusCode::UNAVAILABLE); - EXPECT_EQ(result.status.error_message(), kErrorMessage); + EXPECT_THAT(result.status.error_message(), + ::testing::MatchesRegex(kErrorMessage)); + } + }); +} + +TEST_P(EdsTest, NoLocalities) { + CreateAndStartBackends(1); + // Initial EDS resource has no localities. + EdsResourceArgs args; + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + // RPCs should fail. + constexpr char kErrorMessage[] = + "no children in weighted_target policy: EDS resource eds_service_name " + "contains no localities"; + CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, kErrorMessage); + // Send EDS resource that has an endpoint. + args = EdsResourceArgs({{"locality0", CreateEndpointsForBackends()}}); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + // RPCs should eventually succeed. + WaitForAllBackends(DEBUG_LOCATION, 0, 1, [&](const RpcResult& result) { + if (!result.status.ok()) { + EXPECT_EQ(result.status.error_code(), StatusCode::UNAVAILABLE); + EXPECT_THAT(result.status.error_message(), + ::testing::MatchesRegex(kErrorMessage)); } }); } @@ -712,14 +735,6 @@ TEST_P(EdsTest, LocalityContainingNoEndpoints) { kNumRpcs / backends_.size()); } -// EDS update with no localities. -TEST_P(EdsTest, NoLocalities) { - balancer_->ads_service()->SetEdsResource(BuildEdsResource({})); - Status status = SendRpc(); - EXPECT_FALSE(status.ok()); - EXPECT_EQ(status.error_code(), StatusCode::UNAVAILABLE); -} - // Tests that the locality map can work properly even when it contains a large // number of localities. TEST_P(EdsTest, ManyLocalitiesStressTest) { diff --git a/test/cpp/end2end/xds/xds_cluster_type_end2end_test.cc b/test/cpp/end2end/xds/xds_cluster_type_end2end_test.cc index 500c47be613..87a03402421 100644 --- a/test/cpp/end2end/xds/xds_cluster_type_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_cluster_type_end2end_test.cc @@ -653,9 +653,8 @@ TEST_P(AggregateClusterTest, ReconfigEdsWhileLogicalDnsChildFails) { } // When an RPC fails, we know the channel has seen the update. constexpr char kErrorMessage[] = - // TODO(roth): Figure out how to get some sort of resolution note - // included here as part of https://github.com/grpc/grpc/issues/22883. - "empty address list: "; + "empty address list: DNS resolution failed for server.example.com:443 " + "\\(UNAVAILABLE: injected error\\)"; CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, kErrorMessage); // Send an EDS update that moves locality1 to priority 0. args1 = EdsResourceArgs({ @@ -669,7 +668,8 @@ TEST_P(AggregateClusterTest, ReconfigEdsWhileLogicalDnsChildFails) { WaitForBackend(DEBUG_LOCATION, 0, [&](const RpcResult& result) { if (!result.status.ok()) { EXPECT_EQ(result.status.error_code(), StatusCode::UNAVAILABLE); - EXPECT_EQ(result.status.error_message(), kErrorMessage); + EXPECT_THAT(result.status.error_message(), + ::testing::MatchesRegex(kErrorMessage)); } }); } diff --git a/test/cpp/end2end/xds/xds_core_end2end_test.cc b/test/cpp/end2end/xds/xds_core_end2end_test.cc index 5fb87ea1a70..106759da906 100644 --- a/test/cpp/end2end/xds/xds_core_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_core_end2end_test.cc @@ -541,21 +541,21 @@ TEST_P(TimeoutTest, CdsSecondResourceNotPresentInRequest) { TEST_P(TimeoutTest, EdsServerIgnoresRequest) { balancer_->ads_service()->IgnoreResourceType(kEdsTypeUrl); - CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, - // TODO(roth): Improve this error message as part of - // https://github.com/grpc/grpc/issues/22883. - "no children in weighted_target policy: ", - RpcOptions().set_timeout_ms(4000)); + CheckRpcSendFailure( + DEBUG_LOCATION, StatusCode::UNAVAILABLE, + "no children in weighted_target policy: EDS resource eds_service_name " + "does not exist", + RpcOptions().set_timeout_ms(4000)); } TEST_P(TimeoutTest, EdsResourceNotPresentInRequest) { // No need to remove EDS resource, since the test suite does not add it // by default. - CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, - // TODO(roth): Improve this error message as part of - // https://github.com/grpc/grpc/issues/22883. - "no children in weighted_target policy: ", - RpcOptions().set_timeout_ms(4000)); + CheckRpcSendFailure( + DEBUG_LOCATION, StatusCode::UNAVAILABLE, + "no children in weighted_target policy: EDS resource eds_service_name " + "does not exist", + RpcOptions().set_timeout_ms(4000)); } TEST_P(TimeoutTest, EdsSecondResourceNotPresentInRequest) { @@ -585,9 +585,8 @@ TEST_P(TimeoutTest, EdsSecondResourceNotPresentInRequest) { if (result.status.ok()) return true; // Keep going. EXPECT_EQ(StatusCode::UNAVAILABLE, result.status.error_code()); EXPECT_EQ(result.status.error_message(), - // TODO(roth): Improve this error message as part of - // https://github.com/grpc/grpc/issues/22883. - "no children in weighted_target policy: "); + "no children in weighted_target policy: EDS resource " + "eds_service_name_does_not_exist does not exist"); return false; }, /*timeout_ms=*/30000, @@ -1048,10 +1047,12 @@ TEST_P(XdsFederationTest, EdsResourceNameAuthorityUnknown) { EchoResponse response; grpc::Status status = stub2->Echo(&context, request, &response); EXPECT_EQ(status.error_code(), StatusCode::UNAVAILABLE); - EXPECT_EQ(status.error_message(), - // TODO(roth): Improve this error message as part of - // https://github.com/grpc/grpc/issues/22883. - "no children in weighted_target policy: "); + EXPECT_EQ( + status.error_message(), + "no children in weighted_target policy: EDS watcher error for resource " + "xdstp://xds.unknown.com/envoy.config.endpoint.v3.ClusterLoadAssignment/" + "edsservice_name (UNAVAILABLE: authority \"xds.unknown.com\" not " + "present in bootstrap config)"); ASSERT_EQ(GRPC_CHANNEL_TRANSIENT_FAILURE, channel2->GetState(false)); } diff --git a/test/cpp/end2end/xds/xds_csds_end2end_test.cc b/test/cpp/end2end/xds/xds_csds_end2end_test.cc index bb571f03ff5..47f7f7f4841 100644 --- a/test/cpp/end2end/xds/xds_csds_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_csds_end2end_test.cc @@ -680,11 +680,11 @@ TEST_P(CsdsShortAdsTimeoutTest, XdsConfigDumpClusterDoesNotExist) { TEST_P(CsdsShortAdsTimeoutTest, XdsConfigDumpEndpointDoesNotExist) { int kTimeoutMillisecond = 1000000; // 1000s wait for the transient failure. balancer_->ads_service()->UnsetResource(kEdsTypeUrl, kDefaultEdsServiceName); - CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, - // TODO(roth): Improve this error message as part of - // https://github.com/grpc/grpc/issues/22883. - "no children in weighted_target policy: ", - RpcOptions().set_timeout_ms(kTimeoutMillisecond)); + CheckRpcSendFailure( + DEBUG_LOCATION, StatusCode::UNAVAILABLE, + "no children in weighted_target policy: EDS resource eds_service_name " + "does not exist", + RpcOptions().set_timeout_ms(kTimeoutMillisecond)); auto csds_response = FetchCsdsResponse(); EXPECT_THAT( csds_response.config(0).generic_xds_configs(), diff --git a/test/cpp/end2end/xds/xds_ring_hash_end2end_test.cc b/test/cpp/end2end/xds/xds_ring_hash_end2end_test.cc index ca211518c6f..0479c6f0de2 100644 --- a/test/cpp/end2end/xds/xds_ring_hash_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_ring_hash_end2end_test.cc @@ -1097,10 +1097,9 @@ TEST_P(RingHashTest, ReattemptWhenGoingFromTransientFailureToIdle) { // Channel should fail RPCs and go into TRANSIENT_FAILURE. CheckRpcSendFailure( DEBUG_LOCATION, StatusCode::UNAVAILABLE, - // TODO(roth): As part of https://github.com/grpc/grpc/issues/22883, - // figure out how to get a useful resolution note plumbed down to - // improve this message. - "empty address list: ", + "empty address list: EDS resource eds_service_name contains empty " + "localities: \\[\\{region=\"xds_default_locality_region\", " + "zone=\"xds_default_locality_zone\", sub_zone=\"locality0\"\\}\\]", RpcOptions().set_timeout_ms(kConnectionTimeoutMilliseconds)); EXPECT_EQ(GRPC_CHANNEL_TRANSIENT_FAILURE, channel_->GetState(false)); // Send EDS update with 1 backend.