Merge pull request #10258 from markdroth/service_config_lb_semantics

Fix semantics of LB policy selection.

doc/service_config.md

@@ -13,12 +13,21 @@ The service config is a JSON string of the following form:
 ```
 {
   // Load balancing policy name.
-  // Supported values are 'round_robin' and 'grpclb'.
-  // Optional; if unset, the default behavior is pick the first available
-  // backend.
-  // Note that if the resolver returns only balancer addresses and no
-  // backend addresses, gRPC will always use the 'grpclb' policy,
-  // regardless of what this field is set to.
+  // Currently, the only selectable client-side policy provided with gRPC
+  // is 'round_robin', but third parties may add their own policies.
+  // This field is optional; if unset, the default behavior is to pick
+  // the first available backend.
+  // If the policy name is set via the client API, that value overrides
+  // the value specified here.
+  //
+  // Note that if the resolver returns at least one balancer address (as
+  // opposed to backend addresses), gRPC will use grpclb (see
+  // https://github.com/grpc/grpc/blob/master/doc/load-balancing.md),
+  // regardless of what LB policy is requested either here or via the
+  // client API.  However, if the resolver returns at least one backend
+  // address in addition to the balancer address(es), the client may fall
+  // back to the requested policy if it is unable to reach any of the
+  // grpclb load balancers.
   'loadBalancingPolicy': string,
 
   // Per-method configuration.  Optional.

src/core/ext/filters/client_channel/client_channel.c

@@ -400,26 +400,24 @@ static void on_resolver_result_changed_locked(grpc_exec_ctx *exec_ctx,
       GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING);
       lb_policy_name = channel_arg->value.string;
     }
-    // Special case: If all of the addresses are balancer addresses,
-    // assume that we should use the grpclb policy, regardless of what the
-    // resolver actually specified.
+    // Special case: If at least one balancer address is present, we use
+    // the grpclb policy, regardless of what the resolver actually specified.
     channel_arg =
         grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES);
     if (channel_arg != NULL && channel_arg->type == GRPC_ARG_POINTER) {
       grpc_lb_addresses *addresses = channel_arg->value.pointer.p;
-      bool found_backend_address = false;
+      bool found_balancer_address = false;
       for (size_t i = 0; i < addresses->num_addresses; ++i) {
-        if (!addresses->addresses[i].is_balancer) {
-          found_backend_address = true;
+        if (addresses->addresses[i].is_balancer) {
+          found_balancer_address = true;
           break;
         }
       }
-      if (!found_backend_address) {
+      if (found_balancer_address) {
         if (lb_policy_name != NULL && strcmp(lb_policy_name, "grpclb") != 0) {
           gpr_log(GPR_INFO,
-                  "resolver requested LB policy %s but provided only balancer "
-                  "addresses, no backend addresses -- forcing use of grpclb LB "
-                  "policy",
+                  "resolver requested LB policy %s but provided at least one "
+                  "balancer address -- forcing use of grpclb LB policy",
                   lb_policy_name);
         }
         lb_policy_name = "grpclb";

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.c

@@ -831,10 +831,10 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx,
   /* Count the number of gRPC-LB addresses. There must be at least one.
    * TODO(roth): For now, we ignore non-balancer addresses, but in the
    * future, we may change the behavior such that we fall back to using
-   * the non-balancer addresses if we cannot reach any balancers. At that
-   * time, this should be changed to allow a list with no balancer addresses,
-   * since the resolver might fail to return a balancer address even when
-   * this is the right LB policy to use. */
+   * the non-balancer addresses if we cannot reach any balancers. In the
+   * fallback case, we should use the LB policy indicated by
+   * GRPC_ARG_LB_POLICY_NAME (although if that specifies grpclb or is
+   * unset, we should default to pick_first). */
   const grpc_arg *arg =
       grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
   if (arg == NULL || arg->type != GRPC_ARG_POINTER) {