xds_cluster_resolver: pass useful resolution_note to children (#30283)

* xds_cluster_resolver: pass useful resolution_note to children

* clang-format

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);

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc

@@ -285,6 +285,7 @@ class PriorityLb : public LoadBalancingPolicy {
   ChannelArgs args_;
   RefCountedPtr<PriorityLbConfig> config_;
   absl::StatusOr<HierarchicalAddressMap> 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)) {

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<ServerAddressList> 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<ServerAddressList> addresses, const ChannelArgs& args) {
+    absl::StatusOr<ServerAddressList> 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)) {

src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc

@@ -266,6 +266,7 @@ class XdsClusterImplLb : public LoadBalancingPolicy {
   OrphanablePtr<LoadBalancingPolicy> CreateChildPolicyLocked(
       const ChannelArgs& args);
   void UpdateChildPolicyLocked(absl::StatusOr<ServerAddressList> 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<LoadBalancingPolicy> XdsClusterImplLb::CreateChildPolicyLocked(
 }
 
 void XdsClusterImplLb::UpdateChildPolicyLocked(
-    absl::StatusOr<ServerAddressList> addresses, const ChannelArgs& args) {
+    absl::StatusOr<ServerAddressList> 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());

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<std::string> 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(),
+        discovery_mechanism_->parent()->OnError(
-                                                status);
+            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<EdsDiscoveryMechanism> 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> resolver_;
   };
 
@@ -295,6 +329,8 @@ class XdsClusterResolverLb : public LoadBalancingPolicy {
     OrphanablePtr<DiscoveryMechanism> discovery_mechanism;
     // Most recent update reported by the discovery mechanism.
     absl::optional<XdsEndpointResource> 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<size_t /*child_number*/> 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 OnEndpointChanged(size_t index, XdsEndpointResource update,
-  void OnError(size_t index, absl::Status status);
+                         std::string resolution_note);
-  void OnResourceDoesNotExist(size_t index);
+  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<LoadBalancingPolicy> CreateChildPolicyLocked(
       const ChannelArgs& args);
   ServerAddressList CreateChildPolicyAddressesLocked();
+  std::string CreateChildPolicyResolutionNoteLocked();
   RefCountedPtr<Config> CreateChildPolicyConfigLocked();
   ChannelArgs CreateChildPolicyArgsLocked(const ChannelArgs& args_in);
 
@@ -440,17 +478,16 @@ void XdsClusterResolverLb::EdsDiscoveryMechanism::Orphan() {
 //
 
 void XdsClusterResolverLb::LogicalDNSDiscoveryMechanism::Start() {
-  std::string target =
+  std::string target;
-      parent()->config_->discovery_mechanisms()[index()].dns_hostname;
   ChannelArgs args = parent()->args_;
   auto* fake_resolver_response_generator =
       args.GetPointer<FakeResolverResponseGenerator>(
           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<ResolverResultHandler>(
           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(),
+    if (result.resolution_note.empty()) {
-                                            result.addresses.status());
+      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<XdsLocalityName>("", "", "");
@@ -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(
+  lb_policy->OnEndpointChanged(index, std::move(update),
-      discovery_mechanism_->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 "",
+            " for discovery mechanism %" PRIuPTR " (resolution_note=\"%s\")",
-            this, index);
+            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",
+          " reported error: %s",
-          this, index, status.ToString().c_str());
+          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
+    // Call OnEndpointChanged() with an empty update just like
-    // OnResourceDoesNotExist.
+    // OnResourceDoesNotExist().
-    OnEndpointChanged(index, XdsEndpointResource());
+    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",
+          " resource does not exist: %s",
-          this, index);
+          this, index, resolution_note.c_str());
   if (shutting_down_) return;
-  // Call OnEndpointChanged with an empty update.
+  // Call OnEndpointChanged() with an empty update.
-  OnEndpointChanged(index, XdsEndpointResource());
+  OnEndpointChanged(index, XdsEndpointResource(), std::move(resolution_note));
 }
 
 //
@@ -780,6 +826,16 @@ ServerAddressList XdsClusterResolverLb::CreateChildPolicyAddressesLocked() {
   return addresses;
 }
 
+std::string XdsClusterResolverLb::CreateChildPolicyResolutionNoteLocked() {
+  std::vector<absl::string_view> 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<LoadBalancingPolicy::Config>
 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);

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, OneLocalityWithNoEndpoints) {
-TEST_P(EdsTest, InitiallyEmptyServerlist) {
   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
+      "empty address list: EDS resource eds_service_name contains empty "
-      // https://github.com/grpc/grpc/issues/22883.
+      "localities: \\[\\{region=\"xds_default_locality_region\", "
-      "empty address list: ";
+      "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) {

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
+      "empty address list: DNS resolution failed for server.example.com:443 "
-      // included here as part of https://github.com/grpc/grpc/issues/22883.
+      "\\(UNAVAILABLE: injected error\\)";
-      "empty address list: ";
   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));
     }
   });
 }

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,
+  CheckRpcSendFailure(
-                      // TODO(roth): Improve this error message as part of
+      DEBUG_LOCATION, StatusCode::UNAVAILABLE,
-                      // https://github.com/grpc/grpc/issues/22883.
+      "no children in weighted_target policy: EDS resource eds_service_name "
-                      "no children in weighted_target policy: ",
+      "does not exist",
-                      RpcOptions().set_timeout_ms(4000));
+      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,
+  CheckRpcSendFailure(
-                      // TODO(roth): Improve this error message as part of
+      DEBUG_LOCATION, StatusCode::UNAVAILABLE,
-                      // https://github.com/grpc/grpc/issues/22883.
+      "no children in weighted_target policy: EDS resource eds_service_name "
-                      "no children in weighted_target policy: ",
+      "does not exist",
-                      RpcOptions().set_timeout_ms(4000));
+      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
+                  "no children in weighted_target policy: EDS resource "
-                  // https://github.com/grpc/grpc/issues/22883.
+                  "eds_service_name_does_not_exist does not exist");
-                  "no children in weighted_target policy: ");
         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(),
+  EXPECT_EQ(
-            // TODO(roth): Improve this error message as part of
+      status.error_message(),
-            // https://github.com/grpc/grpc/issues/22883.
+      "no children in weighted_target policy: EDS watcher error for resource "
-            "no children in weighted_target policy: ");
+      "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));
 }
 

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,
+  CheckRpcSendFailure(
-                      // TODO(roth): Improve this error message as part of
+      DEBUG_LOCATION, StatusCode::UNAVAILABLE,
-                      // https://github.com/grpc/grpc/issues/22883.
+      "no children in weighted_target policy: EDS resource eds_service_name "
-                      "no children in weighted_target policy: ",
+      "does not exist",
-                      RpcOptions().set_timeout_ms(kTimeoutMillisecond));
+      RpcOptions().set_timeout_ms(kTimeoutMillisecond));
   auto csds_response = FetchCsdsResponse();
   EXPECT_THAT(
       csds_response.config(0).generic_xds_configs(),

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,
+      "empty address list: EDS resource eds_service_name contains empty "
-      // figure out how to get a useful resolution note plumbed down to
+      "localities: \\[\\{region=\"xds_default_locality_region\", "
-      // improve this message.
+      "zone=\"xds_default_locality_zone\", sub_zone=\"locality0\"\\}\\]",
-      "empty address list: ",
       RpcOptions().set_timeout_ms(kConnectionTimeoutMilliseconds));
   EXPECT_EQ(GRPC_CHANNEL_TRANSIENT_FAILURE, channel_->GetState(false));
   // Send EDS update with 1 backend.