backup: pass URIs instead of stores to ResolveBackupManifests

First attempt to reduce ResolveBackupManifests signature and metadata loading
logic bloat.

Epic: none

Release note: none

Author: msbutler

pkg/backup/backupdest/backup_destination.go

@@ -557,8 +557,6 @@ func ResolveBackupManifests(
 	mem *mon.BoundAccount,
 	defaultCollectionURI string,
 	collectionURIs []string,
-	baseStores []cloud.ExternalStorage,
-	incStores []cloud.ExternalStorage,
 	mkStore cloud.ExternalStorageFromURIFactory,
 	resolvedSubdir string,
 	fullyResolvedBaseDirectory []string,
@@ -590,7 +588,7 @@ func ResolveBackupManifests(
 
 	if !ReadBackupIndexEnabled.Get(&execCfg.Settings.SV) || !exists || isCustomIncLocation {
 		return legacyResolveBackupManifests(
-			ctx, execCfg, mem, defaultCollectionURI, baseStores, incStores, mkStore,
+			ctx, execCfg, mem, defaultCollectionURI, mkStore,
 			resolvedSubdir, fullyResolvedBaseDirectory, fullyResolvedIncrementalsDirectory,
 			endTime, encryption, kmsEnv, user, includeSkipped, includeCompacted,
 		)
@@ -609,8 +607,6 @@ func legacyResolveBackupManifests(
 	execCfg *sql.ExecutorConfig,
 	mem *mon.BoundAccount,
 	defaultCollectionURI string,
-	baseStores []cloud.ExternalStorage,
-	incStores []cloud.ExternalStorage,
 	mkStore cloud.ExternalStorageFromURIFactory,
 	resolvedSubdir string,
 	fullyResolvedBaseDirectory []string,
@@ -637,6 +633,27 @@ func legacyResolveBackupManifests(
 			mem.Shrink(ctx, ownedMemSize)
 		}
 	}()
+
+	baseStores, baseCleanupFn, err := MakeBackupDestinationStores(ctx, user, mkStore, fullyResolvedBaseDirectory)
+	if err != nil {
+		return nil, nil, nil, 0, err
+	}
+	defer func() {
+		if err := baseCleanupFn(); err != nil {
+			log.Dev.Warningf(ctx, "failed to close base store: %+v", err)
+		}
+	}()
+
+	incStores, incCleanupFn, err := MakeBackupDestinationStores(ctx, user, mkStore, fullyResolvedIncrementalsDirectory)
+	if err != nil {
+		return nil, nil, nil, 0, err
+	}
+	defer func() {
+		if err := incCleanupFn(); err != nil {
+			log.Dev.Warningf(ctx, "failed to close inc store: %+v", err)
+		}
+	}()
+
 	baseManifest, memSize, err := backupinfo.ReadBackupManifestFromStore(ctx, mem, baseStores[0], fullyResolvedBaseDirectory[0],
 		encryption, kmsEnv)
 	if err != nil {
@@ -646,11 +663,6 @@ func legacyResolveBackupManifests(
 
 	var incrementalBackups []string
 	if len(incStores) > 0 {
-		rootStore, err := mkStore(ctx, defaultCollectionURI, user)
-		if err != nil {
-			return nil, nil, nil, 0, err
-		}
-		defer rootStore.Close()
 		incrementalBackups, err = LegacyFindPriorBackups(ctx, incStores[0], includeManifest)
 		if err != nil {
 			return nil, nil, nil, 0, err
@@ -771,23 +783,15 @@ func indexedResolveBackupManifests(
 	ctx, sp := tracing.ChildSpan(ctx, "backupdest.ResolveBackupManifestsWithIndexes")
 	defer sp.Finish()
 
-	rootStores := make([]cloud.ExternalStorage, 0, len(collectionURIs))
+	rootStores, rootCleanupFn, err := MakeBackupDestinationStores(ctx, user, mkStore, collectionURIs)
+	if err != nil {
+		return nil, nil, nil, 0, err
+	}
 	defer func() {
-		for _, store := range rootStores {
-			if store != nil {
-				if err := store.Close(); err != nil {
-					log.Dev.Errorf(ctx, "error closing store: %v", err)
-				}
-			}
+		if err := rootCleanupFn(); err != nil {
+			log.Dev.Warningf(ctx, "failed to close collection store: %s", err)
 		}
 	}()
-	for _, uri := range collectionURIs {
-		store, err := mkStore(ctx, uri, user)
-		if err != nil {
-			return nil, nil, nil, 0, err
-		}
-		rootStores = append(rootStores, store)
-	}
 
 	indexes, err := GetBackupTreeIndexMetadata(
 		ctx, rootStores[0], resolvedSubdir,

pkg/backup/backupdest/backup_destination_test.go

@@ -396,31 +396,13 @@ func TestResolveBackupManifests(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	baseStores, err := util.MapE(fullyResolvedBaseDirs, func(uri string) (cloud.ExternalStorage, error) {
-		return execCfg.DistSQLSrv.ExternalStorageFromURI(ctx, uri, username.RootUserName())
-	})
-	require.NoError(t, err)
-	for i := range baseStores {
-		defer baseStores[i].Close()
-	}
-
-	incStores, err := util.MapE(fullyResolvedIncDirs, func(uri string) (cloud.ExternalStorage, error) {
-		return execCfg.DistSQLSrv.ExternalStorageFromURI(ctx, uri, username.RootUserName())
-	})
-	require.NoError(t, err)
-	for i := range incStores {
-		defer incStores[i].Close()
-	}
-
 	t.Run("resolve backup manifests with latest AOST", func(t *testing.T) {
 		uris, manifests, locality, memSize, err := ResolveBackupManifests(
 			ctx,
 			&execCfg,
 			&mem,
 			collections[0],
 			collections,
-			baseStores,
-			incStores,
 			execCfg.DistSQLSrv.ExternalStorageFromURI,
 			fullSubdir,
 			fullyResolvedBaseDirs,
@@ -448,8 +430,6 @@ func TestResolveBackupManifests(t *testing.T) {
 			&mem,
 			collections[0],
 			collections,
-			baseStores,
-			incStores,
 			execCfg.DistSQLSrv.ExternalStorageFromURI,
 			fullSubdir,
 			fullyResolvedBaseDirs,

pkg/backup/compaction_job.go

@@ -745,17 +745,6 @@ func getBackupChain(
 			log.Dev.Warningf(ctx, "failed to cleanup base backup stores: %+v", err)
 		}
 	}()
-	incStores, incCleanup, err := backupdest.MakeBackupDestinationStores(
-		ctx, user, mkStore, resolvedIncDirs,
-	)
-	if err != nil {
-		return nil, nil, nil, nil, err
-	}
-	defer func() {
-		if err := incCleanup(); err != nil {
-			log.Dev.Warningf(ctx, "failed to cleanup incremental backup stores: %+v", err)
-		}
-	}()
 	baseEncryptionInfo := encryptionOpts
 	if encryptionOpts != nil && !encryptionOpts.HasKey() {
 		baseEncryptionInfo, err = backupencryption.GetEncryptionFromBaseStore(
@@ -770,7 +759,7 @@ func getBackupChain(
 	defer mem.Close(ctx)
 
 	_, manifests, localityInfo, memReserved, err := backupdest.ResolveBackupManifests(
-		ctx, execCfg, &mem, defaultCollectionURI, dest.To, baseStores, incStores, mkStore, resolvedSubdir,
+		ctx, execCfg, &mem, defaultCollectionURI, dest.To, mkStore, resolvedSubdir,
 		resolvedBaseDirs, resolvedIncDirs, endTime, baseEncryptionInfo, kmsEnv,
 		user, false /*includeSkipped */, true /*includeCompacted */, len(dest.IncrementalStorage) > 0,
 	)

pkg/backup/restore_planning.go

@@ -1738,18 +1738,7 @@ func doRestorePlan(
 	}
 	defer func() {
 		if err := cleanupFn(); err != nil {
-			log.Dev.Warningf(ctx, "failed to close incremental store: %+v", err)
-		}
-	}()
-
-	incStores, cleanupFn, err := backupdest.MakeBackupDestinationStores(ctx, p.User(), mkStore,
-		fullyResolvedIncrementalsDirectory)
-	if err != nil {
-		return err
-	}
-	defer func() {
-		if err := cleanupFn(); err != nil {
-			log.Dev.Warningf(ctx, "failed to close incremental store: %+v", err)
+			log.Dev.Warningf(ctx, "failed to close base store: %+v", err)
 		}
 	}()
 
@@ -1805,7 +1794,7 @@ func doRestorePlan(
 	// directories, return the URIs and manifests of all backup layers in all
 	// localities. Incrementals will be searched for automatically.
 	defaultURIs, mainBackupManifests, localityInfo, memReserved, err := backupdest.ResolveBackupManifests(
-		ctx, p.ExecCfg(), &mem, defaultCollectionURI, from, baseStores, incStores, mkStore,
+		ctx, p.ExecCfg(), &mem, defaultCollectionURI, from, mkStore,
 		fullyResolvedSubdir, fullyResolvedBaseDirectory, fullyResolvedIncrementalsDirectory, endTime,
 		encryption, &kmsEnv, p.User(), false, includeCompacted, len(incFrom) > 0,
 	)

pkg/backup/show.go

@@ -346,20 +346,10 @@ you must pass the 'encryption_info_dir' parameter that points to the directory o
 		info.enc = encryption
 
 		mkStore := p.ExecCfg().DistSQLSrv.ExternalStorageFromURI
-		incStores, cleanupFn, err := backupdest.MakeBackupDestinationStores(ctx, p.User(), mkStore,
-			fullyResolvedIncrementalsDirectory)
-		if err != nil {
-			return err
-		}
-		defer func() {
-			if err := cleanupFn(); err != nil {
-				log.Dev.Warningf(ctx, "failed to close incremental store: %+v", err)
-			}
-		}()
 
 		info.defaultURIs, info.manifests, info.localityInfo, memReserved,
 			err = backupdest.ResolveBackupManifests(
-			ctx, p.ExecCfg(), &mem, defaultCollectionURI, dest, baseStores, incStores, mkStore, subdir,
+			ctx, p.ExecCfg(), &mem, defaultCollectionURI, dest, mkStore, subdir,
 			fullyResolvedDest, fullyResolvedIncrementalsDirectory, hlc.Timestamp{},
 			encryption, &kmsEnv, p.User(), true /* includeSkipped */, true, /* includeCompacted */
 			len(explicitIncPaths) > 0,