Merge #148861 #148892

148861: roachtest: add `s390x-test-failure` GH label when arch == "s390x" r=darrylwong,rickystewart a=srosenberg

To simplify tracking roachtest failures on s390x, we add `s390x-test-failure` Github label.

Epic: none
Release note: None

148892: roachtest: fix test monitor error r=srosenberg a=herkolategan

Previously, the global test monitor would fatal on an error. This is similar to
how the original monitor worked. But the original monitor would fatal where a
panic could be recovered by the goroutine running the test. This would
immediately short circuit the test. However, the global monitor can not use
fatal since it's running in a separate goroutine which causes the panic to be
unrecoverable. This change makes the global monitor call `Errorf` instead of
`Fatalf`. The test context will now instead be cancelled.

Epic: None
Release note: None

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

Author: 3 people

pkg/cmd/roachtest/BUILD.bazel

@@ -98,6 +98,7 @@ go_test(
         "main_test.go",
         "test_filter_test.go",
         "test_impl_test.go",
+        "test_monitor_test.go",
         "test_registry_test.go",
         "test_test.go",
         "zip_util_test.go",

pkg/cmd/roachtest/github.go

@@ -217,6 +217,7 @@ func (g *githubIssues) createPostRequest(
 	const infraFlakeLabel = "X-infra-flake"
 	const runtimeAssertionsLabel = "B-runtime-assertions-enabled"
 	const coverageLabel = "B-coverage-enabled"
+	const s390xTestFailureLabel = "s390x-test-failure"
 	labels := []string{"O-roachtest"}
 	if infraFlake {
 		labels = append(labels, infraFlakeLabel)
@@ -235,6 +236,11 @@ func (g *githubIssues) createPostRequest(
 			labels = append(labels, coverageLabel)
 		}
 	}
+	// N.B. To simplify tracking failures on s390x, we add the designated s390x-test-failure label. This could be removed
+	// in the future, i.e., after several major releases, when we expect s390x to be sufficiently stable.
+	if arch := params["arch"]; vm.CPUArch(arch) == vm.ArchS390x {
+		labels = append(labels, s390xTestFailureLabel)
+	}
 	labels = append(labels, spec.ExtraLabels...)
 
 	teams, err := g.teamLoader()

pkg/cmd/roachtest/test_monitor.go

@@ -11,23 +11,28 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 )
 
-var _ test.Monitor = &testMonitorImpl{}
-
 type testMonitorImpl struct {
-	*monitorImpl
+	monitor interface {
+		test.Monitor
+		WaitForNodeDeath() error
+	}
+	t test.Test
 }
 
-func newTestMonitor(ctx context.Context, t test.Test, c *clusterImpl) *testMonitorImpl {
+// newTestMonitor is a function that creates a new test monitor. It can be used for testing
+// purposes to replace the default monitor with a custom implementation.
+var newTestMonitor = func(ctx context.Context, t test.Test, c *clusterImpl) *testMonitorImpl {
 	return &testMonitorImpl{
-		monitorImpl: newMonitor(ctx, t, c, true /* expectExactProcessDeath */),
+		monitor: newMonitor(ctx, t, c, true /* expectExactProcessDeath */),
+		t:       t,
 	}
 }
 
 func (m *testMonitorImpl) start() {
 	go func() {
-		err := m.WaitForNodeDeath()
+		err := m.monitor.WaitForNodeDeath()
 		if err != nil {
-			m.t.Fatal(err)
+			m.t.Errorf("test monitor: %v", err)
 		}
 	}()
 }

pkg/cmd/roachtest/test_monitor_test.go

@@ -0,0 +1,77 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package main
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/require"
+)
+
+type stubTestMonitorError struct{}
+
+func (s *stubTestMonitorError) ExpectProcessDead(
+	nodes option.NodeListOption, opts ...option.OptionFunc,
+) {
+}
+
+func (s *stubTestMonitorError) ExpectProcessAlive(
+	nodes option.NodeListOption, opts ...option.OptionFunc,
+) {
+}
+
+func (s *stubTestMonitorError) WaitForNodeDeath() error {
+	return errors.New("test error")
+}
+
+func TestGlobalMonitorError(t *testing.T) {
+	newTestMonitor = func(_ context.Context, t test.Test, c *clusterImpl) *testMonitorImpl {
+		return &testMonitorImpl{
+			monitor: &stubTestMonitorError{},
+			t:       t,
+		}
+	}
+
+	ctx := context.Background()
+	stopper := stop.NewStopper()
+	defer stopper.Stop(ctx)
+	cr := newClusterRegistry()
+	runner := newUnitTestRunner(cr, stopper)
+
+	var buf syncedBuffer
+	copt := defaultClusterOpt()
+	lopt := defaultLoggingOpt(&buf)
+
+	mockTest := registry.TestSpec{
+		Name:             `mock test`,
+		Owner:            OwnerUnitTest,
+		Cluster:          spec.MakeClusterSpec(0),
+		CompatibleClouds: registry.AllExceptAWS,
+		Suites:           registry.Suites(registry.Nightly),
+		CockroachBinary:  registry.StandardCockroach,
+		Monitor:          true,
+		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
+			select {
+			case <-ctx.Done():
+				return
+			case <-time.After(5 * time.Minute):
+				return
+			}
+		},
+	}
+	err := runner.Run(ctx, []registry.TestSpec{mockTest}, 1, /* count */
+		defaultParallelism, copt, testOpts{}, lopt)
+	require.Error(t, err)
+}

pkg/cmd/roachtest/test_runner.go

@@ -1351,7 +1351,7 @@ func (r *testRunner) runTest(
 
 	t.taskManager = task.NewManager(runCtx, t.L())
 	testMonitor := newTestMonitor(runCtx, t, c)
-	t.monitor = testMonitor
+	t.monitor = testMonitor.monitor
 
 	t.mu.Lock()
 	// t.Fatal() will cancel this context.