roachtest: improvements to mixedversion package
#96989
Conversation
renatolabs
requested review from
herkolategan and
smg260
and removed request for
a team
|
This change is |
|
This is part of a stacked PR comprised of 3 parts. Please review them in order. |
renatolabs
force-pushed
the
rc/1-mixed-version-roachtest-improvements
branch
2 times, most recently
from
5b7dabd to
ac248b6
Compare
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @smg260)
pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 39 at r1 (raw file):
Helper struct { ctx context.Context context *Context
Nit: ctx vs. context might be a little confusing here, could help to make the field naming a little more descriptive.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status:
This commit introduces a few improvements to the `mixedversion` package, the recently introduced framework for mixed-version (and mixed-binary) roachtests. Specifically, the following improvements are made: * Removal of `DBNode()` function: having the planner pick a database that the individual steps will connect to is insufficient in many cases and could be misleading. The idea was that the user would be able to see, from the test plan itself, what node a certain step would be interacting with. However, the reality is that steps often need to run statements on multiple different nodes, or perhaps they need to pick one node specifically (e.g., the statement needs to run on a node in the old version). For that reason, the `DBNode()` function was dropped. Instead, steps have access to a random number generator that they can use to pick an arbitrary node themselves. The random number generators are unique to each user function, meaning each test run will see the same numbers being generated even if other steps are scheduled concurrently. The numbers observed by a user function will also be the same if the seed passed to `mixedversion.Test` is the same. * Definition of a "test context" that is available to mixed-version tests. For now, the test context includes things like which version we are upgrading (or downgrading) to and from and which nodes are running which version. This allows tests to take actions based on, for example, the number of nodes upgraded. It also allows them to run certain operations on nodes that are known to be in a specific version. * Introduction of a `helper` struct that is passed to user-functions. For now, the helper includes functions to connect to a specific node and get the current test context. The struct will help us provide common functionality to tests so that they don't have to duplicate code. * Log cached binary and cluster versions before executing a step. This makes it easier to understand the state of the cluster when looking at the logs of one specific step. * Internal improvement to the test runner: instead of assuming the first step of a mixed-version test plan will start the the cockroach nodes, we now check that that is the case, providing a clear error message if/when that assumption doesn't hold anymore (instead of a cryptic connection failure error). Epic: CRDB-19321 Release note: None
renatolabs
force-pushed
the
rc/1-mixed-version-roachtest-improvements
branch
from
ac248b6 to
304e0b4
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 39 at r1 (raw file):
Previously, herkolategan (Herko Lategan) wrote…
Nit:
ctxvs.contextmight be a little confusing here, could help to make the field naming a little more descriptive.
Good point, renamed to testContext.
|
bors r=herkolategan TFTR! |
|
Build succeeded: |
This commit introduces a few improvements to the
mixedversionpackage, the recently introduced framework for mixed-version (and mixed-binary) roachtests.Specifically, the following improvements are made:
Removal of
DBNode()function: having the planner pick a database that the individual steps will connect to is insufficient in many cases and could be misleading. The idea was that the user would be able to see, from the test plan itself, what node a certain step would be interacting with. However, the reality is that steps often need to run statements on multiple different nodes, or perhaps they need to pick one node specifically (e.g., the statement needs to run on a node in the old version). For that reason, theDBNode()function was dropped. Instead, steps have access to a random number generator that they can use to pick an arbitrary node themselves. The random number generators are unique to each user function, meaning each test run will see the same numbers being generated even if other steps are scheduled concurrently. The numbers observed by a user function will also be the same if the seed passed tomixedversion.Testis the same.Definition of a "test context" that is available to mixed-version tests. For now, the test context includes things like which version we are upgrading (or downgrading) to and from and which nodes are running which version. This allows tests to take actions based on, for example, the number of nodes upgraded. It also allows them to run certain operations on nodes that are known to be in a specific version.
Introduction of a
helperstruct that is passed to user-functions. For now, the helper includes functions to connect to a specific node and get the current test context. The struct will help us provide common functionality to tests so that they don't have to duplicate code.Log cached binary and cluster versions before executing a step. This makes it easier to understand the state of the cluster when looking at the logs of one specific step.
Internal improvement to the test runner: instead of assuming the first step of a mixed-version test plan will start the the cockroach nodes, we now check that that is the case, providing a clear error message if/when that assumption doesn't hold anymore (instead of a cryptic connection failure error).
Epic: CRDB-19321
Release note: None