Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @herkolategan)

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 480 at r1 (raw file):

// Run performs `cluster.RunE` on a randomly picked node.
func (h *Helper) Run(rng *rand.Rand, cmd ...string) error {

I'm not convinced this function needs to be part of *Helper (I'd like to only add functions to this struct once there are many call sites that would benefit from it). I know we have Query related functions here already, but one distinction is that when we need to run SQL in a test, we will always connect to one gateway node; for running commands, it's very common that commands will need to run on multiple nodes at the same time, and it's not great to have behavior/logging be different if we are running a command in one vs many nodes.

In the mixed-version closure, you can achieve the same behavior with

node := h.RandomNode(rng, c.All())
c.RunE(...)

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 487 at r1 (raw file):

// InitLegacyWorkload initializes a legacy workload on all nodes.
func (h *Helper) InitLegacyWorkload(workloadPath string, workload string) {

I also believe this function is doing something too specific to warrant a function in *Helper. In addition, it sounds like a function that the caller would always need to make sure to call just once. This is not quite the type of function that should live here. I'll make a comment down below.

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 490 at r1 (raw file):

	// Stage workload on all nodes as the load node to run workload is chosen
	// randomly.
	h.runner.cluster.Put(h.ctx, workloadPath, "./workload", h.runner.crdbNodes)

What's the reason we need to use ./workload instead of ./cockroach workload?

pkg/cmd/roachtest/tests/versionupgrade.go line 134 at r1 (raw file):

		func(ctx context.Context, l *logger.Logger, rng *rand.Rand, h *mixedversion.Helper) error {
			initWorkload.Do(func() {
				h.InitLegacyWorkload(t.DeprecatedWorkload(), "schemachange")

Instead of using the sync.Once, I think it makes more sense to call mvt.OnStartup and init the workload there.

pkg/cmd/roachtest/tests/versionupgrade.go line 142 at r1 (raw file):

				fmt.Sprintf("--concurrency %d", 2),
				fmt.Sprintf("{pgurl:1-%d}", len(c.All())),
			}

Nit: consider using commandbuilder, we've been trying to use more structured data types for commands in roachtest.

pkg/cmd/roachtest/tests/versionupgrade.go line 143 at r1 (raw file):

				fmt.Sprintf("{pgurl:1-%d}", len(c.All())),
			}
			return h.Run(rng, runCmd...)

Please use cluster.RunE. We want to return errors instead of calling t.Fatal in these callbacks.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 480 at r1 (raw file):

I was debating if it should be in the helper at all. I'll move this code out.

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 490 at r1 (raw file):

The schemachange workload is not supported under the cockroach binary, that's something that we should get done. I'll get an issue open for it.

pkg/cmd/roachtest/tests/versionupgrade.go line 134 at r1 (raw file):

Good point I'll switch this.

pkg/cmd/roachtest/tests/versionupgrade.go line 143 at r1 (raw file):

The run wrapper was doing this, but I"ll remove it and put the calls here directly on cluster.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go line 487 at r1 (raw file):

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @herkolategan)

pkg/cmd/roachtest/tests/versionupgrade.go line 112 at r2 (raw file):

		if err := c.PutE(ctx, t.L(), t.DeprecatedWorkload(), "./workload", c.All()); err != nil {
			return err
		}

Probably cleaner to stash ./workload "out-of-band" -- i.e., you can just call c.Put() (no need for PutE) before the mvt := mixedversion.NewTest line. If that fails, we don't waste time setting up the mixed-version test, and it also simplifies this closure too.

pkg/cmd/roachtest/tests/versionupgrade.go line 113 at r2 (raw file):

			return err
		}
		return c.RunE(ctx, c.All(), "./workload init", "schemachange")

Nit: since these are all consts, probably reads better as a single string: c.RunE(ctx, c.All(), "./workload init schemachange")

pkg/cmd/roachtest/tests/versionupgrade.go line 145 at r2 (raw file):

			runCmd.Flag("max-ops", 10)
			runCmd.Flag("concurrency", 2)
			runCmd.Arg("{pgurl:1-%d}", len(c.All()))

Nit: these can be chained

runCmd := roachtestutil.NewCommand("...")
    .Flag("verbose", 1)
    .Flag("max-ops", 10)

pkg/cmd/roachtest/tests/versionupgrade.go line 146 at r2 (raw file):

			runCmd.Flag("concurrency", 2)
			runCmd.Arg("{pgurl:1-%d}", len(c.All()))
			randomNode := c.All().RandNode()

We should use h.RandomNode() here (i.e., using the rng provided as parameter instead of the global rand).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)

pkg/cmd/roachtest/tests/versionupgrade.go line 112 at r2 (raw file):

Done.

pkg/cmd/roachtest/tests/versionupgrade.go line 146 at r2 (raw file):

Done.

Thank you for adding the schemachange workload back!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan)