Skip to content

Commit

Permalink
Merge pull request #73050 from stevendanna/ssd/fix-the-fix
Browse files Browse the repository at this point in the history
backupccl: ignore ErrDescriptorNotFound on system table lookup
  • Loading branch information
stevendanna authored Nov 22, 2021
2 parents 6e40695 + 2f5a6da commit af7f257
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 24 deletions.
13 changes: 8 additions & 5 deletions pkg/ccl/backupccl/system_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
descpb "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
Expand Down Expand Up @@ -382,18 +383,20 @@ func GetSystemTableIDsToExcludeFromClusterBackup(
if backupConfig.shouldIncludeInClusterBackup == optOutOfClusterBackup {
err := sql.DescsTxn(ctx, execCfg, func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error {
tn := tree.MakeTableNameWithSchema("system", tree.PublicSchemaName, tree.Name(systemTableName))
found, desc, err := col.GetMutableTableByName(ctx, txn, &tn, tree.ObjectLookupFlags{})
if err != nil {
found, desc, err := col.GetImmutableTableByName(ctx, txn, &tn, tree.ObjectLookupFlags{})
isNotFoundErr := errors.Is(err, catalog.ErrDescriptorNotFound)
if err != nil && !isNotFoundErr {
return err
}

// Some system tables are not present when running inside a secondary
// tenant egs: `systemschema.TenantsTable`. In such situations we are
// print a warning and move on.
if !found {
log.Warningf(ctx, "could not find system table descriptor %s", systemTableName)
if !found || isNotFoundErr {
log.Warningf(ctx, "could not find system table descriptor %q", systemTableName)
return nil
}
systemTableIDsToExclude[desc.ID] = struct{}{}
systemTableIDsToExclude[desc.GetID()] = struct{}{}
return nil
})
if err != nil {
Expand Down
50 changes: 31 additions & 19 deletions pkg/cmd/roachtest/tests/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,6 @@ func registerBackupMixedVersion(r registry.Registry) {
// `cockroach` will be used.
const mainVersion = ""
roachNodes := c.All()
newNode := 1
oldNode := 3
predV, err := PredecessorVersion(*t.BuildVersion())
require.NoError(t, err)
c.Put(ctx, t.DeprecatedWorkload(), "./workload")
Expand All @@ -179,32 +177,46 @@ func registerBackupMixedVersion(r registry.Registry) {
}
runImportBankDataSplit(ctx, rows, 0 /* ranges */, t, u.c)
}
successfulBackupStep := func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
backupQuery := fmt.Sprintf("BACKUP bank.bank TO 'nodelocal://%d/%s'", newNode, destinationName(c))
gatewayDB := c.Conn(ctx, newNode)
runBackup := func(nodeID int, opts ...string) error {
backupOpts := ""
if len(opts) > 0 {
backupOpts = fmt.Sprintf("WITH %s", strings.Join(opts, ", "))
}
backupQuery := fmt.Sprintf("BACKUP bank.bank TO 'nodelocal://%d/%s' %s",
nodeID, destinationName(c), backupOpts)

gatewayDB := c.Conn(ctx, nodeID)
defer gatewayDB.Close()
_, err = gatewayDB.ExecContext(ctx, backupQuery)
require.NoError(t, err)
t.Status("Running: ", backupQuery)
_, err := gatewayDB.ExecContext(ctx, backupQuery)
return err
}
backupFailsWithExpectedErrorStep := func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
backupQuery := fmt.Sprintf("BACKUP bank.bank TO 'nodelocal://%d/%s'", oldNode, destinationName(c))
// We run the backup on an oldNode, which should result in an error message
gatewayDB := c.Conn(ctx, oldNode)
defer gatewayDB.Close()
_, err = gatewayDB.ExecContext(ctx, backupQuery)
require.Error(t, err)
require.True(t, strings.Contains(err.Error(), "ExportRequest was issued from a node running"))
successfulBackupStep := func(nodeID int, opts ...string) versionStep {
return func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
require.NoError(t, runBackup(nodeID, opts...))
}
}
backupFailsWithExpectedErrorStep := func(nodeID int, errStr string) versionStep {
return func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
err := runBackup(nodeID)
require.Error(t, err)
require.True(t, strings.Contains(err.Error(), errStr))
}
}

u := newVersionUpgradeTest(c,
uploadAndStartFromCheckpointFixture(roachNodes, predV),
waitForUpgradeStep(roachNodes),
preventAutoUpgradeStep(1),
loadBackupDataStep,
// Upgrade some of the nodes.
binaryUpgradeStep(c.Node(1), mainVersion),
binaryUpgradeStep(c.Node(2), mainVersion),
successfulBackupStep,
backupFailsWithExpectedErrorStep,
binaryUpgradeStep(c.Nodes(1, 2), mainVersion),
// Backup from new node should succeed
successfulBackupStep(1),
// Backup from new node with revision history should succeed
successfulBackupStep(2, "revision_history"),
// Backup from old node fails (see #72852)
backupFailsWithExpectedErrorStep(3, "ExportRequest was issued from a node running"),
)
u.run(ctx, t)
},
Expand Down

0 comments on commit af7f257

Please sign in to comment.