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 7ce51aa9a06..b9fa5fa9d3a 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 @@ -273,6 +273,7 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { absl::optional latest_update; // State used to retain child policy names for priority policy. std::vector priority_child_numbers; + size_t next_available_child_number = 0; const XdsClusterResolverLbConfig::DiscoveryMechanism& config() const; @@ -666,10 +667,11 @@ void XdsClusterResolverLb::OnEndpointChanged(size_t index, } // If we didn't find an existing child number, assign a new one. if (!child_number.has_value()) { - for (child_number = 0; + for (child_number = discovery_entry.next_available_child_number; child_locality_map.find(*child_number) != child_locality_map.end(); ++(*child_number)) { } + discovery_entry.next_available_child_number = *child_number + 1; // Add entry so we know that the child number is in use. // (Don't need to add the list of localities, since we won't use them.) child_locality_map[*child_number]; diff --git a/test/cpp/end2end/xds/xds_end2end_test.cc b/test/cpp/end2end/xds/xds_end2end_test.cc index 83e2a997ad8..a462d793bf1 100644 --- a/test/cpp/end2end/xds/xds_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_end2end_test.cc @@ -12059,6 +12059,57 @@ TEST_P(FailoverTest, MoveAllLocalitiesInCurrentPriorityToHigherPriority) { EXPECT_TRUE(balancer_->ads_service()->eds_response_state().has_value()); } +// This tests a bug triggered by the xds_cluster_resolver policy reusing +// a child name for the priority policy when that child name was still +// present but deactivated. +TEST_P(FailoverTest, PriorityChildNameChurn) { + CreateAndStartBackends(4); + auto non_existant_endpoint = MakeNonExistantEndpoint(); + // Initial update: + // - P0:locality0, child number 0 (unreachable) + // - P1:locality1, child number 1 + // - P2:locality2, child number 2 + EdsResourceArgs args({ + {"locality0", {non_existant_endpoint}, kDefaultLocalityWeight, 0}, + {"locality1", CreateEndpointsForBackends(0, 1), kDefaultLocalityWeight, + 1}, + {"locality2", CreateEndpointsForBackends(1, 2), kDefaultLocalityWeight, + 2}, + }); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + WaitForBackend(0); + // Next update: + // - P0:locality0, child number 0 (still unreachable) + // - P1:locality2, child number 2 (moved from P2 to P1) + // - P2:locality3, child number 3 (new child) + // Child number 1 will be deactivated. + args = EdsResourceArgs({ + {"locality0", {non_existant_endpoint}, kDefaultLocalityWeight, 0}, + {"locality2", CreateEndpointsForBackends(1, 2), kDefaultLocalityWeight, + 1}, + {"locality3", CreateEndpointsForBackends(2, 3), kDefaultLocalityWeight, + 2}, + }); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + WaitForBackend(1); + // Next update: + // - P0:locality0, child number 0 (still unreachable) + // - P1:locality4, child number 4 (new child number -- should not reuse #1) + // - P2:locality3, child number 3 + // Child number 1 will be deactivated. + args = EdsResourceArgs({ + {"locality0", {non_existant_endpoint}, kDefaultLocalityWeight, 0}, + {"locality4", CreateEndpointsForBackends(3, 4), kDefaultLocalityWeight, + 1}, + {"locality3", CreateEndpointsForBackends(2, 3), kDefaultLocalityWeight, + 2}, + }); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + WaitForBackend(3, WaitForBackendOptions().set_reset_counters(false)); + // P2 should not have gotten any traffic in this change. + EXPECT_EQ(0UL, backends_[2]->backend_service()->request_count()); +} + using DropTest = XdsEnd2endTest; // Tests that RPCs are dropped according to the drop config.