Merge #117872 #117894 #117898

117872: bench: ignore cluster startup and shutdown r=yuzefovich a=yuzefovich

This commit adjusts our benchmark harness to ignore the overhead of starting up and shutting down the test cluster. This allows us to avoid spurious "regressions" like that we've seen due to #117116.

Epic: None

Release note: None

117894: tests: skip some tests under `race` r=rail a=rickystewart

Some of these are skipped only under `StressRace` but not simply under `race` as they should have been.

Additionally, two tests: `spanconfigsqltranslatorccl.TestDataDriven` and `kvserver.TestStoreMetrics` are so long running that they stall or fail to complete under `race`, which has gone unnoticed before.

Epic: CRDB-8308
Release note: None

117898: testcluster: re-enable tenant in multi-node clusters under race r=yuzefovich a=yuzefovich

This commit is a partial revert of adfc98a. In that commit we disabled tenant randomization in multi-node-clusters under race because EngFlow executors were too overloaded in such a setup. We now use beefier executors, so let's see how they fare.

Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

pkg/bench/foreachdb.go

@@ -42,6 +42,14 @@ var runSepProcessTenant = flag.Bool("run-sep-process-tenant", false, "run separa
 // BenchmarkFn is a function that runs a benchmark using the given SQLRunner.
 type BenchmarkFn func(b *testing.B, db *sqlutils.SQLRunner)
 
+// timerUtil is a helper method that should be called right before the
+// invocation of BenchmarkFn and the returned function should be deferred.
+func timerUtil(b *testing.B) func() {
+	b.ResetTimer()
+	b.StartTimer()
+	return b.StopTimer
+}
+
 func benchmarkCockroach(b *testing.B, f BenchmarkFn) {
 	s, db, _ := serverutils.StartServer(
 		b, base.TestServerArgs{
@@ -54,6 +62,7 @@ func benchmarkCockroach(b *testing.B, f BenchmarkFn) {
 		b.Fatal(err)
 	}
 
+	defer timerUtil(b)()
 	f(b, sqlutils.MakeSQLRunner(db))
 }
 
@@ -103,6 +112,7 @@ func benchmarkSharedProcessTenantCockroach(b *testing.B, f BenchmarkFn) {
 	_, err = tenantDB.Exec(`CREATE DATABASE bench`)
 	require.NoError(b, err)
 
+	defer timerUtil(b)()
 	f(b, sqlutils.MakeSQLRunner(tenantDB))
 }
 
@@ -133,6 +143,7 @@ func benchmarkSepProcessTenantCockroach(b *testing.B, f BenchmarkFn) {
 	_, err = tenantDB.Exec(`CREATE DATABASE bench`)
 	require.NoError(b, err)
 
+	defer timerUtil(b)()
 	f(b, sqlutils.MakeSQLRunner(tenantDB))
 }
 
@@ -150,6 +161,7 @@ func benchmarkMultinodeCockroach(b *testing.B, f BenchmarkFn) {
 	}
 	defer tc.Stopper().Stop(context.TODO())
 
+	defer timerUtil(b)()
 	f(b, sqlutils.MakeRoundRobinSQLRunner(tc.Conns[0], tc.Conns[1], tc.Conns[2]))
 }
 
@@ -198,6 +210,7 @@ func benchmarkPostgres(b *testing.B, f BenchmarkFn) {
 	r := sqlutils.MakeSQLRunner(db)
 	r.Exec(b, `CREATE SCHEMA IF NOT EXISTS bench`)
 
+	defer timerUtil(b)()
 	f(b, r)
 }
 
@@ -218,6 +231,7 @@ func benchmarkMySQL(b *testing.B, f BenchmarkFn) {
 	r := sqlutils.MakeSQLRunner(db)
 	r.Exec(b, `CREATE DATABASE IF NOT EXISTS bench`)
 
+	defer timerUtil(b)()
 	f(b, r)
 }
 

pkg/ccl/kvccl/kvfollowerreadsccl/boundedstaleness_test.go

@@ -262,7 +262,9 @@ func TestBoundedStalenessDataDriven(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderStress(t, "1μs staleness reads may actually succeed due to the slow environment")
+	const msg = "1μs staleness reads may actually succeed due to the slow environment"
+	skip.UnderStress(t, msg)
+	skip.UnderRace(t, msg)
 	defer ccl.TestingEnableEnterprise()()
 
 	ctx := context.Background()

pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/BUILD.bazel

@@ -34,6 +34,7 @@ go_test(
         "//pkg/sql/catalog/tabledesc",
         "//pkg/testutils/datapathutils",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",

pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -85,6 +86,8 @@ func TestDataDriven(t *testing.T) {
 		scope.Close(t)
 	})
 
+	skip.UnderRace(t, "very long-running test under race")
+
 	ctx := context.Background()
 	datadriven.Walk(t, datapathutils.TestDataPath(t), func(t *testing.T, path string) {
 		t.Parallel() // SAFE FOR TESTING

pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go

@@ -681,7 +681,7 @@ func TestStreamingAutoReplan(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderStressRace(t, "multi cluster/node config exhausts hardware")
+	skip.UnderRace(t, "multi cluster/node config exhausts hardware")
 
 	ctx := context.Background()
 	args := replicationtestutils.DefaultTenantStreamingClustersArgs
@@ -1209,7 +1209,7 @@ func TestLoadProducerAndIngestionProgress(t *testing.T) {
 func TestStreamingRegionalConstraint(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	skip.UnderStressRace(t, "takes too long under stress race")
+	skip.UnderRace(t, "takes too long under race")
 
 	ctx := context.Background()
 	regions := []string{"mars", "venus", "mercury"}

pkg/kv/kvserver/client_metrics_test.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -236,6 +237,8 @@ func TestStoreMetrics(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
+	skip.UnderRace(t, "does not complete under race")
+
 	ctx := context.Background()
 	const numServers int = 3
 	stickyVFSRegistry := server.NewStickyVFSRegistry()

pkg/kv/kvserver/client_raft_test.go

@@ -6498,9 +6498,9 @@ func TestRaftLeaderRemovesItself(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	// Timing-sensitive, so skip under deadlock detector and stressrace.
+	// Timing-sensitive, so skip under deadlock detector and race.
 	skip.UnderDeadlock(t)
-	skip.UnderStressRace(t)
+	skip.UnderRace(t)
 
 	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
 	defer cancel()

pkg/kv/kvserver/client_replica_circuit_breaker_test.go

@@ -750,7 +750,7 @@ type circuitBreakerTest struct {
 }
 
 func setupCircuitBreakerTest(t *testing.T) *circuitBreakerTest {
-	skip.UnderStressRace(t)
+	skip.UnderRace(t)
 	manualClock := hlc.NewHybridManualClock()
 	var rangeID int64             // atomic
 	slowThresh := &atomic.Value{} // supports .SetSlowThreshold(x)

pkg/sql/logictest/logic.go

@@ -4061,7 +4061,7 @@ func RunLogicTest(
 	// with less concurrency in the nightly stress runs. If you see problems
 	// please make adjustments there.
 	// As of 6/4/2019, the logic tests never complete under race.
-	skip.UnderStressRace(t, "logic tests and race detector don't mix: #37993")
+	skip.UnderRace(t, "logic tests and race detector don't mix: #37993")
 
 	// This test relies on repeated sequential storage.EventuallyFileOnlySnapshot
 	// acquisitions. Reduce the max wait time for each acquisition to speed up

pkg/sql/logictest/parallel_test.go

@@ -230,11 +230,6 @@ func TestParallel(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	skip.UnderRace(t, "takes >1 min under race")
-	// Note: there is special code in teamcity-trigger/main.go to run this package
-	// with less concurrency in the nightly stress runs. If you see problems
-	// please make adjustments there.
-	// As of 6/4/2019, the logic tests never complete under race.
-	skip.UnderStressRace(t, "logic tests and race detector don't mix: #37993")
 
 	glob := *paralleltestdata
 	paths, err := filepath.Glob(glob)

pkg/testutils/serverutils/test_server_shim.go

@@ -85,7 +85,7 @@ var PreventStartTenantError = errors.New("attempting to manually start a virtual
 // directly so that it only gets linked into test code (and to avoid a linter
 // error that 'skip' must only be used in test code).
 func ShouldStartDefaultTestTenant(
-	t TestLogger, baseArg base.DefaultTestTenantOptions, multiNodeCluster bool,
+	t TestLogger, baseArg base.DefaultTestTenantOptions,
 ) (retval base.DefaultTestTenantOptions) {
 	// Explicit case for disabling the default test tenant.
 	if baseArg.TestTenantAlwaysDisabled() {
@@ -149,17 +149,6 @@ func ShouldStartDefaultTestTenant(
 		return override
 	}
 
-	if multiNodeCluster && util.RaceEnabled {
-		// Race builds now run in the EngFlow environment which seems to be
-		// often overloaded if we have multi-node clusters and start a default
-		// test tenant, so we disable the tenant randomization in such a
-		// scenario.
-		if t != nil {
-			t.Log("cluster virtualization disabled under race")
-		}
-		return base.InternalNonDefaultDecision(baseArg, false /* enable */, false /* shared */)
-	}
-
 	// Note: we ask the metamorphic framework for a "disable" value, instead
 	// of an "enable" value, because it probabilistically returns its default value
 	// more often than not and that is what we want.
@@ -269,9 +258,7 @@ func StartServerOnlyE(t TestLogger, params base.TestServerArgs) (TestServerInter
 	allowAdditionalTenants := params.DefaultTestTenant.AllowAdditionalTenants()
 	// Update the flags with the actual decision as to whether we should
 	// start the service for a default test tenant.
-	params.DefaultTestTenant = ShouldStartDefaultTestTenant(
-		t, params.DefaultTestTenant, false, /* multiNodeCluster */
-	)
+	params.DefaultTestTenant = ShouldStartDefaultTestTenant(t, params.DefaultTestTenant)
 
 	s, err := NewServer(params)
 	if err != nil {

pkg/testutils/testcluster/testcluster.go

@@ -294,9 +294,7 @@ func NewTestCluster(
 				args.DefaultTestTenant, defaultTestTenantOptions)
 		}
 	}
-	tc.defaultTestTenantOptions = serverutils.ShouldStartDefaultTestTenant(
-		t, defaultTestTenantOptions, nodes > 1, /* multiNodeCluster */
-	)
+	tc.defaultTestTenantOptions = serverutils.ShouldStartDefaultTestTenant(t, defaultTestTenantOptions)
 
 	var firstListener net.Listener
 	for i := 0; i < nodes; i++ {