Merge #148244

148244: roachtest: jepsen returns with exit code 255 when encountering unhandled exceptions r=darrylwong a=srosenberg

Jepsen exits with `255` if an uhandled exception is raised. The error code is typically associated with SSH errors. Thus, the framework ends up misclassifying it as an SSH flake.

This change wraps the jepsen runner script with a remapping of the exit code so that `255` is now mapped into `254`.

In the process of testing the change, a couple of other issues were uncovered. `FetchLogs` now correctly skips downloading non-existent logs. Also, as of [1], Jepsen should be compiled and executed with JDK 1.17+, so we updated `tc-nightly-main` [2].

[1] cockroachdb/jepsen@d3e8f55
[2] cockroachdb/jepsen@50a97a1

Epic: none
Fixes: #99681
Release note: None

Co-authored-by: Stan Rosenberg <[email protected]>

Author: craig[bot]

pkg/BUILD.bazel

@@ -328,6 +328,7 @@ ALL_TESTS = [
     "//pkg/roachprod/vm/ibm:ibm_test",
     "//pkg/roachprod/vm/local:local_test",
     "//pkg/roachprod/vm:vm_test",
+    "//pkg/roachprod:roachprod_disallowed_imports_test",
     "//pkg/roachprod:roachprod_test",
     "//pkg/rpc/nodedialer:nodedialer_test",
     "//pkg/rpc:rpc_test",

pkg/cmd/roachtest/test/test_interface.go

@@ -16,6 +16,8 @@ import (
 // DefaultCockroachPath is the path where the binary passed to the
 // `--cockroach` flag will be made available in every node in the
 // cluster.
+// N.B. This constant is duplicated from roachtest to break build dependency.
+// Thus, any update should be reflected in both places.
 const DefaultCockroachPath = "./cockroach"
 
 // DefaultDeprecatedWorkloadPath is the path where the binary passed

pkg/cmd/roachtest/tests/jepsen.go

@@ -48,7 +48,7 @@ a pull request for tc-nightly-main branch and after merging build a new
 artifact using:
 
 # install build dependencies and build tools
-sudo apt-get -qqy install openjdk-8-jre openjdk-8-jre-headless libjna-java gnuplot
+sudo apt-get -qqy install openjdk-17-jre openjdk-17-jre-headless libjna-java gnuplot
 curl -o lein https://raw.githubusercontent.com/technomancy/leiningen/stable/bin/lein
 chmod +x lein
 
@@ -74,7 +74,7 @@ to running roachtest and update repository URLs in the file to your liking.
 const envBuildJepsen = "ROACHTEST_BUILD_JEPSEN"
 
 const jepsenRepo = "https://github.com/cockroachdb/jepsen"
-const repoBranch = "tc-nightly"
+const repoBranch = "tc-nightly-main"
 
 const gcpPath = "https://storage.googleapis.com/cockroach-jepsen"
 const binaryVersion = "0.1.0-6699eb4-standalone"
@@ -119,7 +119,7 @@ func initJepsen(ctx context.Context, t test.Test, c cluster.Cluster, j jepsenCon
 	// so do it before the initialization check for ease of iteration.
 	if err := c.GitClone(
 		ctx, t.L(),
-		"https://github.com/cockroachdb/jepsen", "/mnt/data1/jepsen", "tc-nightly", controller,
+		jepsenRepo, "/mnt/data1/jepsen", repoBranch, controller,
 	); err != nil {
 		t.Fatal(err)
 	}
@@ -163,10 +163,11 @@ func initJepsen(ctx context.Context, t test.Test, c cluster.Cluster, j jepsenCon
 	// (which is not how our official builds are laid out).
 	c.Run(ctx, option.WithNodes(c.All()), "tar --transform s,^,cockroach/, -c -z -f cockroach.tgz cockroach")
 
+	deps := `"sudo DEBIAN_FRONTEND=noninteractive apt-get -qqy install openjdk-17-jre openjdk-17-jre-headless libjna-java gnuplot > /dev/null 2>&1"`
+
 	// Install Jepsen's prereqs on the controller.
 	if result, err := c.RunWithDetailsSingleNode(
-		ctx, t.L(), option.WithNodes(controller), "sh", "-c",
-		`"sudo DEBIAN_FRONTEND=noninteractive apt-get -qqy install openjdk-17-jre openjdk-17-jre-headless libjna-java gnuplot > /dev/null 2>&1"`,
+		ctx, t.L(), option.WithNodes(controller), "sh", "-c", deps,
 	); err != nil {
 		if result.RemoteExitStatus == 100 {
 			t.Skip("apt-get failure (#31944)", result.Stdout+result.Stderr)
@@ -211,7 +212,7 @@ type jepsenConfig struct {
 }
 
 func makeJepsenConfig() jepsenConfig {
-	if e := os.Getenv(envBuildJepsen); e != "" {
+	if os.Getenv(envBuildJepsen) != "" {
 		return jepsenConfig{
 			buildFromSource: true,
 			repoURL:         jepsenRepo,
@@ -267,6 +268,8 @@ func (j jepsenConfig) startTest(
 	ctx context.Context, t test.Test, run func(args ...string) error, testArgs string,
 ) <-chan error {
 	errCh := make(chan error, 1)
+	var script string
+
 	if j.buildFromSource {
 		// Install the jepsen package (into ~/.m2) before running tests in
 		// the cockroach package. Clojure doesn't really understand
@@ -288,20 +291,28 @@ func (j jepsenConfig) startTest(
 			}
 			t.Fatalf("error installing Jepsen deps: %+v", err)
 		}
-		t.Go(func(context.Context, *logger.Logger) error {
-			errCh <- run("bash", "-e", "-c", fmt.Sprintf(
-				`"cd /mnt/data1/jepsen/cockroachdb && set -eo pipefail && ~/lein run %s > invoke.log 2>&1"`,
-				testArgs))
-			return nil
-		})
+		// N.B. jepsen exits with `255` if it encounters an unhandled exception.
+		// (See https://github.com/cockroachdb/cockroach/issues/99681)
+		// 255 is designated as SSH, hence we remap it to 254 below. (See errors.ClassifyCmdError)
+		script = fmt.Sprintf(`"
+cd /mnt/data1/jepsen/cockroachdb &&
+set -eo pipefail &&
+rc=0; ~/lein run %s > invoke.log 2>&1 || rc=\$?;
+exit \$(( rc == 255 ? 254 : rc ))"`, testArgs)
 	} else {
-		t.Go(func(context.Context, *logger.Logger) error {
-			errCh <- run("bash", "-e", "-c", fmt.Sprintf(
-				`"cd /mnt/data1/jepsen/cockroachdb && set -eo pipefail && java -jar %s %s > invoke.log 2>&1"`,
-				j.binaryName(), testArgs))
-			return nil
-		})
+		// N.B. jepsen exits with `255` if it encounters an unhandled exception.
+		// (See https://github.com/cockroachdb/cockroach/issues/99681)
+		// 255 is designated as SSH, hence we remap it to 254 below. (See errors.ClassifyCmdError)
+		script = fmt.Sprintf(`"
+cd /mnt/data1/jepsen/cockroachdb &&
+set -eo pipefail &&
+rc=0; java -jar %s %s > invoke.log 2>&1 || rc=\$?;
+exit \$(( rc == 255 ? 254 : rc ))"`, j.binaryName(), testArgs)
 	}
+	t.Go(func(context.Context, *logger.Logger) error {
+		errCh <- run("bash", "-e", "-c", script)
+		return nil
+	})
 	return errCh
 }
 

pkg/roachprod/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg/testutils:buildutil/buildutil.bzl", "disallowed_imports_test")
 
 go_library(
     name = "roachprod",
@@ -14,7 +15,6 @@ go_library(
         "//pkg/build",
         "//pkg/cli/exit",
         "//pkg/cmd/roachprod/grafana",
-        "//pkg/cmd/roachtest/test",
         "//pkg/roachprod/cloud",
         "//pkg/roachprod/config",
         "//pkg/roachprod/fluentbit",
@@ -58,3 +58,10 @@ go_test(
         "@com_github_stretchr_testify//require",
     ],
 )
+
+disallowed_imports_test(
+    src = "roachprod",
+    disallowed_prefixes = [
+        "pkg/cmd/roachtest",
+    ],
+)

pkg/roachprod/roachprod.go

@@ -31,7 +31,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachprod/grafana"
-	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/cloud"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/config"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/fluentbit"
@@ -70,6 +69,13 @@ func (e *MalformedClusterNameError) Error() string {
 	return fmt.Sprintf("Malformed cluster name %s, %s. Did you mean one of %s", e.name, e.reason, e.suggestions)
 }
 
+// DefaultCockroachPath is the path where the binary passed to the
+// `--cockroach` flag will be made available in every node in the
+// cluster.
+// N.B. This constant is duplicated from roachtest to break build dependency.
+// Thus, any update should be reflected in both places.
+const DefaultCockroachPath = "./cockroach"
+
 // findActiveAccounts is a hook for tests to inject their own FindActiveAccounts
 // implementation. i.e. unit tests that don't want to actually access a provider.
 var findActiveAccounts = vm.FindActiveAccounts
@@ -3026,40 +3032,48 @@ func FetchLogs(
 			if err != nil {
 				return err
 			}
-
+			// Found logs and corresponding nodes, which contain at least one log directory.
 			logDirs := make(map[string]struct{})
+			nodes := install.Nodes{}
+
 			for _, r := range results {
 				if r.Err != nil {
 					l.Printf("will not fetch logs for n%d due to error: %v", r.Node, r.Err)
+					continue
 				}
-
+				nodes = append(nodes, r.Node)
 				for _, logDir := range strings.Fields(r.Stdout) {
 					logDirs[logDir] = struct{}{}
 				}
 			}
-
+			if len(logDirs) == 0 {
+				l.Printf("no log directories found")
+				return nil
+			}
+			// For each found log directory, recursively scp its contents under the destination, prefixed by node id.
+			// E.g., n1's contents will show up under 1.unredacted, etc.
 			for logDir := range logDirs {
 				dirPath := filepath.Join(destination, logDir, "unredacted")
 				if err := os.MkdirAll(filepath.Dir(dirPath), 0755); err != nil {
 					return err
 				}
-
-				if err := c.Get(ctx, l, c.Nodes, logDir /* src */, dirPath /* dest */); err != nil {
+				// Runs a recursive scp in parallel.
+				if err := c.Get(ctx, l, nodes, logDir /* src */, dirPath /* dest */); err != nil {
 					l.Printf("failed to fetch log directory %s: %v", logDir, err)
 					if ctx.Err() != nil {
 						return errors.Wrap(err, "cluster.FetchLogs")
 					}
 				}
 			}
-
-			if err := c.Run(ctx, l, l.Stdout, l.Stderr, install.WithNodes(c.Nodes), "", fmt.Sprintf("mkdir -p logs/redacted && %s debug merge-logs --redact logs/*.log > logs/redacted/combined.log", test.DefaultCockroachPath)); err != nil {
+			// Create a single redacted log.
+			if err := c.Run(ctx, l, l.Stdout, l.Stderr, install.WithNodes(nodes), "", fmt.Sprintf("mkdir -p logs/redacted && %s debug merge-logs --redact logs/*.log > logs/redacted/combined.log", DefaultCockroachPath)); err != nil {
 				l.Printf("failed to redact logs: %v", err)
 				if ctx.Err() != nil {
 					return err
 				}
 			}
 			dest := filepath.Join(destination, "logs/cockroach.log")
-			return errors.Wrap(c.Get(ctx, l, c.Nodes, "logs/redacted/combined.log" /* src */, dest), "cluster.FetchLogs")
+			return errors.Wrap(c.Get(ctx, l, nodes, "logs/redacted/combined.log" /* src */, dest), "cluster.FetchLogs")
 		})
 }