Merge pull request #156 from rafiss/improve-stdout

.github/workflows/ci.yaml

@@ -26,6 +26,7 @@ jobs:
 
     - name: Install dependencies
       run: |
+        # The latest crlfmt requires go1.19.
         go install github.com/cockroachdb/crlfmt@024b567ce87bf2b89f2cffabb7a8f4ea0cfa8b98
         go install github.com/kisielk/errcheck@latest
         go install github.com/mdempsky/unconvert@latest

testserver/binaries.go

@@ -232,9 +232,6 @@ func downloadBinary(tc *TestConfig, desiredVersion string, nonStable bool) (stri
 func GetDownloadFilename(
 	response *http.Response, nonStableDB bool, desiredVersion string,
 ) (string, error) {
-	if nonStableDB {
-		log.Printf("Obsolete usage of GetDownloadFilename() with `nonStable` set to `true`")
-	}
 	filename := fmt.Sprintf("cockroach-%s", desiredVersion)
 	if runtime.GOOS == "windows" {
 		filename += ".exe"

testserver/tenant.go

@@ -235,6 +235,8 @@ func (ts *testServerImpl) NewTenantServer(proxy bool) (TestServer, error) {
 			// TODO(asubiotto): Specify listeningURLFile once we support dynamic
 			//  ports.
 			listeningURLFile: "",
+			stdout:           filepath.Join(ts.baseDir, logsDirName, fmt.Sprintf("cockroach.tenant.%d.stdout", tenantID)),
+			stderr:           filepath.Join(ts.baseDir, logsDirName, fmt.Sprintf("cockroach.tenant.%d.stderr", tenantID)),
 		},
 	}
 
@@ -243,8 +245,6 @@ func (ts *testServerImpl) NewTenantServer(proxy bool) (TestServer, error) {
 		version:     ts.version,
 		serverState: stateNew,
 		baseDir:     ts.baseDir,
-		stdout:      filepath.Join(ts.baseDir, logsDirName, fmt.Sprintf("cockroach.tenant.%d.stdout", tenantID)),
-		stderr:      filepath.Join(ts.baseDir, logsDirName, fmt.Sprintf("cockroach.tenant.%d.stderr", tenantID)),
 		nodes:       nodes,
 	}
 

testserver/testserver.go

@@ -93,10 +93,14 @@ type TestServer interface {
 	// Stop stops the server and cleans up any associated resources.
 	Stop()
 
-	// Stdout returns the entire contents of the process' stdout.
+	// Stdout returns the entire contents of the first node's stdout.
 	Stdout() string
-	// Stdout returns the entire contents of the process' stderr.
+	// Stdout returns the entire contents of the first node's stderr.
 	Stderr() string
+	// StdoutForNode returns the entire contents of the node's stdout.
+	StdoutForNode(i int) string
+	// StderrForNode returns the entire contents of the node's stderr.
+	StderrForNode(i int) string
 	// PGURL returns the postgres connection URL to this server.
 	PGURL() *url.URL
 	// WaitForInit retries until a SQL connection is successfully established to
@@ -134,6 +138,10 @@ type nodeInfo struct {
 	startCmdArgs     []string
 	listeningURLFile string
 	state            int
+	stdout           string
+	stderr           string
+	stdoutBuf        logWriter
+	stderrBuf        logWriter
 }
 
 // testServerImpl is a TestServer implementation.
@@ -146,10 +154,6 @@ type testServerImpl struct {
 	pgURL       []pgURLChan
 	initCmd     *exec.Cmd
 	initCmdArgs []string
-	stdout      string
-	stderr      string
-	stdoutBuf   logWriter
-	stderrBuf   logWriter
 	nodes       []nodeInfo
 
 	// curTenantID is used to allocate tenant IDs. Refer to NewTenantServer for
@@ -457,13 +461,6 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
 		}
 		return path, nil
 	}
-	// TODO(janexing): Make sure the log is written to logDir instead of shown in console.
-	// Should be done once issue #109 is solved:
-	// https://github.com/cockroachdb/cockroach-go/issues/109
-	logDir, err := mkDir(logsDirName)
-	if err != nil {
-		return nil, fmt.Errorf("%s: %w", testserverMessagePrefix, err)
-	}
 	certsDir, err := mkDir(certsDirName)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", testserverMessagePrefix, err)
@@ -517,13 +514,6 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
 		startCmd = "start"
 	}
 
-	var storeArg string
-	if serverArgs.storeOnDisk {
-		storeArg = "--store=path=" + baseDir
-	} else {
-		storeArg = fmt.Sprintf("--store=type=mem,size=%.2f", serverArgs.storeMemSize)
-	}
-
 	nodes := make([]nodeInfo, serverArgs.numNodes)
 	var initArgs []string
 	joinAddrs := make([]string, 3)
@@ -541,15 +531,25 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
 	}
 
 	for i := 0; i < serverArgs.numNodes; i++ {
+		storeArg := fmt.Sprintf("--store=type=mem,size=%.2f", serverArgs.storeMemSize)
+		nodeBaseDir := filepath.Join(baseDir, strconv.Itoa(i))
+		if serverArgs.storeOnDisk {
+			storeArg = fmt.Sprintf("--store=path=%s", nodeBaseDir)
+		}
+		// TODO(janexing): Make sure the log is written to logDir instead of shown in console.
+		// Should be done once issue #109 is solved:
+		// https://github.com/cockroachdb/cockroach-go/issues/109
+		nodes[i].stdout = filepath.Join(nodeBaseDir, "cockroach.stdout")
+		nodes[i].stderr = filepath.Join(nodeBaseDir, "cockroach.stderr")
+		nodes[i].listeningURLFile = filepath.Join(nodeBaseDir, "listen-url")
 		nodes[i].state = stateNew
-		nodes[i].listeningURLFile = filepath.Join(baseDir, fmt.Sprintf("listen-url%d", i))
 		if serverArgs.numNodes > 1 {
 			joinArg := fmt.Sprintf("--join=%s", strings.Join(joinAddrs, ","))
 			nodes[i].startCmdArgs = []string{
 				serverArgs.cockroachBinary,
 				startCmd,
 				secureOpt,
-				storeArg + strconv.Itoa(i),
+				storeArg,
 				fmt.Sprintf("--listen-addr=localhost:%d", serverArgs.listenAddrPorts[i]),
 				fmt.Sprintf("--http-addr=localhost:%d", serverArgs.httpPorts[i]),
 				"--listening-url-file=" + nodes[i].listeningURLFile,
@@ -592,8 +592,6 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
 		serverState: stateNew,
 		baseDir:     baseDir,
 		initCmdArgs: initArgs,
-		stdout:      filepath.Join(logDir, "cockroach.stdout"),
-		stderr:      filepath.Join(logDir, "cockroach.stderr"),
 		curTenantID: firstTenantID,
 		nodes:       nodes,
 	}
@@ -614,14 +612,24 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
 	return ts, nil
 }
 
-// Stdout returns the entire contents of the process' stdout.
+// Stdout returns the entire contents of the first node stdout.
 func (ts *testServerImpl) Stdout() string {
-	return ts.stdoutBuf.String()
+	return ts.StdoutForNode(0)
 }
 
-// Stderr returns the entire contents of the process' stderr.
+// Stderr returns the entire contents of the first node stderr.
 func (ts *testServerImpl) Stderr() string {
-	return ts.stderrBuf.String()
+	return ts.StderrForNode(0)
+}
+
+// StdoutForNode returns the entire contents of the node's stdout.
+func (ts *testServerImpl) StdoutForNode(i int) string {
+	return ts.nodes[i].stdoutBuf.String()
+}
+
+// StderrForNode returns the entire contents of the node's stderr.
+func (ts *testServerImpl) StderrForNode(i int) string {
+	return ts.nodes[i].stderrBuf.String()
 }
 
 // BaseDir returns directory StoreOnDiskOpt writes to if used.
@@ -675,6 +683,12 @@ func (ts *testServerImpl) WaitForInitFinishForNode(nodeIdx int) error {
 		log.Printf("%s: WaitForInitFinishForNode %d: Trying again after error: %v", testserverMessagePrefix, nodeIdx, err)
 		time.Sleep(time.Millisecond * 100)
 	}
+	log.Printf(
+		"init did not finish for node %d\n\nstdout:\n%s\n\nstderr:\n:%s",
+		nodeIdx,
+		ts.StdoutForNode(nodeIdx),
+		ts.StderrForNode(nodeIdx),
+	)
 	return fmt.Errorf("init did not finish for node %d", nodeIdx)
 }
 
