Merge pull request #7023 from dcantah/fix-panicfile-shimlogging

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
 }