Add log file close and reopen on receipt of SIGUSR1 #1761

rjshep · 2017-06-16T12:56:05Z

See Issue #1696

Traefik can currently write to two log files, the traefik log and an access log. These log files grow
continuously and there is no mechanism for rotating, compressing, or archiving them.

This PR lets Traefik to respond to a USR1 signal by closing and reopening its log files, allowing external software, such as log rotate to be used for the above purposes.

m3co-code · 2017-06-16T14:03:51Z

Isn't SIGHUP usually used for signalling reloading of logs/configs files?

rjshep · 2017-06-16T14:08:08Z

There isn't a standard from what I can tell, both are used, but SIGUSR1 seems a better fit as SIGHUP is more about process restart, which we're not doing.

m3co-code · 2017-06-16T14:19:16Z

Not 100% sure about this one, all programs I worked with were listening for a SIGHUP to reopen log files and the logrotate docs show it also as canonical example.

emilevauge · 2017-06-20T10:23:40Z

@marco-jantke logrotate docs uses syslogd, which indeed reloads its logs files on SIGHUP but especially its configuration files.
@rjshep's suggestion is identical to Nginx behavior https://nginx.org/en/docs/control.html.
There is no standard, but sticking with Nginx signal management seems good to me:)

m3co-code · 2017-06-27T13:35:40Z

Thanks for the info! LGTM :)

ldez · 2017-07-07T22:37:01Z

@rjshep Could rebase and resolve conflicts?

ldez

Thanks for you contributions 👍

You need to resolve conflicts with access_log_test.go

ldez · 2017-07-18T13:11:18Z

integration/log_rotation_test.go

