backupccl: disallow restore of backups older than the minimum supported binary version#98597
Conversation
|
This change is |
msbutler
left a comment
There was a problem hiding this comment.
looks good! left mostly nits/clarification qs
| return errors.AssertionFailedf("minimum restoreable version %s is less than the dev offset", | ||
| minimumRestorableVersion) | ||
| } | ||
| minimumRestorableVersion.Major -= clusterversion.DevOffset |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
When running in a development branch as defined by this bool -
, all versions including theBinaryMinSupportedVersion 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.
There was a problem hiding this comment.
let me know if you want company in the "why do we have both a IsRelease() and developmentBranch" rabbit hole :)
There was a problem hiding this comment.
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 -
a8fd5a7 to
6f9fecc
Compare
| // 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) { |
There was a problem hiding this comment.
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
6f9fecc to
c39e5e4
Compare
| # More investigation required. | ||
|
|
||
|
|
||
| new-cluster name=s4 share-io-dir=s1 allow-implicit-access beforeVersion=Start22_2 disable-tenant |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sad to see this noble subtest go :'(
but it's for the better.
There was a problem hiding this comment.
It was a 💯 test, it will live on in release-22.2 for eternity
| ForceTenantID Expr | ||
| SchemaOnly bool | ||
| VerifyData bool | ||
| EncryptionPassphrase Expr |
There was a problem hiding this comment.
sometimes I question gofmt's insistence on vertical alignment.
|
TFTR! bors r=msbutler,dt |
|
Build succeeded: |
|
Is it possible this breaking a bunch of roachtests? See #98918 |
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_VERSIONoptionthat 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