1. docker
  2. Commit 1ae9dcf97dc6ccaf7a7977fe8ae1812b691ccba2
Browse code

Fixes some issues with jsonfile write/read

This cleans up some of the use of the filepoller which makes reading
significantly more robust and gives fewer changes to fallback to the
polling based watcher.
In a lot of cases, if the file was being rotated while we were adding it
to the watcher, it would return an error that the file doesn't exist and
would fallback.
In some cases this fallback could be triggered multiple times even if we
were already on the fallback/poll-based watcher.

It also fixes an open file leak caused by not closing files properly on
rotate, as well as not closing files that were read via the `tail`
function until after the log reader is completed.

Prior to the above changes, it was relatively simple to cause the log
reader to error out by having quick rotations, for example:
```
$ docker run --name test --log-opt max-size=10b --log-opt max-files=10
-d busybox sh -c 'while true; do usleep 500000; echo hello; done'
$ docker logs -f test
```
After these changes I can run this forever without error.

Another fix removes 2 `os.Stat` calls when rotating files. The stat
calls are not needed since we are just calling `os.Rename` anyway, which
will in turn also just produce the same error that `Stat` would.
These `Stat` calls were also quite expensive.
Removing these stat calls also seemed to resolve an issue causing slow
memory growth on the daemon.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Brian Goff authored on 2016/03/18 01:13:23
Showing 3 changed files
daemon/logger/jsonfilelog/read.go
History View file @ 1ae9dcf
... ... 
@@ -54,7 +54,6 @@ func (l *JSONFileLogger) readLogs(logWatcher *logger.LogWatcher, config logger.R
54 54 
 			}
55 55 
 			continue
56 56 
 		}
57 
-		defer f.Close()
58 57 
 		files = append(files, f)
59 58 
 	}
60 59
... ... 
@@ -63,15 +62,19 @@ func (l *JSONFileLogger) readLogs(logWatcher *logger.LogWatcher, config logger.R
63 63 
 		logWatcher.Err <- err
64 64 
 		return
65 65 
 	}
66 
-	defer latestFile.Close()
67 
-
68 
-	files = append(files, latestFile)
69 
-	tailer := ioutils.MultiReadSeeker(files...)
70 66
71 67 
 	if config.Tail != 0 {
68 
+		tailer := ioutils.MultiReadSeeker(append(files, latestFile)...)
72 69 
 		tailFile(tailer, logWatcher, config.Tail, config.Since)
73 70 
 	}
74 71
72 
+	// close all the rotated files
73 
+	for _, f := range files {
74 
+		if err := f.(io.Closer).Close(); err != nil {
75 
+			logrus.WithField("logger", "json-file").Warnf("error closing tailed log file: %v", err)
76 
+		}
77 
+	}
78 
+
75 79 
 	if !config.Follow {
76 80 
 		return
77 81 
 	}
... ... 
@@ -129,7 +132,23 @@ func followLogs(f *os.File, logWatcher *logger.LogWatcher, notifyRotate chan int
129 129 
 	if err != nil {
130 130 
 		logWatcher.Err <- err
131 131 
 	}
132 
-	defer fileWatcher.Close()
132 
+	defer func() {
133 
+		f.Close()
134 
+		fileWatcher.Close()
135 
+	}()
136 
+	name := f.Name()
137 
+
138 
+	if err := fileWatcher.Add(name); err != nil {
139 
+		logrus.WithField("logger", "json-file").Warnf("falling back to file poller due to error: %v", err)
140 
+		fileWatcher.Close()
141 
+		fileWatcher = filenotify.NewPollingWatcher()
142 
+
143 
+		if err := fileWatcher.Add(name); err != nil {
144 
+			logrus.Debugf("error watching log file for modifications: %v", err)
145 
+			logWatcher.Err <- err
146 
+			return
147 
+		}
148 
+	}
133 149
134 150 
 	var retries int
135 151 
 	for {
... ... 
@@ -153,42 +172,42 @@ func followLogs(f *os.File, logWatcher *logger.LogWatcher, notifyRotate chan int
153 153 
 					retries++
154 154 
 					continue
155 155 
 				}
156 
-				logWatcher.Err <- err
156 
+
157 157 
 				return
158 158 
 			}
159 159
160 
-			logrus.WithField("logger", "json-file").Debugf("waiting for events")
161 
-			if err := fileWatcher.Add(f.Name()); err != nil {
162 
-				logrus.WithField("logger", "json-file").Warn("falling back to file poller")
163 
-				fileWatcher.Close()
164 
-				fileWatcher = filenotify.NewPollingWatcher()
165 
-				if err := fileWatcher.Add(f.Name()); err != nil {
166 
-					logrus.Errorf("error watching log file for modifications: %v", err)
167 
-					logWatcher.Err <- err
168 
-				}
169 
-			}
170 160 
 			select {
171 161 
 			case <-fileWatcher.Events():
172 162 
 				dec = json.NewDecoder(f)
173 
-				fileWatcher.Remove(f.Name())
174 163 
 				continue
175 164 
 			case <-fileWatcher.Errors():
176 
-				fileWatcher.Remove(f.Name())
177 165 
 				logWatcher.Err <- err
178 166 
 				return
179 167 
 			case <-logWatcher.WatchClose():
180 
-				fileWatcher.Remove(f.Name())
168 
+				fileWatcher.Remove(name)
181 169 
 				return
182 170 
 			case <-notifyRotate:
183 
-				f, err = os.Open(f.Name())
171 
+				f.Close()
172 
+				fileWatcher.Remove(name)
173 
+
174 
+				// retry when the file doesn't exist
175 
+				for retries := 0; retries <= 5; retries++ {
176 
+					f, err = os.Open(name)
177 
+					if err == nil || !os.IsNotExist(err) {
178 
+						break
179 
+					}
180 
+				}
181 
+
182 
+				if err = fileWatcher.Add(name); err != nil {
183 
+					logWatcher.Err <- err
184 
+					return
185 
+				}
184 186 
 				if err != nil {
185 187 
 					logWatcher.Err <- err
186 188 
 					return
187 189 
 				}
188 190
189 191 
 				dec = json.NewDecoder(f)
190 
-				fileWatcher.Remove(f.Name())
191 
-				fileWatcher.Add(f.Name())
192 192 
 				continue
193 193 
 			}
194 194 
 		}
daemon/logger/loggerutils/rotatefilewriter.go
History View file @ 1ae9dcf
... ... 
@@ -87,33 +87,17 @@ func rotate(name string, maxFiles int) error {
87 87 
 	for i := maxFiles - 1; i > 1; i-- {
88 88 
 		toPath := name + "." + strconv.Itoa(i)
89 89 
 		fromPath := name + "." + strconv.Itoa(i-1)
90 
-		if err := backup(fromPath, toPath); err != nil && !os.IsNotExist(err) {
90 
+		if err := os.Rename(fromPath, toPath); err != nil && !os.IsNotExist(err) {
91 91 
 			return err
92 92 
 		}
93 93 
 	}
94 94
95 
-	if err := backup(name, name+".1"); err != nil {
95 
+	if err := os.Rename(name, name+".1"); err != nil && !os.IsNotExist(err) {
96 96 
 		return err
97 97 
 	}
98 98 
 	return nil
99 99 
 }
100 100
101 
-// backup renames a file from fromPath to toPath
102 
-func backup(fromPath, toPath string) error {
103 
-	if _, err := os.Stat(fromPath); os.IsNotExist(err) {
104 
-		return err
105 
-	}
106 
-
107 
-	if _, err := os.Stat(toPath); !os.IsNotExist(err) {
108 
-		err := os.Remove(toPath)
109 
-		if err != nil {
110 
-			return err
111 
-		}
112 
-	}
113 
-
114 
-	return os.Rename(fromPath, toPath)
115 
-}
116 
-
117 101 
 // LogPath returns the location the given writer logs to.
118 102 
 func (w *RotateFileWriter) LogPath() string {
119 103 
 	return w.f.Name()
integration-cli/docker_cli_logs_bench_test.go
History View file @ 1ae9dcf
120 104 
new file mode 100644
... ... 
@@ -0,0 +1,32 @@
0 
+package main
1 
+
2 
+import (
3 
+	"fmt"
4 
+	"strings"
5 
+	"time"
6 
+
7 
+	"github.com/go-check/check"
8 
+)
9 
+
10 
+func (s *DockerSuite) BenchmarkLogsCLIRotateFollow(c *check.C) {
11 
+	out, _ := dockerCmd(c, "run", "-d", "--log-opt", "max-size=1b", "--log-opt", "max-file=10", "busybox", "sh", "-c", "while true; do usleep 50000; echo hello; done")
12 
+	id := strings.TrimSpace(out)
13 
+	ch := make(chan error, 1)
14 
+	go func() {
15 
+		ch <- nil
16 
+		out, _, _ := dockerCmdWithError("logs", "-f", id)
17 
+		// if this returns at all, it's an error
18 
+		ch <- fmt.Errorf(out)
19 
+	}()
20 
+
21 
+	<-ch
22 
+	select {
23 
+	case <-time.After(30 * time.Second):
24 
+		// ran for 30 seconds with no problem
25 
+		return
26 
+	case err := <-ch:
27 
+		if err != nil {
28 
+			c.Fatal(err)
29 
+		}
30 
+	}
31 
+}