clusterresolver: comply with A37 for handling errors from discovery mechanisms #6461

zasweq · 2023-07-20T16:33:00Z

Fixes #6462. This PR fixes cluster_resolver error handing. The desired behavior as per gRFC A37 is to create a priority and send down an empty address list if an EDS Discovery Mechanism or DNS Discovery Mechanism errors before a first good update, triggering Transient Failure for that priority and causing fallback if another lower level priority. In master, we simply don't create the priority child in the EDS case and send down a resolver error in the DNS case.

RELEASE NOTES: N/A

easwars · 2023-07-21T01:02:29Z

xds/internal/balancer/clusterresolver/configbuilder.go

@@ -190,7 +190,19 @@ func buildClusterImplConfigForEDS(g *nameGenerator, edsResp xdsresource.Endpoint
 		})
 	}

-	priorities := groupLocalitiesByPriority(edsResp.Localities)
+	var priorities [][]xdsresource.Locality
+	// Triggered by an EDS error before update, or empty localities list in a


Or resource-not-found error?

To me, EDS error meant resource not found || NACK. I'll change language to be explicit about these two error cases though.

easwars · 2023-07-21T01:03:52Z

xds/internal/balancer/clusterresolver/configbuilder.go

+		// data, and it has not previously reported any results, it should
+		// report a result that is a single priority with no endpoints." - A37
+		priorities = make([][]xdsresource.Locality, 1)
+		priorities[0] = make([]xdsresource.Locality, 0)


How about:

priorities := [][]xdsresource.Locality{{}}

Since the priorities var is defined as a slice of slice of xdsresource.Locality, the compiler can figure out the type of the elements in the slice (when defined statically) without them having to be mentioned explicitly. So, in the above statement {} indicates an zero-value xdsresource.Locality.

Wouldn't the {} be a zero value of []xdsresource.Locality? Thus setting to nil rather than an empty slice? I don't think it matters though, I forget why I didn't want this to be nil I'll try switching it over.

easwars · 2023-07-21T01:08:04Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

-	dr.topLevelResolver.onError(err)
+	dr.mu.Lock()
+	// Don't report any errors after first error or good address update.
+	if dr.addrs != nil {


Should this be if !dr.updateReceived { ... } instead?

Yeah. It can be. I was debating whether using the addrs or the bool. I guess if you don't like the addrs := make(resolver.Address, 0) call I can use the bool.

easwars · 2023-07-21T01:10:39Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

@@ -142,7 +142,17 @@ func (dr *dnsDiscoveryMechanism) ReportError(err error) {
 		dr.logger.Infof("DNS discovery mechanism for resource %q reported error: %v", dr.target, err)
 	}

-	dr.topLevelResolver.onError(err)
+	dr.mu.Lock()
+	// Don't report any errors after first error or good address update.


Why is it important to suppress subsequent errors? It probably doesn't have any downstream effects today, but are there any other good reasons?

Hmmm btw I talked to Doug yesterday about this in our 1:1. The channel eats errors from the resolver if it's already received a good address update, however he mentioned it's also fine to add an extra eat for that scenario at this level. My logic was to suppress since no downstream effects, and also to keep it similar to how you handled subsequent errors in eds resolver (write an empty update, next error returns early).

Maybe you can be a little more verbose in this comment. Maybe this is too verbose, but something to indicate why suppressing the errors is beneficial here:

// If a previous good update was received, suppress the error and continue using the
// previous update. If RPCs were succeeding prior to this, they will continue to do so.
// Also suppress errors if we previously received an error, since there will be no
// downstream effects of propagating this error.

Thanks for suggestion. Used your comment.

easwars · 2023-07-21T01:11:23Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

+		dr.mu.Unlock()
+		return
+	}
+	dr.addrs = make([]string, 0)


dr.addrs = nil is more readable, and avoids an unnecessary call to make.

As per comment above, I did this to make the conditional at beginning gating subsequent calls on addrs based off this addrs struct. However, I don't actually think you need to set it to nil here. The only way it hits this codeblock is if it's an error before update, in which case the addrs will be nil already. In the case that the addrs get set to non nil, it won't hit this codeblock. I'll set it anyway though. Also, I was scared that the getAddrs() switch won't see the type as []string if it's nil, but thinking about it I think it will.

easwars · 2023-07-21T01:13:01Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

+	dr.updateReceived = true
+	dr.mu.Unlock()
+
+	dr.topLevelResolver.onUpdate()


