Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

logictest: randomly backup/restore in logic tests #74174

Merged
merged 1 commit into from
Mar 1, 2022
Merged

logictest: randomly backup/restore in logic tests #74174

merged 1 commit into from
Mar 1, 2022

Conversation

adityamaru
Copy link
Contributor

@adityamaru adityamaru commented Dec 21, 2021 •
edited
Loading

This commit introduces a new configuration to logic tests that will
randomly backup the cluster, re-create the cluster and restore that
backup to the new cluster.

This is primarily beneficial since it runs backup/restore under a much
more comprehensive set of SQL states.

This configuration is currently disabled via the
COCKROACH_LOGIC_TEST_BACKUP_RESTORE_PROBABILITY environment variable
being set to 0. The goal is to set up a nightly that
will set this var to a non-zero number for a growing set of logic
tests.

Co-authored-by: Paul Bardea [email protected]

Closes #54060.

Release note: None

Release justification: non-production code changes, adds a logictest
config that randomly runs backup and restore between existing logic tests.
This is currently disabled.

@adityamaru adityamaru requested review from dt and stevendanna December 21, 2021 22:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru mentioned this pull request Dec 21, 2021
Closed
@adityamaru adityamaru force-pushed the add-backup-restore-logic-disabled branch 2 times, most recently from c62f94a to 363f040 Compare December 28, 2021 15:05
@adityamaru
Copy link
Contributor Author

@dt @stevendanna friendly ping!

@adityamaru adityamaru force-pushed the add-backup-restore-logic-disabled branch from 363f040 to 6935130 Compare February 9, 2022 20:14
@adityamaru adityamaru requested a review from msbutler February 9, 2022 20:15
stevendanna
stevendanna reviewed Feb 10, 2022
View reviewed changes
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice this is exciting. I've left a few minor nits, but am looking forward to this getting in. I think we should double check that our new configuration definitely won't get run in the current CI jobs to avoid further slowing down CI.

pkg/sql/logictest/logic.go Outdated Show resolved Hide resolved
pkg/sql/logictest/logic.go
@@ -2058,6 +2101,153 @@ func (t *logicTest) processTestFile(path string, config testClusterConfig) error
return nil
}

func (t *logicTest) hasOpenTxns() bool {
for _, user := range t.clients {
if _, err := user.Exec("SET TRANSACTION PRIORITY NORMAL;"); !testutils.IsError(err, "there is no transaction in progress") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this end up changing the semantics of a test that has already tried to set the transaction priority to something other than normal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, added a statement to reset it to the original txn priority.

pkg/sql/logictest/logic.go Outdated
if err != nil {
// We might get an error depending on the state of the test. Let's just
// ignore it.
// BEFOREMERGE: Add a comment here about when this is the case.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, this was an artifact from Paul's original PR. I've switched to returning an error for now. When we do enable this we can revisit this and add a comment to that effect.

pkg/sql/logictest/logic.go Outdated Show resolved Hide resolved
@adityamaru adityamaru force-pushed the add-backup-restore-logic-disabled branch from 6935130 to 2e21902 Compare February 24, 2022 19:42
@adityamaru adityamaru force-pushed the add-backup-restore-logic-disabled branch from 2e21902 to 6175c6c Compare February 28, 2022 14:30
@adityamaru
Copy link
Contributor Author

@stevendanna this should be RFAL, I removed the TestBackupRestore func so that this is not picked up in CI at all. When we do write a nightly to run logictests with this config we can just write a script to ./dev testlogic --config=3backup-node.

stevendanna
stevendanna approved these changes Feb 28, 2022
View reviewed changes
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I vote we get this in and iterate on it. I imagine we'll find oddities in the session serialisation stuff and other issues we haven't thought of, but that is kinda the point of the test in the first place.

@adityamaru adityamaru mentioned this pull request Feb 28, 2022
Closed
@pbardea @adityamaru
logictest: randomly backup/restore in logic tests
05e9034 
This commit introduces a new configuration to logic tests that will
randomly backup the cluster, re-create the cluster and restore that
backup to the new cluster.

This is primarily beneficial since it runs backup/restore under a much
more comprehensive set of SQL states.

This configuration is currently disabled via the
`COCKROACH_LOGIC_TEST_BACKUP_RESTORE_PROBABILITY` environment variable
being set to 0. The goal is to set up a nightly that
will set this var to a non-zero number for a growing set of logic
tests.

Co-authored-by: Paul Bardea <[email protected]>

Closes cockroachdb#54060.

Release note: None

Release justification: non-production code changes, adds a logictest
config that randomly runs backup and restore between existing logic tests.
This is currently disabled.
@adityamaru adityamaru force-pushed the add-backup-restore-logic-disabled branch from 6175c6c to 05e9034 Compare February 28, 2022 21:59
@adityamaru
Copy link
Contributor Author

TFTR!

bors r=stevendanna

@jordanlewis
Copy link
Member

Yay! 🎉

@craig
Copy link
Contributor

craig bot commented Mar 1, 2022

Build succeeded:

@craig craig bot merged commit 12c968f into cockroachdb:master Mar 1, 2022
@adityamaru adityamaru deleted the add-backup-restore-logic-disabled branch March 1, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevendanna stevendanna stevendanna approved these changes

@dt dt Awaiting requested review from dt

@msbutler msbutler Awaiting requested review from msbutler

Assignees
No one assigned
Labels
T-disaster-recovery
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bulkio: add backup/restore mode to logic tests
5 participants
@adityamaru @cockroach-teamcity @jordanlewis @stevendanna @pbardea