Merge pull request grpc#11237 from y-zeng/srv_record

Support grpclb address in the c-ares resolver

BUILD

@@ -998,6 +998,7 @@ grpc_cc_library(
         "src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c",
         "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c",
         "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c",
+        "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c",
     ],
     hdrs = [
         "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h",

CMakeLists.txt

@@ -1133,6 +1133,7 @@ add_library(grpc
   src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c
   src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c
   src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c
+  src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c
   src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c
   src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c
   src/core/ext/filters/load_reporting/load_reporting.c
@@ -2014,6 +2015,7 @@ add_library(grpc_unsecure
   src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c
   src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c
   src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c
+  src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c
   src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c
   src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c
   src/core/ext/filters/client_channel/resolver/fake/fake_resolver.c

Makefile

@@ -3116,6 +3116,7 @@ LIBGRPC_SRC = \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c \
+    src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c \
     src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c \
     src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c \
     src/core/ext/filters/load_reporting/load_reporting.c \
@@ -3966,6 +3967,7 @@ LIBGRPC_UNSECURE_SRC = \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c \
+    src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c \
     src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c \
     src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c \
     src/core/ext/filters/client_channel/resolver/fake/fake_resolver.c \

binding.gyp

@@ -864,6 +864,7 @@
         'src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c',
         'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c',
         'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c',
+        'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c',
         'src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c',
         'src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c',
         'src/core/ext/filters/load_reporting/load_reporting.c',

build.yaml

@@ -594,6 +594,7 @@ filegroups:
   - src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c
   - src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c
   - src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c
+  - src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c
   plugin: grpc_resolver_dns_ares
   uses:
   - grpc_base

config.m4

@@ -311,6 +311,7 @@ if test "$PHP_GRPC" != "no"; then
     src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c \
     src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c \
+    src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c \
     src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c \
     src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c \
     src/core/ext/filters/load_reporting/load_reporting.c \

config.w32

@@ -288,6 +288,7 @@ if (PHP_GRPC != "no") {
     "src\\core\\ext\\filters\\client_channel\\resolver\\dns\\c_ares\\dns_resolver_ares.c " +
     "src\\core\\ext\\filters\\client_channel\\resolver\\dns\\c_ares\\grpc_ares_ev_driver_posix.c " +
     "src\\core\\ext\\filters\\client_channel\\resolver\\dns\\c_ares\\grpc_ares_wrapper.c " +
+    "src\\core\\ext\\filters\\client_channel\\resolver\\dns\\c_ares\\grpc_ares_wrapper_fallback.c " +
     "src\\core\\ext\\filters\\client_channel\\resolver\\dns\\native\\dns_resolver.c " +
     "src\\core\\ext\\filters\\client_channel\\resolver\\sockaddr\\sockaddr_resolver.c " +
     "src\\core\\ext\\filters\\load_reporting\\load_reporting.c " +

gRPC-Core.podspec

@@ -690,6 +690,7 @@ Pod::Spec.new do |s|
                       'src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c',
                       'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c',
                       'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c',
+                      'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c',
                       'src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c',
                       'src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c',
                       'src/core/ext/filters/load_reporting/load_reporting.c',

grpc.gemspec

@@ -621,6 +621,7 @@ Gem::Specification.new do |s|
   s.files += %w( src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c )
   s.files += %w( src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c )
   s.files += %w( src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c )
+  s.files += %w( src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c )
   s.files += %w( src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c )
   s.files += %w( src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c )
   s.files += %w( src/core/ext/filters/load_reporting/load_reporting.c )

package.xml

@@ -635,6 +635,7 @@
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.c" role="src" />
+    <file baseinstalldir="/" name="src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.c" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.c" role="src" />
     <file baseinstalldir="/" name="src/core/ext/filters/load_reporting/load_reporting.c" role="src" />

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c

@@ -65,6 +65,8 @@ typedef struct {
   grpc_combiner *combiner;
   /** are we currently resolving? */
   bool resolving;
+  /** the pending resolving request */
+  grpc_ares_request *pending_request;
   /** which version of the result have we published? */
   int published_version;
   /** which version of the result is current? */
@@ -82,7 +84,7 @@ typedef struct {
   gpr_backoff backoff_state;
 
   /** currently resolving addresses */
-  grpc_resolved_addresses *addresses;
+  grpc_lb_addresses *lb_addresses;
 } ares_dns_resolver;
 
 static void dns_ares_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *r);
@@ -109,6 +111,9 @@ static void dns_ares_shutdown_locked(grpc_exec_ctx *exec_ctx,
   if (r->have_retry_timer) {
     grpc_timer_cancel(exec_ctx, &r->retry_timer);
   }
+  if (r->pending_request != NULL) {
+    grpc_cancel_ares_request(exec_ctx, r->pending_request);
+  }
   if (r->next_completion != NULL) {
     *r->target_result = NULL;
     grpc_closure_sched(
@@ -145,19 +150,11 @@ static void dns_ares_on_resolved_locked(grpc_exec_ctx *exec_ctx, void *arg,
   grpc_channel_args *result = NULL;
   GPR_ASSERT(r->resolving);
   r->resolving = false;
-  if (r->addresses != NULL) {
-    grpc_lb_addresses *addresses = grpc_lb_addresses_create(
-        r->addresses->naddrs, NULL /* user_data_vtable */);
-    for (size_t i = 0; i < r->addresses->naddrs; ++i) {
-      grpc_lb_addresses_set_address(
-          addresses, i, &r->addresses->addrs[i].addr,
-          r->addresses->addrs[i].len, false /* is_balancer */,
-          NULL /* balancer_name */, NULL /* user_data */);
-    }
-    grpc_arg new_arg = grpc_lb_addresses_create_channel_arg(addresses);
+  r->pending_request = NULL;
+  if (r->lb_addresses != NULL) {
+    grpc_arg new_arg = grpc_lb_addresses_create_channel_arg(r->lb_addresses);
     result = grpc_channel_args_copy_and_add(r->channel_args, &new_arg, 1);
-    grpc_resolved_addresses_destroy(r->addresses);
-    grpc_lb_addresses_destroy(exec_ctx, addresses);
+    grpc_lb_addresses_destroy(exec_ctx, r->lb_addresses);
   } else {
     const char *msg = grpc_error_string(error);
     gpr_log(GPR_DEBUG, "dns resolution failed: %s", msg);
@@ -209,10 +206,11 @@ static void dns_ares_start_resolving_locked(grpc_exec_ctx *exec_ctx,
   GRPC_RESOLVER_REF(&r->base, "dns-resolving");
   GPR_ASSERT(!r->resolving);
   r->resolving = true;
-  r->addresses = NULL;
-  grpc_dns_lookup_ares(exec_ctx, r->dns_server, r->name_to_resolve,
-                       r->default_port, r->interested_parties,
-                       &r->dns_ares_on_resolved_locked, &r->addresses);
+  r->lb_addresses = NULL;
+  r->pending_request = grpc_dns_lookup_ares(
+      exec_ctx, r->dns_server, r->name_to_resolve, r->default_port,
+      r->interested_parties, &r->dns_ares_on_resolved_locked, &r->lb_addresses,
+      true /* check_grpclb */);
 }
 
 static void dns_ares_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx,
@@ -222,6 +220,7 @@ static void dns_ares_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx,
     *r->target_result = r->resolved_result == NULL
                             ? NULL
                             : grpc_channel_args_copy(r->resolved_result);
+    gpr_log(GPR_DEBUG, "dns_ares_maybe_finish_next_locked");
     grpc_closure_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE);
     r->next_completion = NULL;
     r->published_version = r->resolved_version;
@@ -245,10 +244,10 @@ static void dns_ares_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) {
 static grpc_resolver *dns_ares_create(grpc_exec_ctx *exec_ctx,
                                       grpc_resolver_args *args,
                                       const char *default_port) {
-  // Get name from args.
+  /* Get name from args. */
   const char *path = args->uri->path;
   if (path[0] == '/') ++path;
-  // Create resolver.
+  /* Create resolver. */
   ares_dns_resolver *r = gpr_zalloc(sizeof(ares_dns_resolver));
   grpc_resolver_init(&r->base, &dns_ares_resolver_vtable, args->combiner);
   if (0 != strcmp(args->uri->authority, "")) {

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h

@@ -47,5 +47,9 @@ grpc_error *grpc_ares_ev_driver_create(grpc_ares_ev_driver **ev_driver,
    of ARES_ECANCELLED. */
 void grpc_ares_ev_driver_destroy(grpc_ares_ev_driver *ev_driver);
 
+/* Shutdown all the grpc_fds used by \a ev_driver */
+void grpc_ares_ev_driver_shutdown(grpc_exec_ctx *exec_ctx,
+                                  grpc_ares_ev_driver *ev_driver);
+
 #endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_RESOLVER_DNS_C_ARES_GRPC_ARES_EV_DRIVER_H \
           */

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c

@@ -99,9 +99,12 @@ static void fd_node_destroy(grpc_exec_ctx *exec_ctx, fd_node *fdn) {
   GPR_ASSERT(!fdn->writable_registered);
   gpr_mu_destroy(&fdn->mu);
   grpc_pollset_set_del_fd(exec_ctx, fdn->ev_driver->pollset_set, fdn->grpc_fd);
-  grpc_fd_shutdown(exec_ctx, fdn->grpc_fd,
-                   GRPC_ERROR_CREATE_FROM_STATIC_STRING("fd node destroyed"));
-  grpc_fd_orphan(exec_ctx, fdn->grpc_fd, NULL, NULL, "c-ares query finished");
+  /* c-ares library has closed the fd inside grpc_fd. This fd may be picked up
+     immediately by another thread, and should not be closed by the following
+     grpc_fd_orphan. To prevent this fd from being closed by grpc_fd_orphan,
+     a fd pointer is provided. */
+  int fd;
+  grpc_fd_orphan(exec_ctx, fdn->grpc_fd, NULL, &fd, "c-ares query finished");
   gpr_free(fdn);
 }
 
@@ -140,6 +143,20 @@ void grpc_ares_ev_driver_destroy(grpc_ares_ev_driver *ev_driver) {
   grpc_ares_ev_driver_unref(ev_driver);
 }
 
+void grpc_ares_ev_driver_shutdown(grpc_exec_ctx *exec_ctx,
+                                  grpc_ares_ev_driver *ev_driver) {
+  gpr_mu_lock(&ev_driver->mu);
+  ev_driver->shutting_down = true;
+  fd_node *fn = ev_driver->fds;
+  while (fn != NULL) {
+    grpc_fd_shutdown(
+        exec_ctx, fn->grpc_fd,
+        GRPC_ERROR_CREATE_FROM_STATIC_STRING("grpc_ares_ev_driver_shutdown"));
+    fn = fn->next;
+  }
+  gpr_mu_unlock(&ev_driver->mu);
+}
+
 // Search fd in the fd_node list head. This is an O(n) search, the max possible
 // value of n is ARES_GETSOCK_MAXNUM (16). n is typically 1 - 2 in our tests.
 static fd_node *pop_fd_node(fd_node **head, int fd) {