Merge #153663

153663: backup: delete handling of pre 22.1 legacy checkpoint files r=jeffswenson a=msbutler

Epic: none

Release note: none

Co-authored-by: Michael Butler <butler@cockroachlabs.com>

Author: craig[bot]

pkg/backup/backup_job.go

@@ -1809,33 +1809,9 @@ func (b *backupResumer) readManifestOnResume(
 	// they could be using either the new or the old foreign key
 	// representations. We should just preserve whatever representation the
 	// table descriptors were using and leave them alone.
-	desc, memSize, err := backupinfo.ReadBackupCheckpointManifest(ctx, mem, defaultStore,
-		backupinfo.BackupManifestCheckpointName, details.EncryptionOptions, kmsEnv)
-	if err != nil {
-		if !errors.Is(err, cloud.ErrFileDoesNotExist) {
-			return nil, 0, errors.Wrapf(err, "reading backup checkpoint")
-		}
-		// Try reading temp checkpoint.
-		tmpCheckpoint := backupinfo.TempCheckpointFileNameForJob(b.job.ID())
-		desc, memSize, err = backupinfo.ReadBackupCheckpointManifest(ctx, mem, defaultStore,
-			tmpCheckpoint, details.EncryptionOptions, kmsEnv)
-		if err != nil {
-			return nil, 0, err
-		}
-		// "Rename" temp checkpoint.
-		if err := backupinfo.WriteBackupManifestCheckpoint(
-			ctx, details.URI, details.EncryptionOptions, kmsEnv, &desc, cfg, user,
-		); err != nil {
-			mem.Shrink(ctx, memSize)
-			return nil, 0, errors.Wrapf(err, "renaming temp checkpoint file")
-		}
-		// Best effort remove temp checkpoint.
-		if err := defaultStore.Delete(ctx, tmpCheckpoint); err != nil {
-			log.Dev.Errorf(ctx, "error removing temporary checkpoint %s", tmpCheckpoint)
-		}
-		if err := defaultStore.Delete(ctx, backupinfo.BackupProgressDirectory+"/"+tmpCheckpoint); err != nil {
-			log.Dev.Errorf(ctx, "error removing temporary checkpoint %s", backupinfo.BackupProgressDirectory+"/"+tmpCheckpoint)
-		}
+	desc, memSize, err := backupinfo.ReadBackupCheckpointManifest(ctx, mem, defaultStore, details.EncryptionOptions, kmsEnv)
+	if err != nil && !errors.Is(err, cloud.ErrFileDoesNotExist) {
+		return nil, 0, errors.Wrapf(err, "reading backup checkpoint")
 	}
 
 	if !desc.ClusterID.Equal(cfg.NodeInfo.LogicalClusterID()) {
@@ -1973,16 +1949,6 @@ func (b *backupResumer) deleteCheckpoint(
 			return err
 		}
 		defer exportStore.Close()
-		// We first attempt to delete from base directory to account for older
-		// backups, and then from the progress directory.
-		err = exportStore.Delete(ctx, backupinfo.BackupManifestCheckpointName)
-		if err != nil {
-			log.Dev.Warningf(ctx, "unable to delete checkpointed backup descriptor file in base directory: %+v", err)
-		}
-		err = exportStore.Delete(ctx, backupinfo.BackupManifestCheckpointName+backupinfo.BackupManifestChecksumSuffix)
-		if err != nil {
-			log.Dev.Warningf(ctx, "unable to delete checkpoint checksum file in base directory: %+v", err)
-		}
 		// Delete will not delete a nonempty directory, so we have to go through
 		// all files and delete each file one by one.
 		return exportStore.List(ctx, backupinfo.BackupProgressDirectory, "", func(p string) error {

pkg/backup/backup_test.go

@@ -1739,7 +1739,6 @@ func TestBackupRestoreResume(t *testing.T) {
 			checkpointDirectory string
 		}{
 			{testName: "backup-progress-directory", checkpointDirectory: "/" + backupinfo.BackupProgressDirectory},
-			{testName: "backup-base-directory", checkpointDirectory: ""},
 		} {
 			item := item
 			t.Run(item.testName, func(t *testing.T) {

pkg/backup/backupinfo/manifest_handling.go

@@ -197,26 +197,27 @@ func DecompressData(ctx context.Context, mem *mon.BoundAccount, descBytes []byte
 
 // ReadBackupCheckpointManifest reads and unmarshals a BACKUP-CHECKPOINT
 // manifest from filename in the provided export store.
+//
+// NB: a checkpoint file should always get written before this is called.
 func ReadBackupCheckpointManifest(
 	ctx context.Context,
 	mem *mon.BoundAccount,
 	exportStore cloud.ExternalStorage,
-	filename string,
 	encryption *jobspb.BackupEncryptionOptions,
 	kmsEnv cloud.KMSEnv,
 ) (backuppb.BackupManifest, int64, error) {
 	ctx, sp := tracing.ChildSpan(ctx, "backupinfo.ReadBackupCheckpointManifest")
 	defer sp.Finish()
 
-	checkpointFile, err := readLatestCheckpointFile(ctx, exportStore, filename)
+	checkpointFile, err := readLatestCheckpointFile(ctx, exportStore, BackupManifestCheckpointName)
 	if err != nil {
 		return backuppb.BackupManifest{}, 0, err
 	}
 	defer checkpointFile.Close(ctx)
 
 	// Look for a checksum, if one is not found it could be an older backup,
 	// but we want to continue anyway.
-	checksumFile, err := readLatestCheckpointFile(ctx, exportStore, filename+BackupManifestChecksumSuffix)
+	checksumFile, err := readLatestCheckpointFile(ctx, exportStore, BackupManifestCheckpointName+BackupManifestChecksumSuffix)
 	if err != nil {
 		if !errors.Is(err, cloud.ErrFileDoesNotExist) {
 			return backuppb.BackupManifest{}, 0, err
@@ -1278,17 +1279,6 @@ func CheckForBackupLock(
 // 2) BACKUP-LOCK: Written by the coordinator node to lay claim on a backup
 // location. This file is suffixed with the ID of the backup job to prevent a
 // node from reading its own lock file on job resumption.
-//
-// 3) BACKUP-CHECKPOINT: Prior to 22.1.1, nodes would use the BACKUP-CHECKPOINT
-// to lay claim on a backup location. To account for a mixed-version cluster
-// where an older coordinator node may be running a concurrent backup to the
-// same location, we must continue to check for a BACKUP-CHECKPOINT file.
-//
-// NB: The node will continue to write a BACKUP-CHECKPOINT file later in its
-// execution, but we do not have to worry about reading our own
-// BACKUP-CHECKPOINT file (and locking ourselves out) since
-// `checkForPreviousBackup` is invoked as the first step on job resumption, and
-// is not called again.
 func CheckForPreviousBackup(
 	ctx context.Context,
 	execCfg *sql.ExecutorConfig,
@@ -1343,35 +1333,9 @@ func CheckForPreviousBackup(
 		}
 		log.Dev.Warningf(ctx, "external storage %s does not support listing: skip checking for BACKUP_LOCK", redactedURI)
 	}
-
-	// Check for a BACKUP-CHECKPOINT that might have been written by a node
-	// running a pre-22.1.1 binary.
-	//
-	// TODO(adityamaru): Delete in 23.1 since we will no longer need to check for
-	// BACKUP-CHECKPOINT files as all backups will rely on BACKUP-LOCK to lock a
-	// location.
-	r, err = readLatestCheckpointFile(ctx, defaultStore, BackupManifestCheckpointName)
-	if err == nil {
-		r.Close(ctx)
-		return pgerror.Newf(pgcode.FileAlreadyExists,
-			"%s already contains a %s file (is another operation already in progress?)",
-			redactedURI, BackupManifestCheckpointName)
-	}
-
-	if !errors.Is(err, cloud.ErrFileDoesNotExist) {
-		return errors.Wrapf(err,
-			"%s returned an unexpected error when checking for the existence of %s file",
-			redactedURI, BackupManifestCheckpointName)
-	}
-
 	return nil
 }
 
-// TempCheckpointFileNameForJob returns temporary filename for backup manifest checkpoint.
-func TempCheckpointFileNameForJob(jobID jobspb.JobID) string {
-	return fmt.Sprintf("%s-%d", BackupManifestCheckpointName, jobID)
-}
-
 // BackupManifestDescriptors returns the descriptors encoded in the manifest as
 // a slice of mutable descriptors.
 func BackupManifestDescriptors(
@@ -1440,11 +1404,8 @@ func newDescriptorBuilder(
 	return nil
 }
 
-// WriteBackupManifestCheckpoint writes a new BACKUP-CHECKPOINT MANIFEST and
-// CHECKSUM file. If it is a pure v22.1 cluster or later, it will write a
-// timestamped BACKUP-CHECKPOINT to the /progress directory. If it is a mixed
-// cluster version, it will write a non timestamped BACKUP-CHECKPOINT to the
-// base directory in order to not break backup jobs that resume on a v21.2 node.
+// WriteBackupManifestCheckpoint writes a BACKUP-CHECKPOINT file and CHECKSUM
+// file.
 func WriteBackupManifestCheckpoint(
 	ctx context.Context,
 	storageURI string,
@@ -1545,12 +1506,9 @@ func readLatestCheckpointFile(
 	ctx context.Context, exportStore cloud.ExternalStorage, filename string,
 ) (ioctx.ReadCloserCtx, error) {
 	filename = strings.TrimPrefix(filename, "/")
-	// First try reading from the progress directory. If the backup is from
-	// an older version, it may not exist there yet so try reading
-	// in the base directory if the first attempt fails.
+
 	var checkpoint string
 	var checkpointFound bool
-	var r ioctx.ReadCloserCtx
 	var err error
 
 	// We name files such that the most recent checkpoint will always
@@ -1571,7 +1529,7 @@ func readLatestCheckpointFile(
 	// directly. This can still fail if it is a mixed cluster and the
 	// checkpoint was written in the base directory.
 	if errors.Is(err, cloud.ErrListingUnsupported) {
-		r, _, err = exportStore.ReadFile(ctx, BackupProgressDirectory+"/"+filename, cloud.ReadOptions{NoFileSize: true})
+		r, _, err := exportStore.ReadFile(ctx, BackupProgressDirectory+"/"+filename, cloud.ReadOptions{NoFileSize: true})
 		// If we found the checkpoint in progress, then don't bother reading
 		// from base, just return the reader.
 		if err == nil {
@@ -1581,26 +1539,18 @@ func readLatestCheckpointFile(
 		return nil, err
 	}
 
-	if checkpointFound {
-		var name string
-		if strings.HasSuffix(filename, BackupManifestChecksumSuffix) {
-			name = BackupProgressDirectory + "/" + checkpoint + BackupManifestChecksumSuffix
-		} else {
-			name = BackupProgressDirectory + "/" + checkpoint
-		}
-		r, _, err = exportStore.ReadFile(ctx, name, cloud.ReadOptions{NoFileSize: true})
-		return r, err
+	if !checkpointFound {
+		return nil, errors.Newf("latest file %s not found", filename)
 	}
 
-	// If the checkpoint wasn't found in the progress directory, then try
-	// reading from the base directory instead.
-	r, _, err = exportStore.ReadFile(ctx, filename, cloud.ReadOptions{NoFileSize: true})
-
-	if err != nil {
-		return nil, errors.Wrapf(err, "%s could not be read in the base or progress directory", filename)
+	var name string
+	if strings.HasSuffix(filename, BackupManifestChecksumSuffix) {
+		name = BackupProgressDirectory + "/" + checkpoint + BackupManifestChecksumSuffix
+	} else {
+		name = BackupProgressDirectory + "/" + checkpoint
 	}
-	return r, nil
-
+	r, _, err := exportStore.ReadFile(ctx, name, cloud.ReadOptions{NoFileSize: true})
+	return r, err
 }
 
 // NewTimestampedCheckpointFileName returns a string of a new checkpoint filename