roachtest: add mixed-version restores to backup test #101730
Conversation
renatolabs
requested review from
herkolategan and
srosenberg
and removed request for
a team
|
This change is |
|
This is a stacked PR, please review them in order (but feel free to review the second or third ones before the first one is merged):
To be reviewed here: the last commit. |
renatolabs
force-pushed
the
rc/6-mixed-version-restores
branch
from
8383e2f to
5e089d4
Compare
renatolabs
force-pushed
the
rc/6-mixed-version-restores
branch
from
5e089d4 to
b9e1b90
Compare
|
Rebased:
To be reviewed here: entire diff. |
|
@smg260 @herkolategan @srosenberg Friendly nudge 🙂 |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, and @srosenberg)
pkg/cmd/roachtest/tests/mixed_version_backup.go line 986 at r2 (raw file):
func (mvb *mixedVersionBackup) loadBackupData( ctx context.Context, l *logger.Logger, rng *rand.Rand, h *mixedversion.Helper, ) (err error) {
what's the point of a named return var if there are no naked returns in the function body?
pkg/cmd/roachtest/tests/mixed_version_backup.go line 1033 at r2 (raw file):
// concurrently with this step) to store some data in the cluster by // the time the backup is taken. wait := 3 * time.Minute
Is this value based on some observation? It seems arbitrary, and the comment suggests it is dependent on workload init; perhaps more context as to why its run concurrently
This commit updates the `backup/mixed-version` (now `backup-restore/mixed-version`) to also perform mixed-version restores. At a high level, it introduces a new function that runs in mixed-version state that will randomly attempt to restore a subset of the backups taken up to that point during the test. The test will verify that we are able to restore, in mixed version, backups taken in the previous version _and_ in mixed version. Resolves: cockroachdb#96367. Epic: CRDB-19321 Release note: None
renatolabs
force-pushed
the
rc/6-mixed-version-restores
branch
from
b9e1b90 to
b5410ea
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/mixed_version_backup.go line 986 at r2 (raw file):
Previously, smg260 (Miral Gadani) wrote…
what's the point of a named return var if there are no naked returns in the function body?
This must have been a bad merge conflict resolution. Fixed, thanks.
pkg/cmd/roachtest/tests/mixed_version_backup.go line 1033 at r2 (raw file):
Previously, smg260 (Miral Gadani) wrote…
Is this value based on some observation? It seems arbitrary, and the comment suggests it is dependent on workload init; perhaps more context as to why its run concurrently
Yeah, somewhat arbitrary. It's just a point in time when the workloads have loaded some (but likely not all) their initial data. IMO, taking the backup while the data is being inserted is kind of a test "feature" rather than a bug. We could add synchronization so that the backup is only after once all initial data has been loaded, but that adds complexity without an obvious gain.
I added more words to the comment to make the intention clearer, let me know if this helps.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
|
TFTRs! bors r=smg260,herkolategan |
|
Build succeeded: |
This commit updates the
backup/mixed-version(nowbackup-restore/mixed-version) to also perform mixed-versionrestores. At a high level, it introduces a new function that runs in
mixed-version state that will randomly attempt to restore a subset of
the backups taken up to that point during the test.
The test will verify that we are able to restore, in mixed version,
backups taken in the previous version and in mixed version.
Resolves: #96367.
Epic: CRDB-19321
Release note: None