From 7e14a322a24b01e1bef77dca94156233494c734a Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Tue, 20 Jun 2023 08:41:36 -0700 Subject: [PATCH] [lb pick_first] Enable random shuffling of address list (#33254) Implementation of [gRFC A62](https://github.com/grpc/proposal/blob/master/A62-pick-first.md) --- CMakeLists.txt | 10 +- build_autogenerated.yaml | 3 + gRPC-C++.podspec | 1 + gRPC-Core.podspec | 1 + grpc.gyp | 2 + src/core/BUILD | 6 + .../lb_policy/pick_first/pick_first.cc | 58 ++++++- test/core/client_channel/lb_policy/BUILD | 1 + .../lb_policy/lb_policy_test_lib.h | 2 +- .../lb_policy/pick_first_test.cc | 164 +++++++++++++++++- .../lb_policy/round_robin_test.cc | 16 +- .../lb_policy/xds_override_host_test.cc | 5 +- test/core/util/test_lb_policies.cc | 8 + 13 files changed, 251 insertions(+), 26 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ccedc14a34..e89aac8edf0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2478,6 +2478,7 @@ target_link_libraries(grpc ${_gRPC_RE2_LIBRARIES} ${_gRPC_UPB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} + absl::algorithm_container absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -3122,6 +3123,7 @@ target_link_libraries(grpc_unsecure ${_gRPC_RE2_LIBRARIES} ${_gRPC_UPB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} + absl::algorithm_container absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -30869,7 +30871,7 @@ generate_pkgconfig( "gRPC" "high performance general RPC framework" "${gRPC_CORE_VERSION}" - "gpr absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "gpr absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "openssl re2 libcares zlib" "-lgrpc" "-laddress_sorting -lupb" @@ -30880,7 +30882,7 @@ generate_pkgconfig( "gRPC unsecure" "high performance general RPC framework without SSL" "${gRPC_CORE_VERSION}" - "gpr absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "gpr absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "re2 libcares zlib" "-lgrpc_unsecure" "-laddress_sorting -lupb" @@ -30891,7 +30893,7 @@ generate_pkgconfig( "gRPC++" "C++ wrapper for gRPC" "${gRPC_CPP_VERSION}" - "grpc absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "grpc absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "" "-lgrpc++" "" @@ -30902,7 +30904,7 @@ generate_pkgconfig( "gRPC++ unsecure" "C++ wrapper for gRPC without SSL" "${gRPC_CPP_VERSION}" - "grpc_unsecure absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "grpc_unsecure absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "" "-lgrpc++_unsecure" "" diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 1a144d20a9f..9b9f77622ff 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1781,6 +1781,7 @@ libs: - src/core/tsi/transport_security.cc - src/core/tsi/transport_security_grpc.cc deps: + - absl/algorithm:container - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -2715,6 +2716,7 @@ libs: - src/core/tsi/transport_security.cc - src/core/tsi/transport_security_grpc.cc deps: + - absl/algorithm:container - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -11252,6 +11254,7 @@ targets: headers: - test/core/client_channel/lb_policy/lb_policy_test_lib.h - test/core/event_engine/mock_event_engine.h + - test/core/util/scoped_env_var.h src: - test/core/client_channel/lb_policy/pick_first_test.cc deps: diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 4f8d3a4a6e9..918cacc9687 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -216,6 +216,7 @@ Pod::Spec.new do |s| ss.dependency "#{s.name}/Interface", version ss.dependency 'gRPC-Core', version abseil_version = '1.20230125.3' + ss.dependency 'abseil/algorithm/container', abseil_version ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version ss.dependency 'abseil/cleanup/cleanup', abseil_version diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index dc417abcc66..760ddacbdd4 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -183,6 +183,7 @@ Pod::Spec.new do |s| ss.libraries = 'z' ss.dependency "#{s.name}/Interface", version ss.dependency 'BoringSSL-GRPC', '0.0.29' + ss.dependency 'abseil/algorithm/container', abseil_version ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version ss.dependency 'abseil/cleanup/cleanup', abseil_version diff --git a/grpc.gyp b/grpc.gyp index 81657590cc5..d5245c52b39 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -249,6 +249,7 @@ 'target_name': 'grpc', 'type': 'static_library', 'dependencies': [ + 'absl/algorithm:container', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', @@ -1079,6 +1080,7 @@ 'target_name': 'grpc_unsecure', 'type': 'static_library', 'dependencies': [ + 'absl/algorithm:container', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', diff --git a/src/core/BUILD b/src/core/BUILD index 14859be4522..4babefde0a9 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -4625,6 +4625,8 @@ grpc_cc_library( "ext/filters/client_channel/lb_policy/pick_first/pick_first.cc", ], external_deps = [ + "absl/algorithm:container", + "absl/random", "absl/status", "absl/status:statusor", "absl/strings", @@ -4633,12 +4635,16 @@ grpc_cc_library( language = "c++", deps = [ "channel_args", + "env", "grpc_lb_subchannel_list", "grpc_outlier_detection_header", "json", + "json_args", + "json_object_loader", "lb_policy", "lb_policy_factory", "subchannel_interface", + "validation_errors", "//:config", "//:debug_location", "//:gpr", diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index d3c3766731d..8cc2b158e73 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -25,6 +25,8 @@ #include #include +#include "absl/algorithm/container.h" +#include "absl/random/random.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" @@ -40,11 +42,16 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/trace.h" +#include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/env.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/gprpp/work_serializer.h" #include "src/core/lib/json/json.h" +#include "src/core/lib/json/json_args.h" +#include "src/core/lib/json/json_object_loader.h" #include "src/core/lib/load_balancing/lb_policy.h" #include "src/core/lib/load_balancing/lb_policy_factory.h" #include "src/core/lib/load_balancing/subchannel_interface.h" @@ -63,6 +70,40 @@ namespace { constexpr absl::string_view kPickFirst = "pick_first"; +// TODO(eostroukhov): Remove once this feature is no longer experimental. +bool ShufflePickFirstEnabled() { + auto value = GetEnv("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); + if (!value.has_value()) return false; + bool parsed_value; + bool parse_succeeded = gpr_parse_bool_value(value->c_str(), &parsed_value); + return parse_succeeded && parsed_value; +} + +class PickFirstConfig : public LoadBalancingPolicy::Config { + public: + absl::string_view name() const override { return kPickFirst; } + bool shuffle_addresses() const { return shuffle_addresses_; } + + static const JsonLoaderInterface* JsonLoader(const JsonArgs&) { + static const auto kJsonLoader = + JsonObjectLoader() + .OptionalField("shuffleAddressList", + &PickFirstConfig::shuffle_addresses_) + .Finish(); + return kJsonLoader; + } + + void JsonPostLoad(const Json& /* json */, const JsonArgs& /* args */, + ValidationErrors* /* errors */) { + if (!ShufflePickFirstEnabled()) { + shuffle_addresses_ = false; + } + } + + private: + bool shuffle_addresses_ = false; +}; + class PickFirst : public LoadBalancingPolicy { public: explicit PickFirst(Args args); @@ -169,6 +210,8 @@ class PickFirst : public LoadBalancingPolicy { bool idle_ = false; // Are we shut down? bool shutdown_ = false; + // Random bit generator used for shuffling addresses if configured + absl::BitGen bit_gen_; }; PickFirst::PickFirst(Args args) : LoadBalancingPolicy(std::move(args)) { @@ -274,6 +317,11 @@ absl::Status PickFirst::UpdateLocked(UpdateArgs args) { status = args.addresses.status(); } else if (args.addresses->empty()) { status = absl::UnavailableError("address list must not be empty"); + } else { + auto config = static_cast(args.config.get()); + if (config->shuffle_addresses()) { + absl::c_shuffle(*args.addresses, bit_gen_); + } } // TODO(roth): This is a hack to disable outlier_detection when used // with pick_first, for the reasons described in @@ -513,11 +561,6 @@ void PickFirst::PickFirstSubchannelData::ProcessUnselectedReadyLocked() { } } -class PickFirstConfig : public LoadBalancingPolicy::Config { - public: - absl::string_view name() const override { return kPickFirst; } -}; - // // factory // @@ -532,8 +575,9 @@ class PickFirstFactory : public LoadBalancingPolicyFactory { absl::string_view name() const override { return kPickFirst; } absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { - return MakeRefCounted(); + ParseLoadBalancingConfig(const Json& json) const override { + return LoadFromJson>( + json, JsonArgs(), "errors validating pick_first LB policy config"); } }; diff --git a/test/core/client_channel/lb_policy/BUILD b/test/core/client_channel/lb_policy/BUILD index 5ed380f4579..cb69061d29f 100644 --- a/test/core/client_channel/lb_policy/BUILD +++ b/test/core/client_channel/lb_policy/BUILD @@ -50,6 +50,7 @@ grpc_cc_test( "//src/core:channel_args", "//src/core:grpc_lb_policy_pick_first", "//test/core/util:grpc_test_util", + "//test/core/util:scoped_env_var", ], ) 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 95af80f806d..70c4b1bbe34 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 @@ -598,7 +598,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { // Constructs an update containing a list of addresses. LoadBalancingPolicy::UpdateArgs BuildUpdate( absl::Span addresses, - RefCountedPtr config = nullptr) { + RefCountedPtr config) { LoadBalancingPolicy::UpdateArgs update; update.addresses.emplace(); for (const absl::string_view& address : addresses) { diff --git a/test/core/client_channel/lb_policy/pick_first_test.cc b/test/core/client_channel/lb_policy/pick_first_test.cc index 98025614048..46d833d99c2 100644 --- a/test/core/client_channel/lb_policy/pick_first_test.cc +++ b/test/core/client_channel/lb_policy/pick_first_test.cc @@ -16,19 +16,32 @@ #include +#include #include +#include +#include +#include +#include #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" +#include "absl/types/span.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" #include +#include #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/work_serializer.h" +#include "src/core/lib/json/json.h" #include "src/core/lib/load_balancing/lb_policy.h" #include "test/core/client_channel/lb_policy/lb_policy_test_lib.h" +#include "test/core/util/scoped_env_var.h" #include "test/core/util/test_config.h" namespace grpc_core { @@ -39,6 +52,80 @@ class PickFirstTest : public LoadBalancingPolicyTest { protected: PickFirstTest() : lb_policy_(MakeLbPolicy("pick_first")) {} + static RefCountedPtr MakePickFirstConfig( + absl::optional shuffle_address_list = absl::nullopt) { + return MakeConfig(Json::FromArray({Json::FromObject( + {{"pick_first", + shuffle_address_list.has_value() + ? Json::FromObject({{"shuffleAddressList", + Json::FromBool(*shuffle_address_list)}}) + : Json::FromObject({})}})})); + } + + // Gets order the addresses are being picked. Return type is void so + // assertions can be used + void GetOrderAddressesArePicked( + absl::Span addresses, + std::vector* out_address_order) { + work_serializer_->Run([&]() { lb_policy_->ExitIdleLocked(); }, + DEBUG_LOCATION); + out_address_order->clear(); + // Construct a map of subchannel to address. + // We will remove entries as each subchannel starts to connect. + std::map subchannels; + for (auto address : addresses) { + auto* subchannel = FindSubchannel( + address, ChannelArgs().Set(GRPC_ARG_INHIBIT_HEALTH_CHECKING, true)); + ASSERT_NE(subchannel, nullptr); + subchannels.emplace(subchannel, address); + } + // Now process each subchannel in the order in which pick_first tries it. + while (!subchannels.empty()) { + // Find the subchannel that is being attempted. + SubchannelState* subchannel = nullptr; + for (const auto& p : subchannels) { + if (p.first->ConnectionRequested()) { + out_address_order->push_back(p.second); + subchannel = p.first; + break; + } + } + ASSERT_NE(subchannel, nullptr); + // The subchannel reports CONNECTING. + subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING); + // If this is the first subchannel being attempted, expect a CONNECTING + // update. + if (subchannels.size() == addresses.size()) { + ExpectConnectingUpdate(); + } + if (subchannels.size() > 1) { + // Not the last subchannel in the list. Connection attempt should fail. + subchannel->SetConnectivityState( + GRPC_CHANNEL_TRANSIENT_FAILURE, + absl::UnavailableError("failed to connect")); + subchannel->SetConnectivityState(GRPC_CHANNEL_IDLE); + } else { + // Last subchannel in the list. Connection attempt should succeed. + subchannel->SetConnectivityState(GRPC_CHANNEL_READY); + auto picker = WaitForConnected(); + ASSERT_NE(picker, nullptr); + EXPECT_EQ(ExpectPickComplete(picker.get()), out_address_order->back()); + // Then it should become disconnected. + subchannel->SetConnectivityState(GRPC_CHANNEL_IDLE); + ExpectReresolutionRequest(); + // We would normally call ExpectStateAndQueueingPicker() here instead of + // just ExpectState(). However, calling the picker would also trigger + // exiting IDLE, which we don't want here, because if the test is going + // to send an address list update and call GetOrderAddressesArePicked() + // again, we don't want to trigger a connection attempt on any + // subchannel until after that next address list update is processed. + ExpectState(GRPC_CHANNEL_IDLE); + } + // Remove the subchannel from the map. + subchannels.erase(subchannel); + } + } + OrphanablePtr lb_policy_; }; @@ -46,7 +133,8 @@ TEST_F(PickFirstTest, FirstAddressWorks) { // Send an update containing two addresses. constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444"}; - absl::Status status = ApplyUpdate(BuildUpdate(kAddresses), lb_policy_.get()); + absl::Status status = ApplyUpdate( + BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); EXPECT_TRUE(status.ok()) << status; // LB policy should have created a subchannel for both addresses with // the GRPC_ARG_INHIBIT_HEALTH_CHECKING channel arg. @@ -81,7 +169,8 @@ TEST_F(PickFirstTest, FirstAddressFails) { // Send an update containing two addresses. constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444"}; - absl::Status status = ApplyUpdate(BuildUpdate(kAddresses), lb_policy_.get()); + absl::Status status = ApplyUpdate( + BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); EXPECT_TRUE(status.ok()) << status; // LB policy should have created a subchannel for both addresses with // the GRPC_ARG_INHIBIT_HEALTH_CHECKING channel arg. @@ -94,7 +183,8 @@ TEST_F(PickFirstTest, FirstAddressFails) { // When the LB policy receives the first subchannel's initial connectivity // state notification (IDLE), it will request a connection. EXPECT_TRUE(subchannel->ConnectionRequested()); - // This causes the subchannel to start to connect, so it reports CONNECTING. + // This causes the subchannel to start to connect, so it reports + // CONNECTING. subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING); // LB policy should have reported CONNECTING state. ExpectConnectingUpdate(); @@ -105,7 +195,8 @@ TEST_F(PickFirstTest, FirstAddressFails) { absl::UnavailableError("failed to connect")); // The LB policy will start a connection attempt on the second subchannel. EXPECT_TRUE(subchannel2->ConnectionRequested()); - // This causes the subchannel to start to connect, so it reports CONNECTING. + // This causes the subchannel to start to connect, so it reports + // CONNECTING. subchannel2->SetConnectivityState(GRPC_CHANNEL_CONNECTING); // The connection attempt succeeds. subchannel2->SetConnectivityState(GRPC_CHANNEL_READY); @@ -123,7 +214,8 @@ TEST_F(PickFirstTest, GoesIdleWhenConnectionFailsThenCanReconnect) { // Send an update containing two addresses. constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444"}; - absl::Status status = ApplyUpdate(BuildUpdate(kAddresses), lb_policy_.get()); + absl::Status status = ApplyUpdate( + BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); EXPECT_TRUE(status.ok()) << status; // LB policy should have created a subchannel for both addresses with // the GRPC_ARG_INHIBIT_HEALTH_CHECKING channel arg. @@ -177,6 +269,68 @@ TEST_F(PickFirstTest, GoesIdleWhenConnectionFailsThenCanReconnect) { } } +TEST_F(PickFirstTest, WithShuffle) { + testing::ScopedExperimentalEnvVar env_var( + "GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); + constexpr std::array kAddresses = { + "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444", "ipv4:127.0.0.1:445", + "ipv4:127.0.0.1:446", "ipv4:127.0.0.1:447", "ipv4:127.0.0.1:448"}; + // 6 addresses have 6! = 720 permutations or roughly 0.14% chance that + // the shuffle returns same permutation. We allow for several tries to + // prevent flake test. + constexpr size_t kMaxTries = 10; + std::vector addresses_after_update; + bool shuffled = false; + for (size_t i = 0; i < kMaxTries; ++i) { + absl::Status status = ApplyUpdate( + BuildUpdate(kAddresses, MakePickFirstConfig(true)), lb_policy_.get()); + EXPECT_TRUE(status.ok()) << status; + GetOrderAddressesArePicked(kAddresses, &addresses_after_update); + if (absl::MakeConstSpan(addresses_after_update) != + absl::MakeConstSpan(kAddresses)) { + shuffled = true; + break; + } + } + ASSERT_TRUE(shuffled); + // Address order should be stable between updates + std::vector addresses_on_another_try; + GetOrderAddressesArePicked(kAddresses, &addresses_on_another_try); + EXPECT_EQ(addresses_on_another_try, addresses_after_update); +} + +TEST_F(PickFirstTest, ShufflingDisabled) { + testing::ScopedExperimentalEnvVar env_var( + "GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); + constexpr std::array kAddresses = { + "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444", "ipv4:127.0.0.1:445", + "ipv4:127.0.0.1:446", "ipv4:127.0.0.1:447", "ipv4:127.0.0.1:448"}; + constexpr static size_t kMaxAttempts = 5; + for (size_t attempt = 0; attempt < kMaxAttempts; ++attempt) { + absl::Status status = ApplyUpdate( + BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); + EXPECT_TRUE(status.ok()) << status; + std::vector address_order; + GetOrderAddressesArePicked(kAddresses, &address_order); + EXPECT_THAT(address_order, ::testing::ElementsAreArray(kAddresses)); + } +} + +// TODO(eugeneo): remove when the env var no longer necessary +TEST_F(PickFirstTest, ShufflingDisabledViaEnvVar) { + constexpr std::array kAddresses = { + "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444", "ipv4:127.0.0.1:445", + "ipv4:127.0.0.1:446", "ipv4:127.0.0.1:447", "ipv4:127.0.0.1:448"}; + constexpr static size_t kMaxAttempts = 5; + for (size_t attempt = 0; attempt < kMaxAttempts; ++attempt) { + absl::Status status = ApplyUpdate( + BuildUpdate(kAddresses, MakePickFirstConfig(true)), lb_policy_.get()); + EXPECT_TRUE(status.ok()) << status; + std::vector address_order; + GetOrderAddressesArePicked(kAddresses, &address_order); + EXPECT_THAT(address_order, ::testing::ElementsAreArray(kAddresses)); + } +} } // namespace } // namespace testing } // namespace grpc_core diff --git a/test/core/client_channel/lb_policy/round_robin_test.cc b/test/core/client_channel/lb_policy/round_robin_test.cc index 72d720722be..ef82ceadebf 100644 --- a/test/core/client_channel/lb_policy/round_robin_test.cc +++ b/test/core/client_channel/lb_policy/round_robin_test.cc @@ -40,7 +40,7 @@ class RoundRobinTest : public LoadBalancingPolicyTest { RoundRobinTest() : lb_policy_(MakeLbPolicy("round_robin")) {} void ExpectStartup(absl::Span addresses) { - EXPECT_EQ(ApplyUpdate(BuildUpdate(addresses), lb_policy_.get()), + EXPECT_EQ(ApplyUpdate(BuildUpdate(addresses, nullptr), lb_policy_.get()), absl::OkStatus()); // Expect the initial CONNECTNG update with a picker that queues. ExpectConnectingUpdate(); @@ -90,14 +90,16 @@ TEST_F(RoundRobinTest, AddressUpdates) { "ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442", "ipv4:127.0.0.1:443"}; ExpectStartup(kAddresses); // Send update to remove address 2. - EXPECT_EQ(ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).first(2)), - lb_policy_.get()), - absl::OkStatus()); + EXPECT_EQ( + ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).first(2), nullptr), + lb_policy_.get()), + absl::OkStatus()); WaitForRoundRobinListChange(kAddresses, absl::MakeSpan(kAddresses).first(2)); // Send update to remove address 0 and re-add address 2. - EXPECT_EQ(ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).last(2)), - lb_policy_.get()), - absl::OkStatus()); + EXPECT_EQ( + ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).last(2), nullptr), + lb_policy_.get()), + absl::OkStatus()); WaitForRoundRobinListChange(absl::MakeSpan(kAddresses).first(2), absl::MakeSpan(kAddresses).last(2)); } 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 c25d957e4b3..541682aff6d 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 @@ -119,8 +119,9 @@ TEST_F(XdsOverrideHostTest, DelegatesToChild) { TEST_F(XdsOverrideHostTest, NoConfigReportsError) { EXPECT_EQ( - ApplyUpdate(BuildUpdate({"ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442"}), - policy_.get()), + ApplyUpdate( + BuildUpdate({"ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442"}, nullptr), + policy_.get()), absl::InvalidArgumentError("Missing policy config")); } diff --git a/test/core/util/test_lb_policies.cc b/test/core/util/test_lb_policies.cc index e24dde223d5..543cd4f5ddc 100644 --- a/test/core/util/test_lb_policies.cc +++ b/test/core/util/test_lb_policies.cc @@ -27,6 +27,7 @@ #include "absl/types/variant.h" #include +#include #include #include "src/core/ext/filters/client_channel/lb_policy/oob_backend_metric.h" @@ -78,6 +79,13 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy { ~ForwardingLoadBalancingPolicy() override = default; absl::Status UpdateLocked(UpdateArgs args) override { + // Use correct config for the delegate load balancing policy + auto config = + CoreConfiguration::Get().lb_policy_registry().ParseLoadBalancingConfig( + Json::FromArray({Json::FromObject( + {{std::string(delegate_->name()), Json::FromObject({})}})})); + GPR_ASSERT(config.ok()); + args.config = *config; return delegate_->UpdateLocked(std::move(args)); }