backupccl: cleanup tests that relied on restoring unsupported backup fixtures #95023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adityamaru
changed the title
|
This change is |
adityamaru
force-pushed
the
cleanup-restore-tests
branch
from
3476e6f to
06b9c1c
Compare
Interleaved tables were removed >1 major version in the past and so a backup restoreable in 23.1 cannot contain interleaved tables. Release note: None
The xDBRef and duplicate-db exceptional backups can no longer be restored since they were generated >1 major version in the past. This change does add a test inspired by the duplicate-db test to expand coverage. Release note: None
adityamaru
force-pushed
the
cleanup-restore-tests
branch
from
06b9c1c to
2fc74bf
Compare
adityamaru
requested review from
herkolategan and
srosenberg
and removed request for
a team
msbutler
approved these changes
Mar 10, 2023
| // Test restoring different objects from the backup. | ||
| sqlDB.Exec(t, `CREATE DATABASE ts`) | ||
| sqlDB.Exec(t, `RESTORE test.rev_times FROM LATEST IN $1 WITH into_db = 'ts'`, localFoo) | ||
| for _, ts := range sqlDB.QueryStr(t, `SELECT logical_time FROM ts.rev_times`) { |
|
|
||
| // Restore a couple tables, including parent but not child_pk. | ||
| sqlDB.Exec(t, `CREATE DATABASE test`) | ||
| sqlDB.Exec(t, fmt.Sprintf(`RESTORE test.circular FROM LATEST IN $1 AS OF SYSTEM TIME %s`, ts[0]), localFoo) |
There was a problem hiding this comment.
is it worth checking that we indeed restored the proper constraint? I realize you validate it.
There was a problem hiding this comment.
Added a call to SHOW CREATE to ensure its the constraint we expect.
| sqlDB.Exec(t, `CREATE DATABASE test`) | ||
| sqlDB.Exec(t, fmt.Sprintf(`RESTORE test.circular FROM LATEST IN $1 AS OF SYSTEM TIME %s`, ts[0]), localFoo) | ||
| sqlDB.Exec(t, fmt.Sprintf(`RESTORE test.parent, test.child FROM LATEST IN $1 AS OF SYSTEM TIME %s WITH skip_missing_foreign_keys`, ts[0]), localFoo) | ||
| sqlDB.Exec(t, `SELECT * FROM pg_catalog.pg_constraint`) |
There was a problem hiding this comment.
could you validate the restore fails without the skip_missing_foreign_keys option?
There was a problem hiding this comment.
Added checks below that a restore without skipping foreign keys fails for table child and child_pk.
| CREATE TABLE test_table(); | ||
| CREATE TABLE test_table2(); | ||
| GRANT ZONECONFIG ON DATABASE test TO testuser; | ||
| GRANT ZONECONFIG ON test_table TO testuser; |
There was a problem hiding this comment.
i suspect you want to grant the test table on a different user? I assume the DATABASE grant above automatically implies a table grant?
There was a problem hiding this comment.
Hmm I think we stopped doing privileges inheritance. So a db privilege does not imply a table privilege afaik.
This change removes the test that was added as part of were correctly upgraded on restore to 20.x+ versions. Backups taken one major release ago will no longer have these old style fks and so this test serves better as a regular test on this branch. Release note: None
This change removes old cluster restore fixtures, and replaces them with a cluster backup from 22.2.0. It also removes a 20.1 fixture that tested restore of a zoneconfig privilege that was overwritten as the usage privilege in 20.2+ versions. This test has been migrated to a regular test instead. Release note: None
This change removes an old MR backup that was being restored and tested against. There were no instructions on how to regenerate the fixture in cockroachdb#69725 so it is being deleted. Release note: None
Prior to 22.2 the public schema was not backed by a descriptor. These fixtures tested that a restore of a pre 22.2 backup would remap the synthetic public schema to a descriptor backed public schema. Since the only restoreable backups on this branch will be from 22.2 onwards, we should never see a synthetic public schema in a restoreable backup. Release note: None
The backup fixtures in this test were generated on 20.2 where cockroachdb#62564 existed. Given this was >1 major version in the past we do not expect to see a reoccurrence of the bug in 22.2+ backups that are restoreable in this release. Release note: None
This fixture captured a bug in 20.2.7 that has since been fixed. This change removes the fixture and moves it to a regular regression test. Release note: None
These fixtures were generate on older version of Cockroach that are no longer restoreable. The test has been migrated to a regular datadriven test. Release note: None
This change regenerate the sequences backup fixture on 22.2.0. While there have been no changes to the representation of sequences between 22.2 and 23.1 it seemed useful to keep a cross version backup test of this nature. Release note: None
All users in 22.2+ clusters will have IDs and so will the backups taken on these clusters. Thus, there is no need to keep around the cross version test that tests the custom restore func that injects a user ID. Release note: None
This change regenerates the fixtures for testing restores of backups that have been taken before or after the backfill portion of a schemachange. Note these tests rely on the old schemachanger and do not increase our test coverage with the new declarative schema changer. Release note: None
This test was testing a bug that was discovered during backups in pre-22.2 versions cockroachdb#88042. Since this bug does not exist on 22.2+ versions this test will not encounter the expected corruption on backups that are restoreable in this cluster. Release note: None
adityamaru
force-pushed
the
cleanup-restore-tests
branch
from
2fc74bf to
2883f93
Compare
|
TFTR! bors r=msbutler |
|
Build succeeded: |
|
👋🏻 Hi, friends. In running the That is, the filenames introduced in this PR are long enough to break More officially, dev inf is aware and may look to introduce some git tooling to warn about long file names at some point in the future. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As outlined in https://www.cockroachlabs.com/docs/v22.2/restoring-backups-across-versions.html,
we do not support restoring backups older than the minimum supported binary version of the cluster. For 23.1 this
minimum supported binary version is going to be 22.2. This patch deletes or modifies existing tests that relied on backup
fixtures outside our minimum restoreable version. Some of the tests that exercised restores of
exceptional backups that could only be generated on pre-22.2 clusters have been deleted since users will have to
restore those backups into 22.2, and take another backup on 22.2 before restoring into 23.1.
A follow up will enforce the minimum restoreable version during restore job execution.
Informs: #94295