Skip to content

Commit

Permalink
Merge #45764
Browse files Browse the repository at this point in the history
45764: cloud: require nodeID in nodelocal URIs r=dt a=dt

Previously nodelocal URIs without a host specified, or with a nodeID of
0, would be interpreted as 'which ever node happens to be evaluating
this URI'. This behavior often leads to confusion and typically a user
is better served by specifying which node, e.g. with
nodelocal://1/foo/bar.

This change now *requires* users specify a nodeID in nodelocal URIs. If
a user truely wants the old behavior of picking the local node, they can
speicfy a nodeID of zero or use the reserved hostname 'self'.

Release note (general change): nodelocal:// format URIs now require a nodeID be specified in the hostname field. The special nodeID of 'self' is equivalent to the old behavior when unspecified.


Co-authored-by: David Taylor <[email protected]>
  • Loading branch information
craig[bot] and dt committed Mar 6, 2020
2 parents ad84601 + 3fc3e14 commit 954fe69
Show file tree
Hide file tree
Showing 20 changed files with 151 additions and 145 deletions.
102 changes: 51 additions & 51 deletions pkg/ccl/backupccl/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const (
multiNode = 3
backupRestoreDefaultRanges = 10
backupRestoreRowPayloadSize = 100
localFoo = "nodelocal:///foo"
localFoo = "nodelocal://0/foo"
)