@@ -798,13 +812,6 @@ func (ts *testServerImpl) Stop() {
 		}
 	}
 
-	if closeErr := ts.stdoutBuf.Close(); closeErr != nil {
-		log.Printf("%s: failed to close stdout: %v", testserverMessagePrefix, closeErr)
-	}
-	if closeErr := ts.stderrBuf.Close(); closeErr != nil {
-		log.Printf("%s: failed to close stderr: %v", testserverMessagePrefix, closeErr)
-	}
-
 	ts.serverState = stateStopped
 	for i, node := range ts.nodes {
 		cmd := node.startCmd
@@ -828,11 +835,16 @@ func (ts *testServerImpl) Stop() {
 		}
 		ts.mu.RLock()
 
-		nodeDir := fmt.Sprintf("%s%d", ts.baseDir, i)
+		nodeDir := filepath.Join(ts.baseDir, strconv.Itoa(i))
 		if err := os.RemoveAll(nodeDir); err != nil {
 			log.Printf("error deleting tmp directory %s for node: %s", nodeDir, err)
 		}
-
+		if closeErr := ts.nodes[i].stdoutBuf.Close(); closeErr != nil {
+			log.Printf("%s: failed to close stdout: %v", testserverMessagePrefix, closeErr)
+		}
+		if closeErr := ts.nodes[i].stderrBuf.Close(); closeErr != nil {
+			log.Printf("%s: failed to close stderr: %v", testserverMessagePrefix, closeErr)
+		}
 	}
 
 	// Only cleanup on intentional stops.
