Merge pull request #22831 from markdroth/weighted_target_fix

weighted_target: Create all children before updating any of them.

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

@@ -278,19 +278,29 @@ void WeightedTargetLb::UpdateLocked(UpdateArgs args) {
       child->DeactivateLocked();
     }
   }
-  // Add or update the targets in the new config.
-  HierarchicalAddressMap address_map =
-      MakeHierarchicalAddressMap(args.addresses);
+  // Create any children that don't already exist.
+  // Note that we add all children before updating any of them, because
+  // an update may trigger a child to immediately update its
+  // connectivity state (e.g., reporting TRANSIENT_FAILURE immediately when
+  // receiving an empty address list), and we don't want to return an
+  // overall state with incomplete data.
   for (const auto& p : config_->target_map()) {
     const std::string& name = p.first;
-    const WeightedTargetLbConfig::ChildConfig& config = p.second;
     auto it = targets_.find(name);
     if (it == targets_.end()) {
       it = targets_.emplace(std::make_pair(name, nullptr)).first;
       it->second = MakeOrphanable<WeightedChild>(
           Ref(DEBUG_LOCATION, "WeightedChild"), it->first);
     }
-    it->second->UpdateLocked(config, std::move(address_map[name]), args.args);
+  }
+  // Update all children.
+  HierarchicalAddressMap address_map =
+      MakeHierarchicalAddressMap(args.addresses);
+  for (const auto& p : config_->target_map()) {
+    const std::string& name = p.first;
+    const WeightedTargetLbConfig::ChildConfig& config = p.second;
+    targets_[name]->UpdateLocked(config, std::move(address_map[name]),
+                                 args.args);
   }
 }
 

test/cpp/end2end/xds_end2end_test.cc

@@ -3196,6 +3196,22 @@ TEST_P(FailoverTest, DoesNotUsePriorityWithNoEndpoints) {
   }
 }
 
+// Does not choose locality with no endpoints.
+TEST_P(FailoverTest, DoesNotUseLocalityWithNoEndpoints) {
+  SetNextResolution({});
+  SetNextResolutionForLbChannelAllBalancers();
+  AdsServiceImpl::EdsResourceArgs args({
+      {"locality0", {}, kDefaultLocalityWeight, 0},
+      {"locality1", GetBackendPorts(), kDefaultLocalityWeight, 0},
+  });
+  balancers_[0]->ads_service()->SetEdsResource(
+      AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName);
+  // Wait for all backends to be used.
+  std::tuple<int, int, int> counts = WaitForAllBackends();
+  // Make sure no RPCs failed in the transition.
+  EXPECT_EQ(0, std::get<1>(counts));
+}
+
 // If the higher priority localities are not reachable, failover to the highest
 // priority among the rest.
 TEST_P(FailoverTest, Failover) {