Merge pull request #88 from grafana/do-not-log-connection-refused-as-…

…warning

Do not log 'connection refused' as warning in memberlist client

kv/memberlist/memberlist_client.go

@@ -193,7 +193,7 @@ func (cfg *KVConfig) RegisterFlagsWithPrefix(f *flag.FlagSet, prefix string) {
 	f.StringVar(&cfg.AdvertiseAddr, prefix+"memberlist.advertise-addr", mlDefaults.AdvertiseAddr, "Gossip address to advertise to other members in the cluster. Used for NAT traversal.")
 	f.IntVar(&cfg.AdvertisePort, prefix+"memberlist.advertise-port", mlDefaults.AdvertisePort, "Gossip port to advertise to other members in the cluster. Used for NAT traversal.")
 
-	cfg.TCPTransport.RegisterFlags(f, prefix)
+	cfg.TCPTransport.RegisterFlagsWithPrefix(f, prefix)
 }
 
 func (cfg *KVConfig) RegisterFlags(f *flag.FlagSet) {

kv/memberlist/tcp_transport.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"strings"
 	"sync"
 	"time"
 
@@ -60,8 +61,12 @@ type TCPTransportConfig struct {
 	TLS        dstls.ClientConfig `yaml:",inline"`
 }
 
-// RegisterFlags registers flags.
-func (cfg *TCPTransportConfig) RegisterFlags(f *flag.FlagSet, prefix string) {
+func (cfg *TCPTransportConfig) RegisterFlags(f *flag.FlagSet) {
+	cfg.RegisterFlagsWithPrefix(f, "")
+}
+
+// RegisterFlagsWithPrefix registers flags with prefix.
+func (cfg *TCPTransportConfig) RegisterFlagsWithPrefix(f *flag.FlagSet, prefix string) {
 	// "Defaults to hostname" -- memberlist sets it to hostname by default.
 	f.Var(&cfg.BindAddrs, prefix+"memberlist.bind-addr", "IP address to listen on for gossip messages. Multiple addresses may be specified. Defaults to 0.0.0.0")
 	f.IntVar(&cfg.BindPort, prefix+"memberlist.bind-port", 7946, "Port to listen on for gossip messages.")
@@ -414,7 +419,13 @@ func (t *TCPTransport) WriteTo(b []byte, addr string) (time.Time, error) {
 	if err != nil {
 		t.sentPacketsErrors.Inc()
 
-		level.Warn(t.logger).Log("msg", "WriteTo failed", "addr", addr, "err", err)
+		logLevel := level.Warn(t.logger)
+		if strings.Contains(err.Error(), "connection refused") {
+			// The connection refused is a common error that could happen during normal operations when a node
+			// shutdown (or crash). It shouldn't be considered a warning condition on the sender side.
+			logLevel = t.debugLog()
+		}
+		logLevel.Log("msg", "WriteTo failed", "addr", addr, "err", err)
 
 		// WriteTo is used to send "UDP" packets. Since we use TCP, we can detect more errors,
 		// but memberlist library doesn't seem to cope with that very well. That is why we return nil instead.

kv/memberlist/tcp_transport_test.go

@@ -0,0 +1,61 @@
+package memberlist
+
+import (
+	"testing"
+
+	"github.com/go-kit/log"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/dskit/concurrency"
+	"github.com/grafana/dskit/flagext"
+)
+
+func TestTCPTransport_WriteTo_ShouldNotLogAsWarningExpectedFailures(t *testing.T) {
+	tests := map[string]struct {
+		setup          func(t *testing.T, cfg *TCPTransportConfig)
+		remoteAddr     string
+		expectedLogs   string
+		unexpectedLogs string
+	}{
+		"should not log 'connection refused' by default": {
+			remoteAddr:     "localhost:12345",
+			unexpectedLogs: "connection refused",
+		},
+		"should log 'connection refused' if debug log level is enabled": {
+			setup: func(t *testing.T, cfg *TCPTransportConfig) {
+				cfg.TransportDebug = true
+			},
+			remoteAddr:   "localhost:12345",
+			expectedLogs: "connection refused",
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			logs := &concurrency.SyncBuffer{}
+			logger := log.NewLogfmtLogger(logs)
+
+			cfg := TCPTransportConfig{}
+			flagext.DefaultValues(&cfg)
+			cfg.BindAddrs = []string{"localhost"}
+			cfg.BindPort = 0
+			if testData.setup != nil {
+				testData.setup(t, &cfg)
+			}
+
+			transport, err := NewTCPTransport(cfg, logger)
+			require.NoError(t, err)
+
+			_, err = transport.WriteTo([]byte("test"), testData.remoteAddr)
+			require.NoError(t, err)
+
+			if testData.expectedLogs != "" {
+				assert.Contains(t, logs.String(), testData.expectedLogs)
+			}
+			if testData.unexpectedLogs != "" {
+				assert.NotContains(t, logs.String(), testData.unexpectedLogs)
+			}
+		})
+	}
+}