From 20ca612a59c45c0bd58c71c199a7ebd2a6bf1a9e Mon Sep 17 00:00:00 2001 From: junzhe and mnussbaum Date: Sat, 10 Feb 2018 00:03:47 +0000 Subject: [PATCH] Fix empty LogPath with non-blocking logging mode This fixes an issue where the container LogPath was empty when the non-blocking logging mode was enabled. This change sets the LogPath on the container as soon as the path is generated, instead of setting the LogPath on a logger struct and then attempting to pull it off that logger at a later point. That attempt to pull the LogPath off the logger was error prone since it assumed that the logger would only ever be a single type. Prior to this change docker inspect returned an empty string for LogPath. This caused issues with tools that rely on docker inspect output to discover container logs, e.g. Kubernetes. This commit also removes some LogPath methods that are now unnecessary and are never invoked. Signed-off-by: junzhe and mnussbaum --- container/container.go | 9 ++-- container/container_unit_test.go | 53 ++++++++++++++++++++++++ daemon/logger/jsonfilelog/jsonfilelog.go | 5 --- daemon/logger/loggerutils/logfile.go | 7 ---- 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/container/container.go b/container/container.go index 938d66f889..451e0f833e 100644 --- a/container/container.go +++ b/container/container.go @@ -38,7 +38,7 @@ import ( "github.com/docker/docker/runconfig" "github.com/docker/docker/volume" "github.com/docker/go-connections/nat" - "github.com/docker/go-units" + units "github.com/docker/go-units" "github.com/docker/libnetwork" "github.com/docker/libnetwork/netlabel" "github.com/docker/libnetwork/options" @@ -391,6 +391,8 @@ func (container *Container) StartLogger() (logger.Logger, error) { if err != nil { return nil, err } + + container.LogPath = info.LogPath } l, err := initDriver(info) @@ -974,11 +976,6 @@ func (container *Container) startLogging() error { copier.Run() container.LogDriver = l - // set LogPath field only for json-file logdriver - if jl, ok := l.(*jsonfilelog.JSONFileLogger); ok { - container.LogPath = jl.LogPath() - } - return nil } diff --git a/container/container_unit_test.go b/container/container_unit_test.go index f6ded8fa9d..858124abdb 100644 --- a/container/container_unit_test.go +++ b/container/container_unit_test.go @@ -1,12 +1,16 @@ package container // import "github.com/docker/docker/container" import ( + "fmt" + "io/ioutil" "path/filepath" "testing" "github.com/docker/docker/api/types/container" swarmtypes "github.com/docker/docker/api/types/swarm" + "github.com/docker/docker/daemon/logger/jsonfilelog" "github.com/docker/docker/pkg/signal" + "github.com/stretchr/testify/require" ) func TestContainerStopSignal(t *testing.T) { @@ -66,3 +70,52 @@ func TestContainerSecretReferenceDestTarget(t *testing.T) { t.Fatalf("expected secret dest %q; received %q", expected, d) } } + +func TestContainerLogPathSetForJSONFileLogger(t *testing.T) { + containerRoot, err := ioutil.TempDir("", "TestContainerLogPathSetForJSONFileLogger") + require.NoError(t, err) + + c := &Container{ + Config: &container.Config{}, + HostConfig: &container.HostConfig{ + LogConfig: container.LogConfig{ + Type: jsonfilelog.Name, + }, + }, + ID: "TestContainerLogPathSetForJSONFileLogger", + Root: containerRoot, + } + + _, err = c.StartLogger() + require.NoError(t, err) + + expectedLogPath, err := filepath.Abs(filepath.Join(containerRoot, fmt.Sprintf("%s-json.log", c.ID))) + require.NoError(t, err) + require.Equal(t, c.LogPath, expectedLogPath) +} + +func TestContainerLogPathSetForRingLogger(t *testing.T) { + containerRoot, err := ioutil.TempDir("", "TestContainerLogPathSetForRingLogger") + require.NoError(t, err) + + c := &Container{ + Config: &container.Config{}, + HostConfig: &container.HostConfig{ + LogConfig: container.LogConfig{ + Type: jsonfilelog.Name, + Config: map[string]string{ + "mode": string(container.LogModeNonBlock), + }, + }, + }, + ID: "TestContainerLogPathSetForRingLogger", + Root: containerRoot, + } + + _, err = c.StartLogger() + require.NoError(t, err) + + expectedLogPath, err := filepath.Abs(filepath.Join(containerRoot, fmt.Sprintf("%s-json.log", c.ID))) + require.NoError(t, err) + require.Equal(t, c.LogPath, expectedLogPath) +} diff --git a/daemon/logger/jsonfilelog/jsonfilelog.go b/daemon/logger/jsonfilelog/jsonfilelog.go index 92484697d5..7d637f8573 100644 --- a/daemon/logger/jsonfilelog/jsonfilelog.go +++ b/daemon/logger/jsonfilelog/jsonfilelog.go @@ -150,11 +150,6 @@ func ValidateLogOpt(cfg map[string]string) error { return nil } -// LogPath returns the location the given json logger logs to. -func (l *JSONFileLogger) LogPath() string { - return l.writer.LogPath() -} - // Close closes underlying file and signals all readers to stop. func (l *JSONFileLogger) Close() error { l.mu.Lock() diff --git a/daemon/logger/loggerutils/logfile.go b/daemon/logger/loggerutils/logfile.go index ff7a3323f9..8c6cbfb42b 100644 --- a/daemon/logger/loggerutils/logfile.go +++ b/daemon/logger/loggerutils/logfile.go @@ -130,13 +130,6 @@ func rotate(name string, maxFiles int) error { return nil } -// LogPath returns the location the given writer logs to. -func (w *LogFile) LogPath() string { - w.mu.Lock() - defer w.mu.Unlock() - return w.f.Name() -} - // MaxFiles return maximum number of files func (w *LogFile) MaxFiles() int { return w.maxFiles