roachtest: validate cluster when using fixtures and provide opt-out #105455
Conversation
renatolabs
requested review from
herkolategan and
smg260
and removed request for
a team
|
This change is |
renatolabs
force-pushed
the
rc/mixedversion-cluster-validation
branch
2 times, most recently
from
4f6a3a8 to
c93fced
Compare
There was a problem hiding this comment.
Reviewed 5 of 9 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, and @smg260)
pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go line 131 at r2 (raw file):
// We use fixtures more often than not as they are more likely to // detect bugs, especially in migrations. useFixturesProbability: 0.7,
Are there many other mixed-version tests (besides backup/restore) which support running without fixtures? We might consider adding requireFixtures to TestSpec.
pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go line 276 at r2 (raw file):
// disable the use of fixtures in the test. Necessary if the test // wants to use a number of cockroach nodes other than 4. func DisableFixtures(opts *testOptions) {
Nit: maybe NeverUseFixtures?
This commit updates the `mixedversion` API with two main changes: * cluster validation: previously, mixed-version tests would always start from checked-in fixtures. Those fixtures have an implied cluster topology and require the cluster to have 4 cockroach nodes -- if that's not the case, the cluster fails to start up with non-obvious error messages. To make this situation clearer, we now validate that the test is being run with the correct number of nodes. * to allow for larger-scale mixed-version tests (more than 4 nodes), we now expose a `DisableFixtures` option that can be passed to `NewTest`. With this option, nodes start with an empty store directory, eliminating any restrictions on cluster size. Epic: CRDB-19321 Release note: None
renatolabs
force-pushed
the
rc/mixedversion-cluster-validation
branch
from
c93fced to
2c3e868
Compare
There was a problem hiding this comment.
Rebased and fixed conflicts.
Reviewable status:
pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go line 131 at r2 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
Are there many other mixed-version tests (besides backup/restore) which support running without fixtures? We might consider adding
requireFixturestoTestSpec.
I believe all tests except acceptance/version-upgrade can run without the baked-in fixtures (i.e., that test is the only one that has assertions that only work if the cluster started from those fixtures). Not sure it would make sense to add something to TestSpec since previous-version fixtures only make sense in the context of mixed-version tests.
pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go line 276 at r2 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
Nit: maybe
NeverUseFixtures?
Good point, renamed.
|
TFTR! bors r=srosenberg |
|
Build succeeded: |
This commit updates the
mixedversionAPI with two main changes:cluster validation: previously, mixed-version tests would always start from checked-in fixtures. Those fixtures have an implied cluster topology and require the cluster to have 4 cockroach nodes -- if that's not the case, the cluster fails to start up with non-obvious error messages. To make this situation clearer, we now validate that the test is being run with the correct number of nodes.
to allow for larger-scale mixed-version tests (more than 4 nodes), we now expose a
DisableFixturesoption that can be passed toNewTest. With this option, nodes start with an empty store directory, eliminating any restrictions on cluster size.Epic: CRDB-19321
Release note: None