weightedtarget: return a more meaningful error when no child policy i…

…s reporting READY (#5391)

balancer/weightedtarget/weightedaggregator/aggregator.go

@@ -245,6 +245,7 @@ func (wbsa *Aggregator) build() balancer.State {
 	// state.
 	var readyN, connectingN, idleN int
 	readyPickerWithWeights := make([]weightedPickerState, 0, len(m))
+	errorPickers := make([]weightedPickerState, 0, len(m))
 	for _, ps := range m {
 		switch ps.stateToAggregate {
 		case connectivity.Ready:
@@ -254,6 +255,8 @@ func (wbsa *Aggregator) build() balancer.State {
 			connectingN++
 		case connectivity.Idle:
 			idleN++
+		case connectivity.TransientFailure:
+			errorPickers = append(errorPickers, *ps)
 		}
 	}
 	var aggregatedState connectivity.State
@@ -272,7 +275,7 @@ func (wbsa *Aggregator) build() balancer.State {
 	var picker balancer.Picker
 	switch aggregatedState {
 	case connectivity.TransientFailure:
-		picker = base.NewErrPicker(balancer.ErrTransientFailure)
+		picker = newWeightedPickerGroup(errorPickers, wbsa.newWRR)
 	case connectivity.Connecting:
 		picker = base.NewErrPicker(balancer.ErrNoSubConnAvailable)
 	default:

balancer/weightedtarget/weightedtarget_test.go

@@ -21,6 +21,7 @@ package weightedtarget
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
@@ -130,6 +131,11 @@ var (
 
 const testBackendAddrsCount = 12
 
+const scConnErrorPrefix = "last connection error:"
+const testSCConnErrMsg = "this is definitely a connection error. please check your wiring"
+
+var mergedErrMsg = fmt.Sprintf("%s %s", scConnErrorPrefix, testSCConnErrMsg)
+
 func init() {
 	balancer.Register(newTestConfigBalancerBuilder())
 	for i := 0; i < testBackendAddrsCount; i++ {
@@ -569,7 +575,9 @@ func (s) TestWeightedTarget_TwoSubBalancers_MoreBackends(t *testing.T) {
 	}
 
 	// Turn sc1's connection down.
-	wtb.UpdateSubConnState(sc1, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	wtb.UpdateSubConnState(sc1, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf(testSCConnErrMsg)})
 	p = <-cc.NewPickerCh
 	want = []balancer.SubConn{sc4}
 	if err := testutils.IsRoundRobin(want, subConnFromPicker(p)); err != nil {
@@ -586,11 +594,13 @@ func (s) TestWeightedTarget_TwoSubBalancers_MoreBackends(t *testing.T) {
 	}
 
 	// Turn all connections down.
-	wtb.UpdateSubConnState(sc4, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	wtb.UpdateSubConnState(sc4, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf(testSCConnErrMsg)})
 	p = <-cc.NewPickerCh
 	for i := 0; i < 5; i++ {
-		if _, err := p.Pick(balancer.PickInfo{}); err != balancer.ErrTransientFailure {
-			t.Fatalf("want pick error %v, got %v", balancer.ErrTransientFailure, err)
+		if _, err := p.Pick(balancer.PickInfo{}); strings.Compare(err.Error(), mergedErrMsg) != 0 {
+			t.Fatalf("want pick error '%v', got error '%v'", mergedErrMsg, err)
 		}
 	}
 }
@@ -793,7 +803,9 @@ func (s) TestWeightedTarget_ThreeSubBalancers_RemoveBalancer(t *testing.T) {
 	}
 
 	// Move balancer 3 into transient failure.
-	wtb.UpdateSubConnState(sc3, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	wtb.UpdateSubConnState(sc3, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf(testSCConnErrMsg)})
 	<-cc.NewPickerCh
 
 	// Remove the first balancer, while the third is transient failure.
@@ -827,8 +839,8 @@ func (s) TestWeightedTarget_ThreeSubBalancers_RemoveBalancer(t *testing.T) {
 		t.Fatalf("RemoveSubConn, want %v, got %v", sc1, scRemoved)
 	}
 	for i := 0; i < 5; i++ {
-		if _, err := p.Pick(balancer.PickInfo{}); err != balancer.ErrTransientFailure {
-			t.Fatalf("want pick error %v, got %v", balancer.ErrTransientFailure, err)
+		if _, err := p.Pick(balancer.PickInfo{}); strings.Compare(err.Error(), mergedErrMsg) != 0 {
+			t.Fatalf("want pick error '%v', got error '%v'", mergedErrMsg, err)
 		}
 	}
 }
@@ -1064,15 +1076,18 @@ func (s) TestBalancerGroup_SubBalancerTurnsConnectingFromTransientFailure(t *tes
 
 	// Set both subconn to TransientFailure, this will put both sub-balancers in
 	// transient failure.
-	wtb.UpdateSubConnState(sc1, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	wtb.UpdateSubConnState(sc1, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf(testSCConnErrMsg)})
 	<-cc.NewPickerCh
-	wtb.UpdateSubConnState(sc2, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	wtb.UpdateSubConnState(sc2, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf(testSCConnErrMsg)})
 	p := <-cc.NewPickerCh
 
 	for i := 0; i < 5; i++ {
-		r, err := p.Pick(balancer.PickInfo{})
-		if err != balancer.ErrTransientFailure {
-			t.Fatalf("want pick to fail with %v, got result %v, err %v", balancer.ErrTransientFailure, r, err)
+		if _, err := p.Pick(balancer.PickInfo{}); strings.Compare(err.Error(), mergedErrMsg) != 0 {
+			t.Fatalf("want pick error '%s', got error '%v'", mergedErrMsg, err)
 		}
 	}
 
@@ -1086,8 +1101,8 @@ func (s) TestBalancerGroup_SubBalancerTurnsConnectingFromTransientFailure(t *tes
 
 	for i := 0; i < 5; i++ {
 		r, err := p.Pick(balancer.PickInfo{})
-		if err != balancer.ErrTransientFailure {
-			t.Fatalf("want pick to fail with %v, got result %v, err %v", balancer.ErrTransientFailure, r, err)
+		if strings.Compare(err.Error(), mergedErrMsg) != 0 {
+			t.Fatalf("want pick error '%s', got result %v, err %v", mergedErrMsg, r, err)
 		}
 	}
 }

xds/internal/balancer/clusterresolver/priority_test.go

@@ -247,7 +247,9 @@ func (s) TestEDSPriority_SwitchPriority(t *testing.T) {
 	}
 
 	// Turn down 1, use 2
-	edsb.UpdateSubConnState(sc1, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	edsb.UpdateSubConnState(sc1, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf("this is definitely a connection issue")})
 	addrs2 := <-cc.NewSubConnAddrsCh
 	if got, want := addrs2[0].Addr, testEndpointAddrs[2]; got != want {
 		t.Fatalf("sc is created with addr %v, want %v", got, want)
@@ -274,7 +276,7 @@ func (s) TestEDSPriority_SwitchPriority(t *testing.T) {
 	}
 
 	// Should get an update with 1's old picker, to override 2's old picker.
-	if err := testErrPickerFromCh(cc.NewPickerCh, balancer.ErrTransientFailure); err != nil {
+	if err := testErrPickerFromCh(cc.NewPickerCh, fmt.Errorf("last connection error: this is definitely a connection issue")); err != nil {
 		t.Fatal(err)
 	}
 
@@ -305,10 +307,12 @@ func (s) TestEDSPriority_HigherDownWhileAddingLower(t *testing.T) {
 	}
 	sc1 := <-cc.NewSubConnCh
 	// Turn down 1, pick should error.
-	edsb.UpdateSubConnState(sc1, balancer.SubConnState{ConnectivityState: connectivity.TransientFailure})
+	edsb.UpdateSubConnState(sc1, balancer.SubConnState{
+		ConnectivityState: connectivity.TransientFailure,
+		ConnectionError:   fmt.Errorf("this is definitely a connection issue")})
 
 	// Test pick failure.
-	if err := testErrPickerFromCh(cc.NewPickerCh, balancer.ErrTransientFailure); err != nil {
+	if err := testErrPickerFromCh(cc.NewPickerCh, fmt.Errorf("last connection error: this is definitely a connection issue")); err != nil {
 		t.Fatal(err)
 	}
 
@@ -463,8 +467,8 @@ func (s) TestEDSPriority_InitTimeout(t *testing.T) {
 
 // Add localities to existing priorities.
 //
-//  - start with 2 locality with p0 and p1
-//  - add localities to existing p0 and p1
+//   - start with 2 locality with p0 and p1
+//   - add localities to existing p0 and p1
 func (s) TestEDSPriority_MultipleLocalities(t *testing.T) {
 	edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
 	defer cleanup()