Merge pull request grpc#21721 from grpc/revert-20803-grpclb_stabiliza…

…tion Revert "grpclb stabilization"
markdroth · Jan 21, 2020 · 4bc2808 · 4bc2808
2 parents eba60d8 + 3f3cf8b
commit 4bc2808
Show file tree

Hide file tree

Showing 35 changed files with 301 additions and 462 deletions.
diff --git a/BUILD b/BUILD
@@ -1231,21 +1231,6 @@ 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 = [
@@ -1266,7 +1251,6 @@ grpc_cc_library(
     deps = [
         "grpc_base",
         "grpc_client_channel",
-        "grpc_grpclb_balancer_addresses",
         "grpc_lb_upb",
         "grpc_resolver_fake",
         "grpc_transport_chttp2_client_insecure",
@@ -1293,7 +1277,6 @@ grpc_cc_library(
     deps = [
         "grpc_base",
         "grpc_client_channel",
-        "grpc_grpclb_balancer_addresses",
         "grpc_lb_upb",
         "grpc_resolver_fake",
         "grpc_secure",
@@ -1619,7 +1602,6 @@ 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
@@ -236,8 +236,6 @@ 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
@@ -1808,7 +1808,6 @@ add_library(grpc
   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
-  src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc
   src/core/ext/upb-generated/src/proto/grpc/lb/v1/load_balancer.upb.c
   src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc
   src/core/ext/filters/client_channel/lb_policy/xds/cds.cc
@@ -3303,7 +3302,6 @@ add_library(grpc_unsecure
   src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_posix.cc
   src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_windows.cc
   src/core/ext/filters/client_channel/resolver/dns/dns_resolver_selection.cc
-  src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc
   src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc
   src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.cc
   src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc

diff --git a/Makefile b/Makefile
@@ -4273,7 +4273,6 @@ LIBGRPC_SRC = \
     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 \
-    src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \
     src/core/ext/upb-generated/src/proto/grpc/lb/v1/load_balancer.upb.c \
     src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc \
     src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \
@@ -5710,7 +5709,6 @@ LIBGRPC_UNSECURE_SRC = \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_posix.cc \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_windows.cc \
     src/core/ext/filters/client_channel/resolver/dns/dns_resolver_selection.cc \
-    src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc \
     src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc \
     src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.cc \
     src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc \

diff --git a/build.yaml b/build.yaml
@@ -1052,14 +1052,6 @@ filegroups:
   plugin: grpc_deadline_filter
   uses:
   - grpc_base
-- name: grpc_grpclb_balancer_addresses
-  headers:
-  - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h
-  src:
-  - src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc
-  uses:
-  - grpc_base
-  - grpc_client_channel
 - name: grpc_health_upb
   headers:
   - src/core/ext/upb-generated/src/proto/grpc/health/v1/health.upb.h
@@ -1115,7 +1107,6 @@ filegroups:
   uses:
   - grpc_base
   - grpc_client_channel
-  - grpc_grpclb_balancer_addresses
   - grpc_lb_upb
   - grpc_resolver_fake
 - name: grpc_lb_policy_grpclb_secure
@@ -1137,7 +1128,6 @@ filegroups:
   uses:
   - grpc_base
   - grpc_client_channel
-  - grpc_grpclb_balancer_addresses
   - grpc_lb_upb
   - grpc_resolver_fake
   - grpc_secure
@@ -1228,7 +1218,6 @@ filegroups:
   - grpc_base
   - grpc_client_channel
   - grpc_resolver_dns_selection
-  - grpc_grpclb_balancer_addresses
 - name: grpc_resolver_dns_native
   src:
   - src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc

diff --git a/config.m4 b/config.m4
@@ -52,7 +52,6 @@ if test "$PHP_GRPC" != "no"; then
     src/core/ext/filters/client_channel/lb_policy.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
@@ -21,7 +21,6 @@ if (PHP_GRPC != "no") {
     "src\\core\\ext\\filters\\client_channel\\lb_policy.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
@@ -232,7 +232,6 @@ Pod::Spec.new do |s|
                       'src/core/ext/filters/client_channel/lb_policy.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',
@@ -654,7 +653,6 @@ Pod::Spec.new do |s|
                               'src/core/ext/filters/client_channel/lb_policy.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
@@ -205,8 +205,6 @@ 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',
@@ -971,7 +969,6 @@ Pod::Spec.new do |s|
                               'src/core/ext/filters/client_channel/lb_policy.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
@@ -127,8 +127,6 @@ 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
@@ -815,7 +815,6 @@
         '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',
-        'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc',
         'src/core/ext/upb-generated/src/proto/grpc/lb/v1/load_balancer.upb.c',
         'src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc',
         'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc',
@@ -1660,7 +1659,6 @@
         'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_posix.cc',
         'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_windows.cc',
         'src/core/ext/filters/client_channel/resolver/dns/dns_resolver_selection.cc',
-        'src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc',
         'src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc',
         'src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.cc',
         'src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc',

diff --git a/package.xml b/package.xml
@@ -110,8 +110,6 @@
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/client_load_reporting_filter.h" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.h" role="src" />
-    <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc" role="src" />
-    <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel.h" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_channel_secure.cc" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_client_stats.cc" role="src" />

diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
@@ -1638,6 +1638,26 @@ void ChannelData::ProcessLbPolicy(
         grpc_channel_args_find(resolver_result.args, GRPC_ARG_LB_POLICY_NAME);
     local_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 (local_policy_name != nullptr &&
+        strcmp(local_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",
+              local_policy_name);
+    }
+    local_policy_name = "grpclb";
+  }
   // Use pick_first if nothing was specified and we didn't select grpclb
   // above.
   lb_policy_name->reset(gpr_strdup(

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
@@ -73,7 +73,6 @@
 #include "src/core/ext/filters/client_channel/client_channel.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"
@@ -1268,11 +1267,25 @@ void GrpcLb::BalancerCallState::OnBalancerStatusReceivedLocked(
 // helper code for creating balancer channel
 //
 
-ServerAddressList ExtractBalancerAddresses(const grpc_channel_args& args) {
-  const ServerAddressList* addresses =
-      FindGrpclbBalancerAddressesInChannelArgs(args);
-  if (addresses != nullptr) return *addresses;
-  return ServerAddressList();
+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;
 }
 
 /* Returns the channel args for the LB channel, used to create a bidirectional
@@ -1478,25 +1491,27 @@ void GrpcLb::UpdateLocked(UpdateArgs args) {
 // helpers for UpdateLocked()
 //
 
-ServerAddressList AddNullLbTokenToAddresses(
-    const ServerAddressList& addresses) {
+// Returns the backend addresses extracted from the given addresses.
+ServerAddressList ExtractBackendAddresses(const ServerAddressList& addresses) {
   static const char* lb_token = "";
   grpc_arg arg = grpc_channel_arg_pointer_create(
       const_cast<char*>(GRPC_ARG_GRPCLB_ADDRESS_LB_TOKEN),
       const_cast<char*>(lb_token), &lb_token_arg_vtable);
-  ServerAddressList addresses_out;
+  ServerAddressList backend_addresses;
   for (size_t i = 0; i < addresses.size(); ++i) {
-    addresses_out.emplace_back(
-        addresses[i].address(),
-        grpc_channel_args_copy_and_add(addresses[i].args(), &arg, 1));
+    if (!addresses[i].IsBalancer()) {
+      backend_addresses.emplace_back(
+          addresses[i].address(),
+          grpc_channel_args_copy_and_add(addresses[i].args(), &arg, 1));
+    }
   }
-  return addresses_out;
+  return backend_addresses;
 }
 
 void GrpcLb::ProcessAddressesAndChannelArgsLocked(
     const ServerAddressList& addresses, const grpc_channel_args& args) {
   // Update fallback address list.
-  fallback_backend_addresses_ = AddNullLbTokenToAddresses(addresses);
+  fallback_backend_addresses_ = ExtractBackendAddresses(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};
@@ -1506,7 +1521,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(args);
+  ServerAddressList balancer_addresses = ExtractBalancerAddresses(addresses);
   grpc_channel_args* lb_channel_args = BuildBalancerChannelArgs(
       balancer_addresses, response_generator_.get(), &args);
   // Create balancer channel if needed.