pickfirst: Use one subConn per address

[arjan-bal]

As part of the Dual Stack implementation, we would like to move the pickfirst algorithm out of the subConn and in the lb policy

We would like to take this opportunity to move toward a more uniform cross-language architecture. Also, moving pick_first up to the LB policy layer in Java and Go will have the nice effect of making their backoff work per-address instead of across all addresses, which is what C-core does and what the (poorly specified) connection backoff spec seems to have originally envisioned.

This change moves the algorithm into the lb policy and ensures the LB creates one subConn per address, but doesn't remove the the pickfirst logic from the subConn. Happy eyeballs is not implemented.

Inspiration is taken from the c-core implementation:

• latest: https://github.com/grpc/grpc/blob/master/src/core/load_balancing/pick_first/pick_first.cc
• before happy eyeballs: https://github.com/grpc/grpc/blob/b6f01c6/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc

Other changes

• Add an ExitIdle() method on stateRecordingBalancer. This is required as the fallback logic of re-connecting all the subConns no longer works. This is because the LB policy closes the active subConn once the connection is dropped and does a pick first over all the addresses when ExitIdle is called. When all the subConns are shutdown, re-connecting will have no effect.
• Update tests with new assertions based on the new behaviour.

Behaviour Changes

• Pickfirst now creates one subchannel per address.
• An exponential back-off is performed per server address when the subchannel enters transient failure.
• After every address has been tried one, pickfirst attempts to re-connect to the addresses in the order in which they complete their back-off, this is not deterministic.

RELEASE NOTES:

• Pickfirst creates one subchannel per address.

[codecov]

Codecov Report

Attention: Patch coverage is 82.91457% with 34 lines in your changes missing coverage. Please review.

Project coverage is 81.22%. Comparing base (ee62e56) to head (cb21828).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7384      +/-   ##
==========================================
- Coverage   81.50%   81.22%   -0.28%     
==========================================
  Files         348      350       +2     
  Lines       26752    26975     +223     
==========================================
+ Hits        21804    21911     +107     
- Misses       3766     3845      +79     
- Partials     1182     1219      +37     

Files	Coverage Δ
balancer/pickfirst/pickfirst.go	82.21% <82.91%> (-1.53%)	⬇️

... and 31 files with indirect coverage changes

[easwars]

Why is it safe to ignore the error here? In our existing code, if NewSubConn fails, we set the connectivity state to TransientFailure, return a picker that always returns an error (and includes the error returned from NewSubConn), and we also return balancer.ErrBadResolverState.

[arjan-bal]

This is what c-core seems to be doing. It throws an error only when we can't create a subConn for all the addresses: https://github.com/grpc/grpc/blob/a3a91a5a3cb6136d8a8d74e87b395f0ab450fd63/src/core/load_balancing/pick_first/pick_first.cc#L1017C5-L1026

I'll change it to fail fast.

[arjan-bal]

After thinking a little more, we use the next address when a connection to an address fails. By ignoring errors during creation of a subchannel for an address, we're treating the failure as a similar issue to a connection failure and using another address.
If we return an error early, we would miss the chance of using another address that we could create a subchannel for and use.

[easwars]

I see this as an error handling step that should have happened when creating the subConnList ran into errors.

[arjan-bal]

Moved the error handling logic into the refreshSubConnList method. I can't move it into the newSubConnList method since its used to create an empty subConnList in the builder where is it expected for the list to be empty initially.

[easwars]

Does this actually work?

We return early from startConnectingIfNeeded if state is not subConnListPending.

[arjan-bal]

In goIdle() we create a subConnList but don't call startConnectingIfNeeded until the idlePicker is called. So the subConnList will remain in pending. If ExitIdle() is called before the picker is used, this will cause the connection attempt to begin.

[easwars]

Some subConnList methods have a nil receiver check while others don't. When is it possible/legal to call methods on a nil subConnList?

[arjan-bal]

The subConnList is nil only when the balancer hasn't received an update from the resolver yet. I've ensured that we set an empty subConnList in the builder and removed the nil receiver checks.

[arjan-bal]

Closing while I figure out the correct sequence of making of the changes for Dual Stack.