1. docker
  2. Commit 20ca612a59c45c0bd58c71c199a7ebd2a6bf1a9e
Browse code

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 <code@getbraintree.com>

junzhe and mnussbaum authored on 2018/02/10 09:03:47
Showing 4 changed files
container/container.go
History View file @ 20ca612
... ... 
@@ -38,7 +38,7 @@ import (
38 38 
 	"github.com/docker/docker/runconfig"
39 39 
 	"github.com/docker/docker/volume"
40 40 
 	"github.com/docker/go-connections/nat"
41 
-	"github.com/docker/go-units"
41 
+	units "github.com/docker/go-units"
42 42 
 	"github.com/docker/libnetwork"
43 43 
 	"github.com/docker/libnetwork/netlabel"
44 44 
 	"github.com/docker/libnetwork/options"
... ... 
@@ -391,6 +391,8 @@ func (container *Container) StartLogger() (logger.Logger, error) {
391 391 
 		if err != nil {
392 392 
 			return nil, err
393 393 
 		}
394 
+
395 
+		container.LogPath = info.LogPath
394 396 
 	}
395 397
396 398 
 	l, err := initDriver(info)
... ... 
@@ -974,11 +976,6 @@ func (container *Container) startLogging() error {
974 974 
 	copier.Run()
975 975 
 	container.LogDriver = l
976 976
977 
-	// set LogPath field only for json-file logdriver
978 
-	if jl, ok := l.(*jsonfilelog.JSONFileLogger); ok {
979 
-		container.LogPath = jl.LogPath()
980 
-	}
981 
-
982 977 
 	return nil
983 978 
 }
984 979
container/container_unit_test.go
History View file @ 20ca612
... ... 
@@ -1,12 +1,16 @@
1 1 
 package container // import "github.com/docker/docker/container"
2 2
3 3 
 import (
4 
+	"fmt"
5 
+	"io/ioutil"
4 6 
 	"path/filepath"
5 7 
 	"testing"
6 8
7 9 
 	"github.com/docker/docker/api/types/container"
8 10 
 	swarmtypes "github.com/docker/docker/api/types/swarm"
11 
+	"github.com/docker/docker/daemon/logger/jsonfilelog"
9 12 
 	"github.com/docker/docker/pkg/signal"
13 
+	"github.com/stretchr/testify/require"
10 14 
 )
11 15
12 16 
 func TestContainerStopSignal(t *testing.T) {
... ... 
@@ -66,3 +70,52 @@ func TestContainerSecretReferenceDestTarget(t *testing.T) {
66 66 
 		t.Fatalf("expected secret dest %q; received %q", expected, d)
67 67 
 	}
68 68 
 }
69 
+
70 
+func TestContainerLogPathSetForJSONFileLogger(t *testing.T) {
71 
+	containerRoot, err := ioutil.TempDir("", "TestContainerLogPathSetForJSONFileLogger")
72 
+	require.NoError(t, err)
73 
+
74 
+	c := &Container{
75 
+		Config: &container.Config{},
76 
+		HostConfig: &container.HostConfig{
77 
+			LogConfig: container.LogConfig{
78 
+				Type: jsonfilelog.Name,
79 
+			},
80 
+		},
81 
+		ID:   "TestContainerLogPathSetForJSONFileLogger",
82 
+		Root: containerRoot,
83 
+	}
84 
+
85 
+	_, err = c.StartLogger()
86 
+	require.NoError(t, err)
87 
+
88 
+	expectedLogPath, err := filepath.Abs(filepath.Join(containerRoot, fmt.Sprintf("%s-json.log", c.ID)))
89 
+	require.NoError(t, err)
90 
+	require.Equal(t, c.LogPath, expectedLogPath)
91 
+}
92 
+
93 
+func TestContainerLogPathSetForRingLogger(t *testing.T) {
94 
+	containerRoot, err := ioutil.TempDir("", "TestContainerLogPathSetForRingLogger")
95 
+	require.NoError(t, err)
96 
+
97 
+	c := &Container{
98 
+		Config: &container.Config{},
99 
+		HostConfig: &container.HostConfig{
100 
+			LogConfig: container.LogConfig{
101 
+				Type: jsonfilelog.Name,
102 
+				Config: map[string]string{
103 
+					"mode": string(container.LogModeNonBlock),
104 
+				},
105 
+			},
106 
+		},
107 
+		ID:   "TestContainerLogPathSetForRingLogger",
108 
+		Root: containerRoot,
109 
+	}
110 
+
111 
+	_, err = c.StartLogger()
112 
+	require.NoError(t, err)
113 
+
114 
+	expectedLogPath, err := filepath.Abs(filepath.Join(containerRoot, fmt.Sprintf("%s-json.log", c.ID)))
115 
+	require.NoError(t, err)
116 
+	require.Equal(t, c.LogPath, expectedLogPath)
117 
+}
daemon/logger/jsonfilelog/jsonfilelog.go
History View file @ 20ca612
... ... 
@@ -150,11 +150,6 @@ func ValidateLogOpt(cfg map[string]string) error {
150 150 
 	return nil
151 151 
 }
152 152
153 
-// LogPath returns the location the given json logger logs to.
154 
-func (l *JSONFileLogger) LogPath() string {
155 
-	return l.writer.LogPath()
156 
-}
157 
-
158 153 
 // Close closes underlying file and signals all readers to stop.
159 154 
 func (l *JSONFileLogger) Close() error {
160 155 
 	l.mu.Lock()
daemon/logger/loggerutils/logfile.go
History View file @ 20ca612
... ... 
@@ -130,13 +130,6 @@ func rotate(name string, maxFiles int) error {
130 130 
 	return nil
131 131 
 }
132 132
133 
-// LogPath returns the location the given writer logs to.
134 
-func (w *LogFile) LogPath() string {
135 
-	w.mu.Lock()
136 
-	defer w.mu.Unlock()
137 
-	return w.f.Name()
138 
-}
139 
-
140 133 
 // MaxFiles return maximum number of files
141 134 
 func (w *LogFile) MaxFiles() int {
142 135 
 	return w.maxFiles