@@ -874,7 +886,11 @@ type fileLogWriter struct {
 }
 
 func newFileLogWriter(file string) (*fileLogWriter, error) {
-	f, err := os.Create(file)
+	if err := os.MkdirAll(filepath.Dir(file), 0755); err != nil {
+		return nil, err
+	}
+	// If the file doesn't exist, create it, else append to the file.
+	f, err := os.OpenFile(file, os.O_APPEND|os.O_CREATE|os.O_RDWR, 0666)
 	if err != nil {
 		return nil, err
 	}

testserver/testservernode.go

@@ -29,7 +29,12 @@ func (ts *testServerImpl) StopNode(nodeNum int) error {
 
 	// Kill the process.
 	if cmd.Process != nil {
-		return cmd.Process.Kill()
+		if err := cmd.Process.Kill(); err != nil {
+			return err
+		}
+		if _, err := cmd.Process.Wait(); err != nil {
+			return err
+		}
 	}
 
 	return nil
@@ -55,23 +60,23 @@ func (ts *testServerImpl) StartNode(i int) error {
 	// folders.
 	currCmd.Dir = ts.baseDir
 
-	if len(ts.stdout) > 0 {
-		wr, err := newFileLogWriter(ts.stdout)
+	if len(ts.nodes[i].stdout) > 0 {
+		wr, err := newFileLogWriter(ts.nodes[i].stdout)
 		if err != nil {
-			return fmt.Errorf("unable to open file %s: %w", ts.stdout, err)
+			return fmt.Errorf("unable to open file %s: %w", ts.nodes[i].stdout, err)
 		}
-		ts.stdoutBuf = wr
+		ts.nodes[i].stdoutBuf = wr
 	}
-	currCmd.Stdout = ts.stdoutBuf
+	currCmd.Stdout = ts.nodes[i].stdoutBuf
 
-	if len(ts.stderr) > 0 {
-		wr, err := newFileLogWriter(ts.stderr)
+	if len(ts.nodes[i].stderr) > 0 {
+		wr, err := newFileLogWriter(ts.nodes[i].stderr)
 		if err != nil {
-			return fmt.Errorf("unable to open file %s: %w", ts.stderr, err)
+			return fmt.Errorf("unable to open file %s: %w", ts.nodes[1].stderr, err)
 		}
-		ts.stderrBuf = wr
+		ts.nodes[i].stderrBuf = wr
 	}
-	currCmd.Stderr = ts.stderrBuf
+	currCmd.Stderr = ts.nodes[i].stderrBuf
 
 	for k, v := range defaultEnv() {
 		currCmd.Env = append(currCmd.Env, k+"="+v)