backup: use separate incremental directory by default by benbardin · Pull Request #75970 · cockroachdb/cockroach

benbardin · 2022-02-03T18:49:51Z

backup: use separate incremental directory by default

Previously support was added to optionally cause interactions with a backup to search for its incremental layers in an alternative path.

This change expands that behavior, adding a default path for incremental layers to interactions with backups in a collection. This new default is the '/incrementals' subdirectory of the collection root. This default may be overridden by the prevoiusly-added incremental_storage parameter. This default means that an operator can easily apply different policies to incremental backups retroactively. This behavior also provides an alternative to passing matching incremental_storage parameters to every interaction with those backups, which improves usability.

During the upgrade, when interacting with existing backups, this behavior remains disabled.
After the upgrade, if an existing chain of incremental backups exists in the old default collection, this behavior also remains disabled.
Also after the upgrade, if no such chain exists, or the chain exists in the new default location, the new behavior will be enabled.

Release note (enterprise change): Incremental backups created by BACKUP INTO and BACKUP TO are now stored by default under the path 'incrementals' within the backup destination, rather than under each backup's path. This enables easier application of cloud-storage provider policies specifically to incremental backups.

Release justification: This feature is very well-tested, and the code refactoring makes it more verifiably correct.

cockroach-teamcity · 2022-02-03T18:49:58Z

This change is

msbutler

Left a few comments on the backup and restore code. I still need to review the tests and the show backup code. Let's discuss if you have questions!

I think it's also worth writing out the whole policy of destination resolution in the commit message, or maybe by revising the backup destination resolution doc I sent you that Liv originally wrote. I'm still not totally clear what the intended policy is, i.e.: given the user passes these params for Backup or Restore, this is what happens.

pkg/ccl/backupccl/backup_destination.go

pkg/ccl/backupccl/constants.go

pkg/ccl/backupccl/backup_destination.go

pkg/ccl/backupccl/constants.go

pkg/ccl/backupccl/manifest_handling.go

pkg/ccl/backupccl/restore_planning.go

dt · 2022-02-08T15:20:44Z

A few nits on commit message style (I promise these become second nature eventually):

The prefix is not usually enclosed in brackets, and slashes in it are generally read as paths within the repo. You'll see commas if you need a separator e.g. jobs,sql/stats: blah, but here I think our team typically just uses the common package name "backup:" (or backupccl:" if we're being pedantic) for changes that touch both backup and restore (you'll notice paths generally elide the meaningless pkg/ prefix, and we often skip the ccl fragments too as they're just an artifact of the license split).
No need for trailing period on title, and ideally is a tad shorter for reading in one-line git log listing.
It's usually good, especially if it isn't linking to an issue with more context, to have more contextual info in the commit message about what is changing, why, and what the edge cases (like mixed version) are, since the doc writers will read the whole commit message when reviewing the change to determine how to doc it.
We don't, however, need to stuff all that in the release note, which is generally a one-line item in a big bulleted list of what changed since the last release, which is intended for an operator to scan to learn what new things they might want to use and should then go read more about; goal here isn't to tell you how to use it right there, just that you might want to. Things like the mixed version handling are generally more granular than the release note covers, but are very good to put in the commit message body above it for docs to then pull into the full doc text.

Putting these nits together, I suggest something like:

backup: use separate incremental directory by default

Previously in support was added to optionally cause interactions with a backup to search for its incremental layers in an alternative path. This change adds that behavior by default to interactions with backups in a collection, defaulting to the 'incrementals' subdirectory of the collection root, unless overridden by an incremental_storage parameter. Doing so by default means that existing backups are already separated by the time an operator realizes they want to apply different policies to these backups, and eliminates the need to pass matching incremental_storage parameters to every interaction with those backups, improving the usability compared to the explicit parameter.

During the upgrade and after the upgrade, when interacting with existing backups, if they have an existing chain of incremental backups directly within them, interactions with and appending to the incremental backup chain will continue to use that chain. Once the upgrade is finalized, if an existing backup has no existing incremental chain under it, and a new incremental backup is being created, it will start the new incremental backup chain under the new separated incrementals prefix.

Release note (enterprise change): Incremental backups created by BACKUP TO LATEST IN are now stored by default under the path 'incrementals' within the backup collection rather than under each backup's path, allowing easier management of cloud-storage provider policies specifically applied to incremental backups.

benbardin · 2022-02-08T18:59:20Z

Thanks David! I updated the message.

msbutler · 2022-02-09T14:16:53Z