+
+func (s *LogRotationSuite) TestAccessLogRotation(c *check.C) {
+	// Start Traefik
+	cmd := exec.Command(traefikBinary, "--configFile=fixtures/access_log_config.toml")


could you replace this line by cmd, _ := s.cmdTraefik(withConfigFile("fixtures/access_log_config.toml"))

ldez · 2017-07-18T13:11:43Z

integration/log_rotation_test.go

+
+func (s *LogRotationSuite) TestTraefikLogRotation(c *check.C) {
+	// Start Traefik
+	cmd := exec.Command(traefikBinary, "--configFile=fixtures/traefik_log_config.toml")


could you replace this line by cmd, _ := s.cmdTraefik(withConfigFile("fixtures/access_log_config.toml"))

ldez · 2017-07-18T13:13:22Z

integration/log_rotation_test.go

+		}
+		if len(traefikLog) > 0 {
+			fmt.Printf("%s\n", string(traefikLog))
+			c.Assert(len(traefikLog), checker.Equals, 0)


could replace this line by c.Assert(traefikLog, checker.HasLen, 0)

ldez · 2017-07-18T13:22:46Z

log/logger.go

+// Close closes the log and sets the Output to stdout
+func Close() error {
+	logrus.SetOutput(os.Stdout)
+	return logFile.Close()


logFile can be nil, maybe you need to protect the call.

ldez · 2017-07-18T13:24:01Z

log/logger.go

@@ -190,6 +194,44 @@ func Fatalln(args ...interface{}) {
 	logger.Fatalln(args...)
 }

+// Open opens the log file using the specified path and
+func Open(path string) error {


maybe rename to OpenFile

ldez · 2017-07-18T13:24:10Z

log/logger.go

+}
+
+// Close closes the log and sets the Output to stdout
+func Close() error {


maybe rename to CloseFile

ldez · 2017-07-18T13:24:42Z

log/logger.go

+// Rotate closes and reopens the log file to allow for rotation
+// by an external source.  If the log isn't backed by a file then
+// does nothing.
+func Rotate() error {


maybe rename to RotateFile

rjshep · 2017-07-18T14:59:22Z

Hi @ldez, I've made the requested changes but I can't seem to get Semaphore to build. It says the build has failed but without giving an error - that I can see anyway. Any ideas?

ldez · 2017-07-18T15:26:36Z

@rjshep I will fix that pb.

timoreimann

Thanks, I have left some comments.

timoreimann · 2017-07-19T22:25:34Z

integration/log_rotation_test.go

+	defer ts3.Close()
+
+	// Allow time to startup
+	time.Sleep(1 * time.Second)


Any chance we can use some of our Try* functions from the integration/try package to poll the access log servers for readiness? We're trying to avoid having static timers these days which are susceptible to flakiness.

timoreimann · 2017-07-19T22:27:01Z

integration/log_rotation_test.go

@@ -0,0 +1,153 @@
+package integration


Could we use a build tag here to avoid running the test on Windows (where it would always fail to my understanding)? Users may want to run the tests natively, i.e., outside of our Linux-based build container).

Yes - added

timoreimann · 2017-07-19T22:27:48Z

integration/log_rotation_test.go

+	defer ts1.Close()
+	ts2 := startAccessLogServer(8082)
+	defer ts2.Close()
+	ts3 := startAccessLogServer(8083)


Why do we need three servers instead of just one?

timoreimann · 2017-07-19T22:29:55Z

integration/log_rotation_test.go

+	}
+	c.Assert(count, checker.Equals, 1)
+
+	// Verify access.log output as expected


How about removing the duplicated block by introducing a loop over both log file names?

timoreimann · 2017-07-19T22:30:47Z

integration/log_rotation_test.go

+	defer os.Remove(traefikTestLogFile)
+
+	// Allow time to startup
+	time.Sleep(1 * time.Second)


Same suggestion regarding a replacement of the Sleep by a Try* function.

timoreimann · 2017-07-19T22:42:08Z

middlewares/accesslog/logger.go

+// Rotate closes and reopens the log file to allow for rotation
+// by an external source.
+func (l *LogHandler) Rotate() error {
+	var err error


Should suffice to be declared on line 150 only if we also scope err on line 147 to the if statement?

I couldn't get that to work.

timoreimann · 2017-07-19T22:44:19Z

middlewares/accesslog/logger.go

+func (l *LogHandler) Rotate() error {
+	var err error
+	if err = l.Close(); err != nil {
+		return fmt.Errorf("unable to close file: %s", err)


What does the error returned by l.Close() look like? Often, stdlib's file-related error messages frequently contain the file name and the problem, so we might not need the "unable to close file" part (and thereby possibly avoid stuttering).

If we need it, please mention the filename in the error message.

You're right, changed to just return the error.

timoreimann · 2017-07-19T22:45:09Z

middlewares/accesslog/logger.go

+
+	l.file, err = os.OpenFile(l.filePath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0664)
+	if err != nil {
+		return fmt.Errorf("unable to open file: %s, %s", l.filePath, err)


Same question as above if we need it.

If we do, please rephrase to

"unable to open file %s: %s"

timoreimann · 2017-07-19T22:48:59Z

server/server_signals_windows.go

+		sig := <-server.signals
+		switch sig {
+		default:
+			log.Infof("I have to go... %+v", sig)


Can we move the block into a central function and call it from both listenSignals implementations?

We can't move the block because syscall.SIGUSR1 isn't defined on Windows.

timoreimann · 2017-07-19T22:51:00Z

server/server_signals.go

+)
+
+func (server *Server) configureSignals() {
+	signal.Notify(server.signals, syscall.SIGINT, syscall.SIGTERM, syscall.SIGUSR1)


What happens if we try to register SIGUSR1 on Windows? Will the machine burn up in flames, or may the signal rather just get transmitted?

In case of the latter, could we maybe drop server_signals_windows.go?

SIGUSR1 isn't present in the Windows build of Go, so the code won't compile. That was the reason for having server_signals_windows.go in the first place.

ldez · 2017-08-02T14:14:39Z

@rjshep could fix the remaining comments of @timoreimann?

emilevauge · 2017-08-10T14:54:54Z

@timoreimann any feedback on this one ?

timoreimann

Sorry for the awfully long delay. LGTM, thanks. 👏

ldez

LGTM!

juliens

LGTM

ldez added area/logs kind/enhancement a new or improved feature. status/2-needs-review labels Jun 16, 2017

rjshep force-pushed the logrotate branch from a06022b to f276c55 Compare June 16, 2017 14:09

ldez added status/1-needs-design-review and removed status/2-needs-review labels Jun 16, 2017

rjshep force-pushed the logrotate branch from 1eff304 to 9f51f36 Compare June 23, 2017 10:46

ldez added status/2-needs-review and removed status/1-needs-design-review labels Jun 27, 2017

rjshep force-pushed the logrotate branch 2 times, most recently from 54945aa to 689a2cb Compare June 29, 2017 13:34

ldez added contributor/waiting-for-corrections and removed contributor/waiting-for-corrections labels Jun 29, 2017

ldez added the contributor/needs-resolve-conflicts label Jul 7, 2017

ldez modified the milestone: 1.4 Jul 7, 2017

rjshep force-pushed the logrotate branch from 689a2cb to 48e28b4 Compare July 9, 2017 09:58

ldez requested changes Jul 18, 2017

View reviewed changes

ldez added the contributor/waiting-for-corrections label Jul 18, 2017

rjshep force-pushed the logrotate branch 3 times, most recently from 56c0fdd to 27573c2 Compare July 18, 2017 14:51

ldez reopened this Jul 18, 2017

rjshep force-pushed the logrotate branch from 27573c2 to 2a5a117 Compare July 18, 2017 15:55

timoreimann suggested changes Jul 19, 2017

View reviewed changes

ldez added the contributor/waiting-for-corrections label Jul 19, 2017

rjshep force-pushed the logrotate branch from 2a5a117 to 1606065 Compare July 24, 2017 08:43

ldez added contributor/waiting-for-feedback contributor/waiting-for-corrections and removed contributor/waiting-for-corrections contributor/waiting-for-feedback labels Jul 24, 2017

rjshep force-pushed the logrotate branch from 1606065 to 3719e24 Compare August 2, 2017 14:47

ldez removed the contributor/waiting-for-corrections label Aug 2, 2017

rjshep force-pushed the logrotate branch from 3719e24 to 7026979 Compare August 3, 2017 08:06

ldez added size/M and removed size/M labels Aug 4, 2017

timoreimann approved these changes Aug 10, 2017

View reviewed changes

ldez approved these changes Aug 11, 2017

View reviewed changes

rjshep force-pushed the logrotate branch from a78bd45 to 1136762 Compare August 11, 2017 06:39

Add log file close and reopen on receipt of SIGUSR1.

cce72a0

ldez force-pushed the logrotate branch from 1136762 to cce72a0 Compare August 11, 2017 08:21

ldez added status/3-needs-merge and removed status/2-needs-review labels Aug 11, 2017

juliens approved these changes Aug 11, 2017

View reviewed changes

ldez merged commit 4e5fcac into traefik:master Aug 11, 2017

ldez removed the status/3-needs-merge label Aug 11, 2017

fcatae mentioned this pull request Apr 27, 2018

Allow log rotation in Windows #3244

Closed

4 tasks

daniel-garcia mentioned this pull request Jun 14, 2018

support log rotation within traefik binary #3489

Closed

altovedo deleted the logrotate branch September 12, 2018 08:59

Sarke mentioned this pull request Aug 19, 2020

Ability to hot-reload certs listed in config files #7202

Closed

Add log file close and reopen on receipt of SIGUSR1 #1761

Add log file close and reopen on receipt of SIGUSR1 #1761

Conversation

rjshep commented Jun 16, 2017

m3co-code commented Jun 16, 2017

rjshep commented Jun 16, 2017

m3co-code commented Jun 16, 2017

emilevauge commented Jun 20, 2017 • edited

m3co-code commented Jun 27, 2017

ldez commented Jul 7, 2017 • edited

ldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjshep commented Jul 18, 2017

ldez commented Jul 18, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented Aug 2, 2017

emilevauge commented Aug 10, 2017

timoreimann left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

emilevauge commented Jun 20, 2017 •

edited

ldez commented Jul 7, 2017 •

edited

timoreimann Jul 19, 2017 •

edited

timoreimann Jul 19, 2017 •

edited