Handle log streams correctly for default stderr, file-only, and file+stderr logging.

Author: lxuan94-pp

cluster-autoscaler/config/flags/klog_flags.go

@@ -0,0 +1,68 @@
+package flags
+
+import (
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+
+	"github.com/spf13/pflag"
+	logsapi "k8s.io/component-base/logs/api/v1"
+)
+
+// getFlagString is a small helper to read a string flag from the given FlagSet.
+func getFlagString(fs *pflag.FlagSet, name string) string {
+	if f := fs.Lookup(name); f != nil {
+		return f.Value.String()
+	}
+	return ""
+}
+
+// getFlagBool is a small helper to read a bool flag from the given FlagSet.
+func getFlagBool(fs *pflag.FlagSet, name string) bool {
+	if f := fs.Lookup(name); f != nil {
+		val, _ := fs.GetBool(name)
+		return val
+	}
+	return false
+}
+
+// ComputeLoggingOptions computes logsapi.LoggingOptions based on klog-related flags present in fs.
+//
+// Semantics:
+// - By default (no log-file OR logtostderr=true), logs go to os.Stderr.
+// - If --log-file is set AND --logtostderr=false, logs go to the file.
+//   - If --alsologtostderr=true, logs also go to os.Stderr
+func ComputeLoggingOptions(fs *pflag.FlagSet) (*logsapi.LoggingOptions, error) {
+	logFilePath := getFlagString(fs, "log-file")
+	logToStderr := getFlagBool(fs, "logtostderr")
+	alsoLogToStderr := getFlagBool(fs, "alsologtostderr")
+
+	// default: both to stderr
+	var infoW, errW io.Writer = os.Stderr, os.Stderr
+
+	if logFilePath != "" && !logToStderr {
+		dir := filepath.Dir(logFilePath)
+		if err := os.MkdirAll(dir, 0o755); err != nil {
+			return nil, fmt.Errorf("failed to create log directory %q: %w", dir, err)
+		}
+		f, err := os.OpenFile(logFilePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644)
+		if err != nil {
+			return nil, fmt.Errorf("failed to open log file %q: %w", logFilePath, err)
+		}
+
+		// Primary output is now the file
+		infoW = f
+		errW = f
+
+		if alsoLogToStderr {
+			infoW = io.MultiWriter(f, os.Stderr)
+			errW = io.MultiWriter(f, os.Stderr)
+		}
+	}
+
+	return &logsapi.LoggingOptions{
+		ErrorStream: errW,
+		InfoStream:  infoW,
+	}, nil
+}

cluster-autoscaler/config/flags/klog_flags_test.go

