diff --git a/cmd/containerd/command/service_windows.go b/cmd/containerd/command/service_windows.go index 6b9be5f17955..2c682b4e852d 100644 --- a/cmd/containerd/command/service_windows.go +++ b/cmd/containerd/command/service_windows.go @@ -18,7 +18,6 @@ package command import ( "fmt" - "io" "log" "os" "path/filepath" @@ -214,7 +213,6 @@ func unregisterService() error { // to handle (un-)registering against Windows Service Control Manager (SCM). // It returns an indication to stop on successful SCM operation, and an error. func registerUnregisterService(root string) (bool, error) { - if unregisterServiceFlag { if registerServiceFlag { return true, fmt.Errorf("--register-service and --unregister-service cannot be used together: %w", errdefs.ErrInvalidArgument) @@ -248,22 +246,57 @@ func registerUnregisterService(root string) (bool, error) { return true, err } - logOutput := io.Discard + // The usual advice for Windows services is to either write to a log file or to the windows event + // log, the former of which we've exposed here via a --log-file flag. We additionally write panic + // stacks to a panic.log file to diagnose crashes. Below details the two different outcomes if + // --log-file is specified or not: + // + // --log-file is *not* specified. + // ------------------------------- + // -logrus, the stdlibs logging package and os.Stderr output will go to + // NUL (Windows' /dev/null equivalent). + // -Panics will write their stack trace to the panic.log file. + // -Writing to the handle returned from GetStdHandle(STD_ERROR_HANDLE) will write + // to the panic.log file as the underlying handle itself has been redirected. + // + // --log-file *is* specified + // ------------------------------- + // -Logging to logrus, the stdlibs logging package or directly to + // os.Stderr will all go to the log file specified. + // -Panics will write their stack trace to the panic.log file. + // -Writing to the handle returned from GetStdHandle(STD_ERROR_HANDLE) will write + // to the panic.log file as the underlying handle itself has been redirected. + var f *os.File if logFileFlag != "" { - f, err := os.OpenFile(logFileFlag, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + f, err = os.OpenFile(logFileFlag, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err != nil { return true, fmt.Errorf("open log file %q: %w", logFileFlag, err) } - logOutput = f + } else { + // Windows services start with NULL stdio handles, and thus os.Stderr and friends will be + // backed by an os.File with a NULL handle. This means writes to os.Stderr will fail, which + // isn't a huge issue as we want output to be discarded if the user doesn't ask for the log + // file. However, writes succeeding but just going to the ether is a much better construct + // so use devnull instead of relying on writes failing. We use devnull instead of io.Discard + // as os.Stderr is an os.File and can't be assigned to io.Discard. + f, err = os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + return true, err + } } - logrus.SetOutput(logOutput) + // Reassign os.Stderr to the log file or NUL. Shim logs are copied to os.Stderr + // directly so this ensures those will end up in the log file as well if specified. + os.Stderr = f + // Assign the stdlibs log package in case of any miscellaneous uses by + // dependencies. + log.SetOutput(f) + logrus.SetOutput(f) } return false, nil } // launchService is the entry point for running the daemon under SCM. func launchService(s *server.Server, done chan struct{}) error { - if !runServiceFlag { return nil } @@ -359,12 +392,6 @@ func initPanicFile(path string) error { return err } - // Reset os.Stderr to the panic file (so fmt.Fprintf(os.Stderr,...) actually gets redirected) - os.Stderr = os.NewFile(panicFile.Fd(), "/dev/stderr") - - // Force threads that panic to write to stderr (the panicFile handle now), otherwise it will go into the ether - log.SetOutput(os.Stderr) - return nil }