Since we are calling onUpdate for both good updates and errors, we no longer need the onError method on the topLevelResolver. EDS discovery mechanism does not use it either. So, you can delete the implementation of that method in resourceResolver and remove that method from the topLevelResolver interface.

Hmmm ok. Will delete.

It's actually being called in DNS Discovery Mechanism Construction errors, but as per the back and forth in chat I need to switch this to do same thing DNS errors before update do - create the priority and send TF down.

I did this in Discovery Mechanism Constructor same way as OnError - set updateReceived to true and then call onUpdate on the top level resolver. Let me know if you want to change the bool (I need to set it for lastUpdate() to return true).

Let me know if you want to change the bool

What do you mean by this? I'm good with how it is looking now.

I changed the construction to also set this bool and trigger TF. Thus, I don't know if the naming is appropriate. Has it received an update, or is it just triggering an error? I'm fine leaving as is also though.

zasweq · 2023-07-21T15:45:57Z

Putting this here so I don't forget; I also need to add a test/codepath for errors in construction of DNS Discovery Mechanism before update triggering TF.

…r TF through empty priority

easwars · 2023-07-21T18:22:50Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

@@ -75,13 +75,15 @@ func newDNSResolver(target string, topLevelResolver topLevelResolver, logger *gr
 	}
 	u, err := url.Parse("dns:///" + target)
 	if err != nil {
-		topLevelResolver.onError(fmt.Errorf("failed to parse dns hostname %q in clusterresolver LB policy", target))
+		ret.updateReceived = true


We should probably throw a warning log here and in the next error case. I would even be OK with an error log.

To keep it consistent with the error logs already in codebase (from ReportError), switched to Info log.

Nit: Log lines should be capitalized, unlike error strings.

Oh interesting, noted. Switched.

easwars · 2023-07-21T18:29:49Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

@@ -142,7 +142,17 @@ func (dr *dnsDiscoveryMechanism) ReportError(err error) {
 		dr.logger.Infof("DNS discovery mechanism for resource %q reported error: %v", dr.target, err)
 	}

-	dr.topLevelResolver.onError(err)
+	dr.mu.Lock()
+	// Don't report any errors after first error or good address update.


Maybe you can be a little more verbose in this comment. Maybe this is too verbose, but something to indicate why suppressing the errors is beneficial here:

// If a previous good update was received, suppress the error and continue using the
// previous update. If RPCs were succeeding prior to this, they will continue to do so.
// Also suppress errors if we previously received an error, since there will be no
// downstream effects of propagating this error.

easwars · 2023-07-21T18:30:26Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

+	dr.updateReceived = true
+	dr.mu.Unlock()
+
+	dr.topLevelResolver.onUpdate()


Let me know if you want to change the bool

What do you mean by this? I'm good with how it is looking now.

easwars · 2023-07-21T18:31:10Z

Putting this here so I don't forget; I also need to add a test/codepath for errors in construction of DNS Discovery Mechanism before update triggering TF.

Haven't reviewed the tests yet. Will wait for this.
The code LGTM, btw.

zasweq · 2023-07-21T21:00:54Z

Oh I finished that, forgot to note (the error in construction). I didn't write a test for it since there was no tests for master and it seems like as per xDS chat we'll be adding validations in xDS Client unmarshaling that makes this impossible to hit (Mark has a pending PR). Right now, switched to what c-core does which is to trigger this TF on errors in DNS Discovery Mechanism construction.

easwars · 2023-07-21T22:06:24Z

xds/internal/balancer/clusterresolver/configbuilder.go

+	var priorities [][]xdsresource.Locality
+	// Triggered by an NACK or resource-not-found error before update, or a
+	// empty localities list in a update. In either case want to create a
+	// priority, and send down empty address list, causing TF for that priority.
+	if len(edsResp.Localities) == 0 {
+		// "If any discovery mechanism instance experiences an error retrieving
+		// data, and it has not previously reported any results, it should
+		// report a result that is a single priority with no endpoints." - A37
+		priorities = [][]xdsresource.Locality{{}}
+	} else {
+		priorities = groupLocalitiesByPriority(edsResp.Localities)
+	}


Minor nit. You can shorten this even further:

priorities := [][]xdsresource.Locality{{}} if len(edsResp.Localities) != 0 { priorities = groupLocalitiesByPriority(edsResp.Localities) }

Thanks for suggestion. Switched.

easwars · 2023-07-21T22:07:30Z

xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go

-	// cluster. Also configure an empty endpoints resource for the EDS cluster
-	// that contains no endpoints.
+	// cluster. Also configure an endpoints resource for the EDS cluster which
+	// triggers an NACK.


Sorry: s/an NACK/a NACK/

Hahaha, switched.

easwars · 2023-07-21T22:12:28Z

xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go

+	for ; ctx.Err() == nil; <-time.After(defaultTestShortTimeout) {
+		peer := &peer.Peer{}
+		if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.Peer(peer)); err != nil {
+			t.Logf("EmptyCall() failed: %v", err)
+			continue
+		}
+		if peer.Addr.String() == addrs[0].Addr {
+			break
+		}
+	}
+	if ctx.Err() != nil {
+		t.Fatalf("Timeout when waiting for RPCs to be routed to backend %q in the DNS cluster", addrs[0].Addr)
+	}


