From f53ad742468b406366627fd6b8a864e5e0dabed8 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Wed, 1 Dec 2021 12:23:13 +0100 Subject: [PATCH 1/3] Do not log 'connection refused' as warning in memberlist client Signed-off-by: Marco Pracucci --- kv/memberlist/memberlist_client.go | 2 +- kv/memberlist/tcp_transport.go | 17 ++++++-- kv/memberlist/tcp_transport_test.go | 60 +++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 kv/memberlist/tcp_transport_test.go diff --git a/kv/memberlist/memberlist_client.go b/kv/memberlist/memberlist_client.go index d074347a4..5880bd394 100644 --- a/kv/memberlist/memberlist_client.go +++ b/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) { diff --git a/kv/memberlist/tcp_transport.go b/kv/memberlist/tcp_transport.go index afbd1b201..0d83b9ea2 100644 --- a/kv/memberlist/tcp_transport.go +++ b/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 happend 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. diff --git a/kv/memberlist/tcp_transport_test.go b/kv/memberlist/tcp_transport_test.go new file mode 100644 index 000000000..37fafc644 --- /dev/null +++ b/kv/memberlist/tcp_transport_test.go @@ -0,0 +1,60 @@ +package memberlist + +import ( + "testing" + + "github.com/go-kit/log" + "github.com/grafana/dskit/concurrency" + "github.com/grafana/dskit/flagext" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +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) + } + }) + } +} From 6a1c252a05b3ee97de5c5649bcd100230c305b37 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Wed, 1 Dec 2021 12:36:48 +0100 Subject: [PATCH 2/3] Fixed import ordering Signed-off-by: Marco Pracucci --- kv/memberlist/tcp_transport_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kv/memberlist/tcp_transport_test.go b/kv/memberlist/tcp_transport_test.go index 37fafc644..13683dd88 100644 --- a/kv/memberlist/tcp_transport_test.go +++ b/kv/memberlist/tcp_transport_test.go @@ -4,10 +4,11 @@ import ( "testing" "github.com/go-kit/log" - "github.com/grafana/dskit/concurrency" - "github.com/grafana/dskit/flagext" "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) { From ce2d2a2f23ea04f9023e3379164fe8bdf1ad9233 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Wed, 1 Dec 2021 12:52:08 +0100 Subject: [PATCH 3/3] Fixed typo in a comment Signed-off-by: Marco Pracucci --- kv/memberlist/tcp_transport.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kv/memberlist/tcp_transport.go b/kv/memberlist/tcp_transport.go index 0d83b9ea2..daccd30ec 100644 --- a/kv/memberlist/tcp_transport.go +++ b/kv/memberlist/tcp_transport.go @@ -421,7 +421,7 @@ func (t *TCPTransport) WriteTo(b []byte, addr string) (time.Time, error) { logLevel := level.Warn(t.logger) if strings.Contains(err.Error(), "connection refused") { - // The connection refused is a common error that could happend during normal operations when a node + // 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() }