grpclb: teach the manual resolver to handle restarts #6635

easwars · 2023-09-14T20:03:42Z

An instance of lbManualResolver is created when the grpclb policy is built. After this, every time the LB policy receives new balancer addresses as part of its configuration, it pushes those addresses to the grpclb channel via this manual resolver.

If the grpclb channel goes IDLE, which it technically should never because it always has a stream open to the remote balancer, grpc shuts down the manual name resolver and when it exits IDLE, grpc recreates the name resolver. But at this point, there is no configuration update on the parent grpclb LB policy. So, the grpclb channel is stuck forever waiting for addresses.

This PR caches the most recent list of addresses received in the manual resolver, and replays the same when it is recreated (when exiting IDLE).

RELEASE NOTES:

grpclb: fix a corner case bug when the grpclb channel goes IDLE

dfawley · 2023-09-14T23:06:24Z

balancer/grpclb/grpclb_util.go

 // When ResolveNow() is called, it calls ResolveNow() on the parent ClientConn,
 // so when grpclb client lose contact with remote balancers, the parent
 // ClientConn's resolver will re-resolve.
 type lbManualResolver struct {


Oh... this has its own manual resolver too? Can we reuse the one in resolver/manual?

If not, we probably should fix that to be tolerant to idleness by setting bootstrapState here:

grpc-go/resolver/manual/manual.go

Line 102 in 62726d4

err := r.CC.UpdateState(s)

Yes, we can reuse the manual resolver here with some changes. I will send a separate PR for that.

Changed it to use the manual resolver, but we still need this lbManualResolver type since it provides some level of wrapping of the one particular piece of functionality that is provided here. Otherwise, we will have to move all this functionality to the top-level lbBalancer type and also store the resolver.ClientConn in there.

it provides some level of wrapping of the one particular piece of functionality that is provided here

...which is what, specifically? Calling ResolveNow on the balancer.ClientConn when the ResolveNowCallback is invoked? Does that necessitate a wrapping type though?

You are right. We don't need this type at all. Got it to work by just using the manual.Resolver.

dfawley · 2023-09-14T23:07:41Z

balancer/grpclb/grpclb_util.go

+	r.stateUpdateRecv = true
+	r.stateMu.Unlock()
+
 	r.ccr.UpdateState(s)


Hmm... so what if the channel this is attached to went idle and then UpdateState is called during that time? This will just be ignored? Maybe that's fine? But there's definitely a race here with leaving idleness now, with Build being called again.

ccr here is the resolver.ClientConn passed to the manual resolver, and is the one implemented by the resolver_conn_wrapper on the grpclb channel. The parent channel going IDLE should not affect any of this interaction. If the grpclb channel went IDLE, we shouldn't have this resolver active.

If the grpclb channel went IDLE, we shouldn't have this resolver active.

I'm not sure what this means. If the grpclb channel is idle (due to a bug, since we always have an active stream) then we could still try to update it while it's exiting idle, causing a race between calling the old CC's UpdateState and the new CC being set in ccr.

We don't have this type anymore. But thinking about the case you are mentioning, let's say the following happens:

grpclb channel is IDLE because of a bug.

the parent grpclb LB policy receives a ClientConnUpdate and it has a new set of balancer addresses, and it calls UpdateState on the manual resolver

At the same time, the grpclb channel is exiting IDLE.

This will cause a new manual.Resolver to be built and will update the resolver.ClientConn inside it.

If the call to UpdateState happens first, then forwarding of that update to the old resolver.ClientConn within the manual.Resolver will have no effect. But it will still update the lastSeenState field. So, when the grpclb channel eventually exits IDLE and a new manual.Resolver is built, it will still send this most recent lastSeenState to the channel.

If the grpclb channel exits IDLE first, we will see two updates pushed on to the channel. One with the previous lastSeenState and one with the new state pushed by the grpclb LB policy.

Eventually, both are consistent. Am I still missing something?

easwars

PTAL

easwars · 2023-09-15T17:57:39Z

balancer/grpclb/grpclb_util.go

 // When ResolveNow() is called, it calls ResolveNow() on the parent ClientConn,
 // so when grpclb client lose contact with remote balancers, the parent
 // ClientConn's resolver will re-resolve.
 type lbManualResolver struct {


Changed it to use the manual resolver, but we still need this lbManualResolver type since it provides some level of wrapping of the one particular piece of functionality that is provided here. Otherwise, we will have to move all this functionality to the top-level lbBalancer type and also store the resolver.ClientConn in there.

dfawley · 2023-09-15T18:02:26Z

balancer/grpclb/grpclb_util.go

+	r.stateUpdateRecv = true
+	r.stateMu.Unlock()
+
 	r.ccr.UpdateState(s)


If the grpclb channel went IDLE, we shouldn't have this resolver active.

I'm not sure what this means. If the grpclb channel is idle (due to a bug, since we always have an active stream) then we could still try to update it while it's exiting idle, causing a race between calling the old CC's UpdateState and the new CC being set in ccr.

dfawley · 2023-09-15T18:06:21Z

balancer/grpclb/grpclb_util.go

-	r.ccb.ResolveNow(o)
+	mr.BuildCallback = func(_ resolver.Target, ccr resolver.ClientConn, _ resolver.BuildOptions) {
+		r.mu.Lock()
+		r.ccr = ccr


Isn't this functionality (and UpdateState) handled by the manual resolver internals? Or is there something different here that I'm missing?

Yup, it is. I didn't realize it was being handled there. So, we don't this type anymore.

dfawley · 2023-09-15T18:07:51Z

balancer/grpclb/grpclb_util.go

+	resolver.Builder                      // The underlying manual resolver builder.
+	resolver.Resolver                     // The underlying manual resolver.


Can this just embed *manual.Resolver instead?

We don't have this type anymore.

dfawley · 2023-09-15T18:09:23Z

balancer/grpclb/grpclb_util.go

+	mr.ResolveNowCallback = func(o resolver.ResolveNowOptions) {
+		r.ccb.ResolveNow(o)
+	}


I think you can just do: mr.ResolveNowCallback = r.ccb.ResolveNow?

dfawley · 2023-09-15T18:10:04Z

balancer/grpclb/grpclb_util.go

 // When ResolveNow() is called, it calls ResolveNow() on the parent ClientConn,
 // so when grpclb client lose contact with remote balancers, the parent
 // ClientConn's resolver will re-resolve.
 type lbManualResolver struct {


it provides some level of wrapping of the one particular piece of functionality that is provided here

...which is what, specifically? Calling ResolveNow on the balancer.ClientConn when the ResolveNowCallback is invoked? Does that necessitate a wrapping type though?

dfawley · 2023-09-18T18:16:43Z

balancer/grpclb/grpclb_util.go

-// The parent ClientConn should re-resolve when grpclb loses connection to the
-// remote balancer. When the ClientConn inside grpclb gets a TransientFailure,
-// it calls lbManualResolver.ResolveNow(), which calls parent ClientConn's
-// ResolveNow, and eventually results in re-resolve happening in parent
-// ClientConn's resolver (DNS for example).
-//
-//                          parent
-//                          ClientConn
-//  +-----------------------------------------------------------------+
-//  |             parent          +---------------------------------+ |
-//  | DNS         ClientConn      |  grpclb                         | |
-//  | resolver    balancerWrapper |                                 | |
-//  | +              +            |    grpclb          grpclb       | |
-//  | |              |            |    ManualResolver  ClientConn   | |
-//  | |              |            |     +              +            | |
-//  | |              |            |     |              | Transient  | |
-//  | |              |            |     |              | Failure    | |
-//  | |              |            |     |  <---------  |            | |
-//  | |              | <--------------- |  ResolveNow  |            | |
-//  | |  <---------  | ResolveNow |     |              |            | |
-//  | |  ResolveNow  |            |     |              |            | |
-//  | |              |            |     |              |            | |
-//  | +              +            |     +              +            | |
-//  |                             +---------------------------------+ |
-//  +-----------------------------------------------------------------+


Is this not worth keeping? It still seems largely accurate for now, except s/lbManualResolver/manual.Resolver/?

I retained only the last paragraph from this comment, and added that to the place where we set the ResolveNowCallback on the manual resolver. I feel that it conveys enough and that probably don't need this nicely drawn ASCII art. What do you think?

Seems fine. This comment was in a pretty random place anyway.

easwars requested a review from dfawley September 14, 2023 20:03

easwars assigned dfawley Sep 14, 2023

easwars added the Type: Bug label Sep 14, 2023

easwars added this to the 1.59 Release milestone Sep 14, 2023

dfawley reviewed Sep 14, 2023

View reviewed changes

dfawley assigned easwars and unassigned dfawley Sep 14, 2023

easwars added 2 commits September 15, 2023 17:51

grpclb: teach the manual resolver to handle restarts

69ecafe

use the existing manual resolver

9b88f99

easwars force-pushed the grpclb_manual_resolver branch from 5d8f0ae to 9b88f99 Compare September 15, 2023 17:54

easwars commented Sep 15, 2023

View reviewed changes

easwars assigned dfawley and unassigned easwars Sep 15, 2023

dfawley reviewed Sep 15, 2023

View reviewed changes

dfawley assigned easwars and unassigned dfawley Sep 15, 2023

completely get rid of lbManualResolver type

7d05be5

easwars assigned dfawley and unassigned easwars Sep 15, 2023

dfawley approved these changes Sep 18, 2023

View reviewed changes

dfawley assigned easwars and unassigned dfawley Sep 18, 2023

easwars merged commit 3156151 into grpc:master Sep 18, 2023

easwars deleted the grpclb_manual_resolver branch September 18, 2023 21:04

github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024

		resolver.Builder // The underlying manual resolver builder.
		resolver.Resolver // The underlying manual resolver.

grpclb: teach the manual resolver to handle restarts #6635

grpclb: teach the manual resolver to handle restarts #6635

Uh oh!

Conversation

easwars commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants