xds/ringhash: make reconnection logic work for a single subConn

[easwars]

Summary of changes:

• Always send a picker update (even if the ring hasn't changed) when the LB policy receives a subConn update.
  • Earlier, we were only sending a picker update when the effective state of the subConn changed. Current change helps with re-queueing RPCs in cases where:
    • there is a single subConn, and/or
    • subConn move from TRANSIENT_FAILURE to a non READY state.
• Regenerate the picker when the subConn moves to READY. This is required when the aggregate state of the channel is changing from TRANSIENT_FAILURE to READY, and we need to overwrite the earlier error picker that we had pushed.
• Ensure that the LB policy is attempting to reconnect (when the aggregate state of the channel is TRANSIENT_FAILURE), even if there is a single subConn.

Fixes #5594

RELEASE NOTES:

• xds/ringhash: fix a bug which sometimes prevents the LB policy from retrying connection attempts

[dfawley]

Always send a picker update

On the other hand, this will cause RPC churn if the effective state of the ring hash hasn't changed as a result of the subconn state change. I.e. if the transition is from TF to IDLE, we can kick the subconn into CONNECTING (presumably that's the right thing to do) but not produce a new picker, because sticky-TF means that nothing has changed. If it doesn't add too much complexity to the code and isn't too much work, it would be nice to suppress updates where we know they are unnecessary.

[dfawley]

Logging the exact same text here could make it hard to distinguish which operation is occurring. Did we just queue the connect attempt, or was it queued when the subconn went IDLE? Maybe this distinction is unimportant?

[easwars]

The connection attempt is queued (by calling queueConnect() method) from UpdateSubConnState() when we want to start connecting on the next available subConn.

But this queueConnect() method actually starts connecting only if the subConn is in IDLE. For all other states, it set the connectQueued bit to true, and when that subConn eventually transitions to IDLE, the connect attempt is made. See setState() which is called from UpdateSubConnState().

I think the distinction is not important here, whether the connection attempt happened right when it was queued or when it eventually moved to IDLE. What do you think?

[easwars]

Always send a picker update

On the other hand, this will cause RPC churn if the effective state of the ring hash hasn't changed as a result of the subconn state change. I.e. if the transition is from TF to IDLE, we can kick the subconn into CONNECTING (presumably that's the right thing to do) but not produce a new picker, because sticky-TF means that nothing has changed. If it doesn't add too much complexity to the code and isn't too much work, it would be nice to suppress updates where we know they are unnecessary.

Undid these changes after an offline discussion.

[easwars]

Thanks for the review.

[easwars]

The connection attempt is queued (by calling queueConnect() method) from UpdateSubConnState() when we want to start connecting on the next available subConn.

But this queueConnect() method actually starts connecting only if the subConn is in IDLE. For all other states, it set the connectQueued bit to true, and when that subConn eventually transitions to IDLE, the connect attempt is made. See setState() which is called from UpdateSubConnState().

I think the distinction is not important here, whether the connection attempt happened right when it was queued or when it eventually moved to IDLE. What do you think?

[dfawley]

Code golf:

for ; ctx.Err() == nil && cc.GetState != connectivity.Idle; <-time.After(...) {}

But why not use the connectivity state API? :

	for state := cc.GetState(); state != connectivity.Idle && cc.WaitForStateChange(ctx, state); state = cc.GetState() {}

[easwars]

Done.