Skip to content

Commit

Permalink
backupccl: remove clusterversion.IncrementalBackupSubdir
Browse files Browse the repository at this point in the history 
Release note: none.
  • Loading branch information
@dt
dt committed Aug 3, 2022
1 parent 9448cdb commit 71becf9
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 91 deletions.
2 changes: 0 additions & 2 deletions pkg/ccl/backupccl/backupdest/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ go_test(
"//pkg/ccl/utilccl",
"//pkg/cloud",
"//pkg/cloud/impl:cloudimpl",
"//pkg/clusterversion",
"//pkg/jobs/jobspb",
"//pkg/roachpb",
"//pkg/security/securityassets",
"//pkg/security/securitytest",
"//pkg/security/username",
Expand Down
63 changes: 13 additions & 50 deletions pkg/ccl/backupccl/backupdest/backup_destination_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backuputils"
"github.com/cockroachdb/cockroach/pkg/cloud"
_ "github.com/cockroachdb/cockroach/pkg/cloud/impl" // register cloud storage providers
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
Expand Down Expand Up @@ -298,7 +296,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
inc8Time := full3Time.Add(time.Minute * 30)
inc9Time := inc8Time.Add(time.Minute * 30)
full4Time := inc9Time.Add(time.Minute * 30)
inc10Time := full4Time.Add(time.Minute * 30)

// firstBackupChain is maintained throughout the tests as the history of
// backups that were taken based on the initial full backup.
Expand All @@ -311,14 +308,12 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
// separate path relative to the full backup in their chain. Otherwise,
// it should be an empty array of strings
noIncrementalStorage := []string(nil)
incrementalBackupSubdirEnabled := true
notIncrementalBackupSubdirEnabled := false

firstRemoteBackupChain := []string(nil)

testCollectionBackup := func(t *testing.T, backupTime time.Time,
expectedDefault, expectedSuffix, expectedIncDir string, expectedPrevBackups []string,
appendToLatest bool, subdir string, incrementalTo []string, incrementalBackupSubdirEnabled bool) {
appendToLatest bool, subdir string, incrementalTo []string) {

endTime := hlc.Timestamp{WallTime: backupTime.UnixNano()}
incrementalFrom := []string(nil)
Expand All @@ -336,17 +331,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
_, localityCollections, err = backupdest.GetURIsByLocalityKV(incrementalTo, "")
require.NoError(t, err)
}
currentVersion := execCfg.Settings.Version.ActiveVersion(ctx)
if !incrementalBackupSubdirEnabled {
// Downgrading is disallowed in normal operation, but fine for testing here.
err = execCfg.Settings.Version.SetActiveVersion(ctx, clusterversion.ClusterVersion{
Version: roachpb.Version{
Major: 21,
Minor: 2,
},
})
require.NoError(t, err)
}

fullBackupExists := false
if expectedIncDir != "" {
Expand All @@ -360,9 +344,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
incrementalFrom,
&execCfg,
)
clusterErr := execCfg.Settings.Version.SetActiveVersion(ctx, currentVersion)
require.NoError(t, err)
require.NoError(t, clusterErr)

localityDests := make(map[string]string, len(localityCollections))
for locality, localityDest := range localityCollections {
Expand All @@ -389,7 +371,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, fullTime,
expectedDefault, expectedSuffix, expectedIncDir, firstBackupChain,
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage, incrementalBackupSubdirEnabled)
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
firstBackupChain = append(firstBackupChain, expectedDefault)
firstRemoteBackupChain = append(firstRemoteBackupChain, expectedDefault)
writeManifest(t, expectedDefault)
Expand All @@ -406,7 +388,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc1Time,
expectedDefault, expectedSuffix, expectedIncDir, firstBackupChain,
true /* intoLatest */, noExplicitSubDir, defaultIncrementalTo, incrementalBackupSubdirEnabled)
true /* intoLatest */, noExplicitSubDir, defaultIncrementalTo)
firstBackupChain = append(firstBackupChain, expectedDefault)
writeManifest(t, expectedDefault)
}
Expand All @@ -420,7 +402,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc2Time,
expectedDefault, expectedSuffix, expectedIncDir, firstBackupChain,
true /* intoLatest */, noExplicitSubDir, defaultIncrementalTo, incrementalBackupSubdirEnabled)
true /* intoLatest */, noExplicitSubDir, defaultIncrementalTo)
firstBackupChain = append(firstBackupChain, expectedDefault)
writeManifest(t, expectedDefault)
}
Expand All @@ -435,7 +417,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, full2Time,
expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage, incrementalBackupSubdirEnabled)
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
writeManifest(t, expectedDefault)
// We also wrote a new full backup, so let's update the latest.
writeLatest(t, collectionLoc, expectedSuffix)
Expand All @@ -450,7 +432,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc3Time,
expectedDefault, expectedSuffix, expectedIncDir, []string{backup2Location},
true /* intoLatest */, noExplicitSubDir, defaultIncrementalTo, incrementalBackupSubdirEnabled)
true /* intoLatest */, noExplicitSubDir, defaultIncrementalTo)
writeManifest(t, expectedDefault)
}

Expand All @@ -463,7 +445,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc4Time,
expectedDefault, expectedSuffix, expectedIncDir, firstBackupChain,
false /* intoLatest */, expectedSubdir, defaultIncrementalTo, incrementalBackupSubdirEnabled)
false /* intoLatest */, expectedSubdir, defaultIncrementalTo)
writeManifest(t, expectedDefault)
}

Expand All @@ -479,7 +461,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc5Time,
expectedDefault, expectedSuffix, expectedIncDir, firstRemoteBackupChain,
false /* intoLatest */, expectedSubdir, customIncrementalTo, incrementalBackupSubdirEnabled)
false /* intoLatest */, expectedSubdir, customIncrementalTo)
writeManifest(t, expectedDefault)

firstRemoteBackupChain = append(firstRemoteBackupChain, expectedDefault)
Expand All @@ -497,7 +479,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc6Time,
expectedDefault, expectedSuffix, expectedIncDir, firstRemoteBackupChain,
false /* intoLatest */, expectedSubdir, customIncrementalTo, incrementalBackupSubdirEnabled)
false /* intoLatest */, expectedSubdir, customIncrementalTo)
writeManifest(t, expectedDefault)
}

Expand All @@ -514,7 +496,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc7Time,
expectedDefault, expectedSuffix, expectedIncDir, []string{backup2Location},
true /* intoLatest */, expectedSubdir, customIncrementalTo, incrementalBackupSubdirEnabled)
true /* intoLatest */, expectedSubdir, customIncrementalTo)
writeManifest(t, expectedDefault)
}

Expand All @@ -528,7 +510,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, full3Time,
expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage, incrementalBackupSubdirEnabled)
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
writeManifest(t, expectedDefault)
// We also wrote a new full backup, so let's update the latest.
writeLatest(t, collectionLoc, expectedSuffix)
Expand Down Expand Up @@ -557,42 +539,23 @@ func TestBackupRestoreResolveDestination(t *testing.T) {

testCollectionBackup(t, inc9Time,
expectedDefault, expectedSuffix, expectedIncDir, []string{backup3Location, oldStyleDefault},
true /* intoLatest */, expectedSubdir, noIncrementalStorage, incrementalBackupSubdirEnabled)
true /* intoLatest */, expectedSubdir, noIncrementalStorage)
writeManifest(t, expectedDefault)
}

// A new full backup: BACKUP INTO collection
var backup4Location string
{
expectedSuffix := "/2020/12/25-120000.00"
expectedIncDir := ""
expectedDefault := fmt.Sprintf("nodelocal://1/%s%s?AUTH=implicit", t.Name(), expectedSuffix)
backup4Location = expectedDefault

testCollectionBackup(t, full4Time,
expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage, notIncrementalBackupSubdirEnabled)
false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
writeManifest(t, expectedDefault)
// We also wrote a new full backup, so let's update the latest.
writeLatest(t, collectionLoc, expectedSuffix)
}

// An automatic incremental into the fourth full backup: BACKUP INTO LATEST
// IN collection, BUT simulating an old cluster that doesn't support
// dedicated incrementals subdir by default yet.
{
expectedSuffix := "/2020/12/25-120000.00"
expectedIncDir := "/20201225/123000.00"
expectedSubdir := expectedSuffix
expectedDefault := fmt.Sprintf("nodelocal://1/%s%s%s?AUTH=implicit",
t.Name(),
expectedSuffix, expectedIncDir)

testCollectionBackup(t, inc10Time,
expectedDefault, expectedSuffix, expectedIncDir, []string{backup4Location},
true /* intoLatest */, expectedSubdir, noIncrementalStorage, notIncrementalBackupSubdirEnabled)
writeManifest(t, expectedDefault)
}
})
})
}
Expand Down
6 changes: 2 additions & 4 deletions pkg/ccl/backupccl/backupdest/incrementals.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backupbase"
"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backuputils"
"github.com/cockroachdb/cockroach/pkg/cloud"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -166,9 +165,8 @@ func ResolveIncrementalsBackupLocation(
"Please choose a location manually with the `incremental_location` parameter.")
}

// If the cluster isn't fully migrated, or we have backups in the old default
// location, continue to use the old location.
if len(prevOld) > 0 || !execCfg.Settings.Version.IsActive(ctx, clusterversion.IncrementalBackupSubdir) {
// If we have backups in the old default location, continue to use the old location.
if len(prevOld) > 0 {
return resolvedIncrementalsBackupLocationOld, nil
}

Expand Down
7 changes: 0 additions & 7 deletions pkg/clusterversion/cockroach_versions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,6 @@ const (
// PebbleFormatSplitUserKeysMarked performs a Pebble-level migration and
// upgrades the Pebble format major version to FormatSplitUserKeysMarked.
PebbleFormatSplitUserKeysMarked
// IncrementalBackupSubdir enables backing up new incremental backups to a
// dedicated subdirectory, to make it easier to apply a different ttl.
IncrementalBackupSubdir
// EnableNewStoreRebalancer enables the new store rebalancer introduced in
// 22.1.
EnableNewStoreRebalancer
Expand Down Expand Up @@ -465,10 +462,6 @@ var versionsSingleton = keyedVersions{
Key: PebbleFormatSplitUserKeysMarked,
Version: roachpb.Version{Major: 21, Minor: 2, Internal: 90},
},
{
Key: IncrementalBackupSubdir,
Version: roachpb.Version{Major: 21, Minor: 2, Internal: 92},
},
{
Key: EnableNewStoreRebalancer,
Version: roachpb.Version{Major: 21, Minor: 2, Internal: 96},
Expand Down
55 changes: 27 additions & 28 deletions pkg/clusterversion/key_string.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 71becf9

Please sign in to comment.