grpc: perform graceful switching of LB policies in the `ClientConn` by default #5285

easwars · 2022-03-31T04:24:39Z

This PR changes the default behavior of ClientConn when it ends up switching the LB policy.

Earlier, it used to close the old policy and start the new one, when it received a service config update pointing to a new LB policy.
Now, it uses the gracefulswitch.Balancer to gracefully switch from the old balancer to the new one.

Summary of code changes:

Use a gracefulswitch.Balancer in ccBalancerWrapper instead of balancer.Balancer
- Add a switchTo method to the balancer wrapper which calls SwitchTo on the gracefulswitch.Balancer
- Ensure calls from grpc to the underlying balancers are ordered
  - For every type of update from grpc, we now have:
    - a method which pushes the update into the update channel
    - a method which handles the update, when processed by the watcher goroutine
  - An enum to represent the type of grpc update pushed into the update channel
- The watcher goroutine is now merely a skeleton, with functionality moved to appropriate handlers
- Add more comments to the balancer wrapper code
The following functionality has been moved out of ClientConn:
- Falling back to the default LB policy when the specified LB policy is not registered is moved to the ccBalancerWrapper
- Filtering of addresses of type GRPCLB is now handled by the ccBalancerWrapper
- Manually calling Connect on the subConns if the underlying balancer does not support ExitIdle is moved to the gracefulswitch.Balancer

Summary of test changes:

Added a test in test/balancer_switching_test.go to exercise the graceful switching functionality e2e
While I was adding the above mentioned test, I figured out that I can make the stubBalancer wrap a real balancer. Updated an existing test in test/resolver_update_test.go which was using a custom balancer for the same to use the stubBalancer instead.
Added ParseConfig functionality to the stubBalancer
Updated a test in clientconn_test.go to fail within a reasonable amount of time. (This test was timing out initially when I was making my changes, so had to fix it).

Fixes #5270

RELEASE NOTES:

perform graceful switching of LB policies in the ClientConn by default

balancer_conn_wrappers.go

dfawley · 2022-03-31T16:51:54Z

balancer_conn_wrappers.go

+	// With the use of gracefulswitch.Balancer in ccBalancerWrapper, handling this
+	// asynchronously is probably not required anymore since the switchTo() method
+	// handles the balancer switch by pushing the update onto the channel.
+	// TODO(easwars): Handle this inline.


Is it worthwhile? This would be the only thing done inline, then? Seems better to keep everything going through the channel unless there's compelling reasons to skip it for this case.

This is the only call from the underlying balancer which goes through the channel instead of being handled inline. And this was being handled inline before we discovered a couple of deadlocks which forced us to release cc.mu before calling ccb.close(). But now, ccb.close() will only be called from cc.Close() and without holding cc.mu. So, it should be safe to handle this inline.

We might be able to move all the outgoing calls to the goroutine, and do all the incoming calls inline.

I have already moved all outgoing calls to the goroutine. Do you want me to switch this to be done inline in this PR?

dfawley · 2022-03-31T16:53:19Z

clientconn.go

