From 42a25af87106e970a8ec160f35511f092ff12f3c Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Thu, 26 Jan 2023 16:23:08 -0800 Subject: [PATCH] xds_override_host LB: make subchannel keys consistent with the filter (#32224) --- .../lb_policy/xds/xds_override_host.cc | 4 +- .../lb_policy/lb_policy_test_lib.h | 20 ++++--- .../lb_policy/xds_override_host_test.cc | 56 +++++++++---------- 3 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc index c3b5f09293a..cdd68c3e268 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc @@ -519,7 +519,7 @@ absl::StatusOr XdsOverrideHostLb::UpdateAddressMap( // Skip draining hosts if not in the override status set. continue; } - auto key = grpc_sockaddr_to_string(&address.address(), false); + auto key = grpc_sockaddr_to_uri(&address.address()); if (key.ok()) { addresses_for_map.emplace(std::move(*key), status); } @@ -550,7 +550,7 @@ absl::StatusOr XdsOverrideHostLb::UpdateAddressMap( RefCountedPtr XdsOverrideHostLb::AdoptSubchannel( ServerAddress address, RefCountedPtr subchannel) { - auto key = grpc_sockaddr_to_string(&address.address(), false); + auto key = grpc_sockaddr_to_uri(&address.address()); if (!key.ok()) { return subchannel; } diff --git a/test/core/client_channel/lb_policy/lb_policy_test_lib.h b/test/core/client_channel/lb_policy/lb_policy_test_lib.h index cff0a04d27d..96bd57c2056 100644 --- a/test/core/client_channel/lb_policy/lb_policy_test_lib.h +++ b/test/core/client_channel/lb_policy/lb_policy_test_lib.h @@ -463,8 +463,12 @@ class LoadBalancingPolicyTest : public ::testing::Test { // A fake CallState implementation, for use in PickArgs. class FakeCallState : public LbCallStateInternal { public: - explicit FakeCallState(std::map attributes) - : attributes_(attributes) {} + explicit FakeCallState( + const std::map& attributes) { + for (const auto& p : attributes) { + attributes_.emplace(p.first, std::string(p.second)); + } + } ~FakeCallState() override { for (void* allocation : allocations_) { @@ -697,7 +701,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { WaitForRoundRobinListChange( absl::Span old_addresses, absl::Span new_addresses, - const std::map& call_attributes = {}, + const std::map& call_attributes = {}, size_t num_iterations = 3, SourceLocation location = SourceLocation()) { gpr_log(GPR_INFO, "Waiting for expected RR addresses..."); RefCountedPtr retval; @@ -757,7 +761,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { // Does a pick and returns the result. LoadBalancingPolicy::PickResult DoPick( LoadBalancingPolicy::SubchannelPicker* picker, - const std::map& call_attributes = {}) { + const std::map& call_attributes = {}) { ExecCtx exec_ctx; FakeMetadata metadata({}); FakeCallState call_state(call_attributes); @@ -767,7 +771,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { // Requests a pick on picker and expects a Queue result. void ExpectPickQueued( LoadBalancingPolicy::SubchannelPicker* picker, - const std::map& call_attributes = {}, + const std::map& call_attributes = {}, SourceLocation location = SourceLocation()) { ASSERT_NE(picker, nullptr); auto pick_result = DoPick(picker, call_attributes); @@ -786,7 +790,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { // automatically to represent a complete call with no backend metric data. absl::optional ExpectPickComplete( LoadBalancingPolicy::SubchannelPicker* picker, - const std::map& call_attributes = {}, + const std::map& call_attributes = {}, std::unique_ptr* subchannel_call_tracker = nullptr, SourceLocation location = SourceLocation()) { @@ -822,7 +826,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { // list of addresses, or nullopt if a non-complete pick was returned. absl::optional> GetCompletePicks( LoadBalancingPolicy::SubchannelPicker* picker, size_t num_picks, - const std::map& call_attributes = {}, + const std::map& call_attributes = {}, std::vector< std::unique_ptr>* subchannel_call_trackers = nullptr, @@ -872,7 +876,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { void ExpectRoundRobinPicks( LoadBalancingPolicy::SubchannelPicker* picker, absl::Span addresses, - const std::map& call_attributes = {}, + const std::map& call_attributes = {}, size_t num_iterations = 3, SourceLocation location = SourceLocation()) { auto picks = GetCompletePicks(picker, num_iterations * addresses.size(), call_attributes, nullptr, location); diff --git a/test/core/client_channel/lb_policy/xds_override_host_test.cc b/test/core/client_channel/lb_policy/xds_override_host_test.cc index 888d1a2d664..a3d001299d6 100644 --- a/test/core/client_channel/lb_policy/xds_override_host_test.cc +++ b/test/core/client_channel/lb_policy/xds_override_host_test.cc @@ -134,11 +134,11 @@ TEST_F(XdsOverrideHostTest, OverrideHost) { auto picker = ExpectStartupWithRoundRobin(kAddresses); ASSERT_NE(picker, nullptr); // Check that the host is overridden - std::map call_attributes{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map call_attributes{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[1]); EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[1]); - call_attributes[XdsOverrideHostTypeName()] = std::string("127.0.0.1:441"); + call_attributes[XdsOverrideHostTypeName()] = kAddresses[0]; EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[0]); EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[0]); } @@ -149,7 +149,7 @@ TEST_F(XdsOverrideHostTest, SubchannelNotFound) { "ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442", "ipv4:127.0.0.1:443"}; auto picker = ExpectStartupWithRoundRobin(kAddresses); ASSERT_NE(picker, nullptr); - std::map call_attributes{ + std::map call_attributes{ {XdsOverrideHostTypeName(), "no such host"}}; ExpectRoundRobinPicks(picker.get(), kAddresses, call_attributes); } @@ -160,8 +160,8 @@ TEST_F(XdsOverrideHostTest, SubchannelsComeAndGo) { auto picker = ExpectStartupWithRoundRobin(kAddresses); ASSERT_NE(picker, nullptr); // Check that the host is overridden - std::map call_attributes{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map call_attributes{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; ExpectRoundRobinPicks(picker.get(), {kAddresses[1]}, call_attributes); // Some other address is gone EXPECT_EQ(ApplyUpdate(BuildUpdate({kAddresses[0], kAddresses[1]}, @@ -206,8 +206,8 @@ TEST_F(XdsOverrideHostTest, FailedSubchannelIsNotPicked) { auto picker = ExpectStartupWithRoundRobin(kAddresses); ASSERT_NE(picker, nullptr); // Check that the host is overridden - std::map pick_arg{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map pick_arg{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; EXPECT_EQ(ExpectPickComplete(picker.get(), pick_arg), kAddresses[1]); auto subchannel = FindSubchannel(kAddresses[1]); ASSERT_NE(subchannel, nullptr); @@ -232,8 +232,8 @@ TEST_F(XdsOverrideHostTest, ConnectingSubchannelIsQueued) { auto picker = ExpectStartupWithRoundRobin(kAddresses); ASSERT_NE(picker, nullptr); // Check that the host is overridden - std::map pick_arg{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map pick_arg{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; EXPECT_EQ(ExpectPickComplete(picker.get(), pick_arg), kAddresses[1]); auto subchannel = FindSubchannel(kAddresses[1]); ASSERT_NE(subchannel, nullptr); @@ -262,8 +262,8 @@ TEST_F(XdsOverrideHostTest, DrainingState) { ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[2]}); ExpectQueueEmpty(); // Draining subchannel is returned - std::map pick_arg{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map pick_arg{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; EXPECT_EQ(ExpectPickComplete(picker.get(), pick_arg), kAddresses[1]); ApplyUpdateWithHealthStatuses( {{kAddresses[0], XdsHealthStatus::HealthStatus::kUnknown}, @@ -281,8 +281,8 @@ TEST_F(XdsOverrideHostTest, DrainingSubchannelIsConnecting) { auto picker = ExpectStartupWithRoundRobin(kAddresses); ASSERT_NE(picker, nullptr); // Check that the host is overridden - std::map pick_arg{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map pick_arg{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; EXPECT_EQ(ExpectPickComplete(picker.get(), pick_arg), kAddresses[1]); ApplyUpdateWithHealthStatuses( {{kAddresses[0], XdsHealthStatus::HealthStatus::kUnknown}, @@ -326,8 +326,8 @@ TEST_F(XdsOverrideHostTest, DrainingToHealthy) { ASSERT_NE(picker, nullptr); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[2]}); ExpectQueueEmpty(); - std::map pick_arg{ - {XdsOverrideHostTypeName(), "127.0.0.1:442"}}; + std::map pick_arg{ + {XdsOverrideHostTypeName(), kAddresses[1]}}; EXPECT_EQ(ExpectPickComplete(picker.get(), pick_arg), kAddresses[1]); ApplyUpdateWithHealthStatuses( {{kAddresses[0], XdsHealthStatus::HealthStatus::kHealthy}, @@ -353,13 +353,13 @@ TEST_F(XdsOverrideHostTest, OverrideHostStatus) { ASSERT_NE(picker, nullptr); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:441"}}), + {{XdsOverrideHostTypeName(), kAddresses[0]}}), kAddresses[0]); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:442"}}), + {{XdsOverrideHostTypeName(), kAddresses[1]}}), kAddresses[1]); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:443"}}), + {{XdsOverrideHostTypeName(), kAddresses[2]}}), kAddresses[2]); // UNKNOWN excluded - first chanel does not get overridden ApplyUpdateWithHealthStatuses( @@ -371,12 +371,12 @@ TEST_F(XdsOverrideHostTest, OverrideHostStatus) { ASSERT_NE(picker, nullptr); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}, - {{XdsOverrideHostTypeName(), "127.0.0.1:441"}}); + {{XdsOverrideHostTypeName(), kAddresses[0]}}); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:442"}}), + {{XdsOverrideHostTypeName(), kAddresses[1]}}), kAddresses[1]); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:443"}}), + {{XdsOverrideHostTypeName(), kAddresses[2]}}), kAddresses[2]); // HEALTHY excluded - second chanel does not get overridden ApplyUpdateWithHealthStatuses( @@ -388,13 +388,13 @@ TEST_F(XdsOverrideHostTest, OverrideHostStatus) { ASSERT_NE(picker, nullptr); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:441"}}), + {{XdsOverrideHostTypeName(), kAddresses[0]}}), kAddresses[0]); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:442"}}), + {{XdsOverrideHostTypeName(), kAddresses[1]}}), kAddresses[1]); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}, - {{XdsOverrideHostTypeName(), "127.0.0.1:443"}}); + {{XdsOverrideHostTypeName(), kAddresses[2]}}); // DRAINING excluded - third chanel does not get overridden ApplyUpdateWithHealthStatuses( {{kAddresses[0], XdsHealthStatus::HealthStatus::kUnknown}, @@ -405,13 +405,13 @@ TEST_F(XdsOverrideHostTest, OverrideHostStatus) { ASSERT_NE(picker, nullptr); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:441"}}), + {{XdsOverrideHostTypeName(), kAddresses[0]}}), kAddresses[0]); EXPECT_EQ(ExpectPickComplete(picker.get(), - {{XdsOverrideHostTypeName(), "127.0.0.1:442"}}), + {{XdsOverrideHostTypeName(), kAddresses[1]}}), kAddresses[1]); ExpectRoundRobinPicks(picker.get(), {kAddresses[0], kAddresses[1]}, - {{XdsOverrideHostTypeName(), "127.0.0.1:443"}}); + {{XdsOverrideHostTypeName(), kAddresses[2]}}); } } // namespace