Merge #31405

31405: roachtest: fix stdout output r=andreimatei a=andreimatei

roachtest's intention is that if there's no parallelism is requested,
logs are teed to stdout/stderr in addition to the test.log file. No
parallelism is specified explicitly by passing -p 1, or implicitly by
running a single test.
This patch fixes a bug where the "implicit" part was no longer behaving
as expected wrt the logs. The bug was introduced by a previous patch
that tried to reduce the scope of the "parallelism" global, but missed
the use in rootLogger(); the inferring of parallelism=1 was no longer
modifying the global. This patch kills the global for good.

Release note: None

Co-authored-by: Andrei Matei <[email protected]>

Author: craig[bot]

pkg/cmd/roachtest/cluster.go

@@ -627,6 +627,7 @@ type clusterConfig struct {
 	nodes        []nodeSpec
 	artifactsDir string
 	localCluster bool
+	teeOpt       teeOptType
 }
 
 // newCluster creates a new roachprod cluster.
@@ -668,7 +669,7 @@ func newCluster(ctx context.Context, cfg clusterConfig) (*cluster, error) {
 	}
 
 	logPath := filepath.Join(cfg.artifactsDir, "test.log")
-	l, err := rootLogger(logPath)
+	l, err := rootLogger(logPath, cfg.teeOpt)
 	if err != nil {
 		return nil, err
 	}
@@ -700,6 +701,9 @@ func newCluster(ctx context.Context, cfg clusterConfig) (*cluster, error) {
 }
 
 type attachOpt struct {
+	// If set, the c.l will output to stdout/stderr.
+	teeToStdout bool
+
 	skipValidation bool
 	// Implies skipWipe.
 	skipStop bool
@@ -719,7 +723,11 @@ func attachToExistingCluster(
 	}
 
 	logPath := filepath.Join(artifactsDir, "test.log")
-	l, err := rootLogger(logPath)
+	teeOpt := noTee
+	if opt.teeToStdout {
+		teeOpt = teeToStdout
+	}
+	l, err := rootLogger(logPath, teeOpt)
 	if err != nil {
 		return nil, err
 	}
@@ -819,9 +827,9 @@ func (c *cluster) validate(ctx context.Context, nodes []nodeSpec, l *logger) err
 
 // clone creates a new cluster object that refers to the same cluster as the
 // receiver, but is associated with the specified test.
-func (c *cluster) clone(t *test) *cluster {
+func (c *cluster) clone(t *test, teeOpt teeOptType) *cluster {
 	logPath := filepath.Join(t.ArtifactsDir(), "test.log")
-	l, err := rootLogger(logPath)
+	l, err := rootLogger(logPath, teeOpt)
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/cmd/roachtest/log.go

@@ -106,10 +106,16 @@ func (cfg *loggerConfig) newLogger(path string) (*logger, error) {
 	}, nil
 }
 
-func rootLogger(path string) (*logger, error) {
+type teeOptType bool
+
+const (
+	teeToStdout teeOptType = true
+	noTee       teeOptType = false
+)
+
+func rootLogger(path string, teeOpt teeOptType) (*logger, error) {
 	var stdout, stderr io.Writer
-	// Log to stdout/stderr if we're not running tests in parallel.
-	if parallelism == 1 {
+	if teeOpt == teeToStdout {
 		stdout = os.Stdout
 		stderr = os.Stderr
 	}

pkg/cmd/roachtest/main.go

@@ -24,6 +24,8 @@ import (
 )
 
 func main() {
+	parallelism := 10
+
 	cobra.EnableCommandSorting = false
 
 	var rootCmd = &cobra.Command{

pkg/cmd/roachtest/test.go

@@ -40,7 +40,6 @@ import (
 )
 
 var (
-	parallelism   = 10
 	count         = 1
 	debugEnabled  = false
 	postIssues    = true
@@ -346,8 +345,14 @@ func (r *registry) Run(filter []string, parallelism int) int {
 				if count == 1 {
 					runNum = 0
 				}
+				// Log to stdout/stderr if we're not running tests in parallel.
+				teeOpt := noTee
+				if parallelism == 1 {
+					teeOpt = teeToStdout
+				}
 				r.runAsync(
-					ctx, tests[i], filterRE, nil /* parent */, nil /* cluster */, runNum, func(failed bool) {
+					ctx, tests[i], filterRE, nil /* parent */, nil, /* cluster */
+					runNum, teeOpt, func(failed bool) {
 						wg.Done()
 						<-sem
 					})
@@ -709,6 +714,7 @@ func (r *registry) runAsync(
 	parent *test,
 	c *cluster,
 	runNum int,
+	teeOpt teeOptType,
 	done func(failed bool),
 ) {
 	artifactsSuffix := ""
@@ -873,6 +879,7 @@ func (r *registry) runAsync(
 					nodes:        t.spec.Nodes,
 					artifactsDir: t.ArtifactsDir(),
 					localCluster: local,
+					teeOpt:       teeOpt,
 				}
 				var err error
 				c, err = newCluster(ctx, cfg)
@@ -898,7 +905,7 @@ func (r *registry) runAsync(
 				}()
 			}
 		} else {
-			c = c.clone(t)
+			c = c.clone(t, teeOpt)
 		}
 
 		// If we have subtests, handle them here and return.
@@ -907,22 +914,23 @@ func (r *registry) runAsync(
 				if t.spec.SubTests[i].matchRegex(filter) {
 					var wg sync.WaitGroup
 					wg.Add(1)
-					r.runAsync(ctx, &t.spec.SubTests[i], filter, t, c, runNum, func(failed bool) {
-						if failed {
-							// Mark the parent test as failed since one of the subtests
-							// failed.
-							t.mu.Lock()
-							t.mu.failed = true
-							t.mu.Unlock()
-						}
-						if failed && debugEnabled {
-							// The test failed and debugging is enabled. Don't try to stumble
-							// forward running another test or subtest, just exit
-							// immediately.
-							os.Exit(1)
-						}
-						wg.Done()
-					})
+					r.runAsync(ctx, &t.spec.SubTests[i], filter, t, c,
+						runNum, teeOpt, func(failed bool) {
+							if failed {
+								// Mark the parent test as failed since one of the subtests
+								// failed.
+								t.mu.Lock()
+								t.mu.failed = true
+								t.mu.Unlock()
+							}
+							if failed && debugEnabled {
+								// The test failed and debugging is enabled. Don't try to stumble
+								// forward running another test or subtest, just exit
+								// immediately.
+								os.Exit(1)
+							}
+							wg.Done()
+						})
 					wg.Wait()
 				}
 			}