diff --git a/BUILD b/BUILD index 11c6500271d..20b78124f7e 100644 --- a/BUILD +++ b/BUILD @@ -2647,7 +2647,6 @@ grpc_cc_library( "//src/core:ext/filters/client_channel/global_subchannel_pool.h", "//src/core:ext/filters/client_channel/health/health_check_client.h", "//src/core:ext/filters/client_channel/http_proxy.h", - "//src/core:ext/filters/client_channel/lb_call_state_internal.h", "//src/core:ext/filters/client_channel/lb_policy/child_policy_handler.h", "//src/core:ext/filters/client_channel/lb_policy/oob_backend_metric.h", "//src/core:ext/filters/client_channel/local_subchannel_pool.h", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 643154e0cf2..20e9490c9b2 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -335,7 +335,6 @@ libs: - src/core/ext/filters/client_channel/global_subchannel_pool.h - src/core/ext/filters/client_channel/health/health_check_client.h - src/core/ext/filters/client_channel/http_proxy.h - - src/core/ext/filters/client_channel/lb_call_state_internal.h - src/core/ext/filters/client_channel/lb_policy/address_filtering.h - src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h - src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h @@ -1936,7 +1935,6 @@ libs: - src/core/ext/filters/client_channel/global_subchannel_pool.h - src/core/ext/filters/client_channel/health/health_check_client.h - src/core/ext/filters/client_channel/http_proxy.h - - src/core/ext/filters/client_channel/lb_call_state_internal.h - src/core/ext/filters/client_channel/lb_policy/address_filtering.h - src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h - src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 8a7b315eaf5..ace5c9cafe1 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -250,7 +250,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/global_subchannel_pool.h', 'src/core/ext/filters/client_channel/health/health_check_client.h', 'src/core/ext/filters/client_channel/http_proxy.h', - 'src/core/ext/filters/client_channel/lb_call_state_internal.h', 'src/core/ext/filters/client_channel/lb_policy/address_filtering.h', 'src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h', 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h', @@ -1162,7 +1161,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/global_subchannel_pool.h', 'src/core/ext/filters/client_channel/health/health_check_client.h', 'src/core/ext/filters/client_channel/http_proxy.h', - 'src/core/ext/filters/client_channel/lb_call_state_internal.h', 'src/core/ext/filters/client_channel/lb_policy/address_filtering.h', 'src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h', 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 81adb310660..4a55c0e1cf9 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -234,7 +234,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/health/health_check_client.h', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/http_proxy.h', - 'src/core/ext/filters/client_channel/lb_call_state_internal.h', 'src/core/ext/filters/client_channel/lb_policy/address_filtering.cc', 'src/core/ext/filters/client_channel/lb_policy/address_filtering.h', 'src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h', @@ -1834,7 +1833,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/global_subchannel_pool.h', 'src/core/ext/filters/client_channel/health/health_check_client.h', 'src/core/ext/filters/client_channel/http_proxy.h', - 'src/core/ext/filters/client_channel/lb_call_state_internal.h', 'src/core/ext/filters/client_channel/lb_policy/address_filtering.h', 'src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h', 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h', diff --git a/grpc.gemspec b/grpc.gemspec index 092a81ae02f..4d12ccdbe37 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -145,7 +145,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/health/health_check_client.h ) s.files += %w( src/core/ext/filters/client_channel/http_proxy.cc ) s.files += %w( src/core/ext/filters/client_channel/http_proxy.h ) - s.files += %w( src/core/ext/filters/client_channel/lb_call_state_internal.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/address_filtering.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/address_filtering.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h ) diff --git a/package.xml b/package.xml index 52b63520658..9a58ea75f2f 100644 --- a/package.xml +++ b/package.xml @@ -127,7 +127,6 @@ - diff --git a/src/core/BUILD b/src/core/BUILD index f6c0c2b5536..93d6d09d28e 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -4455,8 +4455,6 @@ grpc_cc_library( language = "c++", deps = [ "channel_args", - "grpc_lb_subchannel_list", - "grpc_stateful_session_filter", "json", "json_args", "json_object_loader", @@ -4475,7 +4473,6 @@ grpc_cc_library( "//:orphanable", "//:ref_counted_ptr", "//:server_address", - "//:sockaddr_utils", ], ) diff --git a/src/core/ext/filters/client_channel/client_channel.h b/src/core/ext/filters/client_channel/client_channel.h index ae59143fd2b..b6afb30f651 100644 --- a/src/core/ext/filters/client_channel/client_channel.h +++ b/src/core/ext/filters/client_channel/client_channel.h @@ -40,7 +40,6 @@ #include "src/core/ext/filters/client_channel/client_channel_factory.h" #include "src/core/ext/filters/client_channel/config_selector.h" #include "src/core/ext/filters/client_channel/dynamic_filters.h" -#include "src/core/ext/filters/client_channel/lb_call_state_internal.h" #include "src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h" #include "src/core/ext/filters/client_channel/subchannel.h" #include "src/core/ext/filters/client_channel/subchannel_pool_interface.h" @@ -391,7 +390,7 @@ class ClientChannel { class ClientChannel::LoadBalancedCall : public InternallyRefCounted { public: - class LbCallState : public LbCallStateInternal { + class LbCallState : public LoadBalancingPolicy::CallState { public: explicit LbCallState(LoadBalancedCall* lb_call) : lb_call_(lb_call) {} @@ -399,7 +398,7 @@ class ClientChannel::LoadBalancedCall // Internal API to allow first-party LB policies to access per-call // attributes set by the ConfigSelector. - absl::string_view GetCallAttribute(UniqueTypeName type) override; + absl::string_view GetCallAttribute(UniqueTypeName type); private: LoadBalancedCall* lb_call_; diff --git a/src/core/ext/filters/client_channel/lb_call_state_internal.h b/src/core/ext/filters/client_channel/lb_call_state_internal.h deleted file mode 100644 index b5e300bdfa0..00000000000 --- a/src/core/ext/filters/client_channel/lb_call_state_internal.h +++ /dev/null @@ -1,37 +0,0 @@ -// -// Copyright 2015 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -#ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_CALL_STATE_INTERNAL_H -#define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_CALL_STATE_INTERNAL_H - -#include - -#include "src/core/lib/gprpp/unique_type_name.h" -#include "src/core/lib/load_balancing/lb_policy.h" - -namespace grpc_core { - -// -// LbCallStateInternal -// -class LbCallStateInternal : public LoadBalancingPolicy::CallState { - public: - virtual absl::string_view GetCallAttribute(UniqueTypeName type) = 0; -}; - -} // namespace grpc_core - -#endif // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_CALL_STATE_INTERNAL_H 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 72fab69ab2e..245b7b618ef 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 @@ -29,11 +29,7 @@ #include #include -#include "src/core/ext/filters/client_channel/lb_call_state_internal.h" #include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h" -#include "src/core/ext/filters/client_channel/lb_policy/subchannel_list.h" -#include "src/core/ext/filters/stateful_session/stateful_session_filter.h" -#include "src/core/lib/address_utils/sockaddr_utils.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/trace.h" @@ -105,13 +101,12 @@ class XdsOverrideHostLb : public LoadBalancingPolicy { // present. class Picker : public SubchannelPicker { public: - Picker(RefCountedPtr xds_override_host_lb, + Picker(XdsOverrideHostLb* xds_override_host_lb, RefCountedPtr picker); PickResult Pick(PickArgs args) override; private: - RefCountedPtr policy_; RefCountedPtr picker_; }; @@ -138,42 +133,6 @@ class XdsOverrideHostLb : public LoadBalancingPolicy { RefCountedPtr xds_override_host_policy_; }; - class SubchannelWrapper : public DelegatingSubchannel { - public: - SubchannelWrapper(RefCountedPtr subchannel, - RefCountedPtr policy, - absl::optional key); - - ~SubchannelWrapper() override; - - private: - const absl::optional key_; - RefCountedPtr policy_; - }; - - class SubchannelEntry { - public: - void SetSubchannel(SubchannelWrapper* subchannel) { - subchannel_ = subchannel; - } - - void ResetSubchannel(SubchannelWrapper* expected) { - if (subchannel_ == expected) { - subchannel_ = nullptr; - } - } - - RefCountedPtr GetSubchannel() { - if (subchannel_ == nullptr) { - return nullptr; - } - return subchannel_->Ref(); - } - - private: - SubchannelWrapper* subchannel_ = nullptr; - }; - ~XdsOverrideHostLb() override; void ShutdownLocked() override; @@ -183,15 +142,6 @@ class XdsOverrideHostLb : public LoadBalancingPolicy { void MaybeUpdatePickerLocked(); - RefCountedPtr LookupSubchannel(absl::string_view address); - - void UpdateAddressMap(const absl::StatusOr& addresses); - - RefCountedPtr AdoptSubchannel( - ServerAddress address, RefCountedPtr subchannel); - - void ResetSubchannel(absl::string_view key, SubchannelWrapper* subchannel); - // Current config from the resolver. RefCountedPtr config_; @@ -204,47 +154,29 @@ class XdsOverrideHostLb : public LoadBalancingPolicy { grpc_connectivity_state state_ = GRPC_CHANNEL_IDLE; absl::Status status_; RefCountedPtr picker_; - absl::Mutex subchannel_map_mu_; - std::map> subchannel_map_ - ABSL_GUARDED_BY(subchannel_map_mu_); }; // // XdsOverrideHostLb::Picker // -XdsOverrideHostLb::Picker::Picker( - RefCountedPtr xds_override_host_lb, - RefCountedPtr picker) - : policy_(std::move(xds_override_host_lb)), picker_(std::move(picker)) { +XdsOverrideHostLb::Picker::Picker(XdsOverrideHostLb* xds_override_host_lb, + RefCountedPtr picker) + : picker_(std::move(picker)) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_xds_override_host_trace)) { gpr_log(GPR_INFO, "[xds_override_host_lb %p] constructed new picker %p", - policy_.get(), this); + xds_override_host_lb, this); } } LoadBalancingPolicy::PickResult XdsOverrideHostLb::Picker::Pick( LoadBalancingPolicy::PickArgs args) { - auto* call_state = static_cast(args.call_state); - auto override_host = call_state->GetCallAttribute(XdsHostOverrideTypeName()); - if (!override_host.empty()) { - auto subchannel = policy_->LookupSubchannel(override_host); - if (subchannel != nullptr) { - return PickResult::Complete(subchannel->wrapped_subchannel()); - } - } if (picker_ == nullptr) { // Should never happen. return PickResult::Fail(absl::InternalError( "xds_override_host picker not given any child picker")); } - auto result = picker_->Pick(args); - auto complete_pick = absl::get_if(&result.result); - if (complete_pick != nullptr) { - complete_pick->subchannel = - static_cast(complete_pick->subchannel.get()) - ->wrapped_subchannel(); - } - return result; + // Delegate to child picker + return picker_->Pick(args); } // @@ -305,7 +237,6 @@ absl::Status XdsOverrideHostLb::UpdateLocked(UpdateArgs args) { if (child_policy_ == nullptr) { child_policy_ = CreateChildPolicyLocked(args.args); } - UpdateAddressMap(args.addresses); // Update child policy. UpdateArgs update_args; update_args.addresses = std::move(args.addresses); @@ -322,7 +253,7 @@ absl::Status XdsOverrideHostLb::UpdateLocked(UpdateArgs args) { void XdsOverrideHostLb::MaybeUpdatePickerLocked() { if (picker_ != nullptr) { - auto xds_override_host_picker = MakeRefCounted(Ref(), picker_); + auto xds_override_host_picker = MakeRefCounted(this, picker_); if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_xds_override_host_trace)) { gpr_log(GPR_INFO, "[xds_override_host_lb %p] updating connectivity: state=%s " @@ -358,87 +289,27 @@ OrphanablePtr XdsOverrideHostLb::CreateChildPolicyLocked( return lb_policy; } -RefCountedPtr -XdsOverrideHostLb::LookupSubchannel(absl::string_view address) { - absl::MutexLock lock(&subchannel_map_mu_); - auto it = subchannel_map_.find(address); - if (it != subchannel_map_.end()) { - return it->second.GetSubchannel(); - } - return nullptr; -} - -void XdsOverrideHostLb::UpdateAddressMap( - const absl::StatusOr& addresses) { - std::unordered_set keys(addresses->size()); - if (addresses.ok()) { - for (const auto& address : *addresses) { - auto key = grpc_sockaddr_to_string(&address.address(), false); - if (key.ok()) { - keys.insert(std::move(*key)); - } - } - } - absl::MutexLock lock(&subchannel_map_mu_); - for (auto it = subchannel_map_.begin(); it != subchannel_map_.end();) { - if (keys.find(it->first) == keys.end()) { - it = subchannel_map_.erase(it); - } else { - ++it; - } - } - for (const auto& key : keys) { - if (subchannel_map_.find(key) == subchannel_map_.end()) { - subchannel_map_.emplace(key, SubchannelEntry()); - } - } -} - -RefCountedPtr -XdsOverrideHostLb::AdoptSubchannel( - ServerAddress address, RefCountedPtr subchannel) { - auto subchannel_key = grpc_sockaddr_to_string(&address.address(), false); - absl::optional key; - if (subchannel_key.ok()) { - key = std::move(*subchannel_key); - } - auto wrapper = - MakeRefCounted(std::move(subchannel), Ref(), key); - if (key.has_value()) { - absl::MutexLock lock(&subchannel_map_mu_); - auto it = subchannel_map_.find(*key); - if (it != subchannel_map_.end()) { - it->second.SetSubchannel(wrapper.get()); - } - } - return wrapper; -} - -void XdsOverrideHostLb::ResetSubchannel(absl::string_view key, - SubchannelWrapper* subchannel) { - absl::MutexLock lock(&subchannel_map_mu_); - auto it = subchannel_map_.find(key); - if (it != subchannel_map_.end()) { - it->second.ResetSubchannel(subchannel); - } -} - // // XdsOverrideHostLb::Helper // RefCountedPtr XdsOverrideHostLb::Helper::CreateSubchannel( ServerAddress address, const ChannelArgs& args) { - auto subchannel = - xds_override_host_policy_->channel_control_helper()->CreateSubchannel( - address, args); - return xds_override_host_policy_->AdoptSubchannel(address, subchannel); + return xds_override_host_policy_->channel_control_helper()->CreateSubchannel( + address, args); } void XdsOverrideHostLb::Helper::UpdateState( grpc_connectivity_state state, const absl::Status& status, RefCountedPtr picker) { if (xds_override_host_policy_->shutting_down_) return; + if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_xds_override_host_trace)) { + gpr_log(GPR_INFO, + "[xds_override_host_lb %p] child connectivity state update: " + "state=%s (%s) picker=%p", + xds_override_host_policy_.get(), ConnectivityStateName(state), + status.ToString().c_str(), picker.get()); + } // Save the state and picker. xds_override_host_policy_->state_ = state; xds_override_host_policy_->status_ = status; @@ -468,24 +339,6 @@ void XdsOverrideHostLb::Helper::AddTraceEvent(TraceSeverity severity, message); } -// -// XdsOverrideHostLb::SubchannelWrapper::SubchannelWrapper -// - -XdsOverrideHostLb::SubchannelWrapper::SubchannelWrapper( - RefCountedPtr subchannel, - RefCountedPtr policy, - absl::optional key) - : DelegatingSubchannel(std::move(subchannel)), - key_(std::move(key)), - policy_(std::move(policy)) {} - -XdsOverrideHostLb::SubchannelWrapper::~SubchannelWrapper() { - if (key_.has_value()) { - policy_->ResetSubchannel(*key_, this); - } -} - // // factory // 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 9dca8743f06..3173013fa94 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 @@ -48,7 +48,6 @@ #include #include -#include "src/core/ext/filters/client_channel/lb_call_state_internal.h" #include "src/core/ext/filters/client_channel/subchannel_pool_interface.h" #include "src/core/lib/address_utils/parse_address.h" #include "src/core/lib/address_utils/sockaddr_utils.h" @@ -359,11 +358,8 @@ class LoadBalancingPolicyTest : public ::testing::Test { }; // A fake CallState implementation, for use in PickArgs. - class FakeCallState : public LbCallStateInternal { + class FakeCallState : public LoadBalancingPolicy::CallState { public: - explicit FakeCallState(std::map attributes) - : attributes_(attributes) {} - ~FakeCallState() override { for (void* allocation : allocations_) { gpr_free(allocation); @@ -377,12 +373,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { return allocation; } - absl::string_view GetCallAttribute(UniqueTypeName type) override { - return attributes_[type]; - } - std::vector allocations_; - std::map attributes_; }; LoadBalancingPolicyTest() @@ -558,17 +549,13 @@ class LoadBalancingPolicyTest : public ::testing::Test { // Waits for the round_robin policy to start using an updated address list. // There can be any number of READY updates where the picker is still using // the old list followed by one READY update where the picker is using the - // new list. Returns a picker if the reported states match expectations. - RefCountedPtr - WaitForRoundRobinListChange( + // new list. Returns true if the reported states match expectations. + bool WaitForRoundRobinListChange( absl::Span old_addresses, absl::Span new_addresses, - const std::map call_attributes = {}, size_t num_iterations = 3, SourceLocation location = SourceLocation()) { gpr_log(GPR_INFO, "Waiting for expected RR addresses..."); - RefCountedPtr retval; - size_t num_picks = - std::max(new_addresses.size(), old_addresses.size()) * num_iterations; + bool retval = false; WaitForStateUpdate( [&](FakeHelper::StateUpdate update) { EXPECT_EQ(update.state, GRPC_CHANNEL_READY) @@ -576,8 +563,8 @@ class LoadBalancingPolicyTest : public ::testing::Test { if (update.state != GRPC_CHANNEL_READY) return false; // Get enough picks to round-robin num_iterations times across all // expected addresses. - auto picks = GetCompletePicks(update.picker.get(), num_picks, - call_attributes, location); + auto picks = GetCompletePicks(update.picker.get(), + new_addresses.size() * num_iterations); EXPECT_TRUE(picks.has_value()) << location.file() << ":" << location.line(); if (!picks.has_value()) return false; @@ -585,14 +572,11 @@ class LoadBalancingPolicyTest : public ::testing::Test { // If the picks still match the old list, then keep going. if (PicksAreRoundRobin(old_addresses, *picks)) return true; // Otherwise, the picks should match the new list. - bool matches = PicksAreRoundRobin(new_addresses, *picks); - EXPECT_TRUE(matches) + retval = PicksAreRoundRobin(new_addresses, *picks); + EXPECT_TRUE(retval) << "Expected: " << absl::StrJoin(new_addresses, ", ") << "\nActual: " << absl::StrJoin(*picks, ", ") << "\nat " << location.file() << ":" << location.line(); - if (matches) { - retval = std::move(update.picker); - } return false; // Stop. }, location); @@ -615,18 +599,12 @@ class LoadBalancingPolicyTest : public ::testing::Test { location); } - static std::unique_ptr MakeMetadata( - std::map init = {}) { - return std::make_unique(init); - } - // Does a pick and returns the result. LoadBalancingPolicy::PickResult DoPick( - LoadBalancingPolicy::SubchannelPicker* picker, - const std::map& call_attributes = {}) { + LoadBalancingPolicy::SubchannelPicker* picker) { ExecCtx exec_ctx; FakeMetadata metadata({}); - FakeCallState call_state(call_attributes); + FakeCallState call_state; return picker->Pick({"/service/method", &metadata, &call_state}); } @@ -645,9 +623,8 @@ class LoadBalancingPolicyTest : public ::testing::Test { // the result was something other than Complete. absl::optional ExpectPickComplete( LoadBalancingPolicy::SubchannelPicker* picker, - const std::map call_attributes = {}, SourceLocation location = SourceLocation()) { - auto pick_result = DoPick(picker, call_attributes); + auto pick_result = DoPick(picker); auto* complete = absl::get_if( &pick_result.result); EXPECT_NE(complete, nullptr) << PickResultString(pick_result) << " at " @@ -662,15 +639,10 @@ 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 = {}, SourceLocation location = SourceLocation()) { - EXPECT_NE(picker, nullptr); - if (picker == nullptr) { - return absl::nullopt; - } std::vector results; for (size_t i = 0; i < num_picks; ++i) { - auto address = ExpectPickComplete(picker, call_attributes, location); + auto address = ExpectPickComplete(picker, location); if (!address.has_value()) return absl::nullopt; results.emplace_back(std::move(*address)); } @@ -696,18 +668,17 @@ class LoadBalancingPolicyTest : public ::testing::Test { // Checks that the picker has round-robin behavior over the specified // set of addresses. - void ExpectRoundRobinPicks( - LoadBalancingPolicy::SubchannelPicker* picker, - absl::Span addresses, - const std::map call_attributes = {}, - size_t num_iterations = 3, SourceLocation location = SourceLocation()) { - auto picks = GetCompletePicks(picker, num_iterations * addresses.size(), - call_attributes, location); + void ExpectRoundRobinPicks(LoadBalancingPolicy::SubchannelPicker* picker, + absl::Span addresses, + size_t num_iterations = 3, + SourceLocation location = SourceLocation()) { + auto picks = + GetCompletePicks(picker, num_iterations * addresses.size(), location); ASSERT_TRUE(picks.has_value()) << location.file() << ":" << location.line(); EXPECT_TRUE(PicksAreRoundRobin(addresses, *picks)) - << " Actual: " << absl::StrJoin(*picks, ", ") - << "\n Expected: " << absl::StrJoin(addresses, ", ") << "\n" - << location.file() << ":" << location.line(); + << "Expected: " << absl::StrJoin(addresses, ", ") + << "Actual: " << absl::StrJoin(*picks, ", ") << location.file() << ":" + << location.line(); } // Requests a picker on picker and expects a Fail result. 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 315bbde8810..c9677f6e557 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 @@ -14,11 +14,22 @@ // limitations under the License. // -#include +#include -#include "gmock/gmock.h" +#include +#include -#include "src/core/ext/filters/stateful_session/stateful_session_filter.h" +#include "absl/status/status.h" +#include "absl/strings/string_view.h" +#include "absl/types/span.h" +#include "gtest/gtest.h" + +#include + +#include "src/core/lib/gprpp/orphanable.h" +#include "src/core/lib/gprpp/ref_counted_ptr.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/test_config.h" @@ -39,38 +50,47 @@ class XdsOverrideHostTest : public LoadBalancingPolicyTest { Json::Object{{"childPolicy", Json::Array{{child_policy_config}}}}}}}); } - RefCountedPtr - ExpectStartupWithRoundRobin(absl::Span addresses) { - RefCountedPtr picker; - EXPECT_EQ(ApplyUpdate(BuildUpdate(addresses, MakeXdsOverrideHostConfig()), - policy_.get()), - absl::OkStatus()); - ExpectConnectingUpdate(); - for (size_t i = 0; i < addresses.size(); ++i) { - auto* subchannel = FindSubchannel(addresses[i]); - EXPECT_NE(subchannel, nullptr); - if (subchannel == nullptr) return nullptr; - EXPECT_TRUE(subchannel->ConnectionRequested()); - subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING); - subchannel->SetConnectivityState(GRPC_CHANNEL_READY); - if (i == 0) { - picker = WaitForConnected(); - ExpectRoundRobinPicks(picker.get(), {addresses[0]}); - } else { - picker = WaitForRoundRobinListChange( - absl::MakeSpan(addresses).subspan(0, i), - absl::MakeSpan(addresses).subspan(0, i + 1)); - } - } - return picker; - } - OrphanablePtr policy_; }; TEST_F(XdsOverrideHostTest, DelegatesToChild) { - ExpectStartupWithRoundRobin( - {"ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442", "ipv4:127.0.0.1:443"}); + // Send address list to LB policy. + const std::array kAddresses = { + "ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442", "ipv4:127.0.0.1:443"}; + EXPECT_EQ(ApplyUpdate(BuildUpdate(kAddresses, MakeXdsOverrideHostConfig()), + policy_.get()), + absl::OkStatus()); + // Expect the initial CONNECTNG update with a picker that queues. + ExpectConnectingUpdate(); + // RR should have created a subchannel for each address. + for (size_t i = 0; i < kAddresses.size(); ++i) { + auto* subchannel = FindSubchannel(kAddresses[i]); + ASSERT_NE(subchannel, nullptr); + // RR should ask each subchannel to connect. + EXPECT_TRUE(subchannel->ConnectionRequested()); + // The subchannel will connect successfully. + subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING); + subchannel->SetConnectivityState(GRPC_CHANNEL_READY); + // As each subchannel becomes READY, we should get a new picker that + // includes the behavior. Note that there may be any number of + // duplicate updates for the previous state in the queue before the + // update that we actually want to see. + if (i == 0) { + // When the first subchannel becomes READY, accept any number of + // CONNECTING updates with a picker that queues followed by a READY + // update with a picker that repeatedly returns only the first address. + auto picker = WaitForConnected(); + ExpectRoundRobinPicks(picker.get(), {kAddresses[0]}); + } else { + // When each subsequent subchannel becomes READY, we accept any number + // of READY updates where the picker returns only the previously + // connected subchannel(s) followed by a READY update where the picker + // returns the previously connected subchannel(s) *and* the newly + // connected subchannel. + WaitForRoundRobinListChange(absl::MakeSpan(kAddresses).subspan(0, i), + absl::MakeSpan(kAddresses).subspan(0, i + 1)); + } + } } TEST_F(XdsOverrideHostTest, NoConfigReportsError) { @@ -80,78 +100,6 @@ TEST_F(XdsOverrideHostTest, NoConfigReportsError) { absl::InvalidArgumentError("Missing policy config")); } -TEST_F(XdsOverrideHostTest, OverrideHost) { - // Send address list to LB policy. - const std::array kAddresses = { - "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); - // Check that the host is overridden - std::map call_attributes{ - {XdsHostOverrideTypeName(), "127.0.0.1:442"}}; - EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[1]); - EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[1]); - call_attributes[XdsHostOverrideTypeName()] = std::string("127.0.0.1:441"); - EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[0]); - EXPECT_EQ(ExpectPickComplete(picker.get(), call_attributes), kAddresses[0]); -} - -TEST_F(XdsOverrideHostTest, OverrideHostSubchannelNotFound) { - // Send address list to LB policy. - const std::array kAddresses = { - "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{ - {XdsHostOverrideTypeName(), "no such host"}}; - ExpectRoundRobinPicks(picker.get(), kAddresses, call_attributes); -} - -TEST_F(XdsOverrideHostTest, SubchannelsComeAndGo) { - const std::array kAddresses = { - "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); - // Check that the host is overridden - std::map call_attributes{ - {XdsHostOverrideTypeName(), "127.0.0.1:442"}}; - ExpectRoundRobinPicks(picker.get(), {kAddresses[1]}, call_attributes); - // Some other address is gone - EXPECT_EQ(ApplyUpdate(BuildUpdate({kAddresses[0], kAddresses[1]}, - MakeXdsOverrideHostConfig()), - policy_.get()), - absl::OkStatus()); - // Wait for LB policy to return a new picker that uses the updated - // addresses. We can't use the host override for this, because then - // we won't know when the new picker is actually using all of the new - // addresses. - picker = - WaitForRoundRobinListChange(kAddresses, {kAddresses[0], kAddresses[1]}); - // Make sure host override still works. - ExpectRoundRobinPicks(picker.get(), {kAddresses[1]}, call_attributes); - // "Our" address is gone so others get returned in round-robin order - EXPECT_EQ(ApplyUpdate(BuildUpdate({kAddresses[0], kAddresses[2]}, - MakeXdsOverrideHostConfig()), - policy_.get()), - absl::OkStatus()); - // Wait for LB policy to return the new picker. - // In this case, we can pass call_attributes while we wait instead of - // checking again afterward, because the host override won't actually - // be used. - WaitForRoundRobinListChange({kAddresses[0], kAddresses[1]}, - {kAddresses[0], kAddresses[2]}, call_attributes); - // And now it is back - EXPECT_EQ(ApplyUpdate(BuildUpdate({kAddresses[1], kAddresses[2]}, - MakeXdsOverrideHostConfig()), - policy_.get()), - absl::OkStatus()); - // Wait for LB policy to return the new picker. - picker = WaitForRoundRobinListChange({kAddresses[0], kAddresses[2]}, - {kAddresses[1], kAddresses[2]}); - // Make sure host override works. - ExpectRoundRobinPicks(picker.get(), {kAddresses[1]}, call_attributes); -} - } // namespace } // namespace testing } // namespace grpc_core diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 7734c1880c1..820db7a49b6 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1101,7 +1101,6 @@ src/core/ext/filters/client_channel/health/health_check_client.cc \ src/core/ext/filters/client_channel/health/health_check_client.h \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/http_proxy.h \ -src/core/ext/filters/client_channel/lb_call_state_internal.h \ src/core/ext/filters/client_channel/lb_policy/address_filtering.cc \ src/core/ext/filters/client_channel/lb_policy/address_filtering.h \ src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 7dcb54749a0..c758dc95a0f 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -908,7 +908,6 @@ src/core/ext/filters/client_channel/health/health_check_client.cc \ src/core/ext/filters/client_channel/health/health_check_client.h \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/http_proxy.h \ -src/core/ext/filters/client_channel/lb_call_state_internal.h \ src/core/ext/filters/client_channel/lb_policy/address_filtering.cc \ src/core/ext/filters/client_channel/lb_policy/address_filtering.h \ src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h \