diff --git a/pkg/backup/backup_job.go b/pkg/backup/backup_job.go index 567ce271ead1..b1e3dca8f944 100644 --- a/pkg/backup/backup_job.go +++ b/pkg/backup/backup_job.go @@ -51,6 +51,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/stats" + "github.com/cockroachdb/cockroach/pkg/util/besteffort" bulkutil "github.com/cockroachdb/cockroach/pkg/util/bulk" "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" @@ -1946,6 +1947,10 @@ func (b *backupResumer) processScheduledBackupCompletion( return nil } +// compactionBackupLockCleanupOp is the besteffort operation name used when +// removing the BACKUP-LOCK file left behind by a failed compaction job. +const compactionBackupLockCleanupOp = "delete-compaction-backup-lock" + // OnFailOrCancel is part of the jobs.Resumer interface. func (b *backupResumer) OnFailOrCancel( ctx context.Context, execCtx interface{}, jobErr error, @@ -1960,6 +1965,20 @@ func (b *backupResumer) OnFailOrCancel( details := b.job.Details().(jobspb.BackupDetails) b.deleteCheckpoint(ctx, cfg, p.User()) + + // For compaction jobs, clean up the BACKUP-LOCK file from the backup + // destination to unblock subsequent compaction attempts that may target + // the same location. A failed compaction does not produce a valid backup at + // the destination, so the lock serves no purpose and only blocks future + // compactions. + if details.Compact && details.URI != "" { + besteffort.Warning(ctx, compactionBackupLockCleanupOp, func(ctx context.Context) error { + return backupinfo.DeleteBackupLock( + ctx, cfg, details.URI, b.job.ID(), p.User(), + ) + }) + } + if err := cfg.InternalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error { pts := cfg.ProtectedTimestampProvider.WithTxn(txn) return releaseProtectedTimestamp(ctx, pts, details.ProtectedTimestampRecord) diff --git a/pkg/backup/backupinfo/manifest_handling.go b/pkg/backup/backupinfo/manifest_handling.go index 7f15be42d855..29c84bf6755d 100644 --- a/pkg/backup/backupinfo/manifest_handling.go +++ b/pkg/backup/backupinfo/manifest_handling.go @@ -541,6 +541,37 @@ func WriteBackupLock( return cloud.WriteFile(ctx, defaultStore, lockFileName, bytes.NewReader([]byte("lock"))) } +// DeleteBackupLock removes the backup lock file for the given jobID from the +// default backup destination. This is used to clean up lock files from failed +// compaction jobs so that subsequent compaction attempts to the same destination +// are not blocked. +func DeleteBackupLock( + ctx context.Context, + execCfg *sql.ExecutorConfig, + defaultURI string, + jobID jobspb.JobID, + user username.SQLUsername, +) error { + ctx, sp := tracing.ChildSpan(ctx, "backupinfo.DeleteBackupLock") + defer sp.Finish() + + defaultStore, err := execCfg.DistSQLSrv.ExternalStorageFromURI(ctx, defaultURI, user) + if err != nil { + return err + } + defer defaultStore.Close() + + lockFileName := fmt.Sprintf("%s%s", BackupLockFilePrefix, strconv.FormatInt(int64(jobID), 10)) + if err := defaultStore.Delete(ctx, lockFileName); err != nil { + // If the lock file does not exist, there is nothing to clean up. + if errors.Is(err, cloud.ErrFileDoesNotExist) { + return nil + } + return err + } + return nil +} + // WriteMetadataWithExternalSSTs writes a "slim" version of manifest to // `exportStore`. This version has the alloc heavy `Files`, `Descriptors`, and // `DescriptorChanges` repeated fields nil'ed out, and written to an diff --git a/pkg/backup/datadriven_test.go b/pkg/backup/datadriven_test.go index b5ff121e7ded..04fea3d8c646 100644 --- a/pkg/backup/datadriven_test.go +++ b/pkg/backup/datadriven_test.go @@ -46,6 +46,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/admission" + "github.com/cockroachdb/cockroach/pkg/util/besteffort" "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/datadriven" "github.com/cockroachdb/errors" @@ -485,6 +486,11 @@ func (d *datadrivenTestState) getSQLDBForVC( // - "sleep ms=TIME" // Sleep for TIME milliseconds. // +// - "besteffort-forbid-skip op=OP" +// Forbids the besteffort operation named OP from being randomly skipped in +// test builds for the remainder of the test, so its side effects run +// deterministically. See pkg/util/besteffort. +// //lint:ignore U1000 unused func runTestDataDriven(t *testing.T, testFilePathFromWorkspace string) { // TODO(at): data driven tests will need some tweaks to work with OR metamorphic, which will @@ -514,6 +520,16 @@ func runTestDataDriven(t *testing.T, testFilePathFromWorkspace string) { var lastCreatedCluster string ds := newDatadrivenTestState() defer ds.cleanup(ctx, t) + + // The "besteffort-forbid-skip" command registers cleanups that must remain + // in effect for the remainder of the test; run them once it finishes. + var besteffortCleanups []func() + defer func() { + for _, cleanup := range besteffortCleanups { + cleanup() + } + }() + datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string { execWithTagAndPausePoint := func(jobType jobspb.Type) string { ds.noticeBuffer = nil @@ -572,6 +588,12 @@ func runTestDataDriven(t *testing.T, testFilePathFromWorkspace string) { skip.UnderDuress(t) return "" + case "besteffort-forbid-skip": + var op string + d.ScanArgs(t, "op", &op) + besteffortCleanups = append(besteffortCleanups, besteffort.TestForbidSkip(op)) + return "" + case "reset": ds.cleanup(ctx, t) ds = newDatadrivenTestState() diff --git a/pkg/backup/testdata/backup-restore/compaction-failed-lock-cleanup b/pkg/backup/testdata/backup-restore/compaction-failed-lock-cleanup new file mode 100644 index 000000000000..4272e4e53563 --- /dev/null +++ b/pkg/backup/testdata/backup-restore/compaction-failed-lock-cleanup @@ -0,0 +1,90 @@ +# Test that when a compaction job is cancelled/fails, its BACKUP-LOCK file is +# cleaned up, allowing subsequent compactions to proceed without being blocked. +# See: https://github.com/cockroachdb/cockroach/issues/172889 + +reset test-nodelocal +---- + +new-cluster name=s1 disable-tenant +---- + +# The BACKUP-LOCK cleanup in OnFailOrCancel is a besteffort operation, which is +# randomly skipped in test builds. Forbid skipping it so the cancelled +# compaction below deterministically removes its lock and the subsequent +# compaction can proceed. +besteffort-forbid-skip op=delete-compaction-backup-lock +---- + +# 1. Setup: create a table and take a full backup followed by two incrementals, +# saving timestamps to use as compaction start/end boundaries. +exec-sql +CREATE DATABASE orig; +USE orig; +CREATE TABLE foo (i INT PRIMARY KEY, s STRING); +INSERT INTO foo VALUES (1, 'a'), (2, 'b'); +---- + +save-cluster-ts tag=start +---- + +backup aost=start +BACKUP INTO 'nodelocal://1/test-root/' AS OF SYSTEM TIME start; +---- + +exec-sql +INSERT INTO orig.foo VALUES (3, 'c'); +---- + +backup +BACKUP INTO LATEST IN 'nodelocal://1/test-root/'; +---- + +exec-sql +INSERT INTO orig.foo VALUES (4, 'd'); +---- + +save-cluster-ts tag=end +---- + +backup aost=end +BACKUP INTO LATEST IN 'nodelocal://1/test-root/' AS OF SYSTEM TIME end; +---- + +let $backup_path +SHOW BACKUPS IN 'nodelocal://1/test-root/'; +---- + +# 2. Set a pausepoint so the compaction pauses right after it writes the +# BACKUP-LOCK, letting us cancel it while the lock is still present. The +# crdb_internal.backup_compaction builtin starts the job asynchronously, so the +# call returns immediately with the job ID; we wait for the job to pause +# separately rather than expecting a synchronous pausepoint error. +exec-sql +SET CLUSTER SETTING jobs.debug.pausepoints = 'backup_compaction.after.details_has_checkpoint'; +---- + +compact start=start end=end tag=comp1 +SELECT crdb_internal.backup_compaction(0, 'BACKUP INTO LATEST IN ''nodelocal://1/test-root/''', '$backup_path', start, end); +---- + +job tag=comp1 wait-for-state=paused +---- + +# 3. Cancel the paused compaction to trigger OnFailOrCancel, which cleans up the +# BACKUP-LOCK file. +job cancel=comp1 +---- + +# 4. Clear pausepoints so the next compaction can run to completion. +exec-sql +SET CLUSTER SETTING jobs.debug.pausepoints = ''; +---- + +# 5. Run a new compaction over the same range. If the stale BACKUP-LOCK from the +# cancelled job was not cleaned up, this would fail with FileAlreadyExists. +compact start=start end=end tag=comp2 +SELECT crdb_internal.backup_compaction(0, 'BACKUP INTO LATEST IN ''nodelocal://1/test-root/''', '$backup_path', start, end); +---- + +job tag=comp2 wait-for-state=succeeded +----