One small nit on the commit message: this feature only applies to backups with the BACKUP INTO syntax, not BACKUP TO LATEST syntax (which i don't think is a thing).

msbutler

left another round of comments in Backup/Restore code. Again, haven't looked at tests and show backup yet.

pkg/ccl/backupccl/backup_destination.go

pkg/ccl/backupccl/manifest_handling.go

pkg/ccl/backupccl/restore_planning.go

Waiting to refactor internal variables until cockroachdb#75970 merges. Release note (sql change): refactored BACKUP, SHOW BACKUP, and RESTORE incremental_storage option to incremental_location.

76345: import: default to writing at ingest time r=dt a=dt Release note: none. 76368: build: turn off metamorphic constants for more gen rules r=ajwerner a=stevendanna This add `COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true` to more tool invocations to cut down on log spam. Informs #76363 Release note: None 76416: backupccl: rename incremental_storage option to incremental_location r=dt a=msbutler Waiting to refactor internal variables until #75970 merges. Release note (sql change): refactored BACKUP, SHOW BACKUP, and RESTORE incremental_storage option to incremental_location. Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Steven Danna <danna@cockroachlabs.com> Co-authored-by: Michael Butler <butler@cockroachlabs.com>

Waiting to refactor internal variables until cockroachdb#75970 merges. Release note (sql change): refactored BACKUP, SHOW BACKUP, and RESTORE incremental_storage option to incremental_location.

msbutler

huge improvement!!!!! have not reviewed the tests or debug_backup

pkg/ccl/backupccl/backup_planning.go

pkg/ccl/backupccl/backup_destination.go

pkg/ccl/backupccl/restore_planning.go

msbutler · 2022-02-17T17:26:55Z

pkg/ccl/backupccl/incrementals.go

+// make sure to resolve the paths correctly here. We don't want to accidentally
+// correct an unauthorized file path to an authorized one, then write a backup
+// to an unexpected place or print the wrong error message on a restore.
+func JoinURLPath(args ...string) string {


i'm not totally clear on the subtle differences between filepath.Join and path.join and this helper function. Perhaps a few unit tests for this func would help illustrate? Maybe this function belongs in pkg/cloud/uris.go ? Perhaps @dt has more to say on this.

We don't want to be using filepath for anything since it is client dependent -- e.g. on a windows node it'd use \ as its separator when joining -- and we're working with network paths. I think we just want to use path.Join?

Thanks david - good catch with the separator, I didn't think that through.

path.Join doesn't quite work - I tweaked the comments and added some unit tests. This any clearer?

Ah, I see, yeah, I remember this somewhat unexpected "path.Join calls path.Clean" behavior from working with nodelocal but it internally avoids that.

We already use path.Join throughout backup resolution though, and changing it will be a user-facing change to how ../ behaves, so my vote would be we don't do that in this change but rather address it, if we decide we should, separately?

I think this actually maintains the expected user behavior - https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/backupccl/backup_test.go#L5430 fails without it. (I think it passed before because path.Join wasn't actually called on that particular path, but with the refactoring path.Join is now called.)

pkg/ccl/backupccl/restore_planning.go

pkg/ccl/backupccl/show.go

pkg/ccl/backupccl/backup_destination.go

pkg/ccl/backupccl/show.go

dt · 2022-02-18T03:01:26Z

pkg/ccl/backupccl/incrementals.go

+// make sure to resolve the paths correctly here. We don't want to accidentally
+// correct an unauthorized file path to an authorized one, then write a backup
+// to an unexpected place or print the wrong error message on a restore.
+func JoinURLPath(args ...string) string {


We don't want to be using filepath for anything since it is client dependent -- e.g. on a windows node it'd use \ as its separator when joining -- and we're working with network paths. I think we just want to use path.Join?

pkg/ccl/backupccl/manifest_handling.go

msbutler

nearly there! I think it's also worth adding a test to show_test to capture the regression Aditya mentioned in slack.

msbutler · 2022-03-03T14:42:29Z

pkg/ccl/backupccl/backup_destination_test.go

@@ -238,10 +225,10 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 				{
 					to := localizeURI(t, baseDir, localities)
 					// The full backup should go into the baseDir.


The comment indicates we're using the old BACKUP TO syntax. Why would this subtest get changed by this PR, which I thought only affects BACKUP INTO?

Discussed in slack - this doesn't get us much from the UX perspective, but the code is probably cleaner since we don't have to make copies and fork more on syntax type. Your thoughts?

just read through the slack thread. I'm ok with this change! To clarify for Kathryn and future users, I'd amend the commit message so it's clear that this change modifies both BACKUP TO and BACKUP INTO.

pkg/ccl/backupccl/backup_destination_test.go

msbutler · 2022-03-03T15:16:28Z

pkg/ccl/backupccl/backup_test.go

+
+		sib := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location = %s", dest, inc)
+		sqlDB.Exec(t, sib)
+		sir := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb'", dest)


to make this test even stronger, what if after you check the query results below, you did the following: ensure the same restore command will behave the same, even in the presence of an inc backup in the default location. I.e. after checkQueryResults:

sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, 200) dib := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location = %s", dest, defaultinc) sqlDB.Exec(t, sib) sir2 := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb2'", dest) sqlDB.Exec(t, sir2) sqlDB.CheckQueryResults(t, `SELECT * FROM inc_fkdb.fk`, sqlDB.QueryStr(t, `SELECT * FROM inc_fkdb2.fk`))

I don't think that I like that so much - while I agree the behavior would right now be as you describe, it's not part of the API contract. RESTORE makes no promises about which incrementals dir it will use if both have layers; in fact, BACKUP promises never to do that by default. I'd rather consider this behavior as "undefined" - not because I think it's likely we'll want to change it, but because I think it makes the functionality simpler and clearer for callers.

Have I convinced?

One worry I have is that it's perfectly legal for the user to use the incremental_location parameter to get into this undefined state. Given that we can detect this undefined state, what if we throw an error during RESTORE and SHOW BACKUPS if we detect this, and ask the user to pass the incremental_location parameter?

OK, this I like! Seems like an easy win.

pkg/ccl/backupccl/incrementals.go

pkg/ccl/backupccl/restore_planning.go

Waiting to refactor internal variables until cockroachdb#75970 merges. Release note (sql change): refactored BACKUP, SHOW BACKUP, and RESTORE incremental_storage option to incremental_location.

benbardin · 2022-03-07T23:15:15Z

@adityamaru @msbutler I think this rounds out the revisions! I fixed the regression Aditya caught (thanks again!), added a unit test, and tweaked a few other things.

I left the commits separate so it's easier to see the differences - if you think we're good here, I'll accept your ✅ , squash and send to David for stability release approval! Thanks!

msbutler

LGTM!

dt · 2022-03-10T23:34:19Z

pkg/ccl/backupccl/show.go

 		}
-		incPaths, err = FindPriorBackups(ctx, incStore, IncludeManifest)

+		collection, computedSubdir := CollectionAndSubdir(dest, subdir)


When do we need this? If someone does SHOW BACKUP <collection>/<backup-date> instead of SHOW BACKUP <backup-date> IN <collection> ?

I'm temped to say that with the move to separated per-collection incremental storage, pointing directly to a raw full backup path like the former should be discouraged, or at least, shouldn't try to be magically collection aware, and maybe just shows the full backup at that dest. That implies implying SHOW BACKUP abc IN col would show something different (all the incs) than SHOW BACKUP col/abc but that seems... maybe okay? i.e. we moved incremtnals to be collection level, and so use the collection syntax to interact with them?

I dunno. Not blocking, just something to chew on / maybe room to remove some code/complexity of supporting both here.

Yes, exactly. Aditya caught the regression, discussed in https://cockroachlabs.slack.com/archives/CJXBR3693/p1646276263127099?thread_ts=1646246621.037659&cid=CJXBR3693.

Naively, I don't think I'd expect those two SHOW commands to have different outputs. It seems like a lot of work for a simple IN to convey "this is the modern collection syntax," in ways I'd expect to surprise users. But I do agree this code introduces a lot of complexity to get the "it just works" magic, and I would love to think of ways to simplify by reducing functionality.

Previously support was added to optionally cause interactions with a backup to search for its incremental layers in an alternative path. This change expands that behavior, adding a default path for incremental layers to interactions with backups in a collection. This new default is the '/incrementals' subdirectory of the collection root. This default may be overridden by the prevoiusly-added incremental_storage parameter. This default means that an operator can easily apply different policies to incremental backups retroactively. This behavior also provides an alternative to passing matching incremental_storage parameters to every interaction with those backups, which improves usability. During the upgrade, when interacting with existing backups, this behavior remains disabled. After the upgrade, if an existing chain of incremental backups exists in the old default collection, this behavior also remains disabled. Also after the upgrade, if no such chain exists, or the chain exists in the new default location, the new behavior will be enabled. Release note (enterprise change): Incremental backups created by BACKUP TO LATEST IN or BACKUP TO are now stored by default under the path 'incrementals' within the backup destination, rather than under each backup's path. This enables easier management of cloud-storage provider policies specifically applied to incremental backups. Release justification: This feature is very well-tested, and the code refactoring makes it more verifiably correct.

benbardin · 2022-03-11T16:09:42Z

bors r+

craig · 2022-03-11T17:19:46Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-03-11T20:04:02Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-03-11T22:52:18Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-03-12T04:41:10Z

Build succeeded:

GitHub CI (Cockroach)

katmayb · 2022-03-14T17:08:00Z

Hey @benbardin, would you mind updating the release note here, to clarify that this is an update to the BACKUP INTO syntax rather than anything to do with the older BACKUP TO syntax? (I assume that is the case.) Just for future reference and docs writers working on release notes.

benbardin · 2022-03-14T17:56:22Z

Hey @benbardin, would you mind updating the release note here, to clarify that this is an update to the BACKUP INTO syntax rather than anything to do with the older BACKUP TO syntax? (I assume that is the case.) Just for future reference and docs writers working on release notes.

It's actually both; I updated the release note to reflect. (Updating the old syntax didn't get us any meaningful functionality, I think, but the code was easier to organize and understand.) Thanks for catching!

