Merge #152996

152996: backup: add generics to ValidateEndTimeAndTruncate r=msbutler a=kev-cao

This patch updates ValidateEndTimeAndTruncate to use generics instead of the zipped BackupEntry. This allows us to pass in anything that conforms to the `ManifestLike` interface, i.e. `BackupIndexMetadata` so that we can run the validation and truncation logic on other types.

Epic: CRDB-47942

Release note: None

Co-authored-by: Kevin Cao <39608887+kev-cao@users.noreply.github.com>

Author: craig[bot]

pkg/backup/backupdest/backup_destination.go

@@ -688,12 +688,18 @@ func ResolveBackupManifests(
 
 	totalMemSize := ownedMemSize
 	ownedMemSize = 0
-	validatedDefaultURIs, validatedMainBackupManifests, validatedLocalityInfo, err := backupinfo.ValidateEndTimeAndTruncate(
-		defaultURIs, mainBackupManifests, localityInfo, endTime, includeSkipped, includeCompacted,
+	manifestEntries, err := backupinfo.ZipBackupTreeEntries(
+		defaultURIs, mainBackupManifests, localityInfo,
+	)
+	if err != nil {
+		return nil, nil, nil, 0, err
+	}
+	validatedEntries, err := backupinfo.ValidateEndTimeAndTruncate(
+		manifestEntries, endTime, includeSkipped, includeCompacted,
 	)
-
 	if err != nil {
 		return nil, nil, nil, 0, err
 	}
-	return validatedDefaultURIs, validatedMainBackupManifests, validatedLocalityInfo, totalMemSize, nil
+	uris, manifests, localityInfo := backupinfo.UnzipBackupTreeEntries(validatedEntries)
+	return uris, manifests, localityInfo, totalMemSize, nil
 }

pkg/backup/backupinfo/manifest_handling.go

@@ -875,6 +875,7 @@ func GetLocalityInfo(
 	return info, nil
 }
 
+// TODO (kev-cao): Remove in v26.2
 type BackupTreeEntry struct {
 	// The URI to the backup manifest.
 	Uri string
@@ -884,11 +885,45 @@ type BackupTreeEntry struct {
 	LocalityInfo jobspb.RestoreDetails_BackupLocalityInfo
 }
 
-// zipBackupTreeEntries zips the default URIs, main backup manifests, and
+func (b BackupTreeEntry) Start() hlc.Timestamp {
+	return b.Manifest.StartTime
+}
+
+func (b BackupTreeEntry) End() hlc.Timestamp {
+	return b.Manifest.EndTime
+}
+
+func (b BackupTreeEntry) Compacted() bool {
+	return b.Manifest.IsCompacted
+}
+
+func (b BackupTreeEntry) MVCC() backuppb.MVCCFilter {
+	return b.Manifest.MVCCFilter
+}
+
+func (b BackupTreeEntry) RevisionStart() hlc.Timestamp {
+	return b.Manifest.RevisionStartTime
+}
+
+// This interface is used as a stop-gap during the transitional period where
+// some restorable backups will not have backup indexes and some will.
+// Afterwards, we can remove this interface and replace instances of
+// ManifestLike with BackupIndexMetadata.
+// TODO (kev-cao): Remove in v26.2
+type ManifestLike interface {
+	Start() hlc.Timestamp
+	End() hlc.Timestamp
+	Compacted() bool
+	MVCC() backuppb.MVCCFilter
+	RevisionStart() hlc.Timestamp
+}
+
+// ZipBackupTreeEntries zips the default URIs, main backup manifests, and
 // locality information into a slice of backupChainEntry structs. It assumes
 // that the passed in slices are of the same length and that each entry at the
 // same index in each slice corresponds to the same backup.
-func zipBackupTreeEntries(
+// TODO (kev-cao): Remove in v26.2
+func ZipBackupTreeEntries(
 	defaultURIs []string,
 	mainBackupManifests []backuppb.BackupManifest,
 	localityInfo []jobspb.RestoreDetails_BackupLocalityInfo,
@@ -912,10 +947,11 @@ func zipBackupTreeEntries(
 	return entries, nil
 }
 
-// unzipBackupTreeEntries unzips a slice of backupChainEntry structs into
+// UnzipBackupTreeEntries unzips a slice of backupChainEntry structs into
 // slices of their constituent parts: default URIs, main backup manifests, and
 // locality info and returns the individual slices.
-func unzipBackupTreeEntries(
+// TODO (kev-cao): Remove in v26.2
+func UnzipBackupTreeEntries(
 	entries []BackupTreeEntry,
 ) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo) {
 	defaultURIs := make([]string, len(entries))
@@ -935,85 +971,70 @@ func unzipBackupTreeEntries(
 // The method also performs additional sanity checks to ensure the backups cover
 // the requested time.
 //
-// TODO (kev-cao): Refactor this function to accept/return the BackupTreeEntry
-// type and update surrounding caller logic to use it. This will allow us to
-// refactor out the zipping and unzipping logic.
-func ValidateEndTimeAndTruncate(
-	defaultURIs []string,
-	mainBackupManifests []backuppb.BackupManifest,
-	localityInfo []jobspb.RestoreDetails_BackupLocalityInfo,
-	endTime hlc.Timestamp,
-	includeSkipped bool,
-	includeCompacted bool,
-) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo, error) {
-	backupEntries, err := zipBackupTreeEntries(defaultURIs, mainBackupManifests, localityInfo)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-
+// Note: This function assumes that the manifests in the chain are sorted by end
+// time in ascending order and ties are broken by start time in ascending order.
+func ValidateEndTimeAndTruncate[E ManifestLike](
+	manifests []E, endTime hlc.Timestamp, includeSkipped bool, includeCompacted bool,
+) ([]E, error) {
 	if !includeCompacted {
-		backupEntries = skipCompactedBackups(backupEntries)
+		manifests = skipCompactedBackups(manifests)
 	}
 
 	if endTime.IsEmpty() {
 		if includeSkipped {
-			uris, manifests, locality := unzipBackupTreeEntries(backupEntries)
-			return uris, manifests, locality, nil
+			return manifests, nil
 		}
-		backupEntries = elideSkippedLayers(backupEntries)
-		if err := validateContinuity(backupEntries); err != nil {
-			return nil, nil, nil, err
+		manifests = elideSkippedLayers(manifests)
+		if err := validateContinuity(manifests); err != nil {
+			return nil, err
 		}
-		uris, manifests, locality := unzipBackupTreeEntries(backupEntries)
-		return uris, manifests, locality, nil
+		return manifests, nil
 	}
-	for i, b := range backupEntries {
+	for i, b := range manifests {
 		// Find the backup that covers the requested time.
-		if !(b.Manifest.StartTime.Less(endTime) && endTime.LessEq(b.Manifest.EndTime)) {
+		if !(b.Start().Less(endTime) && endTime.LessEq(b.End())) {
 			continue
 		}
 
 		// Ensure that the backup actually has revision history.
-		if !endTime.Equal(b.Manifest.EndTime) {
-			if b.Manifest.MVCCFilter != backuppb.MVCCFilter_All {
+		if !endTime.Equal(b.End()) {
+			if b.MVCC() != backuppb.MVCCFilter_All {
 				const errPrefix = "invalid RESTORE timestamp: restoring to arbitrary time requires that BACKUP for requested time be created with 'revision_history' option."
 				if i == 0 {
-					return nil, nil, nil, errors.Errorf(
+					return nil, errors.Errorf(
 						errPrefix+" nearest backup time is %s",
-						timeutil.Unix(0, b.Manifest.EndTime.WallTime).UTC(),
+						timeutil.Unix(0, b.End().WallTime).UTC(),
 					)
 				}
-				return nil, nil, nil, errors.Errorf(
+				return nil, errors.Errorf(
 					errPrefix+" nearest BACKUP times are %s or %s",
-					timeutil.Unix(0, mainBackupManifests[i-1].EndTime.WallTime).UTC(),
-					timeutil.Unix(0, b.Manifest.EndTime.WallTime).UTC(),
+					timeutil.Unix(0, manifests[i-1].End().WallTime).UTC(),
+					timeutil.Unix(0, b.End().WallTime).UTC(),
 				)
 			}
 			// Ensure that the revision history actually covers the requested time -
 			// while the BACKUP's start and end might contain the requested time for
 			// example if start time is 0 (full backup), the revision history was
 			// only captured since the GC window. Note that the RevisionStartTime is
 			// the latest for ranges backed up.
-			if endTime.LessEq(b.Manifest.RevisionStartTime) {
-				return nil, nil, nil, errors.Errorf(
+			if endTime.LessEq(b.RevisionStart()) {
+				return nil, errors.Errorf(
 					"invalid RESTORE timestamp: BACKUP for requested time only has revision history"+
-						" from %v", timeutil.Unix(0, b.Manifest.RevisionStartTime.WallTime).UTC(),
+						" from %v", timeutil.Unix(0, b.RevisionStart().WallTime).UTC(),
 				)
 			}
 		}
 		if includeSkipped {
-			uris, manifests, locality := unzipBackupTreeEntries(backupEntries[:i+1])
-			return uris, manifests, locality, nil
+			return manifests[:i+1], nil
 		}
-		backupEntries = elideSkippedLayers(backupEntries[:i+1])
-		if err := validateContinuity(backupEntries); err != nil {
-			return nil, nil, nil, err
+		manifests = elideSkippedLayers(manifests[:i+1])
+		if err := validateContinuity(manifests); err != nil {
+			return nil, err
 		}
-		uris, manifests, locality := unzipBackupTreeEntries(backupEntries)
-		return uris, manifests, locality, nil
+		return manifests, nil
 	}
 
-	return nil, nil, nil, errors.Errorf(
+	return nil, errors.Errorf(
 		"invalid RESTORE timestamp: supplied backups do not cover requested time",
 	)
 }
@@ -1022,26 +1043,30 @@ func ValidateEndTimeAndTruncate(
 // backups and returns the updated list backup entries.
 //
 // NOTE: This function modifies the underlying memory of the slices passed in.
-func skipCompactedBackups(backupEntries []BackupTreeEntry) []BackupTreeEntry {
-	for i := len(backupEntries) - 1; i >= 0; i-- {
-		if backupEntries[i].Manifest.IsCompacted {
-			backupEntries = slices.Delete(backupEntries, i, i+1)
+func skipCompactedBackups[E ManifestLike](manifests []E) []E {
+	for i := len(manifests) - 1; i >= 0; i-- {
+		if manifests[i].Compacted() {
+			manifests = slices.Delete(manifests, i, i+1)
 		}
 	}
-	return backupEntries
+	return manifests
 }
 
 // validateContinuity checks that the backups are continuous.
-func validateContinuity(backupEntries []BackupTreeEntry) error {
-	if len(backupEntries) == 0 {
+//
+// Note: This fucntion assumes the backups are sorted by end time in ascending
+// order.
+func validateContinuity[E ManifestLike](manifests []E) error {
+	if len(manifests) == 0 {
 		return errors.AssertionFailedf("an empty chain of backups cannot cover an end time")
 	}
-	for i := range len(backupEntries) - 1 {
-		if !backupEntries[i].Manifest.EndTime.Equal(backupEntries[i+1].Manifest.StartTime) {
+	for i := range len(manifests) - 1 {
+		end := manifests[i].End()
+		if !end.Equal(manifests[i+1].Start()) {
 			return errors.AssertionFailedf(
 				"backups are not continuous: %dth backup ends at %+v, %dth backup starts at %+v",
-				i, backupEntries[i].Manifest.EndTime,
-				i+1, backupEntries[i+1].Manifest.StartTime,
+				i, manifests[i].End(),
+				i+1, manifests[i+1].Start(),
 			)
 		}
 	}
@@ -1051,49 +1076,53 @@ func validateContinuity(backupEntries []BackupTreeEntry) error {
 // elideSkippedLayers removes backups that are skipped in the backup chain and
 // ensures only backups that will be used in the restore are returned.
 //
-// Note: This assumes that the provided backups are sorted in increasing order
-// by end time, and then sorted in increasing order by start time to break ties.
-func elideSkippedLayers(backupEntries []BackupTreeEntry) []BackupTreeEntry {
-	backupEntries = elideDuplicateEndTimes(backupEntries)
-	i := len(backupEntries) - 1
+// Note: This function assumes that the manifests in the chain are sorted by end
+// time in ascending order and ties are broken by start time in ascending order.
+func elideSkippedLayers[E ManifestLike](manifests []E) []E {
+	manifests = elideDuplicateEndTimes(manifests)
+	i := len(manifests) - 1
 	for i > 0 {
 		// Find j such that backups[j] is parent of backups[i].
 		j := i - 1
-		for j >= 0 && !backupEntries[i].Manifest.StartTime.Equal(backupEntries[j].Manifest.EndTime) {
+		start := manifests[i].Start()
+		for j >= 0 && !start.Equal(manifests[j].End()) {
 			j--
 		}
 		// If there are backups between i and j, remove them.
 		// If j is less than 0, then no parent was found so nothing to skip.
 		if j != i-1 && j >= 0 {
-			backupEntries = slices.Delete(backupEntries, j+1, i)
+			manifests = slices.Delete(manifests, j+1, i)
 		}
 		// Move up to check the chain from j now.
 		i = j
 	}
-	return backupEntries
+	return manifests
 }
 
 // elideDuplicateEndTimes ensures that backups in a list of backups are
 // functionally unique by removing any duplicates that have the same end time,
 // choosing backups with earlier start times and eliding the rest.
 //
-// Note: This assumes that the provided backups are sorted in increasing order
-// by end time, and then sorted in increasing order by start time to break ties.
-func elideDuplicateEndTimes(backupEntries []BackupTreeEntry) []BackupTreeEntry {
-	for i := range len(backupEntries) - 1 {
+// Note: This function assumes that the manifests in the chain are sorted by end
+// time in ascending order and ties are broken by start time in ascending order.
+func elideDuplicateEndTimes[E ManifestLike](manifests []E) []E {
+	for i := range len(manifests) - 1 {
+		if i >= len(manifests) {
+			break
+		}
 		j := i + 1
 		// Find j such that backups[j] no longer shares the same end time as
 		// backups[i].
-		for j < len(backupEntries) &&
-			backupEntries[i].Manifest.EndTime.Equal(backupEntries[j].Manifest.EndTime) {
+		end := manifests[i].End()
+		for j < len(manifests) && end.Equal(manifests[j].End()) {
 			j++
 		}
 		// If there exists backups between i and j, remove them.
 		if j > i+1 {
-			backupEntries = slices.Delete(backupEntries, i+1, j)
+			manifests = slices.Delete(manifests, i+1, j)
 		}
 	}
-	return backupEntries
+	return manifests
 }
 
 // GetBackupIndexAtTime returns the index of the latest backup in

pkg/backup/backupinfo/manifest_handling_test.go

@@ -576,8 +576,10 @@ func TestValidateEndTimeAndTruncate(t *testing.T) {
 				inputLocs[i].URIsByOriginalLocalityKV[index] = index
 			}
 
-			uris, res, locs, err := backupinfo.ValidateEndTimeAndTruncate(
-				inputURIs, tc.manifests, inputLocs,
+			manifestEntries, err := backupinfo.ZipBackupTreeEntries(inputURIs, tc.manifests, inputLocs)
+			require.NoError(t, err)
+			manifestEntries, err = backupinfo.ValidateEndTimeAndTruncate(
+				manifestEntries,
 				hlc.Timestamp{WallTime: int64(tc.endTime)},
 				false, /* includeSkipped */
 				tc.includeCompacted,
@@ -586,6 +588,7 @@ func TestValidateEndTimeAndTruncate(t *testing.T) {
 				require.ErrorContains(t, err, tc.err)
 				return
 			}
+			uris, res, locs := backupinfo.UnzipBackupTreeEntries(manifestEntries)
 			require.Equal(t, len(tc.expected), len(res))
 			require.Equal(t, expectedOrder, uris)
 			require.Len(t, locs, len(tc.expected))