Merge #61232

61232: cli: ensure that `--log` is recognized for non-server commands r=aaron-crl a=knz cc @taroface Release justification: bug fixes and low-risk updates to new functionality Release note (bug fix): The non-server `cockroach` commands now recognize the new `--log` flag properly. This had been broken unadvertently in one of the earlier 21.1 alpha releases. Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Feb 28, 2021 · 4bb49c3 · 4bb49c3
2 parents 9028433 + d1cd456
commit 4bb49c3
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 5 deletions.
diff --git a/pkg/acceptance/cluster/dockercluster.go b/pkg/acceptance/cluster/dockercluster.go
@@ -523,7 +523,8 @@ func (l *DockerCluster) RunInitCommand(ctx context.Context, nodeIdx int) {
 			"init",
 			"--certs-dir=/certs/",
 			"--host=" + l.Nodes[nodeIdx].nodeStr,
-			"--logtostderr",
+			"--log-dir=/logs/init-command",
+			"--logtostderr=NONE",
 		},
 	}
 
@@ -639,14 +640,15 @@ func (l *DockerCluster) Start(ctx context.Context) {
 	log.Infof(ctx, "creating node certs (%dbit) in: %s", keyLen, certsDir)
 	l.createNodeCerts()
 
+	log.Infof(ctx, "starting %d nodes", len(l.Nodes))
 	l.monitorCtx, l.monitorCtxCancelFunc = context.WithCancel(context.Background())
 	go l.monitor(ctx)
 	var wg sync.WaitGroup
 	wg.Add(len(l.Nodes))
 	for _, node := range l.Nodes {
 		go func(node *testNode) {
+			defer wg.Done()
 			l.startNode(ctx, node)
-			wg.Done()
 		}(node)
 	}
 	wg.Wait()

diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go
@@ -81,9 +81,24 @@ func Main() {
 func doMain(cmd *cobra.Command, cmdName string) error {
 	if cmd != nil && !cmdHasCustomLoggingSetup(cmd) {
 		// the customLoggingSetupCmds do their own calls to setupLogging().
-		if err := setupLogging(context.Background(), cmd,
-			false /* isServerCmd */, true /* applyConfig */); err != nil {
-			return err
+		//
+		// We use a PreRun function, to ensure setupLogging() is only
+		// called after the command line flags have been parsed.
+		//
+		// NB: we cannot use PersistentPreRunE,like in flags.go, because
+		// overriding that here will prevent the persistent pre-run from
+		// running on parent commands. (See the difference between PreRun
+		// and PersistentPreRun in `(*cobra.Command) execute()`.)
+		wrapped := cmd.PreRunE
+		cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
+			if wrapped != nil {
+				if err := wrapped(cmd, args); err != nil {
+					return err
+				}
+			}
+
+			return setupLogging(context.Background(), cmd,
+				false /* isServerCmd */, true /* applyConfig */)
 		}
 	}
 

diff --git a/pkg/cli/interactive_tests/test_log_flags.tcl b/pkg/cli/interactive_tests/test_log_flags.tcl
@@ -63,5 +63,23 @@ eexpect "parsing \"cantparse\": invalid syntax"
 eexpect ":/# "
 end_test
 
+start_test "Check that conflicting legacy and new flags are properly rejected for server commands"
+send "$argv start-single-node --insecure --logtostderr=true --log=abc\r"
+eexpect "log is incompatible with legacy discrete logging flag"
+eexpect ":/# "
+end_test
+
+start_test "Check that conflicting legacy and new flags are properly rejected for client commands"
+send "$argv sql --insecure --logtostderr=true --log=abc\r"
+eexpect "log is incompatible with legacy discrete logging flag"
+eexpect ":/# "
+end_test
+
+start_test "Check that the log flag is properly recognized for non-server commands"
+send "$argv debug reset-quorum 123 --log='sinks: {stderr: {format: json }}'\r"
+eexpect "\"severity\":\"ERROR\""
+eexpect "connection to server failed"
+eexpect ":/# "
+
 send "exit 0\r"
 eexpect eof