Merge #156306 #156317 #156387 #156388

156306: changefeedccl: enable a couple of tests with test tenant r=yuzefovich a=yuzefovich

Fixes: #98816
Informs: #142799
Release note: None

156317: insights: skip contention test under duress r=alyshanjahani-crl a=dhartunian

Resolves: #155838
Resolves: #155962

Epic: None

Release note: None

156387: catalog/lease: address deadlock with locked lease timestamps r=fqazi a=fqazi

Previously, when locked lease timestamps were used, we held the descriptor state while acquiring close timestamp handles. This could lead to a deadlock, since in other code paths we acquire the close timestamp handle first. To address this, this patch avoids holding the descriptor state lock longer than necessary.

Fixes: #156177

Release note: None

156388: cliccl: fix FIPS-readiness check r=rail a=rickystewart

The logic here is flipped.

Release note: none
Epic: DEVINF-1477

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

Author: 5 people

pkg/ccl/changefeedccl/alter_changefeed_test.go

@@ -57,7 +57,7 @@ func TestAlterChangefeedAddTargetPrivileges(t *testing.T) {
 	ctx := context.Background()
 
 	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		DefaultTestTenant: base.TODOTestTenantDisabled,
+		DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 		Knobs: base.TestingKnobs{
 			JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
 			DistSQL: &execinfra.TestingKnobs{

pkg/ccl/changefeedccl/changefeed_test.go

@@ -2621,7 +2621,7 @@ func TestChangefeedExternalIODisabled(t *testing.T) {
 		}
 		ctx := context.Background()
 		s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-			DefaultTestTenant: base.TODOTestTenantDisabled,
+			DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 			ExternalIODirConfig: base.ExternalIODirConfig{
 				DisableOutbound: true,
 			},
@@ -4067,7 +4067,7 @@ func TestChangefeedCreateAuthorizationWithChangefeedPriv(t *testing.T) {
 
 	ctx := context.Background()
 	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		DefaultTestTenant: base.TODOTestTenantDisabled,
+		DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 		Knobs: base.TestingKnobs{
 			JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
 			DistSQL: &execinfra.TestingKnobs{
@@ -8384,9 +8384,7 @@ func TestChangefeedHandlesDrainingNodes(t *testing.T) {
 
 	tc := serverutils.StartCluster(t, 4, base.TestClusterArgs{
 		ServerArgs: base.TestServerArgs{
-			// Test uses SPLIT AT, which isn't currently supported for
-			// secondary tenants. Tracked with #76378.
-			DefaultTestTenant: base.TODOTestTenantDisabled,
+			DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 			UseDatabase:       "test",
 			Knobs:             knobs,
 			ExternalIODir:     sinkDir,
@@ -8549,9 +8547,7 @@ func TestChangefeedHandlesRollingRestart(t *testing.T) {
 			return perNode
 		}(),
 		ServerArgs: base.TestServerArgs{
-			// Test uses SPLIT AT, which isn't currently supported for
-			// secondary tenants. Tracked with #76378.
-			DefaultTestTenant: base.TODOTestTenantDisabled,
+			DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 		},
 	})
 	defer tc.Stopper().Stop(context.Background())
@@ -8707,9 +8703,7 @@ func TestChangefeedTimelyResolvedTimestampUpdatePostRollingRestart(t *testing.T)
 		base.TestClusterArgs{
 			ServerArgsPerNode: perServerKnobs,
 			ServerArgs: base.TestServerArgs{
-				// Test uses SPLIT AT, which isn't currently supported for
-				// secondary tenants. Tracked with #76378.
-				DefaultTestTenant: base.TODOTestTenantDisabled,
+				DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 			},
 			ReusableListenerReg: listenerReg,
 		})
@@ -8802,9 +8796,7 @@ func TestChangefeedPropagatesTerminalError(t *testing.T) {
 			ServerArgsPerNode: perServerKnobs,
 			ReplicationMode:   base.ReplicationManual,
 			ServerArgs: base.TestServerArgs{
-				// Test uses SPLIT AT, which isn't currently supported for
-				// secondary tenants. Tracked with #76378.
-				DefaultTestTenant: base.TODOTestTenantDisabled,
+				DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 			},
 		})
 	defer tc.Stopper().Stop(context.Background())
@@ -11309,7 +11301,7 @@ func TestChangefeedExecLocality(t *testing.T) {
 	const nodes = 4
 	args := base.TestClusterArgs{
 		ServerArgs: base.TestServerArgs{
-			DefaultTestTenant: base.TODOTestTenantDisabled, // need nodelocal and splits.
+			DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799),
 		},
 		ServerArgsPerNode: map[int]base.TestServerArgs{},
 	}

pkg/ccl/changefeedccl/parquet_test.go

@@ -48,17 +48,16 @@ func TestParquetRows(t *testing.T) {
 	skip.UnderStress(t)
 
 	ctx := context.Background()
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// TODO(#98816): cdctest.GetHydratedTableDescriptor does not work with tenant dbs.
-		// Once it is fixed, this flag can be removed.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-	})
-	defer s.Stopper().Stop(ctx)
+	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer srv.Stopper().Stop(ctx)
+	s := srv.ApplicationLayer()
+
+	sysDB := sqlutils.MakeSQLRunner(srv.SystemLayer().SQLConn(t))
+	sysDB.Exec(t, "SET CLUSTER SETTING kv.rangefeed.enabled = true")
 
 	maxRowGroupSize := int64(4)
 
 	sqlDB := sqlutils.MakeSQLRunner(db)
-	sqlDB.Exec(t, "SET CLUSTER SETTING kv.rangefeed.enabled = true")
 
 	newDecimal := func(s string) *tree.DDecimal {
 		d, _, _ := apd.NewFromString(s)
@@ -264,7 +263,7 @@ func TestParquetRows(t *testing.T) {
 }
 
 func makeRangefeedReaderAndDecoder(
-	t *testing.T, s serverutils.TestServerInterface,
+	t *testing.T, s serverutils.ApplicationLayerInterface,
 ) (func(t testing.TB) *kvpb.RangeFeedValue, func(), cdcevent.Decoder) {
 	tableDesc := cdctest.GetHydratedTableDescriptor(t, s.ExecutorConfig(), "foo")
 	popRow, cleanup := cdctest.MakeRangeFeedValueReader(t, s.ExecutorConfig(), tableDesc)

pkg/ccl/changefeedccl/scheduled_changefeed_test.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	gosql "database/sql"
 	"fmt"
-	"net/url"
 	"strconv"
 	"strings"
 	"testing"
@@ -304,8 +303,7 @@ func TestCreateChangefeedScheduleChecksPermissionsDuringDryRun(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	ctx := context.Background()
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		DefaultTestTenant: base.TODOTestTenantDisabled,
+	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{
 		Knobs: base.TestingKnobs{
 			JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
 			DistSQL: &execinfra.TestingKnobs{
@@ -320,25 +318,20 @@ func TestCreateChangefeedScheduleChecksPermissionsDuringDryRun(t *testing.T) {
 			},
 		},
 	})
-	defer s.Stopper().Stop(ctx)
-	rootDB := sqlutils.MakeSQLRunner(db)
-	rootDB.Exec(t, `SET CLUSTER SETTING kv.rangefeed.enabled = true`)
+	defer srv.Stopper().Stop(ctx)
+	s := srv.ApplicationLayer()
+
+	sysDB := sqlutils.MakeSQLRunner(srv.SystemLayer().SQLConn(t))
+	sysDB.Exec(t, "SET CLUSTER SETTING kv.rangefeed.enabled = true")
+
+	sqlDB := sqlutils.MakeSQLRunner(db)
 	enableEnterprise := utilccl.TestingDisableEnterprise()
 	enableEnterprise()
 
-	rootDB.Exec(t, `CREATE TABLE table_a (i int)`)
-	rootDB.Exec(t, `CREATE USER testuser WITH PASSWORD 'test'`)
+	sqlDB.Exec(t, `CREATE TABLE table_a (i int)`)
+	sqlDB.Exec(t, `CREATE USER testuser WITH PASSWORD 'test'`)
 
-	pgURL := url.URL{
-		Scheme: "postgres",
-		User:   url.UserPassword("testuser", "test"),
-		Host:   s.SQLAddr(),
-	}
-	db2, err := gosql.Open("postgres", pgURL.String())
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer db2.Close()
+	db2 := s.SQLConn(t, serverutils.UserPassword("testuser", "test"))
 	userDB := sqlutils.MakeSQLRunner(db2)
 
 	userDB.ExpectErr(t, `Failed to dry run create changefeed: user "testuser" requires the CHANGEFEED privilege on all target tables to be able to run an enterprise changefeed`,

pkg/ccl/cliccl/flags.go

@@ -42,7 +42,7 @@ func (f *requireFipsFlag) Set(s string) error {
 	// this behavior globally (PersistentPreRun functions don't help because
 	// they are inherited across different levels of the command hierarchy only
 	// if that level does not have its own hook).
-	if v && fips140.Enabled() {
+	if v && !fips140.Enabled() {
 		err := errors.WithHint(errors.New("FIPS readiness checks failed"), "Run `cockroach debug enterprise-check-fips` for details")
 		clierror.OutputError(os.Stderr, err, true, false)
 		exit.WithCode(exit.UnspecifiedError())

pkg/sql/catalog/lease/lease.go

@@ -1321,12 +1321,14 @@ func (m *Manager) purgeOldVersions(
 				handle.release(ctx)
 			}
 		}()
-		state := m.findDescriptorState(id, false)
-		state.mu.Lock()
-		defer state.mu.Unlock()
-		// Find the previous version and determine the timestamp handle that
-		// it belongs to.
-		prev := state.mu.active.findPrevious()
+		prev, newest := func() (*descriptorVersionState, *descriptorVersionState) {
+			state := m.findDescriptorState(id, false)
+			state.mu.Lock()
+			defer state.mu.Unlock()
+			// Find the previous version and determine the timestamp handle that
+			// it belongs to.
+			return state.mu.active.findPrevious(), state.mu.active.findNewest()
+		}()
 		// If there is no previous or the previous version is a historical descriptor,
 		// nothing needs to be done (i.e. no active lease or already setup for
 		// expiration).
@@ -1358,8 +1360,8 @@ func (m *Manager) purgeOldVersions(
 						handle.mu.leasesToRelease[id].(*descriptorVersionState).getExpiration(ctx),
 						prev.GetVersion(),
 						prev.getExpiration(ctx),
-						state.mu.active.findNewest().GetVersion(),
-						state.mu.active.findNewest().getExpiration(ctx))
+						newest.GetVersion(),
+						newest.getExpiration(ctx))
 				}
 				// Bump the refcount on the previous version of the descriptor and attach
 				// it to the relevant timestamp.

pkg/sql/sqlstats/insights/integration/insights_test.go

@@ -780,6 +780,8 @@ func TestInsightsIntegrationForContention(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
+	skip.UnderDuress(t)
+
 	// Start the cluster. (One node is sufficient; the outliers system is currently in-memory only.)
 	ctx := context.Background()
 	settings := cluster.MakeTestingClusterSettings()