Is it possible to use this helper function here: https://github.com/grpc/grpc-go/blob/master/internal/testutils/pickfirst/pickfirst.go#L41

Yup, that worked :). Note that the other tests in this file don't use this helper (since you always rewrite these haha :D).

easwars · 2023-07-21T22:18:05Z

xds/internal/balancer/clusterresolver/resource_resolver_dns.go

@@ -75,13 +75,15 @@ func newDNSResolver(target string, topLevelResolver topLevelResolver, logger *gr
 	}
 	u, err := url.Parse("dns:///" + target)
 	if err != nil {
-		topLevelResolver.onError(fmt.Errorf("failed to parse dns hostname %q in clusterresolver LB policy", target))
+		ret.updateReceived = true


Nit: Log lines should be capitalized, unlike error strings.

Fix cluster_resolver error handling

70d3e23

zasweq requested a review from easwars July 20, 2023 16:33

zasweq assigned easwars Jul 20, 2023

zasweq added the Type: Bug label Jul 20, 2023

zasweq added this to the 1.57 Release milestone Jul 20, 2023

Unused function vet

0065af2

easwars requested changes Jul 21, 2023

View reviewed changes

easwars assigned zasweq and unassigned easwars Jul 21, 2023

Repsonded to Easwar's comments and errors in construction also trigge…

649efc6

…r TF through empty priority

easwars reviewed Jul 21, 2023

View reviewed changes

Responded to Easwar's comments

c4f49aa

zasweq assigned easwars and unassigned zasweq Jul 21, 2023

easwars changed the title ~~xds/internal/balancer/clusterresolver: Fix cluster_resolver error handling~~ clusterresolver: comply with A37 for handling errors from discovery mechanisms Jul 21, 2023

easwars approved these changes Jul 21, 2023

View reviewed changes

Responded to Easwar's comments

4b1a54e

zasweq merged commit 2aa2615 into grpc:master Jul 24, 2023
1 check passed

github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clusterresolver: comply with A37 for handling errors from discovery mechanisms #6461

clusterresolver: comply with A37 for handling errors from discovery mechanisms #6461

zasweq commented Jul 20, 2023 •

edited

Loading

easwars Jul 21, 2023

zasweq Jul 21, 2023 •

edited

Loading

easwars Jul 21, 2023

zasweq Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 21, 2023 •

edited

Loading

easwars Jul 21, 2023

zasweq Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 21, 2023

zasweq Jul 21, 2023

zasweq Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 21, 2023

zasweq commented Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 21, 2023

easwars Jul 21, 2023

zasweq Jul 24, 2023

easwars Jul 21, 2023

easwars Jul 21, 2023

easwars commented Jul 21, 2023

zasweq commented Jul 21, 2023 •

edited

Loading

easwars Jul 21, 2023

zasweq Jul 24, 2023

easwars Jul 21, 2023

zasweq Jul 24, 2023

easwars Jul 21, 2023

zasweq Jul 24, 2023

easwars Jul 21, 2023

clusterresolver: comply with A37 for handling errors from discovery mechanisms #6461

clusterresolver: comply with A37 for handling errors from discovery mechanisms #6461

Conversation

zasweq commented Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

zasweq Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jul 21, 2023

zasweq commented Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Jul 20, 2023 •

edited

Loading

zasweq Jul 21, 2023 •

edited

Loading

zasweq Jul 21, 2023 •

edited

Loading

zasweq commented Jul 21, 2023 •

edited

Loading