From 88405f70c16ab7a7679bb189b321bb565764e829 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 3 Oct 2016 08:24:52 -0700 Subject: [PATCH] Centralized logic for choosing the right LB policy. --- src/core/ext/client_config/client_channel.c | 32 ++++++++++++++++--- .../ext/resolver/dns/native/dns_resolver.c | 11 ++----- .../ext/resolver/sockaddr/sockaddr_resolver.c | 8 ++--- 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index feb4cbde7b5..31f435ef625 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -185,10 +185,34 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, lb_policy_args.additional_args = grpc_resolver_result_get_lb_policy_args(chand->resolver_result); lb_policy_args.client_channel_factory = chand->client_channel_factory; - lb_policy = grpc_lb_policy_create( - exec_ctx, - grpc_resolver_result_get_lb_policy_name(chand->resolver_result), - &lb_policy_args); + + // 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. + const char* lb_policy_name = + grpc_resolver_result_get_lb_policy_name(chand->resolver_result); + bool found_backend_address = false; + for (size_t i = 0; i < lb_policy_args.addresses->num_addresses; ++i) { + if (!lb_policy_args.addresses->addresses[i].is_balancer) { + found_backend_address = true; + break; + } + } + if (!found_backend_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", (lb_policy_name == NULL ? "(none)" : lb_policy_name)); + } + lb_policy_name = "grpclb"; + } + // Use pick_first if nothing was specified and we didn't select grpclb + // above. + if (lb_policy_name == NULL) lb_policy_name = "pick_first"; + + lb_policy = + grpc_lb_policy_create(exec_ctx, lb_policy_name, &lb_policy_args); if (lb_policy != NULL) { GRPC_LB_POLICY_REF(lb_policy, "config_change"); GRPC_ERROR_UNREF(state_error); diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index e8ac1b12ae8..3908547893b 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -61,8 +61,6 @@ typedef struct { char *name_to_resolve; /** default port to use */ char *default_port; - /** load balancing policy name */ - char *lb_policy_name; /** mutex guarding the rest of the state */ gpr_mu mu; @@ -181,7 +179,7 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, } grpc_resolved_addresses_destroy(r->addresses); result = grpc_resolver_result_create(r->target_name, addresses, - r->lb_policy_name, NULL); + NULL /* lb_policy_name */, NULL); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); @@ -245,13 +243,11 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { gpr_free(r->target_name); gpr_free(r->name_to_resolve); gpr_free(r->default_port); - gpr_free(r->lb_policy_name); gpr_free(r); } static grpc_resolver *dns_create(grpc_resolver_args *args, - const char *default_port, - const char *lb_policy_name) { + const char *default_port) { if (0 != strcmp(args->uri->authority, "")) { gpr_log(GPR_ERROR, "authority based dns uri's not supported"); return NULL; @@ -272,7 +268,6 @@ static grpc_resolver *dns_create(grpc_resolver_args *args, r->default_port = gpr_strdup(default_port); gpr_backoff_init(&r->backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER, BACKOFF_MIN_SECONDS * 1000, BACKOFF_MAX_SECONDS * 1000); - r->lb_policy_name = gpr_strdup(lb_policy_name); return &r->base; } @@ -286,7 +281,7 @@ static void dns_factory_unref(grpc_resolver_factory *factory) {} static grpc_resolver *dns_factory_create_resolver( grpc_resolver_factory *factory, grpc_resolver_args *args) { - return dns_create(args, "https", "pick_first"); + return dns_create(args, "https"); } static char *dns_factory_get_default_host_name(grpc_resolver_factory *factory, diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index f232e0460b6..d8c18057f78 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -164,7 +164,7 @@ char *unix_get_default_authority(grpc_resolver_factory *factory, static void do_nothing(void *ignored) {} static grpc_resolver *sockaddr_create( - grpc_resolver_args *args, const char *default_lb_policy_name, + grpc_resolver_args *args, int parse(grpc_uri *uri, struct sockaddr_storage *dst, size_t *len)) { bool errors_found = false; sockaddr_resolver *r; @@ -198,10 +198,6 @@ static grpc_resolver *sockaddr_create( abort(); } - if (r->lb_policy_name == NULL) { - r->lb_policy_name = gpr_strdup(default_lb_policy_name); - } - path_slice = gpr_slice_new(args->uri->path, strlen(args->uri->path), do_nothing); gpr_slice_buffer_init(&path_parts); @@ -251,7 +247,7 @@ static void sockaddr_factory_unref(grpc_resolver_factory *factory) {} #define DECL_FACTORY(name) \ static grpc_resolver *name##_factory_create_resolver( \ grpc_resolver_factory *factory, grpc_resolver_args *args) { \ - return sockaddr_create(args, "pick_first", parse_##name); \ + return sockaddr_create(args, parse_##name); \ } \ static const grpc_resolver_factory_vtable name##_factory_vtable = { \ sockaddr_factory_ref, sockaddr_factory_unref, \