-	cc.curBalancerName = builder.Name()
-	cc.balancerWrapper = newCCBalancerWrapper(cc, builder, cc.balancerBuildOpts)
-}
-
 func (cc *ClientConn) handleSubConnStateChange(sc balancer.SubConn, s connectivity.State, err error) {
 	cc.mu.Lock()
 	if cc.conns == nil {


Maybe this is unnecessary too? Add a TODO to remove if you agree, perhaps.

Removed this.

easwars

Thanks for your review.

balancer_conn_wrappers.go

easwars · 2022-03-31T18:18:26Z

balancer_conn_wrappers.go

+	// With the use of gracefulswitch.Balancer in ccBalancerWrapper, handling this
+	// asynchronously is probably not required anymore since the switchTo() method
+	// handles the balancer switch by pushing the update onto the channel.
+	// TODO(easwars): Handle this inline.


This is the only call from the underlying balancer which goes through the channel instead of being handled inline. And this was being handled inline before we discovered a couple of deadlocks which forced us to release cc.mu before calling ccb.close(). But now, ccb.close() will only be called from cc.Close() and without holding cc.mu. So, it should be safe to handle this inline.

easwars · 2022-03-31T19:20:01Z

clientconn.go

-	cc.curBalancerName = builder.Name()
-	cc.balancerWrapper = newCCBalancerWrapper(cc, builder, cc.balancerBuildOpts)
-}
-
 func (cc *ClientConn) handleSubConnStateChange(sc balancer.SubConn, s connectivity.State, err error) {
 	cc.mu.Lock()
 	if cc.conns == nil {


Removed this.

easwars · 2022-03-31T22:01:27Z

I was able to run the test 1000 times with the race detector without failures. Ready to be looked at again.

menghanl · 2022-03-31T21:43:50Z

internal/balancer/gracefulswitch/gracefulswitch.go

@@ -178,6 +178,10 @@ func (gsb *Balancer) ResolverError(err error) {
 }

 // ExitIdle forwards the call to the latest balancer created.
+//
+// If the latest balancer does not support ExitIdle, the subConns need to be
+// re-connected manually. This used to be done in the ClientConn earlier, but


I would delete the sentence about how it used to be. Or at least it doesn't belong here.

menghanl · 2022-03-31T21:47:24Z

balancer_conn_wrappers.go

-type exitIdle struct{}
-
-// ccBalancerWrapper is a wrapper on top of cc for balancers.
+// ccBalancerWrapper is a wrapper on top of cc for balancers. It ensures that


Hmm it's actually both a cc wrapper and a balancer wrapper.

How about now?

menghanl · 2022-03-31T21:50:42Z

balancer_conn_wrappers.go

+	updateCh *buffer.Unbounded // Updates written on this channel are processed by watcher().
+	resultCh *buffer.Unbounded // Results of calls to UpdateClientConnState() are pushed here.
+	closed   *grpcsync.Event   // Indicates if close has been called.
+	done     *grpcsync.Event   // Indicates if close has completed its work.

 	mu       sync.Mutex
 	subConns map[*acBalancerWrapper]struct{}


Do we still need this? Doesn't look like we use it for anything now.
And the gracefulswitch balancer also keeps maps of SubConns. Is that good enough?

Yes, you are right. I was able to get rid of this.

menghanl · 2022-03-31T22:32:56Z

balancer_conn_wrappers.go

+// back to grpc which propagates that to the resolver.
+func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnState) error {
+	ccb.updateCh.Put(watcherUpdate{typ: updateTypeClientConnState, update: ccs})
+	res := <-ccb.resultCh.Get()


Any chance this will block forever? When things are closing?

Thanks. I added a select and made it exit if close is called.

menghanl · 2022-03-31T22:34:31Z

balancer_conn_wrappers.go

+// and the selected LB policy is not "grpclb", these addresses will be filtered
+// out and ccs will be modified with the updated address list.
+func (ccb *ccBalancerWrapper) handleClientConnStateChange(ccs *balancer.ClientConnState) {
+	ccb.balancerMu.Lock()


Why does this hold balancerMu?

Because it calls ccb.balancer.UpdateClientConnState in the last line. Would you rather that I grab the lock around that statement?

menghanl · 2022-03-31T22:36:09Z

balancer_conn_wrappers.go

-// forward to the resolver.
-func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnState) error {
+// handleSubConnStateChange handles a SubConnState update from the update
+// channel and invokes the appropriate method on the underlying balancer.


What's the reason to not push this to the channel?
And document that.

And if we do all the outgoing calls in the watcher goroutine, maybe we don't need the balancerMu?
There were two methods that need to happen inline: Close() and ClientConnStateChange() (because of the return error). But both are handled now.

What's the reason to not push this to the channel?

SubConn state change is pushed onto the channel in updateSubConnState(). This is the method which is called when the watcher reads that update from the channel.

And if we do all the outgoing calls in the watcher goroutine, maybe we don't need the balancerMu?

Woohoo !!

balancer_conn_wrappers.go

menghanl · 2022-03-31T22:45:15Z

balancer_conn_wrappers.go

+	// With the use of gracefulswitch.Balancer in ccBalancerWrapper, handling this
+	// asynchronously is probably not required anymore since the switchTo() method
+	// handles the balancer switch by pushing the update onto the channel.
+	// TODO(easwars): Handle this inline.


We might be able to move all the outgoing calls to the goroutine, and do all the incoming calls inline.

easwars · 2022-04-01T00:17:18Z

I got rid of the checking for trace events thingy from the tests. All tests are happy now. Definitely this time around, good to be looked at another time.
@dfawley @menghanl

easwars · 2022-04-01T20:14:26Z

Thank you very much for the review @dfawley @menghanl

use gracefulswitch.Balancer in ccBalancerWrapper

07737b6

easwars added the Type: Feature label Mar 31, 2022

easwars added this to the 1.46 Release milestone Mar 31, 2022

easwars requested review from menghanl, dfawley and zasweq March 31, 2022 04:24

easwars assigned dfawley Mar 31, 2022

dfawley reviewed Mar 31, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley Mar 31, 2022

first round of review comments

b745f67

easwars commented Mar 31, 2022

View reviewed changes

dont check for round_robin after checking for grpclb trace event

d6fc583

easwars assigned dfawley and unassigned easwars Mar 31, 2022

another test fix

9af61d9

menghanl reviewed Mar 31, 2022

View reviewed changes

menghanl assigned easwars Mar 31, 2022

easwars added 5 commits March 31, 2022 15:53

don't keep of subConns in the balancer wrapper anymore

aaa96fe

improve comment on the ccBalancerWrapper type

9504370

exit early while waiting for clientconn update result if close is called

e963f80

get rid of balancerMu

1409a60

fix a comment in gsb

6f91640

easwars assigned menghanl and unassigned easwars Mar 31, 2022

easwars added 4 commits March 31, 2022 16:46

fix a few tests to not rely on the channelz trace event

a1624be

fix more tests

7d43c45

fix one more test

270c562

one last one

936ddd6

easwars mentioned this pull request Apr 1, 2022

xds: Add RLS in xDS e2e test #5281

Merged

easwars added 2 commits April 1, 2022 11:32

no need to hold cc.mu from some methods

c52b13c

add wrapper types for updates instead of an enum

d90a017

dfawley approved these changes Apr 1, 2022

View reviewed changes

do a safe type assertion in remove subConn

03232f6

dfawley approved these changes Apr 1, 2022

View reviewed changes

easwars merged commit 0066bf6 into grpc:master Apr 1, 2022

dfawley mentioned this pull request Apr 27, 2022

Gracefully switch balancers #2731

Closed

easwars deleted the graceful_switch_final branch July 15, 2022 17:25

github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: perform graceful switching of LB policies in the `ClientConn` by default #5285

grpc: perform graceful switching of LB policies in the `ClientConn` by default #5285

easwars commented Mar 31, 2022

dfawley Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

dfawley Mar 31, 2022

easwars Mar 31, 2022

easwars left a comment

easwars Mar 31, 2022

easwars Mar 31, 2022

easwars commented Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars Mar 31, 2022

menghanl Mar 31, 2022

easwars commented Apr 1, 2022

easwars commented Apr 1, 2022

grpc: perform graceful switching of LB policies in the ClientConn by default #5285

grpc: perform graceful switching of LB policies in the ClientConn by default #5285

Conversation

easwars commented Mar 31, 2022

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Mar 31, 2022

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

easwars commented Apr 1, 2022

easwars commented Apr 1, 2022

grpc: perform graceful switching of LB policies in the `ClientConn` by default #5285

grpc: perform graceful switching of LB policies in the `ClientConn` by default #5285