diff --git a/BUILD b/BUILD index a62ca7e8ec2..992d1d403cc 100644 --- a/BUILD +++ b/BUILD @@ -1235,6 +1235,21 @@ grpc_cc_library( ], ) +grpc_cc_library( + name = "grpc_grpclb_balancer_addresses", + srcs = [ + "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc", + ], + hdrs = [ + "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h", + ], + language = "c++", + deps = [ + "grpc_base", + "grpc_client_channel", + ], +) + grpc_cc_library( name = "grpc_lb_policy_grpclb", srcs = [ @@ -1255,6 +1270,7 @@ grpc_cc_library( deps = [ "grpc_base", "grpc_client_channel", + "grpc_grpclb_balancer_addresses", "grpc_lb_upb", "grpc_resolver_fake", "grpc_transport_chttp2_client_insecure", @@ -1281,6 +1297,7 @@ grpc_cc_library( deps = [ "grpc_base", "grpc_client_channel", + "grpc_grpclb_balancer_addresses", "grpc_lb_upb", "grpc_resolver_fake", "grpc_secure", @@ -1606,6 +1623,7 @@ grpc_cc_library( deps = [ "grpc_base", "grpc_client_channel", + "grpc_grpclb_balancer_addresses", "grpc_resolver_dns_selection", ], ) diff --git a/BUILD.gn b/BUILD.gn index ee40a4db057..306f2bfa6d8 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -229,6 +229,8 @@ config("grpc_config") { "src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h", "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc", "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h", + "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc", + "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h", "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h", "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc", "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc", diff --git a/CMakeLists.txt b/CMakeLists.txt index c0b5279d257..5a52205e480 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1319,6 +1319,7 @@ add_library(grpc src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc + src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc @@ -1972,6 +1973,7 @@ add_library(grpc_unsecure src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc + src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.cc src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc diff --git a/Makefile b/Makefile index c4c6332736b..08ee1cbe217 100644 --- a/Makefile +++ b/Makefile @@ -3649,6 +3649,7 @@ LIBGRPC_SRC = \ src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc \ + src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc \ @@ -4277,6 +4278,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc \ + src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc \ diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 56b33c42228..14daa48e2b7 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -385,6 +385,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h - src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h + - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.h - src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.h @@ -742,6 +743,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc + - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc @@ -1279,6 +1281,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h - src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h + - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.h - src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.h @@ -1571,6 +1574,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc + - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc - src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc diff --git a/config.m4 b/config.m4 index 7a38e36cd71..66922ad5527 100644 --- a/config.m4 +++ b/config.m4 @@ -53,6 +53,7 @@ if test "$PHP_GRPC" != "no"; then src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc \ + src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc \ diff --git a/config.w32 b/config.w32 index 089504a1841..541cc74b602 100644 --- a/config.w32 +++ b/config.w32 @@ -22,6 +22,7 @@ if (PHP_GRPC != "no") { "src\\core\\ext\\filters\\client_channel\\lb_policy\\child_policy_handler.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\grpclb\\client_load_reporting_filter.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\grpclb\\grpclb.cc " + + "src\\core\\ext\\filters\\client_channel\\lb_policy\\grpclb\\grpclb_balancer_addresses.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\grpclb\\grpclb_channel_secure.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\grpclb\\grpclb_client_stats.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\grpclb\\load_balancer_api.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 8315e96f6ad..c8b176b23ea 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -236,6 +236,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.h', @@ -685,6 +686,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index f87d5f4603a..d140b005d7f 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -212,6 +212,8 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc', @@ -1033,6 +1035,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.h', 'src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.h', diff --git a/grpc.gemspec b/grpc.gemspec index cf0b389f50a..c9a1a8835fd 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -134,6 +134,8 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h ) + s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc ) + s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc ) diff --git a/grpc.gyp b/grpc.gyp index c32c0dca814..b065ee6f2af 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -445,6 +445,7 @@ 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc', @@ -934,6 +935,7 @@ 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc', diff --git a/package.xml b/package.xml index d9d5d1516d8..32930b6f65c 100644 --- a/package.xml +++ b/package.xml @@ -114,6 +114,8 @@ + + diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 0a5759b6012..7dafecc59ba 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1637,25 +1637,6 @@ void ChannelData::ProcessLbPolicy( grpc_channel_args_find(resolver_result.args, GRPC_ARG_LB_POLICY_NAME); policy_name = grpc_channel_arg_get_string(channel_arg); } - // Special case: If at least one balancer address is present, we use - // the grpclb policy, regardless of what the resolver has returned. - bool found_balancer_address = false; - for (size_t i = 0; i < resolver_result.addresses.size(); ++i) { - const ServerAddress& address = resolver_result.addresses[i]; - if (address.IsBalancer()) { - found_balancer_address = true; - break; - } - } - if (found_balancer_address) { - if (policy_name != nullptr && strcmp(policy_name, "grpclb") != 0) { - gpr_log(GPR_INFO, - "resolver requested LB policy %s but provided at least one " - "balancer address -- forcing use of grpclb LB policy", - policy_name); - } - policy_name = "grpclb"; - } // Use pick_first if nothing was specified and we didn't select grpclb // above. if (policy_name == nullptr) policy_name = "pick_first"; diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 670c4ffdd56..da72451d549 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -74,6 +74,7 @@ #include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h" #include "src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h" #include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h" +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h" #include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.h" #include "src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.h" @@ -1241,25 +1242,11 @@ void GrpcLb::BalancerCallState::OnBalancerStatusReceivedLocked( // helper code for creating balancer channel // -ServerAddressList ExtractBalancerAddresses(const ServerAddressList& addresses) { - ServerAddressList balancer_addresses; - for (size_t i = 0; i < addresses.size(); ++i) { - if (addresses[i].IsBalancer()) { - // Strip out the is_balancer channel arg, since we don't want to - // recursively use the grpclb policy in the channel used to talk to - // the balancers. Note that we do NOT strip out the balancer_name - // channel arg, since we need that to set the authority correctly - // to talk to the balancers. - static const char* args_to_remove[] = { - GRPC_ARG_ADDRESS_IS_BALANCER, - }; - balancer_addresses.emplace_back( - addresses[i].address(), - grpc_channel_args_copy_and_remove(addresses[i].args(), args_to_remove, - GPR_ARRAY_SIZE(args_to_remove))); - } - } - return balancer_addresses; +ServerAddressList ExtractBalancerAddresses(const grpc_channel_args& args) { + const ServerAddressList* addresses = + FindGrpclbBalancerAddressesInChannelArgs(args); + if (addresses != nullptr) return *addresses; + return ServerAddressList(); } /* Returns the channel args for the LB channel, used to create a bidirectional @@ -1452,27 +1439,25 @@ void GrpcLb::UpdateLocked(UpdateArgs args) { // helpers for UpdateLocked() // -// Returns the backend addresses extracted from the given addresses. -ServerAddressList ExtractBackendAddresses(const ServerAddressList& addresses) { +ServerAddressList AddNullLbTokenToAddresses( + const ServerAddressList& addresses) { static const char* lb_token = ""; grpc_arg arg = grpc_channel_arg_pointer_create( const_cast(GRPC_ARG_GRPCLB_ADDRESS_LB_TOKEN), const_cast(lb_token), &lb_token_arg_vtable); - ServerAddressList backend_addresses; + ServerAddressList addresses_out; for (size_t i = 0; i < addresses.size(); ++i) { - if (!addresses[i].IsBalancer()) { - backend_addresses.emplace_back( - addresses[i].address(), - grpc_channel_args_copy_and_add(addresses[i].args(), &arg, 1)); - } + addresses_out.emplace_back( + addresses[i].address(), + grpc_channel_args_copy_and_add(addresses[i].args(), &arg, 1)); } - return backend_addresses; + return addresses_out; } void GrpcLb::ProcessAddressesAndChannelArgsLocked( const ServerAddressList& addresses, const grpc_channel_args& args) { // Update fallback address list. - fallback_backend_addresses_ = ExtractBackendAddresses(addresses); + fallback_backend_addresses_ = AddNullLbTokenToAddresses(addresses); // Make sure that GRPC_ARG_LB_POLICY_NAME is set in channel args, // since we use this to trigger the client_load_reporting filter. static const char* args_to_remove[] = {GRPC_ARG_LB_POLICY_NAME}; @@ -1482,7 +1467,7 @@ void GrpcLb::ProcessAddressesAndChannelArgsLocked( args_ = grpc_channel_args_copy_and_add_and_remove( &args, args_to_remove, GPR_ARRAY_SIZE(args_to_remove), &new_arg, 1); // Construct args for balancer channel. - ServerAddressList balancer_addresses = ExtractBalancerAddresses(addresses); + ServerAddressList balancer_addresses = ExtractBalancerAddresses(args); grpc_channel_args* lb_channel_args = BuildBalancerChannelArgs( balancer_addresses, response_generator_.get(), &args); // Create balancer channel if needed. diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc new file mode 100644 index 00000000000..2888c3b94ae --- /dev/null +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc @@ -0,0 +1,89 @@ +// +// Copyright 2019 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. +// + +#include + +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" + +#include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gpr/useful.h" + +// Channel arg key for the list of balancer addresses. +#define GRPC_ARG_GRPCLB_BALANCER_ADDRESSES "grpc.grpclb_balancer_addresses" +// Channel arg key for a string indicating an address's balancer name. +#define GRPC_ARG_ADDRESS_BALANCER_NAME "grpc.address_balancer_name" + +namespace grpc_core { + +namespace { + +void* BalancerAddressesArgCopy(void* p) { + ServerAddressList* address_list = static_cast(p); + return new ServerAddressList(*address_list); +} + +void BalancerAddressesArgDestroy(void* p) { + ServerAddressList* address_list = static_cast(p); + delete address_list; +} + +int BalancerAddressesArgCmp(void* p, void* q) { + ServerAddressList* address_list1 = static_cast(p); + ServerAddressList* address_list2 = static_cast(q); + if (address_list1 == nullptr || address_list2 == nullptr) { + return GPR_ICMP(address_list1, address_list2); + } + if (address_list1->size() > address_list2->size()) return 1; + if (address_list1->size() < address_list2->size()) return -1; + for (size_t i = 0; i < address_list1->size(); ++i) { + int retval = (*address_list1)[i].Cmp((*address_list2)[i]); + if (retval != 0) return retval; + } + return 0; +} + +const grpc_arg_pointer_vtable kBalancerAddressesArgVtable = { + BalancerAddressesArgCopy, BalancerAddressesArgDestroy, + BalancerAddressesArgCmp}; + +} // namespace + +grpc_arg CreateGrpclbBalancerAddressesArg( + const ServerAddressList* address_list) { + return grpc_channel_arg_pointer_create( + const_cast(GRPC_ARG_GRPCLB_BALANCER_ADDRESSES), + const_cast(address_list), + &kBalancerAddressesArgVtable); +} + +const ServerAddressList* FindGrpclbBalancerAddressesInChannelArgs( + const grpc_channel_args& args) { + return grpc_channel_args_find_pointer( + &args, const_cast(GRPC_ARG_GRPCLB_BALANCER_ADDRESSES)); +} + +grpc_arg CreateGrpclbBalancerNameArg(const char* balancer_name) { + return grpc_channel_arg_string_create( + const_cast(GRPC_ARG_ADDRESS_BALANCER_NAME), + const_cast(balancer_name)); +} + +const char* FindGrpclbBalancerNameInChannelArgs(const grpc_channel_args& args) { + return grpc_channel_args_find_string( + &args, const_cast(GRPC_ARG_ADDRESS_BALANCER_NAME)); +} + +} // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h new file mode 100644 index 00000000000..9b6b259deca --- /dev/null +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h @@ -0,0 +1,40 @@ +// +// Copyright 2019 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_POLICY_GRPCLB_GRPCLB_BALANCER_ADDRESSES_H +#define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_GRPCLB_GRPCLB_BALANCER_ADDRESSES_H + +#include + +#include + +#include "src/core/ext/filters/client_channel/server_address.h" + +namespace grpc_core { + +grpc_arg CreateGrpclbBalancerAddressesArg( + const ServerAddressList* address_list); +const ServerAddressList* FindGrpclbBalancerAddressesInChannelArgs( + const grpc_channel_args& args); + +grpc_arg CreateGrpclbBalancerNameArg(const char* balancer_name); +const char* FindGrpclbBalancerNameInChannelArgs(const grpc_channel_args& args); + +} // namespace grpc_core + +#endif /* \ +GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_GRPCLB_GRPCLB_BALANCER_ADDRESSES_H \ + */ diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc index 5bc4f5157ad..414f8274317 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc @@ -27,6 +27,7 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/server_address.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gpr/string.h" @@ -55,8 +56,8 @@ RefCountedPtr CreateTargetAuthorityTable( grpc_sockaddr_to_string(&addr_str, &addresses[i].address(), true) > 0); target_authority_entries[i].key = grpc_slice_from_copied_string(addr_str); gpr_free(addr_str); - char* balancer_name = grpc_channel_arg_get_string(grpc_channel_args_find( - addresses[i].args(), GRPC_ARG_ADDRESS_BALANCER_NAME)); + const char* balancer_name = + FindGrpclbBalancerNameInChannelArgs(*addresses[i].args()); target_authority_entries[i].value.reset(gpr_strdup(balancer_name)); } RefCountedPtr target_authority_table = diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h index 4932a6869f1..2d8988926e4 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h @@ -370,13 +370,6 @@ SubchannelList::SubchannelList( GRPC_ARG_SERVICE_CONFIG}; // Create a subchannel for each address. for (size_t i = 0; i < addresses.size(); i++) { - // TODO(roth): we should ideally hide this from the LB policy code. In - // principle, if we're dealing with this special case in the client_channel - // code for selecting grpclb, then we should also strip out these addresses - // there if we're not using grpclb. - if (addresses[i].IsBalancer()) { - continue; - } InlinedVector args_to_add; const size_t subchannel_address_arg_index = args_to_add.size(); args_to_add.emplace_back( diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc index a8215439d2a..e2a05d03ad3 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc @@ -30,6 +30,7 @@ #include #include "src/core/ext/filters/client_channel/http_connect_handshaker.h" +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/lb_policy_registry.h" #include "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h" #include "src/core/ext/filters/client_channel/resolver/dns/dns_resolver_selection.h" @@ -107,8 +108,10 @@ class AresDnsResolver : public Resolver { grpc_millis last_resolution_timestamp_ = -1; /// retry backoff state BackOff backoff_; - /// currently resolving addresses + /// currently resolving backend addresses std::unique_ptr addresses_; + /// currently resolving balancer addresses + std::unique_ptr balancer_addresses_; /// currently resolving service config char* service_config_json_ = nullptr; // has shutdown been initiated @@ -328,9 +331,11 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) { r->Unref(DEBUG_LOCATION, "OnResolvedLocked() shutdown"); return; } - if (r->addresses_ != nullptr) { + if (r->addresses_ != nullptr || r->balancer_addresses_ != nullptr) { Result result; - result.addresses = std::move(*r->addresses_); + if (r->addresses_ != nullptr) { + result.addresses = std::move(*r->addresses_); + } if (r->service_config_json_ != nullptr) { std::string service_config_string = ChooseServiceConfig( r->service_config_json_, &result.service_config_error); @@ -343,9 +348,16 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) { service_config_string, &result.service_config_error); } } - result.args = grpc_channel_args_copy(r->channel_args_); + InlinedVector new_args; + if (r->balancer_addresses_ != nullptr) { + new_args.push_back( + CreateGrpclbBalancerAddressesArg(r->balancer_addresses_.get())); + } + result.args = grpc_channel_args_copy_and_add( + r->channel_args_, new_args.data(), new_args.size()); r->result_handler()->ReturnResult(std::move(result)); r->addresses_.reset(); + r->balancer_addresses_.reset(); // Reset backoff state so that we start from the beginning when the // next request gets triggered. r->backoff_.Reset(); @@ -424,7 +436,8 @@ void AresDnsResolver::StartResolvingLocked() { GRPC_CLOSURE_INIT(&on_resolved_, OnResolved, this, grpc_schedule_on_exec_ctx); pending_request_ = grpc_dns_lookup_ares_locked( dns_server_, name_to_resolve_, kDefaultPort, interested_parties_, - &on_resolved_, &addresses_, enable_srv_queries_ /* check_grpclb */, + &on_resolved_, &addresses_, + enable_srv_queries_ ? &balancer_addresses_ : nullptr, request_service_config_ ? &service_config_json_ : nullptr, query_timeout_ms_, combiner()); last_resolution_timestamp_ = grpc_core::ExecCtx::Get()->Now(); diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc index 761bed9a82d..b9960453ccf 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc @@ -33,6 +33,7 @@ #include #include +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/parse_address.h" #include "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h" #include "src/core/lib/gpr/string.h" @@ -60,6 +61,8 @@ struct grpc_ares_request { grpc_closure* on_done; /** the pointer to receive the resolved addresses */ std::unique_ptr* addresses_out; + /** the pointer to receive the resolved balancer addresses */ + std::unique_ptr* balancer_addresses_out; /** the pointer to receive the service config in JSON */ char** service_config_json_out; /** the evernt driver used by this request */ @@ -184,17 +187,17 @@ static void on_hostbyname_done_locked(void* arg, int status, int /*timeouts*/, GRPC_CARES_TRACE_LOG( "request:%p on_hostbyname_done_locked host=%s ARES_SUCCESS", r, hr->host); - if (*r->addresses_out == nullptr) { - *r->addresses_out = absl::make_unique(); + std::unique_ptr* address_list_ptr = + hr->is_balancer ? r->balancer_addresses_out : r->addresses_out; + if (*address_list_ptr == nullptr) { + *address_list_ptr = absl::make_unique(); } - ServerAddressList& addresses = **r->addresses_out; + ServerAddressList& addresses = **address_list_ptr; for (size_t i = 0; hostent->h_addr_list[i] != nullptr; ++i) { - grpc_core::InlinedVector args_to_add; + grpc_core::InlinedVector args_to_add; if (hr->is_balancer) { - args_to_add.emplace_back(grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ADDRESS_IS_BALANCER), 1)); - args_to_add.emplace_back(grpc_channel_arg_string_create( - const_cast(GRPC_ARG_ADDRESS_BALANCER_NAME), hr->host)); + args_to_add.emplace_back( + grpc_core::CreateGrpclbBalancerNameArg(hr->host)); } grpc_channel_args* args = grpc_channel_args_copy_and_add( nullptr, args_to_add.data(), args_to_add.size()); @@ -350,7 +353,7 @@ done: void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( grpc_ares_request* r, const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, - bool check_grpclb, int query_timeout_ms, grpc_core::Combiner* combiner) { + int query_timeout_ms, grpc_core::Combiner* combiner) { grpc_error* error = GRPC_ERROR_NONE; grpc_ares_hostbyname_request* hr = nullptr; ares_channel* channel = nullptr; @@ -425,7 +428,7 @@ void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( /*is_balancer=*/false); ares_gethostbyname(*channel, hr->host, AF_INET, on_hostbyname_done_locked, hr); - if (check_grpclb) { + if (r->balancer_addresses_out != nullptr) { /* Query the SRV record */ grpc_ares_request_ref_locked(r); char* service_name; @@ -588,7 +591,8 @@ static bool grpc_ares_maybe_resolve_localhost_manually_locked( static grpc_ares_request* grpc_dns_lookup_ares_locked_impl( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addrs, bool check_grpclb, + std::unique_ptr* addrs, + std::unique_ptr* balancer_addrs, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner) { grpc_ares_request* r = @@ -596,6 +600,7 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl( r->ev_driver = nullptr; r->on_done = on_done; r->addresses_out = addrs; + r->balancer_addresses_out = balancer_addrs; r->service_config_json_out = service_config_json; r->error = GRPC_ERROR_NONE; r->pending_queries = 0; @@ -618,20 +623,21 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl( // as to cut down on lookups over the network, especially in tests: // https://github.com/grpc/proposal/pull/79 if (target_matches_localhost(name)) { - check_grpclb = false; + r->balancer_addresses_out = nullptr; r->service_config_json_out = nullptr; } // Look up name using c-ares lib. grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( - r, dns_server, name, default_port, interested_parties, check_grpclb, - query_timeout_ms, combiner); + r, dns_server, name, default_port, interested_parties, query_timeout_ms, + combiner); return r; } grpc_ares_request* (*grpc_dns_lookup_ares_locked)( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addrs, bool check_grpclb, + std::unique_ptr* addrs, + std::unique_ptr* balancer_addrs, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner) = grpc_dns_lookup_ares_locked_impl; @@ -709,7 +715,6 @@ static void on_dns_lookup_done_locked(void* arg, grpc_error* error) { static_cast(gpr_zalloc( sizeof(grpc_resolved_address) * (*resolved_addresses)->naddrs)); for (size_t i = 0; i < (*resolved_addresses)->naddrs; ++i) { - GPR_ASSERT(!(*r->addresses)[i].IsBalancer()); memcpy(&(*resolved_addresses)->addrs[i], &(*r->addresses)[i].address(), sizeof(grpc_resolved_address)); } @@ -736,9 +741,9 @@ static void grpc_resolve_address_invoke_dns_lookup_ares_locked( grpc_schedule_on_exec_ctx); r->ares_request = grpc_dns_lookup_ares_locked( nullptr /* dns_server */, r->name, r->default_port, r->interested_parties, - &r->on_dns_lookup_done_locked, &r->addresses, false /* check_grpclb */, - nullptr /* service_config_json */, GRPC_DNS_ARES_DEFAULT_QUERY_TIMEOUT_MS, - r->combiner); + &r->on_dns_lookup_done_locked, &r->addresses, + nullptr /* balancer_addresses */, nullptr /* service_config_json */, + GRPC_DNS_ARES_DEFAULT_QUERY_TIMEOUT_MS, r->combiner); } static void grpc_resolve_address_ares_impl(const char* name, diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h index 115018155b3..bfa888ce9f5 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h @@ -63,7 +63,8 @@ extern void (*grpc_resolve_address_ares)(const char* name, extern grpc_ares_request* (*grpc_dns_lookup_ares_locked)( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addresses, bool check_grpclb, + std::unique_ptr* addresses, + std::unique_ptr* balancer_addresses, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner); diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc index fe631f873e9..be880f6c1c2 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc @@ -29,7 +29,8 @@ struct grpc_ares_request { static grpc_ares_request* grpc_dns_lookup_ares_locked_impl( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addrs, bool check_grpclb, + std::unique_ptr* addrs, + std::unique_ptr* balancer_addrs, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner) { return NULL; @@ -38,7 +39,8 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl( grpc_ares_request* (*grpc_dns_lookup_ares_locked)( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addrs, bool check_grpclb, + std::unique_ptr* addrs, + std::unique_ptr* balancer_addrs, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner) = grpc_dns_lookup_ares_locked_impl; diff --git a/src/core/ext/filters/client_channel/server_address.cc b/src/core/ext/filters/client_channel/server_address.cc index d46896b754b..93d361a8154 100644 --- a/src/core/ext/filters/client_channel/server_address.cc +++ b/src/core/ext/filters/client_channel/server_address.cc @@ -37,15 +37,12 @@ ServerAddress::ServerAddress(const void* address, size_t address_len, address_.len = static_cast(address_len); } -bool ServerAddress::operator==(const ServerAddress& other) const { - return address_.len == other.address_.len && - memcmp(address_.addr, other.address_.addr, address_.len) == 0 && - grpc_channel_args_compare(args_, other.args_) == 0; -} - -bool ServerAddress::IsBalancer() const { - return grpc_channel_arg_get_bool( - grpc_channel_args_find(args_, GRPC_ARG_ADDRESS_IS_BALANCER), false); +int ServerAddress::Cmp(const ServerAddress& other) const { + if (address_.len > other.address_.len) return 1; + if (address_.len < other.address_.len) return -1; + int retval = memcmp(address_.addr, other.address_.addr, address_.len); + if (retval != 0) return retval; + return grpc_channel_args_compare(args_, other.args_); } } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/server_address.h b/src/core/ext/filters/client_channel/server_address.h index acd71358810..10f49f2f344 100644 --- a/src/core/ext/filters/client_channel/server_address.h +++ b/src/core/ext/filters/client_channel/server_address.h @@ -25,13 +25,6 @@ #include "src/core/lib/gprpp/inlined_vector.h" #include "src/core/lib/iomgr/resolve_address.h" -// Channel arg key for a bool indicating whether an address is a grpclb -// load balancer (as opposed to a backend). -#define GRPC_ARG_ADDRESS_IS_BALANCER "grpc.address_is_balancer" - -// Channel arg key for a string indicating an address's balancer name. -#define GRPC_ARG_ADDRESS_BALANCER_NAME "grpc.address_balancer_name" - namespace grpc_core { // @@ -73,13 +66,13 @@ class ServerAddress { return *this; } - bool operator==(const ServerAddress& other) const; + bool operator==(const ServerAddress& other) const { return Cmp(other) == 0; } + + int Cmp(const ServerAddress& other) const; const grpc_resolved_address& address() const { return address_; } const grpc_channel_args* args() const { return args_; } - bool IsBalancer() const; - private: grpc_resolved_address address_; grpc_channel_args* args_; diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index dd26915d14b..46cce67f77e 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -31,6 +31,7 @@ CORE_SOURCE_FILES = [ 'src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc', + 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc', 'src/core/ext/filters/client_channel/lb_policy/grpclb/load_balancer_api.cc', diff --git a/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc b/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc index 56a552ea9ca..2dc86f10291 100644 --- a/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc +++ b/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc @@ -64,8 +64,9 @@ static grpc_ares_request* my_dns_lookup_ares_locked( const char* /*dns_server*/, const char* addr, const char* /*default_port*/, grpc_pollset_set* /*interested_parties*/, grpc_closure* on_done, std::unique_ptr* addresses, - bool /*check_grpclb*/, char** /*service_config_json*/, - int /*query_timeout_ms*/, grpc_core::Combiner* /*combiner*/) { + std::unique_ptr* /*balancer_addresses*/, + char** /*service_config_json*/, int /*query_timeout_ms*/, + grpc_core::Combiner* /*combiner*/) { gpr_mu_lock(&g_mu); GPR_ASSERT(0 == strcmp("test", addr)); grpc_error* error = GRPC_ERROR_NONE; diff --git a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc index 94699bd3b31..14b0d9fe40e 100644 --- a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc +++ b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc @@ -42,7 +42,8 @@ static grpc_core::Combiner* g_combiner; static grpc_ares_request* (*g_default_dns_lookup_ares_locked)( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addresses, bool check_grpclb, + std::unique_ptr* addresses, + std::unique_ptr* balancer_addresses, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner); @@ -93,12 +94,14 @@ static grpc_address_resolver_vtable test_resolver = { static grpc_ares_request* test_dns_lookup_ares_locked( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* /*interested_parties*/, grpc_closure* on_done, - std::unique_ptr* addresses, bool check_grpclb, + std::unique_ptr* addresses, + std::unique_ptr* balancer_addresses, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner) { grpc_ares_request* result = g_default_dns_lookup_ares_locked( dns_server, name, default_port, g_iomgr_args.pollset_set, on_done, - addresses, check_grpclb, service_config_json, query_timeout_ms, combiner); + addresses, balancer_addresses, service_config_json, query_timeout_ms, + combiner); ++g_resolution_count; static grpc_millis last_resolution_time = 0; grpc_millis now = diff --git a/test/core/client_channel/resolvers/fake_resolver_test.cc b/test/core/client_channel/resolvers/fake_resolver_test.cc index 01d82cbdd05..c9b5828d92e 100644 --- a/test/core/client_channel/resolvers/fake_resolver_test.cc +++ b/test/core/client_channel/resolvers/fake_resolver_test.cc @@ -86,29 +86,18 @@ static grpc_core::Resolver::Result create_new_resolver_result() { static size_t test_counter = 0; const size_t num_addresses = 2; char* uri_string; - char* balancer_name; // Create address list. grpc_core::Resolver::Result result; for (size_t i = 0; i < num_addresses; ++i) { gpr_asprintf(&uri_string, "ipv4:127.0.0.1:100%" PRIuPTR, test_counter * num_addresses + i); grpc_uri* uri = grpc_uri_parse(uri_string, true); - gpr_asprintf(&balancer_name, "balancer%" PRIuPTR, - test_counter * num_addresses + i); grpc_resolved_address address; GPR_ASSERT(grpc_parse_uri(uri, &address)); grpc_core::InlinedVector args_to_add; - const bool is_balancer = num_addresses % 2; - if (is_balancer) { - args_to_add.emplace_back(grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ADDRESS_IS_BALANCER), 1)); - args_to_add.emplace_back(grpc_channel_arg_string_create( - const_cast(GRPC_ARG_ADDRESS_BALANCER_NAME), balancer_name)); - } - grpc_channel_args* args = grpc_channel_args_copy_and_add( - nullptr, args_to_add.data(), args_to_add.size()); - result.addresses.emplace_back(address.addr, address.len, args); - gpr_free(balancer_name); + result.addresses.emplace_back( + address.addr, address.len, + grpc_channel_args_copy_and_add(nullptr, nullptr, 0)); grpc_uri_destroy(uri); gpr_free(uri_string); } diff --git a/test/core/end2end/goaway_server_test.cc b/test/core/end2end/goaway_server_test.cc index 64603cbc1fc..1ad61154158 100644 --- a/test/core/end2end/goaway_server_test.cc +++ b/test/core/end2end/goaway_server_test.cc @@ -47,7 +47,8 @@ static int g_resolve_port = -1; static grpc_ares_request* (*iomgr_dns_lookup_ares_locked)( const char* dns_server, const char* addr, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addresses, bool check_grpclb, + std::unique_ptr* addresses, + std::unique_ptr* balancer_addresses, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner); @@ -104,13 +105,14 @@ static grpc_address_resolver_vtable test_resolver = { static grpc_ares_request* my_dns_lookup_ares_locked( const char* dns_server, const char* addr, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, - std::unique_ptr* addresses, bool check_grpclb, + std::unique_ptr* addresses, + std::unique_ptr* balancer_addresses, char** service_config_json, int query_timeout_ms, grpc_core::Combiner* combiner) { if (0 != strcmp(addr, "test")) { return iomgr_dns_lookup_ares_locked( dns_server, addr, default_port, interested_parties, on_done, addresses, - check_grpclb, service_config_json, query_timeout_ms, combiner); + balancer_addresses, service_config_json, query_timeout_ms, combiner); } grpc_error* error = GRPC_ERROR_NONE; diff --git a/test/cpp/client/BUILD b/test/cpp/client/BUILD index b895d99724f..011d937bcb8 100644 --- a/test/cpp/client/BUILD +++ b/test/cpp/client/BUILD @@ -34,6 +34,7 @@ grpc_cc_test( grpc_cc_test( name = "client_channel_stress_test", + size = "large", srcs = ["client_channel_stress_test.cc"], # TODO(jtattermusch): test fails frequently on Win RBE, but passes locally # reenable the tests once it works reliably on Win RBE. diff --git a/test/cpp/client/client_channel_stress_test.cc b/test/cpp/client/client_channel_stress_test.cc index 5fcfc11cc4c..8e92d5373ad 100644 --- a/test/cpp/client/client_channel_stress_test.cc +++ b/test/cpp/client/client_channel_stress_test.cc @@ -35,6 +35,7 @@ #include #include +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/parse_address.h" #include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h" #include "src/core/ext/filters/client_channel/server_address.h" @@ -151,7 +152,7 @@ class ClientChannelStressTest { for (const auto& balancer_server : balancer_servers_) { // Select each address with probability of 0.8. if (std::rand() % 10 < 8) { - addresses.emplace_back(AddressData{balancer_server.port_, true, ""}); + addresses.emplace_back(AddressData{balancer_server.port_, ""}); } } std::shuffle(addresses.begin(), addresses.end(), @@ -213,13 +214,12 @@ class ClientChannelStressTest { struct AddressData { int port; - bool is_balancer; grpc::string balancer_name; }; - void SetNextResolution(const std::vector& address_data) { - grpc_core::ExecCtx exec_ctx; - grpc_core::Resolver::Result result; + static grpc_core::ServerAddressList CreateAddressListFromAddressDataList( + const std::vector& address_data) { + grpc_core::ServerAddressList addresses; for (const auto& addr : address_data) { char* lb_uri_str; gpr_asprintf(&lb_uri_str, "ipv4:127.0.0.1:%d", addr.port); @@ -227,20 +227,34 @@ class ClientChannelStressTest { GPR_ASSERT(lb_uri != nullptr); grpc_resolved_address address; GPR_ASSERT(grpc_parse_uri(lb_uri, &address)); - std::vector args_to_add; - if (addr.is_balancer) { - args_to_add.emplace_back(grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ADDRESS_IS_BALANCER), 1)); - args_to_add.emplace_back(grpc_channel_arg_string_create( - const_cast(GRPC_ARG_ADDRESS_BALANCER_NAME), - const_cast(addr.balancer_name.c_str()))); - } - grpc_channel_args* args = grpc_channel_args_copy_and_add( - nullptr, args_to_add.data(), args_to_add.size()); - result.addresses.emplace_back(address.addr, address.len, args); + grpc_arg arg = + grpc_core::CreateGrpclbBalancerNameArg(addr.balancer_name.c_str()); + grpc_channel_args* args = + grpc_channel_args_copy_and_add(nullptr, &arg, 1); + addresses.emplace_back(address.addr, address.len, args); grpc_uri_destroy(lb_uri); gpr_free(lb_uri_str); } + return addresses; + } + + static grpc_core::Resolver::Result MakeResolverResult( + const std::vector& balancer_address_data) { + grpc_core::Resolver::Result result; + grpc_error* error = GRPC_ERROR_NONE; + result.service_config = grpc_core::ServiceConfig::Create( + "{\"loadBalancingConfig\":[{\"grpclb\":{}}]}", &error); + GPR_ASSERT(error == GRPC_ERROR_NONE); + grpc_core::ServerAddressList balancer_addresses = + CreateAddressListFromAddressDataList(balancer_address_data); + grpc_arg arg = CreateGrpclbBalancerAddressesArg(&balancer_addresses); + result.args = grpc_channel_args_copy_and_add(nullptr, &arg, 1); + return result; + } + + void SetNextResolution(const std::vector& address_data) { + grpc_core::ExecCtx exec_ctx; + grpc_core::Resolver::Result result = MakeResolverResult(address_data); response_generator_->SetResponse(std::move(result)); } diff --git a/test/cpp/client/destroy_grpclb_channel_with_active_connect_stress_test.cc b/test/cpp/client/destroy_grpclb_channel_with_active_connect_stress_test.cc index 7ba76f0da75..d4a942b9870 100644 --- a/test/cpp/client/destroy_grpclb_channel_with_active_connect_stress_test.cc +++ b/test/cpp/client/destroy_grpclb_channel_with_active_connect_stress_test.cc @@ -37,6 +37,7 @@ #include #include +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/parse_address.h" #include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h" #include "src/core/ext/filters/client_channel/server_address.h" @@ -59,24 +60,21 @@ void TryConnectAndDestroy() { // The precise behavior is dependant on the test runtime environment though, // since connect() attempts on this address may unfortunately result in // "network unreachable" errors in some test runtime environments. - char* uri_str; - gpr_asprintf(&uri_str, "ipv6:[0100::1234]:443"); + const char* uri_str = "ipv6:[0100::1234]:443"; grpc_uri* lb_uri = grpc_uri_parse(uri_str, true); - gpr_free(uri_str); - GPR_ASSERT(lb_uri != nullptr); + ASSERT_NE(lb_uri, nullptr); grpc_resolved_address address; - GPR_ASSERT(grpc_parse_uri(lb_uri, &address)); - std::vector address_args_to_add = { - grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ADDRESS_IS_BALANCER), 1), - }; + ASSERT_TRUE(grpc_parse_uri(lb_uri, &address)); + grpc_uri_destroy(lb_uri); grpc_core::ServerAddressList addresses; - grpc_channel_args* address_args = grpc_channel_args_copy_and_add( - nullptr, address_args_to_add.data(), address_args_to_add.size()); - addresses.emplace_back(address.addr, address.len, address_args); + addresses.emplace_back(address.addr, address.len, nullptr); grpc_core::Resolver::Result lb_address_result; - lb_address_result.addresses = addresses; - grpc_uri_destroy(lb_uri); + grpc_error* error = GRPC_ERROR_NONE; + lb_address_result.service_config = grpc_core::ServiceConfig::Create( + "{\"loadBalancingConfig\":[{\"grpclb\":{}}]}", &error); + ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); + grpc_arg arg = grpc_core::CreateGrpclbBalancerAddressesArg(&addresses); + lb_address_result.args = grpc_channel_args_copy_and_add(nullptr, &arg, 1); response_generator->SetResponse(lb_address_result); grpc::ChannelArguments args; args.SetPointer(GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR, @@ -95,9 +93,9 @@ void TryConnectAndDestroy() { // unreachable balancer to begin. The connection should never become ready // because the LB we're trying to connect to is unreachable. channel->GetState(true /* try_to_connect */); - GPR_ASSERT( - !channel->WaitForConnected(grpc_timeout_milliseconds_to_deadline(100))); - GPR_ASSERT("grpclb" == channel->GetLoadBalancingPolicyName()); + ASSERT_FALSE( + channel->WaitForConnected(grpc_timeout_milliseconds_to_deadline(100))); + ASSERT_EQ("grpclb", channel->GetLoadBalancingPolicyName()); channel.reset(); }; diff --git a/test/cpp/end2end/grpclb_end2end_test.cc b/test/cpp/end2end/grpclb_end2end_test.cc index 79969f9c5e3..36c52b9798a 100644 --- a/test/cpp/end2end/grpclb_end2end_test.cc +++ b/test/cpp/end2end/grpclb_end2end_test.cc @@ -35,6 +35,7 @@ #include #include "src/core/ext/filters/client_channel/backup_poller.h" +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/parse_address.h" #include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h" #include "src/core/ext/filters/client_channel/server_address.h" @@ -83,6 +84,13 @@ namespace grpc { namespace testing { namespace { +constexpr char kDefaultServiceConfig[] = + "{\n" + " \"loadBalancingConfig\":[\n" + " { \"grpclb\":{} }\n" + " ]\n" + "}"; + template class CountedService : public ServiceType { public: @@ -514,11 +522,10 @@ class GrpclbEnd2endTest : public ::testing::Test { struct AddressData { int port; - bool is_balancer; grpc::string balancer_name; }; - grpc_core::ServerAddressList CreateLbAddressesFromAddressDataList( + static grpc_core::ServerAddressList CreateLbAddressesFromAddressDataList( const std::vector& address_data) { grpc_core::ServerAddressList addresses; for (const auto& addr : address_data) { @@ -528,16 +535,10 @@ class GrpclbEnd2endTest : public ::testing::Test { GPR_ASSERT(lb_uri != nullptr); grpc_resolved_address address; GPR_ASSERT(grpc_parse_uri(lb_uri, &address)); - std::vector args_to_add; - if (addr.is_balancer) { - args_to_add.emplace_back(grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ADDRESS_IS_BALANCER), 1)); - args_to_add.emplace_back(grpc_channel_arg_string_create( - const_cast(GRPC_ARG_ADDRESS_BALANCER_NAME), - const_cast(addr.balancer_name.c_str()))); - } - grpc_channel_args* args = grpc_channel_args_copy_and_add( - nullptr, args_to_add.data(), args_to_add.size()); + grpc_arg arg = + grpc_core::CreateGrpclbBalancerNameArg(addr.balancer_name.c_str()); + grpc_channel_args* args = + grpc_channel_args_copy_and_add(nullptr, &arg, 1); addresses.emplace_back(address.addr, address.len, args); grpc_uri_destroy(lb_uri); gpr_free(lb_uri_str); @@ -545,34 +546,50 @@ class GrpclbEnd2endTest : public ::testing::Test { return addresses; } + static grpc_core::Resolver::Result MakeResolverResult( + const std::vector& balancer_address_data, + const std::vector& backend_address_data = {}, + const char* service_config_json = kDefaultServiceConfig) { + grpc_core::Resolver::Result result; + result.addresses = + CreateLbAddressesFromAddressDataList(backend_address_data); + grpc_error* error = GRPC_ERROR_NONE; + result.service_config = + grpc_core::ServiceConfig::Create(service_config_json, &error); + GPR_ASSERT(error == GRPC_ERROR_NONE); + grpc_core::ServerAddressList balancer_addresses = + CreateLbAddressesFromAddressDataList(balancer_address_data); + grpc_arg arg = CreateGrpclbBalancerAddressesArg(&balancer_addresses); + result.args = grpc_channel_args_copy_and_add(nullptr, &arg, 1); + return result; + } + void SetNextResolutionAllBalancers( - const char* service_config_json = nullptr) { + const char* service_config_json = kDefaultServiceConfig) { std::vector addresses; for (size_t i = 0; i < balancers_.size(); ++i) { - addresses.emplace_back(AddressData{balancers_[i]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[i]->port_, ""}); } - SetNextResolution(addresses, service_config_json); + SetNextResolution(addresses, {}, service_config_json); } - void SetNextResolution(const std::vector& address_data, - const char* service_config_json = nullptr) { + void SetNextResolution( + const std::vector& balancer_address_data, + const std::vector& backend_address_data = {}, + const char* service_config_json = kDefaultServiceConfig) { grpc_core::ExecCtx exec_ctx; - grpc_core::Resolver::Result result; - result.addresses = CreateLbAddressesFromAddressDataList(address_data); - if (service_config_json != nullptr) { - grpc_error* error = GRPC_ERROR_NONE; - result.service_config = - grpc_core::ServiceConfig::Create(service_config_json, &error); - GRPC_ERROR_UNREF(error); - } + grpc_core::Resolver::Result result = MakeResolverResult( + balancer_address_data, backend_address_data, service_config_json); response_generator_->SetResponse(std::move(result)); } void SetNextReresolutionResponse( - const std::vector& address_data) { + const std::vector& balancer_address_data, + const std::vector& backend_address_data = {}, + const char* service_config_json = kDefaultServiceConfig) { grpc_core::ExecCtx exec_ctx; - grpc_core::Resolver::Result result; - result.addresses = CreateLbAddressesFromAddressDataList(address_data); + grpc_core::Resolver::Result result = MakeResolverResult( + balancer_address_data, backend_address_data, service_config_json); response_generator_->SetReresolutionResponse(std::move(result)); } @@ -747,44 +764,11 @@ TEST_F(SingleBalancerTest, SelectGrpclbWithMigrationServiceConfig) { EXPECT_EQ("grpclb", channel_->GetLoadBalancingPolicyName()); } -TEST_F(SingleBalancerTest, - DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTest) { - const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor(); - ResetStub(kFallbackTimeoutMs); - SetNextResolution({AddressData{backends_[0]->port_, false, ""}, - AddressData{balancers_[0]->port_, true, ""}}, - "{\n" - " \"loadBalancingConfig\":[\n" - " {\"pick_first\":{} }\n" - " ]\n" - "}"); - CheckRpcSendOk(); - // Check LB policy name for the channel. - EXPECT_EQ("pick_first", channel_->GetLoadBalancingPolicyName()); -} - -TEST_F( - SingleBalancerTest, - DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTestAndNoBackendAddress) { - const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor(); - ResetStub(kFallbackTimeoutMs); - SetNextResolution({AddressData{balancers_[0]->port_, true, ""}}, - "{\n" - " \"loadBalancingConfig\":[\n" - " {\"pick_first\":{} }\n" - " ]\n" - "}"); - // This should fail since we do not have a non-balancer backend - CheckRpcSendFailure(); - // Check LB policy name for the channel. - EXPECT_EQ("pick_first", channel_->GetLoadBalancingPolicyName()); -} - TEST_F(SingleBalancerTest, SelectGrpclbWithMigrationServiceConfigAndNoAddresses) { const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor(); ResetStub(kFallbackTimeoutMs); - SetNextResolution({}, + SetNextResolution({}, {}, "{\n" " \"loadBalancingConfig\":[\n" " { \"does_not_exist\":{} },\n" @@ -804,23 +788,6 @@ TEST_F(SingleBalancerTest, EXPECT_EQ("grpclb", channel_->GetLoadBalancingPolicyName()); } -TEST_F(SingleBalancerTest, - SelectGrpclbWithMigrationServiceConfigAndNoBalancerAddresses) { - const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor(); - ResetStub(kFallbackTimeoutMs); - // Resolution includes fallback address but no balancers. - SetNextResolution({AddressData{backends_[0]->port_, false, ""}}, - "{\n" - " \"loadBalancingConfig\":[\n" - " { \"does_not_exist\":{} },\n" - " { \"grpclb\":{} }\n" - " ]\n" - "}"); - CheckRpcSendOk(1, 1000 /* timeout_ms */, true /* wait_for_ready */); - // Check LB policy name for the channel. - EXPECT_EQ("grpclb", channel_->GetLoadBalancingPolicyName()); -} - TEST_F(SingleBalancerTest, UsePickFirstChildPolicy) { SetNextResolutionAllBalancers( "{\n" @@ -875,7 +842,7 @@ TEST_F(SingleBalancerTest, SwapChildPolicy) { EXPECT_EQ(backends_[i]->service_.request_count(), 0UL); } // Send new resolution that removes child policy from service config. - SetNextResolutionAllBalancers("{}"); + SetNextResolutionAllBalancers(); WaitForAllBackends(); CheckRpcSendOk(kNumRpcs, 1000 /* timeout_ms */, true /* wait_for_ready */); // Check that every backend saw the same number of requests. This verifies @@ -903,9 +870,11 @@ TEST_F(SingleBalancerTest, UpdatesGoToMostRecentChildPolicy) { SetNextResolution( { // Unreachable balancer. - {unreachable_balancer_port, true, ""}, + {unreachable_balancer_port, ""}, + }, + { // Fallback address: first backend. - {backends_[0]->port_, false, ""}, + {backends_[0]->port_, ""}, }, "{\n" " \"loadBalancingConfig\":[\n" @@ -923,9 +892,11 @@ TEST_F(SingleBalancerTest, UpdatesGoToMostRecentChildPolicy) { SetNextResolution( { // Unreachable balancer. - {unreachable_balancer_port, true, ""}, + {unreachable_balancer_port, ""}, + }, + { // Fallback address: unreachable backend. - {unreachable_backend_port, false, ""}, + {unreachable_backend_port, ""}, }, "{\n" " \"loadBalancingConfig\":[\n" @@ -946,10 +917,12 @@ TEST_F(SingleBalancerTest, UpdatesGoToMostRecentChildPolicy) { SetNextResolution( { // Unreachable balancer. - {unreachable_balancer_port, true, ""}, + {unreachable_balancer_port, ""}, + }, + { // Fallback address: second and third backends. - {backends_[1]->port_, false, ""}, - {backends_[2]->port_, false, ""}, + {backends_[1]->port_, ""}, + {backends_[2]->port_, ""}, }, "{\n" " \"loadBalancingConfig\":[\n" @@ -988,7 +961,7 @@ TEST_F(SingleBalancerTest, SameBackendListedMultipleTimes) { TEST_F(SingleBalancerTest, SecureNaming) { ResetStub(0, kApplicationTargetName_ + ";lb"); - SetNextResolution({AddressData{balancers_[0]->port_, true, "lb"}}); + SetNextResolution({AddressData{balancers_[0]->port_, "lb"}}); const size_t kNumRpcsPerAddress = 100; ScheduleResponseForBalancer( 0, BalancerServiceImpl::BuildResponseForBackends(GetBackendPorts(), {}), @@ -1020,7 +993,7 @@ TEST_F(SingleBalancerTest, SecureNamingDeathTest) { ASSERT_DEATH_IF_SUPPORTED( { ResetStub(0, kApplicationTargetName_ + ";lb"); - SetNextResolution({AddressData{balancers_[0]->port_, true, "woops"}}); + SetNextResolution({AddressData{balancers_[0]->port_, "woops"}}); channel_->WaitForConnected(grpc_timeout_seconds_to_deadline(1)); }, ""); @@ -1080,12 +1053,13 @@ TEST_F(SingleBalancerTest, Fallback) { const size_t kNumBackendsInResolution = backends_.size() / 2; ResetStub(kFallbackTimeoutMs); - std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); + std::vector balancer_addresses; + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + std::vector backend_addresses; for (size_t i = 0; i < kNumBackendsInResolution; ++i) { - addresses.emplace_back(AddressData{backends_[i]->port_, false, ""}); + backend_addresses.emplace_back(AddressData{backends_[i]->port_, ""}); } - SetNextResolution(addresses); + SetNextResolution(balancer_addresses, backend_addresses); // Send non-empty serverlist only after kServerlistDelayMs. ScheduleResponseForBalancer( @@ -1148,12 +1122,13 @@ TEST_F(SingleBalancerTest, FallbackUpdate) { const size_t kNumBackendsInResolutionUpdate = backends_.size() / 3; ResetStub(kFallbackTimeoutMs); - std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); + std::vector balancer_addresses; + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + std::vector backend_addresses; for (size_t i = 0; i < kNumBackendsInResolution; ++i) { - addresses.emplace_back(AddressData{backends_[i]->port_, false, ""}); + backend_addresses.emplace_back(AddressData{backends_[i]->port_, ""}); } - SetNextResolution(addresses); + SetNextResolution(balancer_addresses, backend_addresses); // Send non-empty serverlist only after kServerlistDelayMs. ScheduleResponseForBalancer( @@ -1183,13 +1158,14 @@ TEST_F(SingleBalancerTest, FallbackUpdate) { EXPECT_EQ(0U, backends_[i]->service_.request_count()); } - addresses.clear(); - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); + balancer_addresses.clear(); + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + backend_addresses.clear(); for (size_t i = kNumBackendsInResolution; i < kNumBackendsInResolution + kNumBackendsInResolutionUpdate; ++i) { - addresses.emplace_back(AddressData{backends_[i]->port_, false, ""}); + backend_addresses.emplace_back(AddressData{backends_[i]->port_, ""}); } - SetNextResolution(addresses); + SetNextResolution(balancer_addresses, backend_addresses); // Wait until the resolution update has been processed and all the new // fallback backends are reachable. @@ -1253,14 +1229,15 @@ TEST_F(SingleBalancerTest, // First two backends are fallback, last two are pointed to by balancer. const size_t kNumFallbackBackends = 2; const size_t kNumBalancerBackends = backends_.size() - kNumFallbackBackends; - std::vector addresses; + std::vector backend_addresses; for (size_t i = 0; i < kNumFallbackBackends; ++i) { - addresses.emplace_back(AddressData{backends_[i]->port_, false, ""}); + backend_addresses.emplace_back(AddressData{backends_[i]->port_, ""}); } + std::vector balancer_addresses; for (size_t i = 0; i < balancers_.size(); ++i) { - addresses.emplace_back(AddressData{balancers_[i]->port_, true, ""}); + balancer_addresses.emplace_back(AddressData{balancers_[i]->port_, ""}); } - SetNextResolution(addresses); + SetNextResolution(balancer_addresses, backend_addresses); ScheduleResponseForBalancer(0, BalancerServiceImpl::BuildResponseForBackends( GetBackendPorts(kNumFallbackBackends), {}), @@ -1307,14 +1284,15 @@ TEST_F(SingleBalancerTest, // First two backends are fallback, last two are pointed to by balancer. const size_t kNumFallbackBackends = 2; const size_t kNumBalancerBackends = backends_.size() - kNumFallbackBackends; - std::vector addresses; + std::vector backend_addresses; for (size_t i = 0; i < kNumFallbackBackends; ++i) { - addresses.emplace_back(AddressData{backends_[i]->port_, false, ""}); + backend_addresses.emplace_back(AddressData{backends_[i]->port_, ""}); } + std::vector balancer_addresses; for (size_t i = 0; i < balancers_.size(); ++i) { - addresses.emplace_back(AddressData{balancers_[i]->port_, true, ""}); + balancer_addresses.emplace_back(AddressData{balancers_[i]->port_, ""}); } - SetNextResolution(addresses); + SetNextResolution(balancer_addresses, backend_addresses); ScheduleResponseForBalancer(0, BalancerServiceImpl::BuildResponseForBackends( GetBackendPorts(kNumFallbackBackends), {}), @@ -1358,10 +1336,12 @@ TEST_F(SingleBalancerTest, FallbackEarlyWhenBalancerChannelFails) { const int kFallbackTimeoutMs = 10000 * grpc_test_slowdown_factor(); ResetStub(kFallbackTimeoutMs); // Return an unreachable balancer and one fallback backend. - std::vector addresses; - addresses.emplace_back(AddressData{grpc_pick_unused_port_or_die(), true, ""}); - addresses.emplace_back(AddressData{backends_[0]->port_, false, ""}); - SetNextResolution(addresses); + std::vector balancer_addresses; + balancer_addresses.emplace_back( + AddressData{grpc_pick_unused_port_or_die(), ""}); + std::vector backend_addresses; + backend_addresses.emplace_back(AddressData{backends_[0]->port_, ""}); + SetNextResolution(balancer_addresses, backend_addresses); // Send RPC with deadline less than the fallback timeout and make sure it // succeeds. CheckRpcSendOk(/* times */ 1, /* timeout_ms */ 1000, @@ -1372,10 +1352,11 @@ TEST_F(SingleBalancerTest, FallbackEarlyWhenBalancerCallFails) { const int kFallbackTimeoutMs = 10000 * grpc_test_slowdown_factor(); ResetStub(kFallbackTimeoutMs); // Return one balancer and one fallback backend. - std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{backends_[0]->port_, false, ""}); - SetNextResolution(addresses); + std::vector balancer_addresses; + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + std::vector backend_addresses; + backend_addresses.emplace_back(AddressData{backends_[0]->port_, ""}); + SetNextResolution(balancer_addresses, backend_addresses); // Balancer drops call without sending a serverlist. balancers_[0]->service_.NotifyDoneWithServerlists(); // Send RPC with deadline less than the fallback timeout and make sure it @@ -1388,10 +1369,11 @@ TEST_F(SingleBalancerTest, FallbackControlledByBalancer_BeforeFirstServerlist) { const int kFallbackTimeoutMs = 10000 * grpc_test_slowdown_factor(); ResetStub(kFallbackTimeoutMs); // Return one balancer and one fallback backend. - std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{backends_[0]->port_, false, ""}); - SetNextResolution(addresses); + std::vector balancer_addresses; + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + std::vector backend_addresses; + backend_addresses.emplace_back(AddressData{backends_[0]->port_, ""}); + SetNextResolution(balancer_addresses, backend_addresses); // Balancer explicitly tells client to fallback. LoadBalanceResponse resp; resp.mutable_fallback_response(); @@ -1404,10 +1386,11 @@ TEST_F(SingleBalancerTest, FallbackControlledByBalancer_BeforeFirstServerlist) { TEST_F(SingleBalancerTest, FallbackControlledByBalancer_AfterFirstServerlist) { // Return one balancer and one fallback backend (backend 0). - std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{backends_[0]->port_, false, ""}); - SetNextResolution(addresses); + std::vector balancer_addresses; + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + std::vector backend_addresses; + backend_addresses.emplace_back(AddressData{backends_[0]->port_, ""}); + SetNextResolution(balancer_addresses, backend_addresses); // Balancer initially sends serverlist, then tells client to fall back, // then sends the serverlist again. // The serverlist points to backend 1. @@ -1483,7 +1466,7 @@ TEST_F(UpdatesTest, UpdateBalancersButKeepUsingOriginalBalancer) { EXPECT_EQ(0U, balancers_[2]->service_.response_count()); std::vector addresses; - addresses.emplace_back(AddressData{balancers_[1]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[1]->port_, ""}); gpr_log(GPR_INFO, "========= ABOUT TO UPDATE 1 =========="); SetNextResolution(addresses); gpr_log(GPR_INFO, "========= UPDATE 1 DONE =========="); @@ -1542,9 +1525,9 @@ TEST_F(UpdatesTest, UpdateBalancersRepeated) { EXPECT_EQ(0U, balancers_[2]->service_.response_count()); std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{balancers_[1]->port_, true, ""}); - addresses.emplace_back(AddressData{balancers_[2]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + addresses.emplace_back(AddressData{balancers_[1]->port_, ""}); + addresses.emplace_back(AddressData{balancers_[2]->port_, ""}); gpr_log(GPR_INFO, "========= ABOUT TO UPDATE 1 =========="); SetNextResolution(addresses); gpr_log(GPR_INFO, "========= UPDATE 1 DONE =========="); @@ -1562,8 +1545,8 @@ TEST_F(UpdatesTest, UpdateBalancersRepeated) { balancers_[0]->service_.NotifyDoneWithServerlists(); addresses.clear(); - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{balancers_[1]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + addresses.emplace_back(AddressData{balancers_[1]->port_, ""}); gpr_log(GPR_INFO, "========= ABOUT TO UPDATE 2 =========="); SetNextResolution(addresses); gpr_log(GPR_INFO, "========= UPDATE 2 DONE =========="); @@ -1583,7 +1566,7 @@ TEST_F(UpdatesTest, UpdateBalancersRepeated) { TEST_F(UpdatesTest, UpdateBalancersDeadUpdate) { std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); SetNextResolution(addresses); const std::vector first_backend{GetBackendPorts()[0]}; const std::vector second_backend{GetBackendPorts()[1]}; @@ -1623,7 +1606,7 @@ TEST_F(UpdatesTest, UpdateBalancersDeadUpdate) { EXPECT_EQ(0U, balancers_[2]->service_.response_count()); addresses.clear(); - addresses.emplace_back(AddressData{balancers_[1]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[1]->port_, ""}); gpr_log(GPR_INFO, "========= ABOUT TO UPDATE 1 =========="); SetNextResolution(addresses); gpr_log(GPR_INFO, "========= UPDATE 1 DONE =========="); @@ -1660,18 +1643,20 @@ TEST_F(UpdatesTest, ReresolveDeadBackend) { ResetStub(500); // The first resolution contains the addresses of a balancer that never // responds, and a fallback backend. - std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{backends_[0]->port_, false, ""}); - SetNextResolution(addresses); + std::vector balancer_addresses; + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + std::vector backend_addresses; + backend_addresses.emplace_back(AddressData{backends_[0]->port_, ""}); + SetNextResolution(balancer_addresses, backend_addresses); // Ask channel to connect to trigger resolver creation. channel_->GetState(true); // The re-resolution result will contain the addresses of the same balancer // and a new fallback backend. - addresses.clear(); - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); - addresses.emplace_back(AddressData{backends_[1]->port_, false, ""}); - SetNextReresolutionResponse(addresses); + balancer_addresses.clear(); + balancer_addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); + backend_addresses.clear(); + backend_addresses.emplace_back(AddressData{backends_[1]->port_, ""}); + SetNextReresolutionResponse(balancer_addresses, backend_addresses); // Start servers and send 10 RPCs per server. gpr_log(GPR_INFO, "========= BEFORE FIRST BATCH =========="); @@ -1720,10 +1705,10 @@ TEST_F(UpdatesWithClientLoadReportingTest, ReresolveDeadBalancer) { // Ask channel to connect to trigger resolver creation. channel_->GetState(true); std::vector addresses; - addresses.emplace_back(AddressData{balancers_[0]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[0]->port_, ""}); SetNextResolution(addresses); addresses.clear(); - addresses.emplace_back(AddressData{balancers_[1]->port_, true, ""}); + addresses.emplace_back(AddressData{balancers_[1]->port_, ""}); SetNextReresolutionResponse(addresses); const std::vector first_backend{GetBackendPorts()[0]}; const std::vector second_backend{GetBackendPorts()[1]}; diff --git a/test/cpp/naming/resolver_component_test.cc b/test/cpp/naming/resolver_component_test.cc index b03901b92ee..f323ae0c012 100644 --- a/test/cpp/naming/resolver_component_test.cc +++ b/test/cpp/naming/resolver_component_test.cc @@ -39,6 +39,7 @@ #include "test/cpp/util/test_config.h" #include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" #include "src/core/ext/filters/client_channel/parse_address.h" #include "src/core/ext/filters/client_channel/resolver.h" #include "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h" @@ -463,19 +464,20 @@ class CheckingResultHandler : public ResultHandler { void CheckResult(const grpc_core::Resolver::Result& result) override { ArgsStruct* args = args_struct(); - gpr_log(GPR_INFO, "num addrs found: %" PRIdPTR ". expected %" PRIdPTR, - result.addresses.size(), args->expected_addrs.size()); - GPR_ASSERT(result.addresses.size() == args->expected_addrs.size()); std::vector found_lb_addrs; - for (size_t i = 0; i < result.addresses.size(); i++) { - const grpc_core::ServerAddress& addr = result.addresses[i]; - char* str; - grpc_sockaddr_to_string(&str, &addr.address(), 1 /* normalize */); - gpr_log(GPR_INFO, "%s", str); - found_lb_addrs.emplace_back( - GrpcLBAddress(std::string(str), addr.IsBalancer())); - gpr_free(str); + AddActualAddresses(result.addresses, /*is_balancer=*/false, + &found_lb_addrs); + const grpc_core::ServerAddressList* balancer_addresses = + grpc_core::FindGrpclbBalancerAddressesInChannelArgs(*result.args); + if (balancer_addresses != nullptr) { + AddActualAddresses(*balancer_addresses, /*is_balancer=*/true, + &found_lb_addrs); } + gpr_log(GPR_INFO, + "found %" PRIdPTR " backend addresses and %" PRIdPTR + " balancer addresses", + result.addresses.size(), + balancer_addresses == nullptr ? 0L : balancer_addresses->size()); if (args->expected_addrs.size() != found_lb_addrs.size()) { gpr_log(GPR_DEBUG, "found lb addrs size is: %" PRIdPTR @@ -495,6 +497,20 @@ class CheckingResultHandler : public ResultHandler { CheckLBPolicyResultLocked(result.args, args); } } + + private: + static void AddActualAddresses(const grpc_core::ServerAddressList& addresses, + bool is_balancer, + std::vector* out) { + for (size_t i = 0; i < addresses.size(); i++) { + const grpc_core::ServerAddress& addr = addresses[i]; + char* str; + grpc_sockaddr_to_string(&str, &addr.address(), 1 /* normalize */); + gpr_log(GPR_INFO, "%s", str); + out->emplace_back(GrpcLBAddress(std::string(str), is_balancer)); + gpr_free(str); + } + } }; int g_fake_non_responsive_dns_server_port = -1; diff --git a/tools/distrib/check_include_guards.py b/tools/distrib/check_include_guards.py index bc774227fff..2983441176d 100755 --- a/tools/distrib/check_include_guards.py +++ b/tools/distrib/check_include_guards.py @@ -44,7 +44,7 @@ class GuardValidator(object): self.ifndef_re = re.compile(r'#ifndef ([A-Z][A-Z_1-9]*)') self.define_re = re.compile(r'#define ([A-Z][A-Z_1-9]*)') self.endif_c_re = re.compile( - r'#endif /\* ([A-Z][A-Z_1-9]*) (?:\\ *\n *)?\*/') + r'#endif /\* (?: *\\\n *)?([A-Z][A-Z_1-9]*) (?:\\\n *)?\*/$') self.endif_cpp_re = re.compile(r'#endif // ([A-Z][A-Z_1-9]*)') self.failed = False @@ -122,7 +122,7 @@ class GuardValidator(object): # Is there a properly commented #endif? endif_re = self.endif_cpp_re if cpp_header else self.endif_c_re flines = fcontents.rstrip().splitlines() - match = endif_re.search('\n'.join(flines[-2:])) + match = endif_re.search('\n'.join(flines[-3:])) if not match: # No endif. Check if we have the last line as just '#endif' and if so # replace it with a properly commented one. diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 5c5d1c9fac5..edbe236dc17 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1097,6 +1097,8 @@ src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filte src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h \ +src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \ +src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 20087420e4e..5c03ac61231 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -894,6 +894,8 @@ src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filte src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h \ +src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \ +src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc \ src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc \