From 17d20387947690c28f1ef1919c5a7f036028aafb Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 22 May 2018 15:02:48 -0700 Subject: [PATCH 1/3] Call `Clone` on the tls config when cloning the mutable TLS config. Signed-off-by: Ying Li --- ca/transport.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ca/transport.go b/ca/transport.go index 6a6309a613..35943afbcb 100644 --- a/ca/transport.go +++ b/ca/transport.go @@ -48,7 +48,7 @@ func (c *MutableTLSCreds) Info() credentials.ProtocolInfo { // It panics if validation of underlying config fails. func (c *MutableTLSCreds) Clone() credentials.TransportCredentials { c.Lock() - newCfg, err := NewMutableTLS(c.config) + newCfg, err := NewMutableTLS(c.config.Clone()) if err != nil { panic("validation error on Clone") } From c39809ef7e03d385d4721b9037adefe4bd29b4b0 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 22 May 2018 17:29:54 -0700 Subject: [PATCH 2/3] To prevent data races when grpc attempts to log, only set the grpc logger to discard in test init functions, rather than in TestMain functions as is documented by the `SetLoggerV2` function. Also, use `SetLoggerV2` since SetLogger is deprecated. Signed-off-by: Ying Li --- manager/controlapi/node_test.go | 3 +-- manager/role_manager_test.go | 16 ---------------- manager/state/raft/membership/cluster_test.go | 10 +++++----- manager/state/raft/raft_test.go | 6 ++---- manager/watchapi/server_test.go | 3 +-- 5 files changed, 9 insertions(+), 29 deletions(-) diff --git a/manager/controlapi/node_test.go b/manager/controlapi/node_test.go index 603b72b894..5977274687 100644 --- a/manager/controlapi/node_test.go +++ b/manager/controlapi/node_test.go @@ -3,7 +3,6 @@ package controlapi import ( "fmt" "io/ioutil" - "log" "testing" "github.com/docker/swarmkit/api" @@ -241,7 +240,7 @@ func TestRemoveNodes(t *testing.T) { } func init() { - grpclog.SetLogger(log.New(ioutil.Discard, "", log.LstdFlags)) + grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, ioutil.Discard, ioutil.Discard)) logrus.SetOutput(ioutil.Discard) } diff --git a/manager/role_manager_test.go b/manager/role_manager_test.go index c8f51916cb..ee9384c8ef 100644 --- a/manager/role_manager_test.go +++ b/manager/role_manager_test.go @@ -2,32 +2,16 @@ package manager import ( "errors" - "io/ioutil" - "log" "testing" - "github.com/pivotal-golang/clock/fakeclock" - - "google.golang.org/grpc/grpclog" - "github.com/docker/swarmkit/api" cautils "github.com/docker/swarmkit/ca/testutils" raftutils "github.com/docker/swarmkit/manager/state/raft/testutils" "github.com/docker/swarmkit/manager/state/store" "github.com/docker/swarmkit/testutils" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" ) -func getRaftCluster(t *testing.T, tc *cautils.TestCA) (map[uint64]*raftutils.TestNode, *fakeclock.FakeClock) { - grpclog.SetLogger(log.New(ioutil.Discard, "", log.LstdFlags)) - logrus.SetOutput(ioutil.Discard) - - nodes, fc := raftutils.NewRaftCluster(t, tc) - raftutils.WaitForCluster(t, fc, nodes) - return nodes, fc -} - // While roleManager is running, if a node is demoted, it is removed from the raft cluster. If a node is // promoted, it is not added to the cluster but its observed role will change to manager. func TestRoleManagerRemovesDemotedNodesAndAddsPromotedNodes(t *testing.T) { diff --git a/manager/state/raft/membership/cluster_test.go b/manager/state/raft/membership/cluster_test.go index fc37de3d50..9a2c253368 100644 --- a/manager/state/raft/membership/cluster_test.go +++ b/manager/state/raft/membership/cluster_test.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io/ioutil" - "log" "os" "testing" "time" @@ -26,12 +25,13 @@ import ( var tc *cautils.TestCA -func TestMain(m *testing.M) { - tc = cautils.NewTestCA(nil) - - grpclog.SetLogger(log.New(ioutil.Discard, "", log.LstdFlags)) +func init() { + grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, ioutil.Discard, ioutil.Discard)) logrus.SetOutput(ioutil.Discard) +} +func TestMain(m *testing.M) { + tc = cautils.NewTestCA(nil) res := m.Run() tc.Stop() os.Exit(res) diff --git a/manager/state/raft/raft_test.go b/manager/state/raft/raft_test.go index b01b2a19bf..b7c7053a9f 100644 --- a/manager/state/raft/raft_test.go +++ b/manager/state/raft/raft_test.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io/ioutil" - "log" "math/rand" "net" "os" @@ -43,6 +42,8 @@ const ( func init() { store.WedgeTimeout = 3 * time.Second + grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, ioutil.Discard, ioutil.Discard)) + logrus.SetOutput(ioutil.Discard) } var tc *cautils.TestCA @@ -50,9 +51,6 @@ var tc *cautils.TestCA func TestMain(m *testing.M) { tc = cautils.NewTestCA(nil) - grpclog.SetLogger(log.New(ioutil.Discard, "", log.LstdFlags)) - logrus.SetOutput(ioutil.Discard) - // Set a smaller segment size so we don't incur cost preallocating // space on old filesystems like HFS+. wal.SegmentSizeBytes = 64 * 1024 diff --git a/manager/watchapi/server_test.go b/manager/watchapi/server_test.go index 2e223ba91a..0df252993f 100644 --- a/manager/watchapi/server_test.go +++ b/manager/watchapi/server_test.go @@ -2,7 +2,6 @@ package watchapi import ( "io/ioutil" - "log" "net" "os" "testing" @@ -102,6 +101,6 @@ func createNode(t *testing.T, ts *testServer, id string, role api.NodeRole, memb } func init() { - grpclog.SetLogger(log.New(ioutil.Discard, "", log.LstdFlags)) + grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, ioutil.Discard, ioutil.Discard)) logrus.SetOutput(ioutil.Discard) } From b21f075e387732808f95b3e90f6f5e5870fcb8f4 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 23 May 2018 10:22:06 -0700 Subject: [PATCH 3/3] Wrap the default logger in a private struct that converts a logrus entry into a grpc LoggerV2, since the grpc Logger is deprecated. Signed-off-by: Ying Li --- log/grpc.go | 19 +++++++++++++++- log/grpc_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 log/grpc_test.go diff --git a/log/grpc.go b/log/grpc.go index 4978d49730..0417c944c5 100644 --- a/log/grpc.go +++ b/log/grpc.go @@ -1,13 +1,30 @@ package log import ( + "github.com/sirupsen/logrus" "golang.org/x/net/context" "google.golang.org/grpc/grpclog" ) +type logrusWrapper struct { + *logrus.Entry +} + +// V provides the functionality that returns whether a particular log level is at +// least l - this is needed to meet the LoggerV2 interface. GRPC's logging levels +// are: https://github.com/grpc/grpc-go/blob/master/grpclog/loggerv2.go#L71 +// 0=info, 1=warning, 2=error, 3=fatal +// logrus's are: https://github.com/sirupsen/logrus/blob/master/logrus.go +// 0=panic, 1=fatal, 2=error, 3=warn, 4=info, 5=debug +func (lw logrusWrapper) V(l int) bool { + // translate to logrus level + logrusLevel := 4 - l + return int(lw.Logger.Level) <= logrusLevel +} + func init() { ctx := WithModule(context.Background(), "grpc") // completely replace the grpc logger with the logrus logger. - grpclog.SetLogger(G(ctx)) + grpclog.SetLoggerV2(logrusWrapper{Entry: G(ctx)}) } diff --git a/log/grpc_test.go b/log/grpc_test.go new file mode 100644 index 0000000000..70e1fbe30d --- /dev/null +++ b/log/grpc_test.go @@ -0,0 +1,57 @@ +package log + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" +) + +func TestGRPCLogrusLevelTranslation(t *testing.T) { + logger := logrus.New() + wrapped := logrusWrapper{Entry: logrus.NewEntry(logger)} + for _, tc := range []struct { + level logrus.Level + grpcLevel int + }{ + { + level: logrus.InfoLevel, + grpcLevel: 0, + }, + { + level: logrus.WarnLevel, + grpcLevel: 1, + }, + { + level: logrus.ErrorLevel, + grpcLevel: 2, + }, + { + level: logrus.FatalLevel, + grpcLevel: 3, + }, + // these don't translate to valid grpc log levels, but should still work + { + level: logrus.DebugLevel, + grpcLevel: -1, + }, + { + level: logrus.PanicLevel, + grpcLevel: 4, + }, + } { + logger.SetLevel(tc.level) + for i := -1; i < 5; i++ { + verbosityAtLeastI := wrapped.V(i) + require.Equal(t, i <= tc.grpcLevel, verbosityAtLeastI, + "Is verbosity at least %d? Logrus level at %v", i, tc.level) + } + } + + // these values should also always work, even though they're not valid grpc log values + logrus.SetLevel(logrus.DebugLevel) + require.True(t, wrapped.V(-100)) + + logrus.SetLevel(logrus.PanicLevel) + require.False(t, wrapped.V(100)) +}