pickfirst: New pick first policy for dualstack

[arjan-bal]

The dualstack support project will add Happy Eyeballs and change pick first to be a universal leaf LB policy. However, it details some architectural differences between C-core and Java/Go for the subchannel and pick first policy.

In Java and Go, pick first logic is implemented within the subchannel itself instead of inside the load balancer unlike C-core.

We will take this opportunity to bring gRPC to a more uniform architecture across implementations and write a new pick first policy. This is important so that different implementations do not continue to diverge as more features are implemented.

This change will include creating a PickFirstLeafLoadBalancer which will contain the pick first logic, as well as redesigning some components such as backoffs and address updates. This will set us up nicely to implement Happy Eyeballs and use pick first as a universal leaf policy.

The new pick first policy is not used by default and can be set using an environment variable. A GitHub actions workflow is created to run the tests with the env var set. Code coverage is also calculated by running with and without the env var set.

This implementation is based on the Java implementation.

RELEASE NOTES:

• An experimental pick first LB policy is added which created one subconn per address. This is disabled by default until the dualstack changes are completed.

[codecov]

Codecov Report

Attention: Patch coverage is 85.90164% with 43 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (d00dd8f) to head (34da793).
Report is 49 commits behind head on master.

Files with missing lines	Patch %	Lines
balancer/pickfirstleaf/pickfirstleaf.go	85.71%	28 Missing and 15 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7498      +/-   ##
==========================================
+ Coverage   81.41%   81.93%   +0.51%     
==========================================
  Files         357      362       +5     
  Lines       27261    28116     +855     
==========================================
+ Hits        22195    23036     +841     
- Misses       3843     3869      +26     
+ Partials     1223     1211      -12     

Files with missing lines	Coverage Δ
balancer/pickfirst/pickfirst.go	84.00% <100.00%> (+0.26%)	⬆️
clientconn.go	93.09% <100.00%> (-0.02%)	⬇️
internal/envconfig/envconfig.go	100.00% <ø> (ø)
balancer/pickfirstleaf/pickfirstleaf.go	85.71% <85.71%> (ø)

... and 83 files with indirect coverage changes

[easwars]

Adding @dfawley for a second set of eyes. The changes are close to LGTM. I'm making another pass to see if I have more comments. But I feel it is good for a second review. Thanks.

[easwars]

What if the name resolver starts off by reporting an error? The balancer would be in Idle in that case, right? Should we move to TF in that case? Interested to know how other languages handle this case.

[arjan-bal]

I copied this behaviour from the existing pickfirst.

grpc-go/balancer/pickfirst/pickfirst.go

Lines 87 to 104 in 845f62c

	func (b *pickfirstBalancer) ResolverError(err error) {
	if b.logger.V(2) {
	b.logger.Infof("Received error from the name resolver: %v", err)
	}
	if b.subConn == nil {
	b.state = connectivity.TransientFailure
	}
	if b.state != connectivity.TransientFailure {
	// The picker will not change since the balancer does not currently
	// report an error.
	return
	}
	b.cc.UpdateState(balancer.State{
	ConnectivityState: connectivity.TransientFailure,
	Picker: &picker{err: fmt.Errorf("name resolver error: %v", err)},
	})
	}

Java unconditionally puts the LB in TF: https://github.com/grpc/grpc-java/blob/c63e3548835e838ded2f9eaf3be03dcb6b2b53a7/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java#L195-L207

c-core handled resolution failures in the client channel instead of calling the LB policy. It seems to behave similar to Go's existing pickfirst by only transitioning to TF when there is no LB policy.
https://github.com/grpc/grpc/blob/24e341be62d4411fb7c00c48e8ebc8c538544a0d/src/core/client_channel/client_channel.cc#L1167-L1183

[easwars]

Java unconditionally puts the LB in TF

That doesn't seem like the correct thing to do. But it also looks like they are invoking this method only when NR returns zero addresses. And if that is the case, then the behavior seems correct to me.

We handle the case where the name resolver returns zero addresses, and the case where the name resolver returns an error in the same method resolverError which doesn't lend itself to doing the right thing under all circumstances looks like.

To me, this is what the behavior should be:

• If we have a previous good update from the NR (i.e. we are currently READY), and we get an error from it, ignore the error and continue to use the old good state. For all other cases where we get an error from the name resolver, move to TF
• If we get zero addresses from the NR, move to TF unconditionally

@dfawley : Thoughts?

[dfawley]

To me, this is what the behavior should be:

That sounds right to me. And I think it's handled by the old PF by that first if subConn == nil which is the "has no addresses already".

However, I don't think I agree with:

doesn't lend itself to doing the right thing under all circumstances

In what situation do you think we're doing it wrong?

Also, as an aside, we should be handling ResolverErrors synchronously just like UpdateClientConnState calls.

[arjan-bal]

@easwars, to ensure point 2 (If we get zero addresses from the NR, move to TF unconditionally) is correctly handled, I have a check in updateClientConnState for an empty address list which sets balancer.sate = TF before calling resolverError.

	if len(state.ResolverState.Addresses) == 0 && len(state.ResolverState.Endpoints) == 0 {
		// Cleanup state pertaining to the previous resolver state.
		// Treat an empty address list like an error by calling b.ResolverError.
		b.state = connectivity.TransientFailure
		b.resolverError(errors.New("produced zero addresses"))
		return balancer.ErrBadResolverState
	}

This should be equivalent to the check for if subConn == nil and setting b.state = TF in the old PF's ResolveError().

[arjan-bal]

@dfawley changed ResolveError to be synchronous.

[easwars]

That sounds right to me. And I think it's handled by the old PF by that first if subConn == nil which is the "has no addresses already".

That's true.

[easwars]

doesn't lend itself to doing the right thing under all circumstances

In what situation do you think we're doing it wrong?

Hmm, I feel more than correctness, its about how easy it is to follow all the possible scenarios. Looks like the second bullet is handled in the old pickfirst and new one. But the first bullet is completely handled only in the old pickfirst. It is not handled in the new pickfirst. Specifically the case where the NR has not sent an update so far, but reports an error. The new pickfirst will see that if b.state != connectivity.TransientFailure { ... } and will return early instead of moving to TF.

Can we also have a test for this scenario?

[easwars]

In Go, if we get an error from the NR before any update, we try to apply the default service config (if one exists). https://github.com/grpc/grpc-go/blob/master/clientconn.go#L734-L745

But if we haven't built the LB policy yet, the gracefulswitch balancer handles this case and puts the channel in TF. So, looks like the expected behavior happens, but it is quite convoluted.

Maybe, we have a comment in the new pickfirst's resolverError() method saying that if the NR returns an error before sending an update, it is handled by the gracefulswitch balancer (which is always the top-level LB on any channel).

[arjan-bal]

Can we also have a test for this scenario?

Added a unit test that verifies that the ClientConn goes from IDLE->TF->Shutdown when a resolver error is reported and the clientconn is shutdown.

we have a comment in the new pickfirst's resolverError()

Added a comment as suggested.

[easwars]

LGTM.

A couple of open questions, where I have tagged Doug.

And a couple of minor nits in the test.

[easwars]

Open issues requiring attention from @dfawley :
#7498 (comment)
#7498 (comment)

[easwars]

Don't you have to block on doneCh at the end of this method?

[arjan-bal]

Oops, added the channel wait.

[easwars]

That sounds right to me. And I think it's handled by the old PF by that first if subConn == nil which is the "has no addresses already".

That's true.

[easwars]

doesn't lend itself to doing the right thing under all circumstances

In what situation do you think we're doing it wrong?

Hmm, I feel more than correctness, its about how easy it is to follow all the possible scenarios. Looks like the second bullet is handled in the old pickfirst and new one. But the first bullet is completely handled only in the old pickfirst. It is not handled in the new pickfirst. Specifically the case where the NR has not sent an update so far, but reports an error. The new pickfirst will see that if b.state != connectivity.TransientFailure { ... } and will return early instead of moving to TF.

Can we also have a test for this scenario?

[easwars]

In Go, if we get an error from the NR before any update, we try to apply the default service config (if one exists). https://github.com/grpc/grpc-go/blob/master/clientconn.go#L734-L745

But if we haven't built the LB policy yet, the gracefulswitch balancer handles this case and puts the channel in TF. So, looks like the expected behavior happens, but it is quite convoluted.

Maybe, we have a comment in the new pickfirst's resolverError() method saying that if the NR returns an error before sending an update, it is handled by the gracefulswitch balancer (which is always the top-level LB on any channel).