Merge #117801 #117833

117801: kvserver: acquire `Replica.mu` when returning reproposal error r=erikgrinaker a=erikgrinaker

Discovered while working on #117612. Looks like it's been there since #42939.

This is a rare error, and these fields are unlikely to change while we're holding the raft mutex, so seems very unlikely to have caused any problems -- I could be convinced we shouldn't backport this, on the off-chance I've missed a potential deadlock.

Epic: none
Release note: None

117833: *: un-skip many tests that were skipped over the last month r=rail a=rickystewart

This partially reverts the following commits:

```
e615aec
99884f8
99f9760
75c91c2
5c64a22
c1778b7
381c96b
f9b20ae
8608499
eb71a83
337777f
```

These tests were skipped due to failures under RBE, typically under `deadlock` and `race`. With the addition of the `heavy` pool for these tests, we anticipate the decreased load will cause these tests to stop failing, therefore we un-skip the skipped tests.

Epic: CRDB-8308
Release note: None

Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

pkg/ccl/backupccl/backupdest/BUILD.bazel

@@ -60,7 +60,6 @@ go_test(
         "//pkg/server",
         "//pkg/sql",
         "//pkg/testutils/serverutils",
-        "//pkg/testutils/skip",
         "//pkg/testutils/testcluster",
         "//pkg/util/hlc",
         "//pkg/util/leaktest",

pkg/ccl/backupccl/backupdest/backup_destination_test.go

@@ -25,7 +25,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -39,8 +38,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderRace(t, "probable OOM")
-
 	tc, _, _, cleanupFn := backuptestutils.StartBackupRestoreTestCluster(t, backuptestutils.MultiNode)
 	defer cleanupFn()
 

pkg/ccl/backupccl/restore_span_covering_test.go

@@ -27,7 +27,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -723,8 +722,6 @@ func sanityCheckFileIterator(
 
 //lint:ignore U1000 unused
 func runTestRestoreEntryCover(t *testing.T, numBackups int) {
-	skip.UnderRace(t, "probable OOM")
-
 	r, _ := randutil.NewTestRand()
 	ctx := context.Background()
 	tc, _, _, cleanupFn := backupRestoreTestSetup(t, singleNode, 1, InitManualReplication)

pkg/ccl/importerccl/BUILD.bazel

@@ -36,7 +36,6 @@ go_test(
         "//pkg/testutils",
         "//pkg/testutils/datapathutils",
         "//pkg/testutils/serverutils",
-        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",

pkg/ccl/importerccl/ccl_test.go

@@ -34,7 +34,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"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"
@@ -58,8 +57,6 @@ func TestImportMultiRegion(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderRaceWithIssue(t, 116049, "probable OOM")
-
 	baseDir := sharedTestdata(t)
 	tc, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(
 		t, 3 /* numServers */, base.TestingKnobs{}, multiregionccltestutils.WithBaseDirectory(baseDir),
@@ -306,8 +303,6 @@ func TestMultiRegionExportImportRoundTrip(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderRace(t, "probable OOM")
-
 	validateNumRows := func(sqlDB *gosql.DB, tableName string, expected int) {
 		res := sqlDB.QueryRow(fmt.Sprintf(`SELECT count(*) FROM %s`, tableName))
 		require.NoError(t, res.Err())

pkg/ccl/kvccl/kvfollowerreadsccl/boundedstaleness_test.go

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

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go

@@ -941,7 +941,7 @@ func TestSecondaryTenantFollowerReadsRouting(t *testing.T) {
 	defer log.Scope(t).Close(t)
 	defer utilccl.TestingEnableEnterprise()()
 
-	skip.UnderRace(t, "times out")
+	skip.UnderStressRace(t, "times out")
 
 	for _, testCase := range []struct {
 		name             string

pkg/ccl/serverccl/statusccl/tenant_grpc_test.go

@@ -19,7 +19,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -34,8 +33,6 @@ func TestTenantGRPCServices(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderRace(t, "probable OOM")
-
 	ctx := context.Background()
 
 	testCluster := serverutils.StartCluster(t, 3, base.TestClusterArgs{

pkg/ccl/serverccl/statusccl/tenant_status_test.go

@@ -390,7 +390,6 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 	skip.UnderStressWithIssue(t, 113984)
-	skip.UnderRace(t, "may OOM")
 
 	ctx := context.Background()
 	testCluster := serverutils.StartCluster(t, 3 /* numNodes */, base.TestClusterArgs{

pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/BUILD.bazel

@@ -34,7 +34,6 @@ 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,7 +33,6 @@ 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"
@@ -81,9 +80,6 @@ import (
 //     Releases the protected timestamp record with id.
 func TestDataDriven(t *testing.T) {
 	t.Cleanup(leaktest.AfterTest(t))
-
-	skip.UnderRace(t, "likely to time out")
-
 	scope := log.Scope(t)
 	t.Cleanup(func() {
 		scope.Close(t)

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.UnderRace(t, "multi cluster/node config exhausts hardware")
+	skip.UnderStressRace(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.UnderRace(t, "takes too long under stress race")
+	skip.UnderStressRace(t, "takes too long under stress race")
 
 	ctx := context.Background()
 	regions := []string{"mars", "venus", "mercury"}

pkg/cmd/generate-logictest/main.go

@@ -32,8 +32,6 @@ type testFileTemplateConfig struct {
 	CockroachGoTestserverTest     bool
 	Ccl                           bool
 	ForceProductionValues         bool
-	IsMultiRegion                 bool
-	Is3NodeTenant                 bool
 	Package, TestRuleName, RelDir string
 	ConfigIdx                     int
 	TestCount                     int
@@ -176,8 +174,6 @@ func (t *testdir) dump() error {
 		tplCfg.RelDir = t.relPathToParent
 		tplCfg.TestCount = testCount
 		tplCfg.CockroachGoTestserverTest = cfg.UseCockroachGoTestserver
-		tplCfg.IsMultiRegion = strings.Contains(cfg.Name, "multiregion")
-		tplCfg.Is3NodeTenant = strings.HasSuffix(cfg.Name, "3node-tenant")
 		// The NumCPU calculation is a guess pulled out of thin air to
 		// allocate the tests which use 3-node clusters 2 vCPUs, and
 		// the ones which use more a bit more.

pkg/cmd/generate-logictest/templates.go

@@ -51,11 +51,7 @@ const templateText = `
 {{- define "runLogicTest" }}
 {{- if .LogicTest -}}
 func runLogicTest(t *testing.T, file string) {
-	{{ if .Is3NodeTenant }}skip.UnderRace(t, "large engflow executor is overloaded by 3node-tenant config")
-	{{ else if eq .TestRuleName "local"}}if file == "lookup_join_local" {
-		skip.UnderRace(t, "this file is too slow under race")
-	}
-	{{ end }}skip.UnderDeadlock(t, "times out and/or hangs")
+	skip.UnderDeadlock(t, "times out and/or hangs")
 	logictest.RunLogicTest(t, logictest.TestServerArgs{}, configIdx, filepath.Join(logicTestDir, file))
 }
 {{ end }}
@@ -64,8 +60,7 @@ func runLogicTest(t *testing.T, file string) {
 {{- define "runCCLLogicTest" }}
 {{- if .CclLogicTest -}}
 func runCCLLogicTest(t *testing.T, file string) {
-	{{ if or .IsMultiRegion .Is3NodeTenant }}skip.UnderRace(t, "large engflow executor is overloaded by this config")
-	{{ end }}skip.UnderDeadlock(t, "times out and/or hangs")
+	skip.UnderDeadlock(t, "times out and/or hangs")
 	logictest.RunLogicTest(t, logictest.TestServerArgs{}, configIdx, filepath.Join(cclLogicTestDir, file))
 }
 {{ end }}

pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go

@@ -32,7 +32,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"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/storageutils"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -49,8 +48,6 @@ func TestEvalAddSSTable(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderDeadlock(t, "probable OOM")
-
 	storage.DisableMetamorphicSimpleValueEncoding(t)
 
 	const intentTS = 100 // values with this timestamp are written as intents

pkg/kv/kvserver/loqrecovery/server_integration_test.go

@@ -478,8 +478,6 @@ func TestRetrieveRangeStatus(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderRace(t, "probable OOM")
-
 	ctx := context.Background()
 
 	tc, _, _ := prepTestCluster(t, 5)
@@ -535,8 +533,6 @@ func TestRetrieveApplyStatus(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderRace(t, "probable OOM")
-
 	ctx := context.Background()
 
 	tc, _, _ := prepTestCluster(t, 5)

pkg/kv/kvserver/replica_application_result.go

@@ -407,13 +407,14 @@ func (r *Replica) tryReproposeWithNewLeaseIndex(ctx context.Context, origCmd *re
 		// The tracker wants us to forward the request timestamp, but we can't
 		// do that without re-evaluating, so give up. The error returned here
 		// will go to back to DistSender, so send something it can digest.
-		err := kvpb.NewNotLeaseHolderError(
+		r.mu.RLock()
+		defer r.mu.RUnlock()
+		return kvpb.NewNotLeaseHolderError(
 			*r.mu.state.Lease,
 			r.store.StoreID(),
 			r.mu.state.Desc,
 			"reproposal failed due to closed timestamp",
 		)
-		return err
 	}
 	// Some tests check for this log message in the trace.
 	log.VEventf(ctx, 2, "retry: proposalIllegalLeaseIndex")

pkg/kv/kvserver/replica_learner_test.go

@@ -1367,9 +1367,6 @@ func TestReplicaGCQueueSeesLearnerOrJointConfig(t *testing.T) {
 func TestRaftSnapshotQueueSeesLearner(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-
-	skip.UnderRace(t, "probable OOM")
-
 	ctx := context.Background()
 	blockSnapshotsCh := make(chan struct{})
 	knobs, ltk := makeReplicationTestKnobs()

pkg/kv/kvserver/reports/reporter_test.go

@@ -125,9 +125,6 @@ func TestConstraintConformanceReportIntegration(t *testing.T) {
 func TestCriticalLocalitiesReportIntegration(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-
-	skip.UnderRace(t, "likely to timeout")
-
 	ctx := context.Background()
 	// 2 regions, 3 dcs per region.
 	tc := serverutils.StartCluster(t, 6, base.TestClusterArgs{

pkg/server/application_api/schema_inspection_test.go

@@ -480,7 +480,6 @@ func TestAdminAPITableStats(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderDeadlock(t, "low ScanMaxIdleTime and deadlock overloads the EngFlow executor")
 	skip.UnderStress(t, "flaky under stress #107156")
 	skip.UnderRace(t, "flaky under race #107156")
 

pkg/server/application_api/stats_test.go

@@ -30,7 +30,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/diagutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -42,8 +41,6 @@ func TestTelemetrySQLStatsIndependence(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderDeadlockWithIssue(t, 115914, "likely to time out")
-
 	ctx := context.Background()
 	var params base.TestServerArgs
 	params.Knobs.SQLStatsKnobs = sqlstats.CreateTestingKnobs()

pkg/server/application_api/stmtdiag_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"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/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -232,8 +231,6 @@ func TestStatementDiagnosticsDoesNotReturnExpiredRequests(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderDeadlockWithIssue(t, 115914, "likely to time out")
-
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 	db := sqlutils.MakeSQLRunner(sqlDB)