outlierdetection: Support health listener for ejection updates (#7908)

balancer/endpointsharding/endpointsharding.go

@@ -35,11 +35,9 @@ import (
 
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/balancer/base"
-	"google.golang.org/grpc/balancer/pickfirst"
 	"google.golang.org/grpc/balancer/pickfirst/pickfirstleaf"
 	"google.golang.org/grpc/connectivity"
 	"google.golang.org/grpc/internal/balancer/gracefulswitch"
-	"google.golang.org/grpc/internal/envconfig"
 	"google.golang.org/grpc/resolver"
 	"google.golang.org/grpc/serviceconfig"
 )
@@ -48,11 +46,7 @@ import (
 var PickFirstConfig string
 
 func init() {
-	name := pickfirst.Name
-	if !envconfig.NewPickFirstEnabled {
-		name = pickfirstleaf.Name
-	}
-	PickFirstConfig = fmt.Sprintf("[{%q: {}}]", name)
+	PickFirstConfig = fmt.Sprintf("[{%q: {}}]", pickfirstleaf.Name)
 }
 
 // ChildState is the balancer state of a child along with the endpoint which

balancer/pickfirst/pickfirstleaf/pickfirstleaf.go

@@ -54,9 +54,18 @@ func init() {
 	balancer.Register(pickfirstBuilder{})
 }
 
-// enableHealthListenerKeyType is a unique key type used in resolver attributes
-// to indicate whether the health listener usage is enabled.
-type enableHealthListenerKeyType struct{}
+type (
+	// enableHealthListenerKeyType is a unique key type used in resolver
+	// attributes to indicate whether the health listener usage is enabled.
+	enableHealthListenerKeyType struct{}
+	// managedByPickfirstKeyType is an attribute key type to inform Outlier
+	// Detection that the generic health listener is being used.
+	// TODO: https://github.com/grpc/grpc-go/issues/7915 - Remove this when
+	// implementing the dualstack design. This is a hack. Once Dualstack is
+	// completed, outlier detection will stop sending ejection updates through
+	// the connectivity listener.
+	managedByPickfirstKeyType struct{}
+)
 
 var (
 	logger = grpclog.Component("pick-first-leaf-lb")
@@ -140,6 +149,17 @@ func EnableHealthListener(state resolver.State) resolver.State {
 	return state
 }
 
+// IsManagedByPickfirst returns whether an address belongs to a SubConn
+// managed by the pickfirst LB policy.
+// TODO: https://github.com/grpc/grpc-go/issues/7915 - This is a hack to disable
+// outlier_detection via the with connectivity listener when using pick_first.
+// Once Dualstack changes are complete, all SubConns will be created by
+// pick_first and outlier detection will only use the health listener for
+// ejection. This hack can then be removed.
+func IsManagedByPickfirst(addr resolver.Address) bool {
+	return addr.BalancerAttributes.Value(managedByPickfirstKeyType{}) != nil
+}
+
 type pfConfig struct {
 	serviceconfig.LoadBalancingConfig `json:"-"`
 
@@ -166,6 +186,7 @@ type scData struct {
 }
 
 func (b *pickfirstBalancer) newSCData(addr resolver.Address) (*scData, error) {
+	addr.BalancerAttributes = addr.BalancerAttributes.WithValue(managedByPickfirstKeyType{}, true)
 	sd := &scData{
 		rawConnectivityState: connectivity.Idle,
 		effectiveState:       connectivity.Idle,

balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go

@@ -58,7 +58,13 @@ const (
 	stateStoringBalancerName = "state_storing"
 )
 
-var stateStoringServiceConfig = fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, stateStoringBalancerName)
+var (
+	stateStoringServiceConfig = fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, stateStoringBalancerName)
+	ignoreBalAttributesOpt    = cmp.Transformer("IgnoreBalancerAttributes", func(a resolver.Address) resolver.Address {
+		a.BalancerAttributes = nil
+		return a
+	})
+)
 
 type s struct {
 	grpctest.Tester
@@ -177,7 +183,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_FirstServerReady(t *testing.T) {
 	wantSCStates := []scState{
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Ready},
 	}
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -219,7 +225,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_FirstServerUnReady(t *testing.T)
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Shutdown},
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -264,7 +270,7 @@ func (s) TestPickFirstLeaf_SimpleResolverUpdate_DuplicateAddrs(t *testing.T) {
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Shutdown},
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -317,7 +323,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_DisjointLists(t *testing.T) {
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -334,7 +340,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_DisjointLists(t *testing.T) {
 		{Addrs: []resolver.Address{addrs[3]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -378,7 +384,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_ActiveBackendInUpdatedList(t *testing
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -398,7 +404,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_ActiveBackendInUpdatedList(t *testing
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -440,7 +446,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_InActiveBackendInUpdatedList(t *testi
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -458,7 +464,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_InActiveBackendInUpdatedList(t *testi
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -502,7 +508,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_IdenticalLists(t *testing.T) {
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -521,7 +527,7 @@ func (s) TestPickFirstLeaf_ResolverUpdates_IdenticalLists(t *testing.T) {
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -576,7 +582,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerRestart(t *testing.T)
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -591,7 +597,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerRestart(t *testing.T)
 		t.Fatal(err)
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -639,7 +645,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerRestart(t *testing.T)
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -660,7 +666,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerRestart(t *testing.T)
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Shutdown},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -708,7 +714,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerToFirst(t *testing.T)
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -729,7 +735,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_SecondServerToFirst(t *testing.T)
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -776,7 +782,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerToSecond(t *testing.T)
 		{Addrs: []resolver.Address{addrs[0]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -796,7 +802,7 @@ func (s) TestPickFirstLeaf_StopConnectedServer_FirstServerToSecond(t *testing.T)
 		{Addrs: []resolver.Address{addrs[1]}, State: connectivity.Ready},
 	}
 
-	if diff := cmp.Diff(wantSCStates, bal.subConnStates()); diff != "" {
+	if diff := cmp.Diff(wantSCStates, bal.subConnStates(), ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn states mismatch (-want +got):\n%s", diff)
 	}
 
@@ -1130,7 +1136,7 @@ func (s) TestPickFirstLeaf_InterleavingIPV4Preffered(t *testing.T) {
 	if err != nil {
 		t.Fatalf("%v", err)
 	}
-	if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" {
+	if diff := cmp.Diff(wantAddrs, gotAddrs, ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn creation order mismatch (-want +got):\n%s", diff)
 	}
 }
@@ -1174,7 +1180,7 @@ func (s) TestPickFirstLeaf_InterleavingIPv6Preffered(t *testing.T) {
 	if err != nil {
 		t.Fatalf("%v", err)
 	}
-	if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" {
+	if diff := cmp.Diff(wantAddrs, gotAddrs, ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn creation order mismatch (-want +got):\n%s", diff)
 	}
 }
@@ -1220,7 +1226,7 @@ func (s) TestPickFirstLeaf_InterleavingUnknownPreffered(t *testing.T) {
 	if err != nil {
 		t.Fatalf("%v", err)
 	}
-	if diff := cmp.Diff(wantAddrs, gotAddrs); diff != "" {
+	if diff := cmp.Diff(wantAddrs, gotAddrs, ignoreBalAttributesOpt); diff != "" {
 		t.Errorf("SubConn creation order mismatch (-want +got):\n%s", diff)
 	}
 }

balancer/weightedroundrobin/balancer.go

@@ -29,6 +29,7 @@ import (
 
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/balancer/endpointsharding"
+	"google.golang.org/grpc/balancer/pickfirst/pickfirstleaf"
 	"google.golang.org/grpc/balancer/weightedroundrobin/internal"
 	"google.golang.org/grpc/balancer/weightedtarget"
 	"google.golang.org/grpc/connectivity"
@@ -218,7 +219,9 @@ type wrrBalancer struct {
 }
 
 func (b *wrrBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error {
-	b.logger.Infof("UpdateCCS: %v", ccs)
+	if b.logger.V(2) {
+		b.logger.Infof("UpdateCCS: %v", ccs)
+	}
 	cfg, ok := ccs.BalancerConfig.(*lbConfig)
 	if !ok {
 		return fmt.Errorf("wrr: received nil or illegal BalancerConfig (type %T): %v", ccs.BalancerConfig, ccs.BalancerConfig)
@@ -232,6 +235,9 @@ func (b *wrrBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error
 	b.updateEndpointsLocked(ccs.ResolverState.Endpoints)
 	b.mu.Unlock()
 
+	// Make pickfirst children use health listeners for outlier detection to
+	// work.
+	ccs.ResolverState = pickfirstleaf.EnableHealthListener(ccs.ResolverState)
 	// This causes child to update picker inline and will thus cause inline
 	// picker update.
 	return b.child.UpdateClientConnState(balancer.ClientConnState{