@@ -0,0 +1,230 @@
+package flags
+
+import (
+	"io"
+	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/spf13/pflag"
+)
+
+// --------------------- HELPERS ---------------------
+
+func newTestFlagSet(t *testing.T, logFile string, logToStderr, alsoLogToStderr bool) *pflag.FlagSet {
+	t.Helper()
+	fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
+	fs.String("log-file", "", "")
+	fs.Bool("logtostderr", true, "")
+	fs.Bool("alsologtostderr", false, "")
+
+	if logFile != "" {
+		if err := fs.Set("log-file", logFile); err != nil {
+			t.Fatalf("set log-file: %v", err)
+		}
+	}
+	if err := fs.Set("logtostderr", boolToString(logToStderr)); err != nil {
+		t.Fatalf("set logtostderr: %v", err)
+	}
+	if err := fs.Set("alsologtostderr", boolToString(alsoLogToStderr)); err != nil {
+		t.Fatalf("set alsologtostderr: %v", err)
+	}
+	return fs
+}
+
+func boolToString(b bool) string {
+	if b {
+		return "true"
+	}
+	return "false"
+}
+
+// assertStreams checks the actual streams against expected values
+func assertStreams(t *testing.T, actualInfo, actualErr any, expectedInfo, expectedErr any) {
+	t.Helper()
+
+	check := func(name string, actual, expected any) {
+		if expected == nil {
+			if actual != nil {
+				t.Fatalf("%s: expected nil, got %T", name, actual)
+			}
+			return
+		}
+
+		if reflect.TypeOf(actual) != reflect.TypeOf(expected) {
+			t.Fatalf("%s type mismatch: expected %T, got %T", name, expected, actual)
+		}
+
+		if f, ok := expected.(*os.File); ok {
+			if actual.(*os.File) != f {
+				t.Fatalf("%s pointer mismatch: expected same *os.File", name)
+			}
+		}
+		if expected == os.Stderr {
+			if actual != os.Stderr {
+				t.Fatalf("%s mismatch: expected os.Stderr", name)
+			}
+		}
+
+		// For io.MultiWriter, type check is sufficient
+	}
+
+	check("InfoStream", actualInfo, expectedInfo)
+	check("ErrorStream", actualErr, expectedErr)
+}
+
+// --------------------- TESTS ---------------------
+
+func TestComputeLoggingOptions_DefaultToStderr(t *testing.T) {
+	fs := newTestFlagSet(t, "", true, false)
+	opts, err := ComputeLoggingOptions(fs)
+	if err != nil {
+		t.Fatalf("ComputeLoggingOptions error: %v", err)
+	}
+	assertStreams(t, opts.InfoStream, opts.ErrorStream, os.Stderr, os.Stderr)
+}
+
+func TestComputeLoggingOptions_LogFileIgnoredWhenLogToStderrTrue(t *testing.T) {
+	tmp := t.TempDir()
+	logPath := filepath.Join(tmp, "subdir", "ca.log")
+
+	fs := newTestFlagSet(t, logPath, true, false)
+	opts, err := ComputeLoggingOptions(fs)
+	if err != nil {
+		t.Fatalf("ComputeLoggingOptions error: %v", err)
+	}
+
+	if _, statErr := os.Stat(logPath); !os.IsNotExist(statErr) {
+		t.Fatalf("expected no log file created at %q, statErr=%v", logPath, statErr)
+	}
+
+	assertStreams(t, opts.InfoStream, opts.ErrorStream, os.Stderr, os.Stderr)
+}
+
+func TestComputeLoggingOptions_LogFileOnly(t *testing.T) {
+	tmp := t.TempDir()
+	logPath := filepath.Join(tmp, "nested", "onlyfile.log")
+
+	fs := newTestFlagSet(t, logPath, false, false)
+	opts, err := ComputeLoggingOptions(fs)
+	if err != nil {
+		t.Fatalf("ComputeLoggingOptions error: %v", err)
+	}
+
+	if _, statErr := os.Stat(logPath); statErr != nil {
+		t.Fatalf("expected log file created at %q, err=%v", logPath, statErr)
+	}
+
+	// Streams should be the same file writer
+	file := opts.InfoStream.(*os.File)
+	assertStreams(t, opts.InfoStream, opts.ErrorStream, file, file)
+
+	// Write and verify content
+	msg := "hello-file-only\n"
+	if _, err := file.Write([]byte(msg)); err != nil {
+		t.Fatalf("write to file failed: %v", err)
+	}
+	data, err := os.ReadFile(logPath)
+	if err != nil {
+		t.Fatalf("read log file failed: %v", err)
+	}
+	if !strings.Contains(string(data), msg) {
+		t.Fatalf("log file does not contain expected content")
+	}
+}
+
+func TestComputeLoggingOptions_LogFileAlsoToStderr(t *testing.T) {
+	tmp := t.TempDir()
+	logPath := filepath.Join(tmp, "withstderr", "combo.log")
+
+	fs := newTestFlagSet(t, logPath, false, true)
+	opts, err := ComputeLoggingOptions(fs)
+	if err != nil {
+		t.Fatalf("ComputeLoggingOptions error: %v", err)
+	}
+
+	// File must exist
+	if _, statErr := os.Stat(logPath); statErr != nil {
+		t.Fatalf("expected log file created at %q, err=%v", logPath, statErr)
+	}
+
+	file, err := os.OpenFile(logPath, os.O_APPEND|os.O_WRONLY, 0o644)
+	if err != nil {
+		t.Fatalf("failed to open log file to build expected writer: %v", err)
+	}
+	defer file.Close()
+
+	expectedWriter := io.MultiWriter(os.Stderr, file)
+
+	// Assert streams against expected MultiWriter
+	assertStreams(t, opts.InfoStream, opts.ErrorStream, expectedWriter, expectedWriter)
+
+	// Write and verify content appears in the file
+	msg := "hello-also-stderr\n"
+	if _, err := opts.InfoStream.Write([]byte(msg)); err != nil {
+		t.Fatalf("write to InfoStream failed: %v", err)
+	}
+	data, err := os.ReadFile(logPath)
+	if err != nil {
+		t.Fatalf("read log file failed: %v", err)
+	}
+	if !strings.Contains(string(data), msg) {
+		t.Fatalf("log file does not contain expected content")
+	}
+}
+
+func TestComputeLoggingOptions_CreateDirError(t *testing.T) {
+	tmp := t.TempDir()
+	notADirPath := filepath.Join(tmp, "notadir")
+	if err := os.WriteFile(notADirPath, []byte("x"), 0o644); err != nil {
+		t.Fatalf("prepare file failed: %v", err)
+	}
+	logPath := filepath.Join(notADirPath, "child", "ca.log")
+
+	fs := newTestFlagSet(t, logPath, false, false)
+	_, err := ComputeLoggingOptions(fs)
+	if err == nil || !strings.Contains(err.Error(), "failed to create log directory") {
+		t.Fatalf("expected create dir error, got: %v", err)
+	}
+}
+
+func TestComputeLoggingOptions_OpenFileError(t *testing.T) {
+	tmp := t.TempDir()
+	dirAsFile := filepath.Join(tmp, "adir")
+	if err := os.MkdirAll(dirAsFile, 0o755); err != nil {
+		t.Fatalf("prepare dir failed: %v", err)
+	}
+
+	fs := newTestFlagSet(t, dirAsFile, false, false)
+	_, err := ComputeLoggingOptions(fs)
+	if err == nil || !strings.Contains(err.Error(), "failed to open log file") {
+		t.Fatalf("expected open file error, got: %v", err)
+	}
+}
+
+func TestComputeLoggingOptions_WriterInterfaceUsable(t *testing.T) {
+	tmp := t.TempDir()
+	logPath := filepath.Join(tmp, "writeusable", "ca.log")
+
+	fs := newTestFlagSet(t, logPath, false, false)
+	opts, err := ComputeLoggingOptions(fs)
+	if err != nil {
+		t.Fatalf("ComputeLoggingOptions error: %v", err)
+	}
+
+	file := opts.ErrorStream.(*os.File)
+	n, err := file.Write([]byte("err-stream\n"))
+	if err != nil || n == 0 {
+		t.Fatalf("failed writing via ErrorStream: n=%d err=%v", n, err)
+	}
+
+	data, err := os.ReadFile(logPath)
+	if err != nil {
+		t.Fatalf("read log file failed: %v", err)
+	}
+	if !strings.Contains(string(data), "err-stream") {
+		t.Fatalf("log file does not contain expected err-stream content")
+	}
+}

cluster-autoscaler/go.sum

@@ -675,4 +675,4 @@ sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxO
 sigs.k8s.io/structured-merge-diff/v6 v6.3.0 h1:jTijUJbW353oVOd9oTlifJqOGEkUw2jB/fXCbTiQEco=
 sigs.k8s.io/structured-merge-diff/v6 v6.3.0/go.mod h1:M3W8sfWvn2HhQDIbGWj3S099YozAsymCo/wrT5ohRUE=
 sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs=
-sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4=
+sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4=

cluster-autoscaler/main.go

@@ -370,7 +370,13 @@ func main() {
 	}
 
 	logs.InitLogs()
-	if err := logsapi.ValidateAndApply(loggingConfig, featureGate); err != nil {
+
+	opts, err := flags.ComputeLoggingOptions(pflag.CommandLine)
+	if err != nil {
+		klog.Fatalf("Failed to configure logging: %v", err)
+	}
+
+	if err := logsapi.ValidateAndApplyWithOptions(loggingConfig, opts, featureGate); err != nil {
 		klog.Fatalf("Failed to validate and apply logging configuration: %v", err)
 	}