grpclb stabilization

[markdroth]

As per gRFC A26 (grpc/proposal#164), remove support in core for special balancer addresses that cause grpclb to be selected by default. Instead, balancer addresses are now passed via a channel arg that is known to both the grpclb policy and the c-ares DNS resolver, and the only way to select the grpclb policy is via the service config.

---

This change is 

[apolcyn]

One main concern, commented on gRFC in https://github.com/grpc/proposal/pull/164/files#r339235228

[AspirinSJL]

Reviewed 13 of 28 files at r1, 19 of 19 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn and @markdroth)

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1271 at r2 (raw file):

  const ServerAddressList* addresses =
      FindGrpclbBalancerAddressesInChannelArgs(args);
  if (addresses != nullptr) balancer_addresses = *addresses;

We can return *addresses directly from here.

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1484 at r2 (raw file):

backend_addresses

This name can be updated to something like addresses_out. Or, maybe we can process address in place now that it only contains the backend addresses.

---

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 712 at r3 (raw file):

            sizeof(grpc_resolved_address) * (*resolved_addresses)->naddrs));
    for (size_t i = 0; i < (*resolved_addresses)->naddrs; ++i) {
      GPR_ASSERT(!(*r->addresses)[i].IsBalancer());

Question about the previous code: why do we have this assert? Isn't it expected that some addresses are balancer addresses?

---

test/cpp/end2end/grpclb_end2end_test.cc, line 539 at r3 (raw file):

      GPR_ASSERT(grpc_parse_uri(lb_uri, &address));
      grpc_arg arg =
          grpc_core::CreateGrpclbBalancerNameArg(addr.balancer_name.c_str());

Should we add balancer name arg for backend addresses?

---

tools/distrib/check_include_guards.py, line 47 at r3 (raw file):

        self.define_re = re.compile(r'#define ([A-Z][A-Z_1-9]*)')
        self.endif_c_re = re.compile(
            r'#endif /\* (?: *\\\n *)?([A-Z][A-Z_1-9]*) (?:\\\n *)?\*/$')

Why should this file be changed?

[markdroth]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn and @AspirinSJL)

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1271 at r2 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

We can return *addresses directly from here.

Done.

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1484 at r2 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

backend_addresses

This name can be updated to something like addresses_out. Or, maybe we can process address in place now that it only contains the backend addresses.

Done.

---

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 712 at r3 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Question about the previous code: why do we have this assert? Isn't it expected that some addresses are balancer addresses?

This code path is only used when SRV lookups are disabled, so there will never be any balancer addresses here.

---

test/cpp/end2end/grpclb_end2end_test.cc, line 539 at r3 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Should we add balancer name arg for backend addresses?

They're not necessary, but they don't hurt anything, because nothing looks for them.

---

tools/distrib/check_include_guards.py, line 47 at r3 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Why should this file be changed?

In the new grpclb_balancer_addresses.h file, the macro name for the include guard is so long that clang-format is putting it on its own line, and check_include_guards was not properly handling that. This change fixes it to work properly in this case.

[AspirinSJL]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)

[markdroth]

Known issues: #21626 #19627 #16003 #15027 #20198 #21627

[jtattermusch]

According to my analysis, #21627 is listed as "known" failure in this PR, while in fact it wasn't failing on master before and has actually been introduced by this PR.