func backupRestoreTestSetupEmptyWithParams(
Expand Down Expand Up @@ -1057,27 +1057,27 @@ func TestBackupRestoreResume(t *testing.T) {
if err != nil {
t.Fatal(err)
}
backupDir := filepath.Join(dir, "backup")
backupDir := dir + "/backup"
if err := os.MkdirAll(backupDir, 0755); err != nil {
t.Fatal(err)
}
checkpointFile := filepath.Join(backupDir, backupccl.BackupManifestCheckpointName)
checkpointFile := backupDir + "/" + backupccl.BackupManifestCheckpointName
if err := ioutil.WriteFile(checkpointFile, mockManifest, 0644); err != nil {
t.Fatal(err)
}
createAndWaitForJob(
t, sqlDB, []sqlbase.ID{backupTableDesc.ID},
jobspb.BackupDetails{
EndTime: tc.Servers[0].Clock().Now(),
URI: "nodelocal:///backup",
URI: "nodelocal://0/backup",
BackupManifest: mockManifest,
},
jobspb.BackupProgress{},
)

// If the backup properly took the (incorrect) checkpoint into account, it
// won't have tried to re-export any keys within backupCompletedSpan.
backupManifestFile := filepath.Join(backupDir, backupccl.BackupManifestName)
backupManifestFile := backupDir + "/" + backupccl.BackupManifestName
backupManifestBytes, err := ioutil.ReadFile(backupManifestFile)
if err != nil {
t.Fatal(err)
Expand All @@ -1095,7 +1095,7 @@ func TestBackupRestoreResume(t *testing.T) {

t.Run("restore", func(t *testing.T) {
sqlDB := sqlutils.MakeSQLRunner(outerDB.DB)
restoreDir := "nodelocal:///restore"
restoreDir := "nodelocal://0/restore"
sqlDB.Exec(t, `BACKUP DATABASE DATA TO $1`, restoreDir)
sqlDB.Exec(t, `CREATE DATABASE restoredb`)
restoreDatabaseID := sqlutils.QueryDatabaseID(t, sqlDB.DB, "restoredb")
Expand Down Expand Up @@ -1216,7 +1216,7 @@ func TestBackupRestoreControlJob(t *testing.T) {
sqlDB := sqlutils.MakeSQLRunner(outerDB.DB)

t.Run("foreign", func(t *testing.T) {
foreignDir := "nodelocal:///foreign"
foreignDir := "nodelocal://0/foreign"
sqlDB.Exec(t, `CREATE DATABASE orig_fkdb`)
sqlDB.Exec(t, `CREATE DATABASE restore_fkdb`)
sqlDB.Exec(t, `CREATE TABLE orig_fkdb.fk (i INT REFERENCES data.bank)`)
Expand Down Expand Up @@ -1245,8 +1245,8 @@ func TestBackupRestoreControlJob(t *testing.T) {
})

t.Run("pause", func(t *testing.T) {
pauseDir := "nodelocal:///pause"
noOfflineDir := "nodelocal:///no-offline"
pauseDir := "nodelocal://0/pause"
noOfflineDir := "nodelocal://0/no-offline"
sqlDB.Exec(t, `CREATE DATABASE pause`)

for i, query := range []string{
Expand Down Expand Up @@ -1285,7 +1285,7 @@ func TestBackupRestoreControlJob(t *testing.T) {
})

t.Run("pause-cancel", func(t *testing.T) {
backupDir := "nodelocal:///backup"
backupDir := "nodelocal://0/backup"

backupJobID, err := jobutils.RunJob(t, sqlDB, &allowResponse, nil, "BACKUP DATABASE data TO $1", backupDir)
if err != nil {
Expand Down Expand Up @@ -1317,7 +1317,7 @@ func TestBackupRestoreControlJob(t *testing.T) {
})

t.Run("cancel", func(t *testing.T) {
cancelDir := "nodelocal:///cancel"
cancelDir := "nodelocal://0/cancel"
sqlDB.Exec(t, `CREATE DATABASE cancel`)

for i, query := range []string{
Expand Down Expand Up @@ -1360,7 +1360,7 @@ func TestRestoreFailCleanup(t *testing.T) {
initNone, base.TestClusterArgs{ServerArgs: params})
defer cleanup()

dir = filepath.Join(dir, "foo")
dir = dir + "/foo"

sqlDB.Exec(t, `CREATE DATABASE restore`)
sqlDB.Exec(t, `BACKUP DATABASE data TO $1`, localFoo)
Expand Down Expand Up @@ -1407,7 +1407,7 @@ func TestRestoreFailDatabaseCleanup(t *testing.T) {
initNone, base.TestClusterArgs{ServerArgs: params})
defer cleanup()

dir = filepath.Join(dir, "foo")
dir = dir + "/foo"

sqlDB.Exec(t, `BACKUP DATABASE data TO $1`, localFoo)
// Bugger the backup by removing the SST files.
Expand Down Expand Up @@ -1902,7 +1902,7 @@ func TestBackupRestoreIncremental(t *testing.T) {

checksums = append(checksums, checksumBankPayload(t, sqlDB))

backupDir := fmt.Sprintf("nodelocal:///%d", backupNum)
backupDir := fmt.Sprintf("nodelocal://0/%d", backupNum)
var from string
if backupNum > 0 {
from = fmt.Sprintf(` INCREMENTAL FROM %s`, strings.Join(backupDirs, `,`))
Expand All @@ -1918,9 +1918,9 @@ func TestBackupRestoreIncremental(t *testing.T) {
sqlDB.Exec(t, `INSERT INTO data.bank VALUES (0, -1, 'final')`)
checksums = append(checksums, checksumBankPayload(t, sqlDB))
sqlDB.Exec(t, fmt.Sprintf(`BACKUP TABLE data.bank TO '%s' %s`,
"nodelocal:///final", fmt.Sprintf(` INCREMENTAL FROM %s`, strings.Join(backupDirs, `,`)),
"nodelocal://0/final", fmt.Sprintf(` INCREMENTAL FROM %s`, strings.Join(backupDirs, `,`)),
))
backupDirs = append(backupDirs, `'nodelocal:///final'`)
backupDirs = append(backupDirs, `'nodelocal://0/final'`)
}

// Start a new cluster to restore into.
Expand All @@ -1940,7 +1940,7 @@ func TestBackupRestoreIncremental(t *testing.T) {
sqlDBRestore.ExpectErr(
t, fmt.Sprintf("belongs to cluster %s", tc.Servers[0].ClusterID()),
`BACKUP TABLE data.bank TO $1 INCREMENTAL FROM $2`,
"nodelocal:///some-other-table", "nodelocal:///0",
"nodelocal://0/some-other-table", "nodelocal://0/0",
)

for i := len(backupDirs); i > 0; i-- {
Expand Down Expand Up @@ -1971,8 +1971,8 @@ func TestBackupRestorePartitionedIncremental(t *testing.T) {

// Each incremental backup is written to two different subdirectories in
// defaultDir and dc1Dir, respectively.
const defaultDir = "nodelocal:///default"
const dc1Dir = "nodelocal:///dc=dc1"
const defaultDir = "nodelocal://0/default"
const dc1Dir = "nodelocal://0/dc=dc1"
var defaultBackupDirs []string
var checksums []uint32
{
Expand Down Expand Up @@ -2157,7 +2157,7 @@ func TestConcurrentBackupRestores(t *testing.T) {
g.Go(func() error {
for j := 0; j < numIterations; j++ {
dbName := fmt.Sprintf("%s_%d", table, j)
backupDir := fmt.Sprintf("nodelocal:///%s", dbName)
backupDir := fmt.Sprintf("nodelocal://0/%s", dbName)
backupQ := fmt.Sprintf(`BACKUP data.%s TO $1`, table)
if _, err := sqlDB.DB.ExecContext(gCtx, backupQ, backupDir); err != nil {
return err
Expand Down Expand Up @@ -2215,9 +2215,9 @@ func TestBackupAsOfSystemTime(t *testing.T) {
t.Fatalf("expected %d rows but found %d", expected, rowCount)
}

beforeDir := filepath.Join(localFoo, `beforeTs`)
beforeDir := localFoo + `/beforeTs`
sqlDB.Exec(t, fmt.Sprintf(`BACKUP DATABASE data TO '%s' AS OF SYSTEM TIME %s`, beforeDir, beforeTs))
equalDir := filepath.Join(localFoo, `equalTs`)
equalDir := localFoo + `/equalTs`
sqlDB.Exec(t, fmt.Sprintf(`BACKUP DATABASE data TO '%s' AS OF SYSTEM TIME %s`, equalDir, equalTs))

sqlDB.Exec(t, `DROP TABLE data.bank`)
Expand All @@ -2241,7 +2241,7 @@ func TestRestoreAsOfSystemTime(t *testing.T) {
const numAccounts = 10
ctx, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()
const dir = "nodelocal:///"
const dir = "nodelocal://0/"

ts := make([]string, 9)

Expand All @@ -2265,8 +2265,8 @@ func TestRestoreAsOfSystemTime(t *testing.T) {
t.Fatal(err)
}

fullBackup, latestBackup := filepath.Join(dir, "full"), filepath.Join(dir, "latest")
incBackup, incLatestBackup := filepath.Join(dir, "inc"), filepath.Join(dir, "inc-latest")
fullBackup, latestBackup := dir+"/full", dir+"/latest"
incBackup, incLatestBackup := dir+"/inc", dir+"/inc-latest"
inc2Backup, inc2LatestBackup := incBackup+".2", incLatestBackup+".2"

sqlDB.Exec(t,
Expand All @@ -2278,7 +2278,7 @@ func TestRestoreAsOfSystemTime(t *testing.T) {
latestBackup,
)

fullTableBackup := filepath.Join(dir, "tbl")
fullTableBackup := dir + "/tbl"
sqlDB.Exec(t,
fmt.Sprintf(`BACKUP data.bank TO $1 AS OF SYSTEM TIME %s WITH revision_history`, ts[2]),
fullTableBackup,
Expand Down Expand Up @@ -2337,7 +2337,7 @@ func TestRestoreAsOfSystemTime(t *testing.T) {
inc2LatestBackup, latestBackup, incLatestBackup,
)

incTableBackup := filepath.Join(dir, "inctbl")
incTableBackup := dir + "/inctbl"
sqlDB.Exec(t,
`BACKUP data.bank TO $1 INCREMENTAL FROM $2 WITH revision_history`,
incTableBackup, fullTableBackup,
Expand Down Expand Up @@ -2464,7 +2464,7 @@ func TestRestoreAsOfSystemTimeGCBounds(t *testing.T) {
const numAccounts = 10
ctx, tc, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()
const dir = "nodelocal:///"
const dir = "nodelocal://0/"
preGC := tree.TimestampToDecimal(tc.Server(0).Clock().Now()).String()

gcr := roachpb.GCRequest{
Expand All @@ -2483,7 +2483,7 @@ func TestRestoreAsOfSystemTimeGCBounds(t *testing.T) {

postGC := tree.TimestampToDecimal(tc.Server(0).Clock().Now()).String()

lateFullTableBackup := filepath.Join(dir, "tbl-after-gc")
lateFullTableBackup := dir + "/tbl-after-gc"
sqlDB.Exec(t, `BACKUP data.bank TO $1 WITH revision_history`, lateFullTableBackup)
sqlDB.Exec(t, `DROP TABLE data.bank`)
sqlDB.ExpectErr(
Expand Down Expand Up @@ -2583,10 +2583,10 @@ func TestTimestampMismatch(t *testing.T) {
sqlDB.Exec(t, `CREATE TABLE data.t2 (a INT PRIMARY KEY)`)
sqlDB.Exec(t, `INSERT INTO data.t2 VALUES (1)`)

fullBackup := filepath.Join(localFoo, "0")
incrementalT1FromFull := filepath.Join(localFoo, "1")
incrementalT2FromT1 := filepath.Join(localFoo, "2")
incrementalT3FromT1OneTable := filepath.Join(localFoo, "3")
fullBackup := localFoo + "/0"
incrementalT1FromFull := localFoo + "/1"
incrementalT2FromT1 := localFoo + "/2"
incrementalT3FromT1OneTable := localFoo + "/3"

sqlDB.Exec(t, `BACKUP DATABASE data TO $1`,
fullBackup)
Expand Down Expand Up @@ -2896,7 +2896,7 @@ func TestBackupRestorePermissions(t *testing.T) {
sqlDB.Exec(t, "GRANT admin TO testuser")

t.Run("backup-table", func(t *testing.T) {
testLocalFoo := fmt.Sprintf("nodelocal:///%s", t.Name())
testLocalFoo := fmt.Sprintf("nodelocal://0/%s", t.Name())
testLocalBackupStmt := fmt.Sprintf(`BACKUP data.bank TO '%s'`, testLocalFoo)
if _, err := testuser.Exec(testLocalBackupStmt); err != nil {
t.Fatal(err)
Expand All @@ -2908,7 +2908,7 @@ func TestBackupRestorePermissions(t *testing.T) {
})

t.Run("backup-database", func(t *testing.T) {
testLocalFoo := fmt.Sprintf("nodelocal:///%s", t.Name())
testLocalFoo := fmt.Sprintf("nodelocal://0/%s", t.Name())
testLocalBackupStmt := fmt.Sprintf(`BACKUP DATABASE data TO '%s'`, testLocalFoo)
if _, err := testuser.Exec(testLocalBackupStmt); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -2940,9 +2940,9 @@ func TestRestoreDatabaseVersusTable(t *testing.T) {
origDB.Exec(t, q)
}

d4foo := "nodelocal:///d4foo"
d4foobar := "nodelocal:///d4foobar"
d4star := "nodelocal:///d4star"
d4foo := "nodelocal://0/d4foo"
d4foobar := "nodelocal://0/d4foobar"
d4star := "nodelocal://0/d4star"

origDB.Exec(t, `BACKUP DATABASE data, d2, d3, d4 TO $1`, localFoo)
origDB.Exec(t, `BACKUP d4.foo TO $1`, d4foo)
Expand Down Expand Up @@ -3063,12 +3063,12 @@ func TestPointInTimeRecovery(t *testing.T) {
_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()

fullBackupDir := filepath.Join(localFoo, "full")
fullBackupDir := localFoo + "/full"
sqlDB.Exec(t, `BACKUP data.* TO $1`, fullBackupDir)

sqlDB.Exec(t, `UPDATE data.bank SET balance = 2`)

incBackupDir := filepath.Join(localFoo, "inc")
incBackupDir := localFoo + "/inc"
sqlDB.Exec(t, `BACKUP data.* TO $1 INCREMENTAL FROM $2`, incBackupDir, fullBackupDir)

var beforeBadThingTs string
Expand All @@ -3089,7 +3089,7 @@ func TestPointInTimeRecovery(t *testing.T) {
// RENAME-ing the table into the final location.
t.Run("recovery=new-backup", func(t *testing.T) {
sqlDB = sqlutils.MakeSQLRunner(sqlDB.DB)
recoveryDir := filepath.Join(localFoo, "new-backup")
recoveryDir := localFoo + "/new-backup"
sqlDB.Exec(t,
fmt.Sprintf(`BACKUP data.* TO $1 AS OF SYSTEM TIME '%s'`, beforeBadThingTs),
recoveryDir,
Expand All @@ -3111,7 +3111,7 @@ func TestPointInTimeRecovery(t *testing.T) {
// using that. Everything else works the same as above.
t.Run("recovery=inc-backup", func(t *testing.T) {
sqlDB = sqlutils.MakeSQLRunner(sqlDB.DB)
recoveryDir := filepath.Join(localFoo, "inc-backup")
recoveryDir := localFoo + "/inc-backup"
sqlDB.Exec(t,
fmt.Sprintf(`BACKUP data.* TO $1 AS OF SYSTEM TIME '%s' INCREMENTAL FROM $2, $3`, beforeBadThingTs),
recoveryDir, fullBackupDir, incBackupDir,
Expand Down Expand Up @@ -3181,7 +3181,7 @@ func TestBackupRestoreIncrementalAddTable(t *testing.T) {
defer cleanupFn()
sqlDB.Exec(t, `CREATE DATABASE data2`)
sqlDB.Exec(t, `CREATE TABLE data.t (s string PRIMARY KEY)`)
full, inc := filepath.Join(localFoo, "full"), filepath.Join(localFoo, "inc")
full, inc := localFoo+"/full", localFoo+"/inc"

sqlDB.Exec(t, `INSERT INTO data.t VALUES ('before')`)
sqlDB.Exec(t, `BACKUP data.*, data2.* TO $1`, full)
Expand All @@ -3199,7 +3199,7 @@ func TestBackupRestoreIncrementalAddTableMissing(t *testing.T) {
defer cleanupFn()
sqlDB.Exec(t, `CREATE DATABASE data2`)
sqlDB.Exec(t, `CREATE TABLE data.t (s string PRIMARY KEY)`)
full, inc := filepath.Join(localFoo, "full"), filepath.Join(localFoo, "inc")
full, inc := localFoo+"/full", localFoo+"/inc"

sqlDB.Exec(t, `INSERT INTO data.t VALUES ('before')`)
sqlDB.Exec(t, `BACKUP data.* TO $1`, full)
Expand All @@ -3219,7 +3219,7 @@ func TestBackupRestoreIncrementalTrucateTable(t *testing.T) {
_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()
sqlDB.Exec(t, `CREATE TABLE data.t (s string PRIMARY KEY)`)
full, inc := filepath.Join(localFoo, "full"), filepath.Join(localFoo, "inc")
full, inc := localFoo+"/full", localFoo+"/inc"

sqlDB.Exec(t, `INSERT INTO data.t VALUES ('before')`)
sqlDB.Exec(t, `BACKUP DATABASE data TO $1`, full)
Expand All @@ -3236,7 +3236,7 @@ func TestBackupRestoreIncrementalDropTable(t *testing.T) {
_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()
sqlDB.Exec(t, `CREATE TABLE data.t (s string PRIMARY KEY)`)
full, inc := filepath.Join(localFoo, "full"), filepath.Join(localFoo, "inc")
full, inc := localFoo+"/full", localFoo+"/inc"

sqlDB.Exec(t, `INSERT INTO data.t VALUES ('before')`)
sqlDB.Exec(t, `BACKUP DATABASE data TO $1`, full)
Expand All @@ -3263,7 +3263,7 @@ func TestFileIOLimits(t *testing.T) {
_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()

elsewhere := "nodelocal:///../../blah"
elsewhere := "nodelocal://0/../../blah"

sqlDB.Exec(t, `BACKUP data.bank TO $1`, localFoo)
sqlDB.ExpectErr(
Expand Down Expand Up @@ -3437,8 +3437,8 @@ func TestBackupRestoreShowJob(t *testing.T) {

sqlDB.Exec(t, `RESTORE data.bank FROM $1 WITH skip_missing_foreign_keys, into_db = $2`, localFoo, "data 2")
sqlDB.CheckQueryResults(t, "SELECT description FROM [SHOW JOBS] ORDER BY description", [][]string{
{"BACKUP DATABASE data TO 'nodelocal:///foo' WITH revision_history"},
{"RESTORE TABLE data.bank FROM 'nodelocal:///foo' WITH into_db = 'data 2', skip_missing_foreign_keys"},
{"BACKUP DATABASE data TO 'nodelocal://0/foo' WITH revision_history"},
{"RESTORE TABLE data.bank FROM 'nodelocal://0/foo' WITH into_db = 'data 2', skip_missing_foreign_keys"},
})
}

Expand Down Expand Up @@ -3523,8 +3523,8 @@ func TestBackupRestoreSubsetCreatedStats(t *testing.T) {
func TestBackupCreatedStatsFromIncrementalBackup(t *testing.T) {
defer leaktest.AfterTest(t)()

const incremental1Foo = "nodelocal:///incremental1foo"
const incremental2Foo = "nodelocal:///incremental2foo"
const incremental1Foo = "nodelocal://0/incremental1foo"
const incremental2Foo = "nodelocal://0/incremental2foo"
const numAccounts = 1
_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
defer cleanupFn()
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/backupccl/bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func BenchmarkClusterRestore(b *testing.B) {
b.SetBytes(backup.Desc.EntryCounts.DataSize / int64(b.N))

b.ResetTimer()
sqlDB.Exec(b, `RESTORE data.* FROM 'nodelocal:///foo'`)
sqlDB.Exec(b, `RESTORE data.* FROM 'nodelocal://0/foo'`)
b.StopTimer()
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/ccl/backupccl/full_cluster_backup_restore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestFullClusterBackup(t *testing.T) {
sqlDB.Exec(t, `ALTER DATABASE data2 CONFIGURE ZONE USING gc.ttlseconds = 900`)
// Populate system.jobs.
// Note: this is not the backup under test, this just serves as a job which should appear in the restore.
sqlDB.Exec(t, `BACKUP data.bank TO 'nodelocal:///throwawayjob'`)
sqlDB.Exec(t, `BACKUP data.bank TO 'nodelocal://0/throwawayjob'`)
preBackupJobs := sqlDB.QueryStr(t, "SELECT * FROM system.jobs")
// Populate system.settings.
sqlDB.Exec(t, `SET CLUSTER SETTING kv.bulk_io_write.concurrent_addsstable_requests = 5`)
Expand Down Expand Up @@ -190,7 +190,7 @@ func TestIncrementalFullClusterBackup(t *testing.T) {
defer leaktest.AfterTest(t)()

const numAccounts = 10
const incrementalBackupLocation = "nodelocal:///inc-full-backup"
const incrementalBackupLocation = "nodelocal://0/inc-full-backup"
_, _, sqlDB, tempDir, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
_, _, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, initNone)
defer cleanupFn()
Expand Down
Loading

0 comments on commit 954fe69

Please sign in to comment.