Skip to content

Commit

Permalink
Merge #106894
Browse files Browse the repository at this point in the history 
106894: backupccl: classify tenant testing exceptions r=knz a=stevendanna

Informs #76378. 
Epic: CRDB-18499

This PR enables some tests for randomized tenant-level testing
and correctly classifies other tests.

See individual commits for details.

Co-authored-by: Steven Danna <[email protected]>
  • Loading branch information
@stevendanna
craig[bot] and stevendanna committed Jul 18, 2023
2 parents 7675ca4 + d13fae1 commit ea8df67
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 97 deletions.
3 changes: 0 additions & 3 deletions pkg/ccl/backupccl/alter_backup_schedule_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ func newAlterSchedulesTestHelper(t *testing.T) (*alterSchedulesTestHelper, func(

args := base.TestServerArgs{
ExternalIODir: dir,
// Some scheduled backup tests fail when run within a tenant. More
// investigation is required. Tracked with #76378.
DefaultTestTenant: base.TODOTestTenantDisabled,
Knobs: base.TestingKnobs{
JobsTestingKnobs: knobs,
},
Expand Down
4 changes: 1 addition & 3 deletions pkg/ccl/backupccl/backup_tenant_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ func TestBackupTenantImportingTable(t *testing.T) {
tc := testcluster.StartTestCluster(t, 1,
base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
// Test is designed to run with explicit tenants. No need to
// implicitly create a tenant.
DefaultTestTenant: base.TODOTestTenantDisabled,
DefaultTestTenant: base.TestControlsTenantsExplicitly,
},
})
defer tc.Stopper().Stop(ctx)
Expand Down
143 changes: 63 additions & 80 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ func TestBackupRestoreJobTagAndLabel(t *testing.T) {
tc, _, _, cleanupFn := backupRestoreTestSetupWithParams(t, numNodes, numAccounts, InitManualReplication,
base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
DefaultTestTenant: base.TODOTestTenantDisabled,
Knobs: base.TestingKnobs{
DistSQL: &execinfra.TestingKnobs{
SetupFlowCb: func(ctx context.Context, _ base.SQLInstanceID, _ *execinfrapb.SetupFlowRequest) error {
Expand Down Expand Up @@ -1004,6 +1003,7 @@ func backupAndRestore(
sqlDB := sqlutils.MakeSQLRunner(conn)
storageConn := tc.StorageClusterConn()
storageSQLDB := sqlutils.MakeSQLRunner(storageConn)
storageSQLDB.Exec(t, "SET DATABASE=defaultdb")
{
sqlDB.Exec(t, `CREATE INDEX balance_idx ON data.bank (balance)`)
testutils.SucceedsSoon(t, func() error {
Expand Down Expand Up @@ -5428,9 +5428,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
const numAccounts = 1
_, origDB, dir, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, InitManualReplication)
defer cleanupFn()
// Test fails when run within a tenant. More investigation is
// required. Tracked with #76378.
args := base.TestServerArgs{ExternalIODir: dir, DefaultTestTenant: base.TODOTestTenantDisabled}
args := base.TestServerArgs{ExternalIODir: dir}

// Setup for sequence ownership backup/restore tests in the same database.
backupLoc := localFoo + `/d`
Expand All @@ -5440,19 +5438,23 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
origDB.Exec(t, `CREATE SEQUENCE d.seq OWNED BY d.t.a`)
origDB.Exec(t, `BACKUP DATABASE d TO $1`, backupLoc)

getTableDescriptorFromTestCluster := func(tc *testcluster.TestCluster, database string, table string) catalog.TableDescriptor {
srv := tc.TenantOrServer(0)
return desctestutils.TestingGetPublicTableDescriptor(srv.DB(), srv.Codec(), database, table)
}

// When restoring a database which has a owning table and an owned sequence,
// the ownership relationship should be preserved and remapped post restore.
t.Run("test restoring database should preserve ownership dependency", func(t *testing.T) {
tc := testcluster.StartTestCluster(t, singleNode, base.TestClusterArgs{ServerArgs: args})
defer tc.Stopper().Stop(context.Background())

newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
kvDB := tc.Server(0).DB()

newDB.Exec(t, `RESTORE DATABASE d FROM $1`, backupLoc)

tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t")
seqDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")
tableDesc := getTableDescriptorFromTestCluster(tc, "d", "t")
seqDesc := getTableDescriptorFromTestCluster(tc, "d", "seq")

require.True(t, seqDesc.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.Equal(t, tableDesc.GetID(), seqDesc.GetSequenceOpts().SequenceOwner.OwnerTableID,
Expand All @@ -5477,14 +5479,14 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
defer tc.Stopper().Stop(context.Background())

newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
kvDB := tc.Server(0).DB()

newDB.Exec(t, `CREATE DATABASE d`)
newDB.Exec(t, `USE d`)
newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner`,
`RESTORE TABLE seq FROM $1`, backupLoc)

newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc)
seqDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")
seqDesc := getTableDescriptorFromTestCluster(tc, "d", "seq")
require.False(t, seqDesc.GetSequenceOpts().HasOwner(), "unexpected owner of restored sequence.")
})

Expand All @@ -5499,7 +5501,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
defer tc.Stopper().Stop(context.Background())

newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
kvDB := tc.Server(0).DB()

newDB.Exec(t, `CREATE DATABASE d`)
newDB.Exec(t, `USE d`)
newDB.ExpectErr(t, `pq: cannot restore sequence "seq" without referenced owner table`,
Expand All @@ -5509,7 +5511,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
`RESTORE TABLE t FROM $1`, backupLoc)
newDB.Exec(t, `RESTORE TABLE t FROM $1 WITH skip_missing_sequence_owners`, backupLoc)

tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "t")
tableDesc := getTableDescriptorFromTestCluster(tc, "d", "t")

require.Equal(t, 0, tableDesc.PublicColumns()[0].NumOwnsSequences(),
"expected restored table to own 0 sequences",
Expand All @@ -5519,7 +5521,7 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
`RESTORE TABLE seq FROM $1`, backupLoc)
newDB.Exec(t, `RESTORE TABLE seq FROM $1 WITH skip_missing_sequence_owners`, backupLoc)

seqDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d", "seq")
seqDesc := getTableDescriptorFromTestCluster(tc, "d", "seq")
require.False(t, seqDesc.GetSequenceOpts().HasOwner(), "unexpected sequence owner after restore")
})

Expand All @@ -5530,13 +5532,12 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
defer tc.Stopper().Stop(context.Background())

newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
kvDB := tc.Server(0).DB()

newDB.Exec(t, `CREATE DATABASE restore_db`)
newDB.Exec(t, `RESTORE d.* FROM $1 WITH into_db='restore_db'`, backupLoc)

tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "t")
seqDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "restore_db", "seq")
tableDesc := getTableDescriptorFromTestCluster(tc, "restore_db", "t")
seqDesc := getTableDescriptorFromTestCluster(tc, "restore_db", "seq")

require.True(t, seqDesc.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.Equal(t, tableDesc.GetID(), seqDesc.GetSequenceOpts().SequenceOwner.OwnerTableID,
Expand Down Expand Up @@ -5579,14 +5580,13 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
defer tc.Stopper().Stop(context.Background())

newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
kvDB := tc.Server(0).DB()

newDB.ExpectErr(t, "pq: cannot restore sequence \"seq\" without referenced owner|"+
"pq: cannot restore table \"t\" without referenced sequence",
`RESTORE DATABASE d2 FROM $1`, backupLocD2D3)
newDB.Exec(t, `RESTORE DATABASE d2 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3)

tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t")
tableDesc := getTableDescriptorFromTestCluster(tc, "d2", "t")
require.Equal(t, 0, tableDesc.PublicColumns()[0].NumOwnsSequences(),
"expected restored table to own no sequences.",
)
Expand All @@ -5596,12 +5596,12 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
`RESTORE DATABASE d3 FROM $1`, backupLocD2D3)
newDB.Exec(t, `RESTORE DATABASE d3 FROM $1 WITH skip_missing_sequence_owners`, backupLocD2D3)

seqDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq")
seqDesc := getTableDescriptorFromTestCluster(tc, "d3", "seq")
require.False(t, seqDesc.GetSequenceOpts().HasOwner(), "unexpected sequence owner after restore")

// Sequence dependencies inside the database should still be preserved.
sd := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2")
td := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t")
sd := getTableDescriptorFromTestCluster(tc, "d3", "seq2")
td := getTableDescriptorFromTestCluster(tc, "d3", "t")

require.True(t, sd.GetSequenceOpts().HasOwner(), "no owner found for seq2")
require.Equal(t, td.GetID(), sd.GetSequenceOpts().SequenceOwner.OwnerTableID,
Expand All @@ -5624,13 +5624,12 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
defer tc.Stopper().Stop(context.Background())

newDB := sqlutils.MakeSQLRunner(tc.Conns[0])
kvDB := tc.Server(0).DB()

newDB.Exec(t, `RESTORE DATABASE d2, d3 FROM $1`, backupLocD2D3)

// d2.t owns d3.seq should be preserved.
tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "t")
seqDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq")
tableDesc := getTableDescriptorFromTestCluster(tc, "d2", "t")
seqDesc := getTableDescriptorFromTestCluster(tc, "d3", "seq")

require.True(t, seqDesc.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.Equal(t, tableDesc.GetID(), seqDesc.GetSequenceOpts().SequenceOwner.OwnerTableID,
Expand All @@ -5647,9 +5646,9 @@ func TestBackupRestoreSequenceOwnership(t *testing.T) {
)

// d3.t owns d2.seq and d3.seq2 should be preserved.
td := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "t")
sd := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d2", "seq")
sdSeq2 := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "d3", "seq2")
td := getTableDescriptorFromTestCluster(tc, "d3", "t")
sd := getTableDescriptorFromTestCluster(tc, "d2", "seq")
sdSeq2 := getTableDescriptorFromTestCluster(tc, "d3", "seq2")

require.True(t, sd.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
require.True(t, sdSeq2.GetSequenceOpts().HasOwner(), "no sequence owner after restore")
Expand Down Expand Up @@ -6035,9 +6034,7 @@ func TestProtectedTimestampsDuringBackup(t *testing.T) {
params := base.TestClusterArgs{}
params.ServerArgs.ExternalIODir = dir
params.ServerArgs.Knobs.JobsTestingKnobs = jobs.NewTestingKnobsWithShortIntervals()
// This test instantiates its own secondary tenants below. No need to run
// it probabilistically under a test tenant.
params.ServerArgs.DefaultTestTenant = base.TODOTestTenantDisabled
params.ServerArgs.DefaultTestTenant = base.TestControlsTenantsExplicitly
tc := testcluster.StartTestCluster(t, 1, params)
defer tc.Stopper().Stop(ctx)

Expand Down Expand Up @@ -6345,12 +6342,17 @@ func TestRestoreErrorPropagates(t *testing.T) {
defer dirCleanupFn()
params := base.TestClusterArgs{}
params.ServerArgs.ExternalIODir = dir
// When this test runs under the default test tenant, the RESTORE command
// below which is expected to fail, doesn't. This may be a problem with the
// testing knobs being incorrectly applied to the cluster. More
// investigation is required. Tracked with #76378.
params.ServerArgs.DefaultTestTenant = base.TODOTestTenantDisabled
jobsTableKey := keys.SystemSQLCodec.TablePrefix(uint32(systemschema.JobsTable.GetID()))
// TODO(ssd): The way we inject the error requires that we
// intercept the actual batch request for job table write. To
// keep this from being flakey, we need to disable various
// automatic jobs. Unfortunately, if we disable the span
// configuration job, we can't start a tenant.
params.ServerArgs.DefaultTestTenant = base.TestNeedsTightIntegrationBetweenAPIsAndTestingKnobs
var jobsTableKey = struct {
syncutil.Mutex
key roachpb.Key
}{}

var shouldFail, failures int64
params.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{
TestingRequestFilter: func(ctx context.Context, ba *kvpb.BatchRequest) *kvpb.Error {
Expand All @@ -6360,12 +6362,18 @@ func TestRestoreErrorPropagates(t *testing.T) {
if !ba.IsWrite() {
return nil
}
jobsTableKey.Lock()
defer jobsTableKey.Unlock()
if len(jobsTableKey.key) == 0 {
return nil
}

for _, ru := range ba.Requests {
r := ru.GetInner()
switch r.(type) {
case *kvpb.ConditionalPutRequest, *kvpb.PutRequest:
key := r.Header().Key
if bytes.HasPrefix(key, jobsTableKey) && atomic.LoadInt64(&shouldFail) > 0 {
if bytes.HasPrefix(key, jobsTableKey.key) && atomic.LoadInt64(&shouldFail) > 0 {
return kvpb.NewError(errors.Errorf("boom %d", atomic.AddInt64(&failures, 1)))
}
}
Expand All @@ -6381,6 +6389,11 @@ func TestRestoreErrorPropagates(t *testing.T) {
defer tc.Stopper().Stop(ctx)
db := tc.ServerConn(0)
runner := sqlutils.MakeSQLRunner(db)

jobsTableKey.Lock()
jobsTableKey.key = tc.TenantOrServer(0).Codec().TablePrefix(uint32(systemschema.JobsTable.GetID()))
jobsTableKey.Unlock()

runner.Exec(t, `SET CLUSTER SETTING jobs.metrics.interval.poll = '30s'`)
runner.Exec(t, `SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false`)
runner.Exec(t, `SET CLUSTER SETTING sql.stats.system_tables_autostats.enabled = false`)
Expand Down Expand Up @@ -6455,10 +6468,8 @@ func TestPaginatedBackupTenant(t *testing.T) {

const numAccounts = 1
serverArgs := base.TestServerArgs{
Knobs: base.TestingKnobs{JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals()},
// Disabled to probabilistically spin up a tenant in each server because the
// test explicitly sets up tenants to test on.
DefaultTestTenant: base.TODOTestTenantDisabled}
Knobs: base.TestingKnobs{JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals()},
DefaultTestTenant: base.TestControlsTenantsExplicitly}
params := base.TestClusterArgs{ServerArgs: serverArgs}
var numExportRequests int

Expand Down Expand Up @@ -6940,9 +6951,7 @@ func TestBackupRestoreTenant(t *testing.T) {
},
},

// Disabled to probabilistically spin up a tenant in each server because the
// test explicitly sets up tenants to test on.
DefaultTestTenant: base.TODOTestTenantDisabled},
DefaultTestTenant: base.TestControlsTenantsExplicitly},
}

const numAccounts = 1
Expand Down Expand Up @@ -7027,12 +7036,8 @@ func TestBackupRestoreTenant(t *testing.T) {
t.Run("restore-tenant10-to-latest", func(t *testing.T) {
restoreTC := testcluster.StartTestCluster(
t, singleNode, base.TestClusterArgs{ServerArgs: base.TestServerArgs{
ExternalIODir: dir,
// This test already exercises the tenant codepaths explicitly
// by creating a tenant. Furthermore, the test requires that
// it run from the system tenant because it restores tenants.
// Disable the default test tenant because it's not necessary.
DefaultTestTenant: base.TODOTestTenantDisabled,
ExternalIODir: dir,
DefaultTestTenant: base.TestControlsTenantsExplicitly,
Knobs: base.TestingKnobs{JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals()},
}},
)
Expand Down Expand Up @@ -7191,12 +7196,8 @@ func TestBackupRestoreTenant(t *testing.T) {
t.Run("restore-t10-from-cluster-backup", func(t *testing.T) {
restoreTC := testcluster.StartTestCluster(
t, singleNode, base.TestClusterArgs{ServerArgs: base.TestServerArgs{
ExternalIODir: dir,
// This test already exercises the tenant codepaths explicitly
// by creating a tenant. Furthermore, the test requires that
// it run from the system tenant because it restores tenants.
// Disable the default test tenant because it's not necessary.
DefaultTestTenant: base.TODOTestTenantDisabled,
ExternalIODir: dir,
DefaultTestTenant: base.TestControlsTenantsExplicitly,
}},
)
defer restoreTC.Stopper().Stop(ctx)
Expand Down Expand Up @@ -7244,13 +7245,8 @@ func TestBackupRestoreTenant(t *testing.T) {
t.Run("restore-all-from-cluster-backup", func(t *testing.T) {
restoreTC := testcluster.StartTestCluster(
t, singleNode, base.TestClusterArgs{ServerArgs: base.TestServerArgs{
ExternalIODir: dir,
// This test already exercises the tenant codepaths explicitly
// by creating a tenant. Furthermore, the test requires that
// it run from the system tenant because it queries the
// system.tenants table. Disable the default test tenant because
// it's not necessary.
DefaultTestTenant: base.TODOTestTenantDisabled,
ExternalIODir: dir,
DefaultTestTenant: base.TestControlsTenantsExplicitly,
Knobs: base.TestingKnobs{
JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
TenantTestingKnobs: &sql.TenantTestingKnobs{
Expand Down Expand Up @@ -7354,12 +7350,8 @@ func TestBackupRestoreTenant(t *testing.T) {
t.Run("restore-tenant10-to-ts1", func(t *testing.T) {
restoreTC := testcluster.StartTestCluster(
t, singleNode, base.TestClusterArgs{ServerArgs: base.TestServerArgs{
ExternalIODir: dir,
// This test already exercises the tenant codepaths explicitly
// by creating a tenant. Furthermore, the test requires that
// it run from the system tenant because it restores tenants.
// Disable the default test tenant because it's not necessary.
DefaultTestTenant: base.TODOTestTenantDisabled,
ExternalIODir: dir,
DefaultTestTenant: base.TestControlsTenantsExplicitly,
}},
)
defer restoreTC.Stopper().Stop(ctx)
Expand All @@ -7379,12 +7371,8 @@ func TestBackupRestoreTenant(t *testing.T) {
t.Run("restore-tenant20-to-latest", func(t *testing.T) {
restoreTC := testcluster.StartTestCluster(
t, singleNode, base.TestClusterArgs{ServerArgs: base.TestServerArgs{
ExternalIODir: dir,
// This test already exercises the tenant codepaths explicitly
// by creating a tenant. Furthermore, the test requires that
// it run from the system tenant because it restores tenants.
// Disable the default test tenant because it's not necessary.
DefaultTestTenant: base.TODOTestTenantDisabled,
ExternalIODir: dir,
DefaultTestTenant: base.TestControlsTenantsExplicitly,
}},
)
defer restoreTC.Stopper().Stop(ctx)
Expand Down Expand Up @@ -11147,7 +11135,6 @@ func TestRestoreMemoryMonitoringWithShadowing(t *testing.T) {
restoreProcessorKnobCount := atomic.Uint32{}

args := base.TestServerArgs{
DefaultTestTenant: base.TODOTestTenantDisabled,
Knobs: base.TestingKnobs{
DistSQL: &execinfra.TestingKnobs{
BackupRestoreTestingKnobs: &sql.BackupRestoreTestingKnobs{
Expand Down Expand Up @@ -11199,11 +11186,7 @@ func TestRestoreMemoryMonitoringMinWorkerMemory(t *testing.T) {
defer log.Scope(t).Close(t)
const numAccounts = 100

args := base.TestServerArgs{
DefaultTestTenant: base.TODOTestTenantDisabled,
}
params := base.TestClusterArgs{ServerArgs: args}
_, sqlDB, _, cleanupFn := backupRestoreTestSetupWithParams(t, singleNode, numAccounts, InitManualReplication, params)
_, sqlDB, _, cleanupFn := backupRestoreTestSetupWithParams(t, singleNode, numAccounts, InitManualReplication, base.TestClusterArgs{})
defer cleanupFn()

// 4 restore workers means we need minimum 2 workers to start restore.
Expand Down
Loading

0 comments on commit ea8df67

Please sign in to comment.