Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 11, 2017

Conversation

rjshep
Copy link
Contributor

@rjshep rjshep commented Jun 16, 2017

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
Copy link
Contributor

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

@rjshep
Copy link
Contributor Author

rjshep commented Jun 16, 2017

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
Copy link
Contributor

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
Copy link
Member

emilevauge commented Jun 20, 2017

@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
Copy link
Contributor

Thanks for the info! LGTM :)

@ldez
Copy link
Contributor

ldez commented Jul 7, 2017

@rjshep Could rebase and resolve conflicts?

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you contributions 👍

You need to resolve conflicts with access_log_test.go


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log/logger.go Outdated
// Close closes the log and sets the Output to stdout
func Close() error {
logrus.SetOutput(os.Stdout)
return logFile.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log/logger.go Outdated
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to OpenFile

log/logger.go Outdated
}

// Close closes the log and sets the Output to stdout
func Close() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to CloseFile

log/logger.go Outdated
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to RotateFile

@rjshep
Copy link
Contributor Author

rjshep commented Jul 18, 2017

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
Copy link
Contributor

ldez commented Jul 18, 2017

@rjshep I will fix that pb.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have left some comments.

defer ts3.Close()

// Allow time to startup
time.Sleep(1 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,153 @@
package integration
Copy link
Contributor

@timoreimann timoreimann Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - added

defer ts1.Close()
ts2 := startAccessLogServer(8082)
defer ts2.Close()
ts3 := startAccessLogServer(8083)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need three servers instead of just one?

}
c.Assert(count, checker.Equals, 1)

// Verify access.log output as expected
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

defer os.Remove(traefikTestLogFile)

// Allow time to startup
time.Sleep(1 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get that to work.

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

@timoreimann timoreimann Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above if we need it.

If we do, please rephrase to

"unable to open file %s: %s"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ldez
Copy link
Contributor

ldez commented Aug 2, 2017

@rjshep could fix the remaining comments of @timoreimann?

@emilevauge
Copy link
Member

@timoreimann any feedback on this one ?

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logs kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants