core, grpclb: change policy selection strategy for Grpclb policy (take two: move logic of querying SRV into Grpclb's own resolver) #6723

voidzcy · 2020-02-19T00:34:29Z

This is the second part of implementation for gRPCLB selection. Previous part is #6637.

Main changes in this PR:

Slightly changed ResourceResolver and its JNDI implementation. ResourceResolver#resolveSrv(String) returns a list of SrvRecord so that it only parse SRV records and does nothing more. It's gRPC's name resolver's logic to use information parsed from SRV records.
Created a GrpclbNameResolver class that extends DnsNameResolver. Logic of using information from SRV records to set balancer addresses as ResolutionResult attributes is implemented in GrpclbNameResolver only.
Refactored DnsNameResolver, mainly the resolveAll(...) method. Logics for resolving backend addresses and service config are modularized into resolveAddresses() and resolveServiceConfig() methods respectively. They are shared implementation for subclasses (i.e., GrpclbNameResolver).

TODO: AltsProtocolNegotiator has some special logic for creating channel handler, which depends on the implementation detail that grpclb balancer addresses are tagged with ATTR_LB_ADDR_AUTHORITY attributes. We may want to eliminate it and clean up GrpcAttributes. ATTR_LB_ADDR_AUTHORITY.

Update: Given that grpc-alts module already depends on grpc-grpclb, we can still clean up GrpcAttributes. ATTR_LB_ADDR_AUTHORITY and let it use GrpclbConstants. ATTR_LB_ADDR_AUTHORITY. I will do that clean up in a separate PR.

…, service config, balancer addresses. Subclass is able to override the resolve logic.

…r will provide GrpclbNameResolver directly. This also eliminate the enableSrv flag.

…ver test.

ST-DDT · 2020-02-23T20:14:27Z

I use the dns (dns+srv) scheme to resolve the target addresses of my services within Kubernetes, so I would like to have something like this in the default libraries. I don't need any extra LB overhead for these slim services.
e.g.: address=dns:///myservice.production

Having to use -Dio.grpc.internal.DnsNameResolverProvider.enable_grpclb=true in all my services is bad enough. Not having DNS+SRV support would be a configuration nightmare.

EDIT: Or am I miss-reading this PR and it turns dns+srv on by default?

voidzcy · 2020-02-24T03:23:04Z

I use the dns (dns+srv) scheme to resolve the target addresses of my services within Kubernetes, so I would like to have something like this in the default libraries. I don't need any extra LB overhead for these slim services.
e.g.: address=dns:///myservice.production

Having to use -Dio.grpc.internal.DnsNameResolverProvider.enable_grpclb=true in all my services is bad enough. Not having DNS+SRV support would be a configuration nightmare.

EDIT: Or am I miss-reading this PR and it turns dns+srv on by default?

This PR is part of the implementation for https://github.com/grpc/proposal/blob/master/A26-grpclb-selection.md. The previous part is #6637. I updated the title of this PR.

We are eliminating the usage of SRV records other than in gRPCLB. Previously, this is special logic in grpc-core for gRPCLB's design. The system variable io.grpc.internal.DnsNameResolverProvider.enable_grpclb will be eliminated.

How do you use SRV records? Did you implement a special way to use SRV resolved addresses? We expect SRV records are for remote balancer addresses. This is true in existing grpc-core as SRV resolved addresses are tagged with ATTR_LB_ADDR_AUTHORITY attribute. Only Grpclb load balancer will look at these addresses. So we choose to move all these Grpclb specific logics into grpc-grpclb.

For your case, I believe you would need to implement your own DNS-based name resolver as you are exploiting the internal implementation of DnsNameResolver.

ejona86 · 2020-02-24T15:52:19Z

@ST-DDT, in 1.25.0 we added a dns name resolver to the grpc-grpclb artifact, that overrides the default one. This name resolver in grpclb has io.grpc.internal.DnsNameResolverProvider.enable_grpclb default to true. You may want to look at the release notes for 1.25.0

You should just need to depend on grpc-grpclb and set "grpclb" to be your load balancing policy via mcb.defaultLoadBalancingPolicy() or mcb.defaultServiceConfig()

ejona86 · 2020-02-26T20:00:03Z

grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolver.java

+  }
+
+  @Override
+  protected boolean doResolve(Listener2 listener) {


I'm not wild about how coupled the GrpclbNameResolver is to DnsNameResolver. It looks quite brittle. This method seems to be the biggest issue.

We may want to discuss in person, but it seems we could try to do a delegation strategy or have the caller of this method notify the listener. Both of those seem to allow a super.doResolve() approach to let the base class do its work (instead of copying it here and treating the base class as a utility class). As it stands, it would be more straight-forward to have a shared utility class instead of extending.

Reworked this method extension. Introduced an struct for grouping components of resolution result, caller of this method will populate items out and invoke the callback.

creamsoup

i am a bit afraid, but refactoring looks good.

grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java

grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java

core/src/main/java/io/grpc/internal/DnsNameResolver.java

core/src/test/java/io/grpc/internal/DnsNameResolverTest.java

…ervice config resolution in child class.

creamsoup

LGTM

voidzcy · 2020-02-28T02:19:38Z

Do not merge until #6705 is merged.

ejona86

This code organization looks much better. I'm not approving since I've not really done a review, but what I see seems good.

…query_srv_logic_into_grpclb

…e two: move logic of querying SRV into Grpclb's own resolver) (grpc#6723) Eliminated the code path of resolving Grpclb balancer addresses in grpc-core and moved it into GrpclbNameResolver, which is a subclass of DnsNameResolver. Main changes: - Slightly changed ResourceResolver and its JNDI implementation. ResourceResolver#resolveSrv(String) returns a list of SrvRecord so that it only parse SRV records and does nothing more. It's gRPC's name resolver's logic to use information parsed from SRV records. - Created a GrpclbNameResolver class that extends DnsNameResolver. Logic of using information from SRV records to set balancer addresses as ResolutionResult attributes is implemented in GrpclbNameResolver only. - Refactored DnsNameResolver, mainly the resolveAll(...) method. Logics for resolving backend addresses and service config are modularized into resolveAddresses() and resolveServiceConfig() methods respectively. They are shared implementation for subclasses (i.e., GrpclbNameResolver).

voidzcy added 4 commits February 18, 2020 12:11

Refactor DnsNameResolver to separate logics of resolving host address…

7162123

…, service config, balancer addresses. Subclass is able to override the resolve logic.

Eliminate BaseDnsNameResolverProvider, Grpclb's name resolver provide…

75a4ce1

…r will provide GrpclbNameResolver directly. This also eliminate the enableSrv flag.

Delete test for verifying grpclb balancer addresses in dns name resol…

b935f46

…ver test.

Eliminate ATTR_LB_ADDRS in GrpcAttributes.

49f7240

voidzcy force-pushed the dev/separate_query_srv_logic_into_grpclb branch from deb0e12 to 49f7240 Compare February 19, 2020 00:35

voidzcy added 3 commits February 18, 2020 16:54

Add tests for SecretGrpclbNameResolverProvider.

a28394f

Add test for GrpclbNameResolver

446f3d2

Modify DnsNameResolver's tests for its behavior changes.

d3c6ba1

voidzcy force-pushed the dev/separate_query_srv_logic_into_grpclb branch from df1d33d to d3c6ba1 Compare February 20, 2020 02:09

Add VisibleForTesting annotations.

afa1e7d

voidzcy marked this pull request as ready for review February 21, 2020 02:11

voidzcy requested review from creamsoup and ejona86 February 21, 2020 02:11

voidzcy changed the title ~~core, grpclb: move logic of querying SRV into Grpclb's own resolver~~ core, grpclb: change policy selection strategy for Grpclb policy (take two: move logic of querying SRV into Grpclb's own resolver) Feb 24, 2020

ejona86 reviewed Feb 26, 2020

View reviewed changes

creamsoup reviewed Feb 26, 2020

View reviewed changes

voidzcy added 4 commits February 27, 2020 17:28

Refactor doResolve method to avoid repeating address resolution and s…

22d9bd5

…ervice config resolution in child class.

Fix year in preface.

4b79152

Avoid introducing if nesting.

8a981ea

Simplify fake service config parser.

740282a

voidzcy requested a review from ejona86 February 28, 2020 02:00

creamsoup approved these changes Feb 28, 2020

View reviewed changes

ejona86 reviewed Feb 29, 2020

View reviewed changes

Merge branch 'master' of github.com:grpc/grpc-java into dev/separate_…

a5f869f

…query_srv_logic_into_grpclb

voidzcy merged commit 6a7e47b into grpc:master Mar 2, 2020

voidzcy mentioned this pull request Mar 2, 2020

core, grpclb: clean up grpclb specific attributes in core #6790

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core, grpclb: change policy selection strategy for Grpclb policy (take two: move logic of querying SRV into Grpclb's own resolver) #6723

core, grpclb: change policy selection strategy for Grpclb policy (take two: move logic of querying SRV into Grpclb's own resolver) #6723

voidzcy commented Feb 19, 2020 •

edited

ST-DDT commented Feb 23, 2020 •

edited

voidzcy commented Feb 24, 2020 •

edited

ejona86 commented Feb 24, 2020

ejona86 Feb 26, 2020

voidzcy Feb 28, 2020

creamsoup left a comment

creamsoup left a comment

voidzcy commented Feb 28, 2020

ejona86 left a comment

core, grpclb: change policy selection strategy for Grpclb policy (take two: move logic of querying SRV into Grpclb's own resolver) #6723

core, grpclb: change policy selection strategy for Grpclb policy (take two: move logic of querying SRV into Grpclb's own resolver) #6723

Conversation

voidzcy commented Feb 19, 2020 • edited

ST-DDT commented Feb 23, 2020 • edited

voidzcy commented Feb 24, 2020 • edited

ejona86 commented Feb 24, 2020

ejona86 Feb 26, 2020

Choose a reason for hiding this comment

voidzcy Feb 28, 2020

Choose a reason for hiding this comment

creamsoup left a comment

Choose a reason for hiding this comment

creamsoup left a comment

Choose a reason for hiding this comment

voidzcy commented Feb 28, 2020

ejona86 left a comment

Choose a reason for hiding this comment

voidzcy commented Feb 19, 2020 •

edited

ST-DDT commented Feb 23, 2020 •

edited

voidzcy commented Feb 24, 2020 •

edited