-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds/clusterresolver: implement gRFC A61 changes for LOGICAL_DNS clusters #8733
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8733 +/- ##
=======================================
Coverage 83.30% 83.31%
=======================================
Files 419 418 -1
Lines 32450 32365 -85
=======================================
- Hits 27033 26964 -69
+ Misses 4039 4028 -11
+ Partials 1378 1373 -5
🚀 New features to boost your workflow:
|
|
This change probably needs a release note. |
| // Endpoint picking policy for DNS is hardcoded to pick_first. | ||
| const childPolicy = "pick_first" | ||
| func buildClusterImplConfigForDNS(g *nameGenerator, endpoints []resolver.Endpoint, mechanism DiscoveryMechanism, xdsLBPolicy *internalserviceconfig.BalancerConfig) (string, *clusterimpl.LBConfig, []resolver.Endpoint) { | ||
| retEndpoints := make([]resolver.Endpoint, len(endpoints)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to return a slice that contains a single endpoint.
| // Use the canonical string representation for the locality to match | ||
| // the keys expected by the parent Load Balancing policy. | ||
| localityStr := xdsinternal.LocalityString(clients.Locality{}) | ||
| retEndpoints[i] = hierarchy.SetInEndpoint(e, []string{pName, localityStr}) | ||
| retEndpoints[i] = wrrlocality.SetAddrInfoInEndpoint(retEndpoints[i], wrrlocality.AddrInfo{LocalityWeight: 1}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not doing what we want.
The DNS resolver implementation returns addresses and not endpoints, because the DNS protocol does not know about endpoints. See:
grpc-go/internal/resolver/dns/dns_resolver.go
Line 345 in 4c27cc8
| state := resolver.State{Addresses: addrs} |
We have code in the channel that converts these addresses to endpoints, with one address per endpoint. See:
Line 213 in 4c27cc8
| func addressesToEndpoints(addrs []resolver.Address) []resolver.Endpoint { |
But the above code will execute only when the DNS resolver is used on the gRPC channel. Here in the cluster_resolver LB policy, we are creating a DNS resolver ourselves, and therefore the code to convert from addresses to endpoints (that exists in the channel) will not be run here. So, the cluster_resolver will see a set of addresses and no endpoints from the DNS resolver in [resource_resolver_dns.go](https://github.com/grpc/grpc-go/pull/8733/files#diff-6249528a41f17a06cec41f598b885840f09ef05ee3740ab297fc5a905f2875ca), and will probably convert it into a single endpoint with all the addresses in it (this is the change that you have in line 138 of resource_resolver_dns.go).
But, at some point, we will change the DNS resolver implementation to do what is being done today in the channel, i.e., return a set of endpoints, with one address per endpoint. That means that this code should handle multiple endpoints as input, but output a single endpoint that contains all the addresses from the input endpoints.
| return endpoint | ||
| } | ||
|
|
||
| func TestBuildClusterImplConfigForDNS(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to test all possibilities here:
- one endpoint with one address
- one endpoint with multiple addresses
- multiple endpoints, all with one address each
- multiple endpoints, all with multiple addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also want to make sure we have a more e2e style test to cover this new logic. A LOGICAL_DNS cluster that specifies a round_robin LB policy and the DNS resolver returning multiple addresses. In this case, because cluster_resolver will convert these addresses into a single endpoint, we should end up with all traffic going to the first address (because RR will delegate to PF).
Fixes : #8153
This PR implements the "Changes to Logical DNS Clusters" section of gRFC A61 (IPv4/IPv6 Dual-stack Backends).
Currently,
LOGICAL_DNSclusters in the xDS cluster resolver have their Load Balancing policy hard-coded topick_first. This ensures the semantics of connecting to only one address at a time.This PR updates the
xds_cluster_resolverlogic. ThebuildClusterImplConfigForDNSfunction removes the hard-codedpick_firstpolicy restriction forLOGICAL_DNSclusters, allowing them to use the configured LB policy.Also, in the DNS update handler, all resolved addresses are now grouped into a single
resolver.Endpoint. This ensures that regardless of the configured parent LB policy, the child policy sees a single "backend" endpoint containing all addresses.RELEASE NOTES:
LOGICAL_DNSclusters now honor the LB policy configured in the cluster resource, rather than defaulting to a hardcodedpick_firstpolicy.