Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from Mar 2, 2020

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Feb 19, 2020

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.

voidzcy added 4 commits Feb 18, 2020
…, service config, balancer addresses. Subclass is able to override the resolve logic.
…r will provide GrpclbNameResolver directly. This also eliminate the enableSrv flag.
@voidzcy voidzcy force-pushed the dev/separate_query_srv_logic_into_grpclb branch from deb0e12 to 49f7240 Compare Feb 19, 2020
@voidzcy voidzcy force-pushed the dev/separate_query_srv_logic_into_grpclb branch from df1d33d to d3c6ba1 Compare Feb 20, 2020
@voidzcy voidzcy marked this pull request as ready for review Feb 21, 2020
@voidzcy voidzcy requested review from creamsoup and ejona86 Feb 21, 2020
@ST-DDT
Copy link
Contributor

@ST-DDT ST-DDT commented Feb 23, 2020

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 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
@voidzcy
Copy link
Contributor Author

@voidzcy voidzcy commented Feb 24, 2020

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
Copy link
Member

@ejona86 ejona86 commented Feb 24, 2020

@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()

}

@Override
protected boolean doResolve(Listener2 listener) {
Copy link
Member

@ejona86 ejona86 Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@voidzcy voidzcy Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@creamsoup creamsoup left a comment

i am a bit afraid, but refactoring looks good.

@voidzcy voidzcy requested a review from ejona86 Feb 28, 2020
Copy link
Contributor

@creamsoup creamsoup left a comment

LGTM

@voidzcy
Copy link
Contributor Author

@voidzcy voidzcy commented Feb 28, 2020

Do not merge until #6705 is merged.

Copy link
Member

@ejona86 ejona86 left a comment

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

@voidzcy voidzcy merged commit 6a7e47b into grpc:master Mar 2, 2020
11 of 13 checks passed
dfawley pushed a commit to dfawley/grpc-java that referenced this issue Jan 15, 2021
…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).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants