backupccl: disallow restore of backups older than the minimum supported binary version #98597

adityamaru · 2023-03-14T17:21:30Z

As outlined in https://www.cockroachlabs.com/docs/v22.2/restoring-backups-across-versions.html,
we do not support restoring backups outside our versioning policy window. This
patch enforces the policy by ensuring that the manifest's version is greater
than equal to the minimum supported binary version that the current binary
can interoperate with.

Additionally, this patch introduces an UNSAFE_RESTORE_INCOMPATIBLE_VERSION option
that can be used to skip compatability checks and forcefully restore backups taken
outside our compatability window. This has primarily been added to allow development
branches to restore backups taken on release branches, but can be used as a break
glass option where restoring a backup is absolutely necessary. No guarantees are
made about the correctness of the restore when using this option.

Release note (sql change): Disallow the restore of backups taken on a cluster version
that is older than the minimum binary version the current cluster can interoperate with.
This will be described in an updated version of the policy
outlined in https://www.cockroachlabs.com/docs/v22.2/restoring-backups-across-versions.html.

Fixes: #94295

cockroach-teamcity · 2023-03-14T17:21:46Z

This change is

msbutler

looks good! left mostly nits/clarification qs

pkg/ccl/backupccl/backup_test.go

pkg/ccl/backupccl/restore_planning.go

msbutler · 2023-03-14T18:41:44Z

pkg/ccl/backupccl/restore_planning.go

+			return errors.AssertionFailedf("minimum restoreable version %s is less than the dev offset",
+				minimumRestorableVersion)
+		}
+		minimumRestorableVersion.Major -= clusterversion.DevOffset


could you clarify in a comment why this decrement is necessary? my read of the code is that BinaryMinSupportedVersion() always returns v22_2, but maybe i misread things?

When running in a development branch as defined by this bool -

cockroach/pkg/clusterversion/cockroach_versions.go

Line 846 in 2d63378

var developmentBranch = !envutil.EnvOrDefaultBool("COCKROACH_TESTING_FORCE_RELEASE_BRANCH", false) ||

, all versions including the BinaryMinSupportedVersion are offset by DevOffset. If we didn't decrement the MinimumBinaryVersion by the dev offset then a backup taken on say 22.2 would not be restoreable into our branch because our MinimumBinaryVersion is 1000022.2. Since 22.2 is far less than this number we would reject the restore.

This has however got me thinking that the condition we decrement the BinaryMinSupportedVersion should not be IsRelease() but instead the developmentBranch bool linked above.

let me know if you want company in the "why do we have both a IsRelease() and developmentBranch" rabbit hole :)

The code has changed so that we don't need to perform this DevOffset math. But to answer your question when we're on a 'development branch" all versions including BinaryMinSupportedVersion are offset by a million. You can read up more here, it explains it much better than I will be able to -

cockroach/pkg/clusterversion/cockroach_versions.go

Line 895 in 56c64e3

// If this is a dev branch, we offset every version +1M major versions into

pkg/ccl/backupccl/backup_test.go

adityamaru · 2023-03-16T19:31:22Z

pkg/ccl/backupccl/restore_planning.go

+		// This is the "cluster" version that does not change between patch releases
+		// but rather just tracks migrations run. If the backup is more migrated
+		// than this cluster, then this cluster isn't ready to restore this backup.
+		if currentActiveVersion.Less(v) {


note, we don't skip this check even if unsafeRestoreIncompatibleVersion is true. That seemed correct to me because we have always been rejecting more migrated backups even before we enforced our new policy.

…ed binary version As outlined in https://www.cockroachlabs.com/docs/v22.2/restoring-backups-across-versions.html, we do not support restoring backups outside our versioning policy window. This patch enforces the policy by ensuring that the manifest's version is greater than equal to the minimum supported binary version that the current binary can interoperate with. Additionally, this patch introduces an `UNSAFE_RESTORE_INCOMPATIBLE_VERSION` option that can be used to skip compatability checks and forcefully restore backups taken outside our compatability window. This has primarily been added to allow development branches to restore backups taken on release branches, but can be used as a break glass option where restoring a backup is absolutely necessary. No guarantees are made about the correctness of the restore when using this option. Release note (sql change): Disallow the restore of backups taken on a cluster version that is older than the minimum binary version the current cluster can interoperate with. This will be described in an updated version of the policy outlined in https://www.cockroachlabs.com/docs/v22.2/restoring-backups-across-versions.html. Fixes: cockroachdb#94295

adityamaru · 2023-03-16T20:43:38Z

pkg/ccl/backupccl/testdata/backup-restore/in-progress-imports

-# More investigation required.
-
-
-new-cluster name=s4 share-io-dir=s1 allow-implicit-access beforeVersion=Start22_2 disable-tenant


The test relied on a cluster not having run 22.2 migrations. This is no longer relevant as our min binary version is 22.2 which means all 22.2 migrations have indeed run.

sad to see this noble subtest go :'(

but it's for the better.

It was a 💯 test, it will live on in release-22.2 for eternity

dt · 2023-03-16T20:48:45Z

pkg/sql/sem/tree/backup.go

-	ForceTenantID              Expr
-	SchemaOnly                 bool
-	VerifyData                 bool
+	EncryptionPassphrase             Expr


sometimes I question gofmt's insistence on vertical alignment.

adityamaru · 2023-03-17T12:49:09Z

TFTR!

bors r=msbutler,dt

craig · 2023-03-17T13:24:21Z

Build succeeded:

Bazel Essential CI (Cockroach)

cucaroach · 2023-03-18T13:35:19Z

Is it possible this breaking a bunch of roachtests? See #98918

adityamaru requested review from dt and msbutler March 14, 2023 17:21

adityamaru requested a review from a team as a code owner March 14, 2023 17:21

adityamaru mentioned this pull request Mar 14, 2023

backupccl: disallow restore of backups older than the minimum supported binary version #93804

Closed

msbutler reviewed Mar 14, 2023

View reviewed changes

pkg/ccl/backupccl/backup_test.go Show resolved Hide resolved

adityamaru force-pushed the enforce-restore-version branch from a8fd5a7 to 6f9fecc Compare March 16, 2023 19:26

adityamaru requested a review from a team as a code owner March 16, 2023 19:26

adityamaru requested a review from msbutler March 16, 2023 19:29

adityamaru commented Mar 16, 2023

View reviewed changes

adityamaru force-pushed the enforce-restore-version branch from 6f9fecc to c39e5e4 Compare March 16, 2023 20:41

adityamaru commented Mar 16, 2023

View reviewed changes

dt approved these changes Mar 16, 2023

View reviewed changes

msbutler approved these changes Mar 17, 2023

View reviewed changes

craig bot merged commit 85c6e38 into cockroachdb:master Mar 17, 2023

adityamaru deleted the enforce-restore-version branch March 17, 2023 13:25

cockroach-teamcity mentioned this pull request Mar 18, 2023

PR #98597 - backupccl: disallow restore of backups older than the minimum support… cockroachdb/docs#16523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: disallow restore of backups older than the minimum supported binary version #98597

backupccl: disallow restore of backups older than the minimum supported binary version #98597

adityamaru commented Mar 14, 2023 •

edited

Loading

cockroach-teamcity commented Mar 14, 2023

msbutler left a comment

msbutler Mar 14, 2023

adityamaru Mar 14, 2023

msbutler Mar 14, 2023

adityamaru Mar 16, 2023 •

edited

Loading

adityamaru Mar 16, 2023

adityamaru Mar 16, 2023

msbutler Mar 17, 2023

adityamaru Mar 17, 2023

dt Mar 16, 2023

adityamaru commented Mar 17, 2023

craig bot commented Mar 17, 2023

cucaroach commented Mar 18, 2023

		# More investigation required.


		new-cluster name=s4 share-io-dir=s1 allow-implicit-access beforeVersion=Start22_2 disable-tenant

backupccl: disallow restore of backups older than the minimum supported binary version #98597

backupccl: disallow restore of backups older than the minimum supported binary version #98597

Conversation

adityamaru commented Mar 14, 2023 • edited Loading

cockroach-teamcity commented Mar 14, 2023

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

adityamaru Mar 16, 2023 • edited Loading

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

adityamaru commented Mar 17, 2023

craig bot commented Mar 17, 2023

cucaroach commented Mar 18, 2023

adityamaru commented Mar 14, 2023 •

edited

Loading

adityamaru Mar 16, 2023 •

edited

Loading