benbardin requested review from a team, dt, msbutler and stevendanna February 3, 2022 18:49

benbardin requested a review from a team as a code owner February 3, 2022 20:41

benbardin force-pushed the default-incrementals branch from a844458 to e4b346b Compare February 7, 2022 16:24

stevendanna added the T-disaster-recovery label Feb 7, 2022

benbardin force-pushed the default-incrementals branch from e4b346b to 286dc4f Compare February 7, 2022 19:38

msbutler reviewed Feb 8, 2022

View reviewed changes

benbardin force-pushed the default-incrementals branch 2 times, most recently from c1ab9d9 to ed89ea8 Compare February 8, 2022 18:58

benbardin changed the title ~~[backup/restore] Use distinct subdirectory 'incrementals' for incremental backups as a default.~~ backup: use separate incremental directory by default Feb 8, 2022

benbardin force-pushed the default-incrementals branch from ed89ea8 to 56c4a96 Compare February 8, 2022 21:18

msbutler reviewed Feb 9, 2022

View reviewed changes

msbutler mentioned this pull request Feb 11, 2022

backupccl: rename incremental_storage option to incremental_location #76416

Merged

benbardin force-pushed the default-incrementals branch from 052cf5b to 5f307e9 Compare February 16, 2022 17:09

msbutler reviewed Feb 17, 2022

View reviewed changes

dt reviewed Feb 18, 2022

View reviewed changes

benbardin force-pushed the default-incrementals branch 4 times, most recently from 2527fdf to 70f8889 Compare February 23, 2022 20:14

msbutler reviewed Mar 3, 2022

View reviewed changes

benbardin force-pushed the default-incrementals branch from e6a6b23 to 258abc5 Compare March 3, 2022 23:07

msbutler approved these changes Mar 8, 2022

View reviewed changes

benbardin force-pushed the default-incrementals branch 5 times, most recently from 5db157b to 4adf8af Compare March 10, 2022 22:53

dt reviewed Mar 10, 2022

View reviewed changes

dt approved these changes Mar 10, 2022

View reviewed changes

benbardin force-pushed the default-incrementals branch from 4adf8af to f36dd64 Compare March 11, 2022 14:53

craig bot merged commit 270eb7b into cockroachdb:master Mar 12, 2022

cockroach-teamcity mentioned this pull request Mar 12, 2022

backup: use separate incremental directory by default cockroachdb/docs#13230

Closed

msbutler mentioned this pull request Mar 14, 2022

ccl/backupccl: TestBackupRestoreResolveDestination failed #77717

Closed

benbardin deleted the default-incrementals branch March 14, 2022 17:56

benbardin mentioned this pull request Mar 14, 2022

backupccl: default to separated incremental_from for collections #75201

Closed

2 tasks

adityamaru mentioned this pull request Mar 20, 2022

backupccl: add deprecation warning to BACKUP TO and non-collection forms of RESTORE and SHOW BACKUP #78153

Closed

Conversation

benbardin commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Feb 3, 2022

Uh oh!

msbutler left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dt commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benbardin commented Feb 8, 2022

Uh oh!

msbutler commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msbutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msbutler left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msbutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benbardin commented Feb 3, 2022 •

edited

Loading

msbutler left a comment •

edited

Loading

dt commented Feb 8, 2022 •

edited

Loading

msbutler commented Feb 9, 2022 •

edited

Loading

msbutler left a comment •

edited

Loading