backup: add resume and onfail/cancel logic to compactions #141753

kev-cao · 2025-02-19T23:48:11Z

Compactions were jobified in #141183, but missing support for job resumption and cancellation handling. This commit adds support for handling those cases.

An additional PR should be made to add telemetry for compactions.

Epic: none

Release note: None

blathers-crl · 2025-02-19T23:48:15Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2025-02-19T23:48:20Z

This change is

kev-cao · 2025-02-25T19:58:05Z

pkg/backup/backup_compaction_test.go

+		jobutils.WaitForJobToPause(t, db, jobID)
+		db.Exec(t, "RESUME JOB $1", jobID)
+		waitForSuccessfulJob(t, tc, jobID)
+		validateCompactedBackupForTables(t, db, []string{"foo", "bar"}, "'nodelocal://1/backup'")


I've realized in my testing that this lets through a couple of false negatives. validateCompactedBackupForTables simply restores from the latest backup. If the tests were created in isolation, this might not be too big of an issue, but since they all share the same cluster, one test that may have failed to create a compaction would actually just restore from a previous test.

I think we can fix this by having validateCompactedBackupForTables take in the start and end of the compaction, and it should also validate that a backup spanning that timespan exists. But I'll do this in another PR.

could you instead use a different collection dir for each subtest instead of nodelocal://1/backup for everything?

Oh yeah that would do the trick. I do think having an extra check that makes sure that there exists a backup that spans the entire time from start to end is good though.

I'll do this in a separate PR though

msbutler

I took a pass at the production code. i'll take a look at that tests later

pkg/backup/backup_job.go

pkg/backup/backup_compaction.go

msbutler · 2025-02-26T14:53:19Z

pkg/backup/backup_compaction.go

+		// TODO (kev-cao): Add telemetry for backup compactions.
+	}
+
+	// For all backups, partitioned or not, the main BACKUP manifest is stored at


once we get all the resolution logic in, i do think there are opportunities to cleanly combine more parts of compaction job and regular backup jobs. But i think it's important to first lay out exactly what we need (resumption logic/ checkpointing, distSQL flow), with all the duplicate code, before refactoring.

to avoid if else conditionals, perhaps we'll create an backupDistSQLPrep interface that implements the prep work for for regular and compacted backups. But lets think through that after we land all the basic blocks.

Yeah I agree, after splitting this up instead of integrating it early, I found some areas where improvements could be made because the code was now split up.

pkg/backup/backup_compaction.go

msbutler · 2025-02-26T15:11:57Z

pkg/backup/backupencryption/encryption.go

@@ -338,12 +338,9 @@ func GetEncryptionFromBase(
 	user username.SQLUsername,
 	makeCloudStorage cloud.ExternalStorageFromURIFactory,
 	baseBackupURI string,
-	encryptionParams jobspb.BackupEncryptionOptions,
+	encryptionParams *jobspb.BackupEncryptionOptions,


this kind of small refactor should be in a separate commit.

I'm also not sure what it is seeking to accomplish: previously we could avoid makeCloudStorage call if there was no encryption, but now we have to make the call regardless?

Oh this issue was caused by the fact that initialDetails would start off with a non-nil encryption because it was always passed an encryption from the builtin. But through our flow of creating the updated details, if we aren't using encryption, encryption actually becomes nil in the updated details (from GetEncryptionFromBaseStore).

So on resume, initialDetails.EncryptionOptions is nil, which led to some nil pointer exceptions because some of our code expects a *jobspb.BackupEncryptionOptions and other parts expect a jobspb.BackupEncryptionOptions. Classic backup happens to avoid this because on resume, we skip all of this work. But with compaction, since we have to validate the compaction chain, we pull all of the manifests for the backup chain, and as a consequence, end up requiring the encryption from initialDetails.

I see. Could you put the encryption patch in a separate, first, commit. And further, could you add

if encryptionParams == nil || encryptionParams.Mode == jobspb.EncryptionMode_None { return nil, nil }

to GetEncryptionFromBase before making cloud storage to avoid that extra work if the backup is not encrypted?

Gotcha, I'll push up another PR for this then.

msbutler · 2025-02-26T15:23:01Z

pkg/backup/backup_compaction.go

+	return &m, nil
+}
+
+func getRestoreChain(


nit: i think we should use the term backup chain, not restore chain.

msbutler

looking better!

msbutler · 2025-02-26T19:43:22Z

pkg/backup/backup_compaction.go

+	// Resolve the backup destination. If we have already resolved and persisted
+	// the destination during a previous resumption of this job, we can re-use
+	// the previous resolution.
+	if needsInitializing {


this block could be put in the needsInitializing block below, that way, we only have one initializing block. then we don't need to declare BackupDest above, i think.

Oh right, the dependency on backupDest got moved into the block so this can be moved too now. That's awesome, I hated this extra block.

pkg/backup/backup_compaction.go

msbutler · 2025-02-26T19:53:01Z

pkg/backup/backupencryption/encryption.go

@@ -338,12 +338,9 @@ func GetEncryptionFromBase(
 	user username.SQLUsername,
 	makeCloudStorage cloud.ExternalStorageFromURIFactory,
 	baseBackupURI string,
-	encryptionParams jobspb.BackupEncryptionOptions,
+	encryptionParams *jobspb.BackupEncryptionOptions,


I see. Could you put the encryption patch in a separate, first, commit. And further, could you add

if encryptionParams == nil || encryptionParams.Mode == jobspb.EncryptionMode_None { return nil, nil }

to GetEncryptionFromBase before making cloud storage to avoid that extra work if the backup is not encrypted?

pkg/backup/backup_compaction.go

msbutler · 2025-02-26T20:02:51Z

pkg/backup/backup_compaction_test.go

+		db.Exec(t, "INSERT INTO foo VALUES (2, 2)")
+		db.Exec(t, incBackupCmd)
+		db.Exec(t, "CREATE TABLE bar (a INT, b INT)")
+		db.Exec(t, "INSERT INTO bar VALUES (1, 1)")


why does this test require 4 inc backups?

Oh I suppose it doesn't, I've gotten into the habit of trying to make sure there are "meaningful" compactions, but I suppose the other tests cover that so there isn't really a need for it in this test.

msbutler · 2025-02-26T20:04:52Z

pkg/backup/backup_compaction_test.go

+		jobutils.WaitForJobToPause(t, db, jobID)
+		db.Exec(t, "RESUME JOB $1", jobID)
+		waitForSuccessfulJob(t, tc, jobID)
+		validateCompactedBackupForTables(t, db, []string{"foo", "bar"}, "'nodelocal://1/backup'")


could you instead use a different collection dir for each subtest instead of nodelocal://1/backup for everything?

msbutler · 2025-02-26T20:06:17Z

pkg/backup/backup_compaction_test.go

+
+		db.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = ''")
+		db.Exec(t, "INSERT INTO bar VALUES (4, 4)")
+		db.Exec(t, incBackupCmd)


is this extra vanilla inc backup necessary?

Nope, had it for the same reason above.

Compactions were jobified in cockroachdb#141183, but missing support for job resumption and cancellation handling. This commit adds support for handling those cases. An additional PR should be made to add telemetry for compactions. Epic: none Release note: None

kev-cao force-pushed the backup/resume-compactions branch 12 times, most recently from afcd540 to ced14ba Compare February 25, 2025 19:56

kev-cao commented Feb 25, 2025

View reviewed changes

kev-cao marked this pull request as ready for review February 25, 2025 19:58

kev-cao requested a review from a team as a code owner February 25, 2025 19:58

kev-cao requested review from jeffswenson, dt, msbutler and a team and removed request for a team February 25, 2025 19:58

kev-cao force-pushed the backup/resume-compactions branch 3 times, most recently from 1a0f8d2 to ec5a023 Compare February 25, 2025 23:48

msbutler reviewed Feb 26, 2025

View reviewed changes

kev-cao force-pushed the backup/resume-compactions branch 3 times, most recently from 354dbbe to 10f1357 Compare February 26, 2025 18:35

kev-cao requested a review from msbutler February 26, 2025 19:05

msbutler reviewed Feb 26, 2025

View reviewed changes

kev-cao force-pushed the backup/resume-compactions branch from 10f1357 to e7b2cfe Compare February 27, 2025 01:26

kev-cao force-pushed the backup/resume-compactions branch from e7b2cfe to 5044582 Compare February 27, 2025 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backup: add resume and onfail/cancel logic to compactions #141753

backup: add resume and onfail/cancel logic to compactions #141753

kev-cao commented Feb 19, 2025

blathers-crl bot commented Feb 19, 2025

cockroach-teamcity commented Feb 19, 2025

kev-cao Feb 25, 2025

msbutler Feb 26, 2025

kev-cao Feb 27, 2025

kev-cao Feb 27, 2025

msbutler left a comment

msbutler Feb 26, 2025

kev-cao Feb 26, 2025

msbutler Feb 26, 2025

kev-cao Feb 26, 2025

msbutler Feb 26, 2025

kev-cao Feb 27, 2025

msbutler Feb 26, 2025

msbutler left a comment

msbutler Feb 26, 2025

kev-cao Feb 27, 2025

msbutler Feb 26, 2025

msbutler Feb 26, 2025

kev-cao Feb 27, 2025

msbutler Feb 26, 2025

msbutler Feb 26, 2025

kev-cao Feb 27, 2025

backup: add resume and onfail/cancel logic to compactions #141753

Are you sure you want to change the base?

backup: add resume and onfail/cancel logic to compactions #141753

Conversation

kev-cao commented Feb 19, 2025

blathers-crl bot commented Feb 19, 2025

cockroach-teamcity commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment