[FAB-7306] delivery-client OSN black-listing fine-tune

When the peer receives a bad status (i.e UNAVAILABLE) from
an ordering service node that it connects to, it disconnects
from it and tries another ordering service node.

FAB-5006 introduced an optimization for v1.1 that makes the peer
temporarily black-list ordering service nodes that send a bad status
in order for the peer to prioritize selection of other OSNs,
instead of connecting to them with the same probability.

The black-listing works via calling Disconnect() with a 'true' parameter.

However - this might be problematic in cases where the OSNs are
behind a LB/NAT where only 1 endpoint is actually known to the peer,
but in practice - several endpoints exist behind.

This would result in the peer always disabling immediately all OSNs
because it only knows 1 endpoint.

This change set overcomes this by avoiding black-listing an endpoint
if it is the last one remaining.

Change-Id: I59ead8af88eb9acff3d3c64227528b18b990fb00
Signed-off-by: yacovm <yacovm@il.ibm.com>

Author: yacovm

core/comm/producer.go

@@ -108,8 +108,13 @@ func (cp *connProducer) DisableEndpoint(endpoint string) {
 	cp.Lock()
 	defer cp.Unlock()
 
-	for i := range cp.endpoints {
-		if cp.endpoints[i] == endpoint {
+	if len(cp.endpoints)-len(cp.disabledEndpoints) == 1 {
+		logger.Warning("Only 1 endpoint remained, will not black-list it")
+		return
+	}
+
+	for _, currEndpoint := range cp.endpoints {
+		if currEndpoint == endpoint {
 			cp.disabledEndpoints[endpoint] = time.Now()
 			break
 		}

core/comm/producer_test.go

@@ -15,11 +15,6 @@ import (
 	"google.golang.org/grpc"
 )
 
-type connMock struct {
-	*grpc.ClientConn
-	endpoint string
-}
-
 func TestEmptyEndpoints(t *testing.T) {
 	noopFactory := func(endpoint string) (*grpc.ClientConn, error) {
 		return nil, nil
@@ -121,20 +116,15 @@ func TestDisableEndpoint(t *testing.T) {
 	// Now disable endpoint for 100 milliseconds
 	producer.DisableEndpoint("a")
 	_, _, err = producer.NewConnection()
-	assert.Error(t, err, "Could not connect")
-	// Wait until disable expire and try to connect again
-	time.Sleep(time.Millisecond * 200)
-	conn, a, err = producer.NewConnection()
+	// Make sure if only 1 endpoint remains, we don't black-list it
 	assert.NoError(t, err)
-	assert.Equal(t, "a", conn2Endpoint[fmt.Sprintf("%p", conn)])
-	assert.Equal(t, "a", a)
+	// Update endpoints - add endpoint 'b'
+	producer.UpdateEndpoints([]string{"a", "b"})
 	// Disable again
 	producer.DisableEndpoint("a")
-	// Update endpoints
-	producer.UpdateEndpoints([]string{"a", "b"})
-
 	conn, a, err = producer.NewConnection()
 	assert.NoError(t, err)
+	// Ensure that only b is returned because 'a' is disabled
 	assert.Equal(t, "b", conn2Endpoint[fmt.Sprintf("%p", conn)])
 	assert.Equal(t, "b", a)
 

core/deliverservice/client_test.go

@@ -675,21 +675,21 @@ func TestDisconnect(t *testing.T) {
 
 func TestDisconnectAndDisableEndpoint(t *testing.T) {
 	// Scenario:
-	// Start one ordering service and one client
-	// Connect client to ordering service endpoint
-	// Check connection to ordering service
-	// Disconnect and disable endpoint
-	// Check that ordering service don't have connection to the client
-	// Try to reconnect to endpoint (orderer) and check that
-	// ordering service still don't have connection to the client
-	// Wait until endpoint disable expired and reconnect again
-	// Check that we have connection between orderer and client
+	// 1)  Start two ordering service nodes and one client
+	// 2) Have the client connect to some ordering service node
+	// 3) Disconnect and disable the endpoint of the current connection,
+	//    and ensure the client connects to the other node.
+	// 4) Black-list the second connection and ensure it still connects
+	//    to the ordering service node because it's the last one remaining.
 
 	defer ensureNoGoroutineLeak(t)()
-	os := mocks.NewOrderer(5613, t)
-	os.SetNextExpectedSeek(5)
+	os1 := mocks.NewOrderer(5613, t)
+	os1.SetNextExpectedSeek(5)
+	os2 := mocks.NewOrderer(5614, t)
+	os2.SetNextExpectedSeek(5)
 
-	defer os.Shutdown()
+	defer os1.Shutdown()
+	defer os2.Shutdown()
 
 	orgEndpointDisableInterval := comm.EndpointDisableInterval
 	comm.EndpointDisableInterval = time.Millisecond * 1500
@@ -698,7 +698,7 @@ func TestDisconnectAndDisableEndpoint(t *testing.T) {
 	connFact := func(endpoint string) (*grpc.ClientConn, error) {
 		return grpc.Dial(endpoint, grpc.WithInsecure(), grpc.WithBlock())
 	}
-	prod := comm.NewConnectionProducer(connFact, []string{"localhost:5613"})
+	prod := comm.NewConnectionProducer(connFact, []string{"localhost:5613", "localhost:5614"})
 	clFact := func(cc *grpc.ClientConn) orderer.AtomicBroadcastClient {
 		return orderer.NewAtomicBroadcastClient(cc)
 	}
@@ -711,45 +711,41 @@ func TestDisconnectAndDisableEndpoint(t *testing.T) {
 	}
 
 	cl := NewBroadcastClient(prod, clFact, onConnect, retryPol)
+	defer cl.Close()
 
 	// First connect to orderer
 	go func() {
 		cl.Recv()
 	}()
 
 	assert.True(t, waitForWithTimeout(time.Millisecond*100, func() bool {
-		return os.ConnCount() == 1
+		return os1.ConnCount() == 1 || os2.ConnCount() == 1
 	}), "Didn't get connection to orderer")
 
+	connectedToOS1 := os1.ConnCount() == 1
+
 	// Disconnect and disable endpoint
 	cl.Disconnect(true)
 
+	// Ensure we reconnected to the other node
 	assert.True(t, waitForWithTimeout(time.Millisecond*100, func() bool {
-		return os.ConnCount() == 0
-	}), "Didn't disconnect from orderer")
-
-	// Try to reconnect while endpoint still disabled
-	go func() {
-		assert.False(t, waitForWithTimeout(time.Millisecond*100, func() bool {
-			return os.ConnCount() == 1
-		}), "Recreated connection to orderer, although shouldn't")
-	}()
-
-	_, err := cl.Recv()
-	assert.Error(t, err, "Connection shouldn't have been established because all endpoints have been disabled")
+		if connectedToOS1 {
+			return os1.ConnCount() == 0 && os2.ConnCount() == 1
+		}
+		return os2.ConnCount() == 0 && os1.ConnCount() == 1
+	}), "Didn't disconnect from orderer, or reconnected to a black-listed node")
 
-	//Wait until endpoint disable expires and reconnect again
-	time.Sleep(time.Millisecond * 1500)
+	// Disconnect from the node we are currently connected to, and attempt to black-list it
+	cl.Disconnect(true)
 
 	go func() {
 		cl.Recv()
 	}()
 
+	// Ensure we are still connected to some orderer, even though both endpoints are now black-listed
 	assert.True(t, waitForWithTimeout(time.Millisecond*100, func() bool {
-		return os.ConnCount() == 1
-	}), "Didn't got connection to orderer after endpoint disable expired")
-
-	cl.Close()
+		return os1.ConnCount() == 1 || os2.ConnCount() == 1
+	}), "Didn't got connection to orderer")
 }
 
 func waitForWithTimeout(timeout time.Duration, f func() bool) bool {