Merge pull request #349 from pohly/init-flags-concurrency

InitFlags concurrency fix
kubernetes · Sep 8, 2022 · cb9292a · cb9292a
2 parents 02fe323 + 28f7906
commit cb9292a
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 32 deletions.
diff --git a/klog.go b/klog.go
@@ -396,45 +396,48 @@ type flushSyncWriter interface {
 	io.Writer
 }
 
-// init sets up the defaults.
+var logging loggingT
+var commandLine flag.FlagSet
+
+// init sets up the defaults and creates command line flags.
 func init() {
+	commandLine.StringVar(&logging.logDir, "log_dir", "", "If non-empty, write log files in this directory (no effect when -logtostderr=true)")
+	commandLine.StringVar(&logging.logFile, "log_file", "", "If non-empty, use this log file (no effect when -logtostderr=true)")
+	commandLine.Uint64Var(&logging.logFileMaxSizeMB, "log_file_max_size", 1800,
+		"Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. "+
+			"If the value is 0, the maximum file size is unlimited.")
+	commandLine.BoolVar(&logging.toStderr, "logtostderr", true, "log to standard error instead of files")
+	commandLine.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files (no effect when -logtostderr=true)")
+	logging.setVState(0, nil, false)
+	commandLine.Var(&logging.verbosity, "v", "number for the log level verbosity")
+	commandLine.BoolVar(&logging.addDirHeader, "add_dir_header", false, "If true, adds the file directory to the header of the log messages")
+	commandLine.BoolVar(&logging.skipHeaders, "skip_headers", false, "If true, avoid header prefixes in the log messages")
+	commandLine.BoolVar(&logging.oneOutput, "one_output", false, "If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true)")
+	commandLine.BoolVar(&logging.skipLogHeaders, "skip_log_headers", false, "If true, avoid headers when opening log files (no effect when -logtostderr=true)")
 	logging.stderrThreshold = severityValue{
 		Severity: severity.ErrorLog, // Default stderrThreshold is ERROR.
 	}
-	logging.setVState(0, nil, false)
-	logging.logDir = ""
-	logging.logFile = ""
-	logging.logFileMaxSizeMB = 1800
-	logging.toStderr = true
-	logging.alsoToStderr = false
-	logging.skipHeaders = false
-	logging.addDirHeader = false
-	logging.skipLogHeaders = false
-	logging.oneOutput = false
+	commandLine.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false)")
+	commandLine.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
+	commandLine.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")
+
+	logging.settings.contextualLoggingEnabled = true
 	logging.flushD = newFlushDaemon(logging.lockAndFlushAll, nil)
 }
 
 // InitFlags is for explicitly initializing the flags.
+// It may get called repeatedly for different flagsets, but not
+// twice for the same one. May get called concurrently
+// to other goroutines using klog. However, only some flags
+// may get set concurrently (see implementation).
 func InitFlags(flagset *flag.FlagSet) {
 	if flagset == nil {
 		flagset = flag.CommandLine
 	}
 
-	flagset.StringVar(&logging.logDir, "log_dir", logging.logDir, "If non-empty, write log files in this directory (no effect when -logtostderr=true)")
-	flagset.StringVar(&logging.logFile, "log_file", logging.logFile, "If non-empty, use this log file (no effect when -logtostderr=true)")
-	flagset.Uint64Var(&logging.logFileMaxSizeMB, "log_file_max_size", logging.logFileMaxSizeMB,
-		"Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. "+
-			"If the value is 0, the maximum file size is unlimited.")
-	flagset.BoolVar(&logging.toStderr, "logtostderr", logging.toStderr, "log to standard error instead of files")
-	flagset.BoolVar(&logging.alsoToStderr, "alsologtostderr", logging.alsoToStderr, "log to standard error as well as files (no effect when -logtostderr=true)")
-	flagset.Var(&logging.verbosity, "v", "number for the log level verbosity")
-	flagset.BoolVar(&logging.addDirHeader, "add_dir_header", logging.addDirHeader, "If true, adds the file directory to the header of the log messages")
-	flagset.BoolVar(&logging.skipHeaders, "skip_headers", logging.skipHeaders, "If true, avoid header prefixes in the log messages")
-	flagset.BoolVar(&logging.oneOutput, "one_output", logging.oneOutput, "If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true)")
-	flagset.BoolVar(&logging.skipLogHeaders, "skip_log_headers", logging.skipLogHeaders, "If true, avoid headers when opening log files (no effect when -logtostderr=true)")
-	flagset.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false)")
-	flagset.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
-	flagset.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")
+	commandLine.VisitAll(func(f *flag.Flag) {
+		flagset.Var(f.Value, f.Name, f.Usage)
+	})
 }
 
 // Flush flushes all pending log I/O.
@@ -549,12 +552,6 @@ type loggingT struct {
 	vmap map[uintptr]Level
 }
 
-var logging = loggingT{
-	settings: settings{
-		contextualLoggingEnabled: true,
-	},
-}
-
 // setVState sets a consistent state for V logging.
 // l.mu is held.
 func (l *loggingT) setVState(verbosity Level, filter []modulePat, setFilter bool) {

diff --git a/klog_test.go b/klog_test.go
@@ -764,6 +764,9 @@ func TestFileSizeCheck(t *testing.T) {
 }
 
 func TestInitFlags(t *testing.T) {
+	defer CaptureState().Restore()
+	setFlags()
+
 	fs1 := flag.NewFlagSet("test1", flag.PanicOnError)
 	InitFlags(fs1)
 	fs1.Set("log_dir", "/test1")
@@ -779,6 +782,48 @@ func TestInitFlags(t *testing.T) {
 	}
 }
 
+func TestCommandLine(t *testing.T) {
+	var fs flag.FlagSet
+	InitFlags(&fs)
+
+	expectedFlags := `  -add_dir_header
+    	If true, adds the file directory to the header of the log messages
+  -alsologtostderr
+    	log to standard error as well as files (no effect when -logtostderr=true)
+  -log_backtrace_at value
+    	when logging hits line file:N, emit a stack trace
+  -log_dir string
+    	If non-empty, write log files in this directory (no effect when -logtostderr=true)
+  -log_file string
+    	If non-empty, use this log file (no effect when -logtostderr=true)
+  -log_file_max_size uint
+    	Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
+  -logtostderr
+    	log to standard error instead of files (default true)
+  -one_output
+    	If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true)
+  -skip_headers
+    	If true, avoid header prefixes in the log messages
+  -skip_log_headers
+    	If true, avoid headers when opening log files (no effect when -logtostderr=true)
+  -stderrthreshold value
+    	logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false) (default 2)
+  -v value
+    	number for the log level verbosity
+  -vmodule value
+    	comma-separated list of pattern=N settings for file-filtered logging
+`
+
+	var output bytes.Buffer
+	fs.SetOutput(&output)
+	fs.PrintDefaults()
+	actualFlags := output.String()
+
+	if expectedFlags != actualFlags {
+		t.Fatalf("Command line changed.\nExpected:\n%q\nActual:\n%q\n", expectedFlags, actualFlags)
+	}
+}
+
 func TestInfoObjectRef(t *testing.T) {
 	defer CaptureState().Restore()
 	setFlags()