roachtest: create simple tpce workload driver

[msbutler]

Currently, the tpce workload is used in 2 roachtests in tpce.go. This patch makes it easier for other roachtests to init and run a tpce workload by creating a tpce driver that abstracts away the actual cmds required to init and run a tpce workload. This will make it easier to address #99787, for example.

Informs #99787

Release note: none

[cockroach-teamcity]

This change is 

[renatolabs]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler and @smg260)

---

pkg/cmd/roachtest/tests/tpce.go line 30 at r1 (raw file):

type tpceSpec struct {
	loadNode         option.NodeListOption

Nit: make this an int if you have any intentions of really having just one "load node", or rename it to loadNodes if having multiple workload nodes is officially supported (as suggested by the current type).

---

pkg/cmd/roachtest/tests/tpce.go line 56 at r1 (raw file):

		builder.WriteString(fmt.Sprintf(" --threads=%d", to.threads))
	}
	return builder.String()

May I suggest the use of commandbuilder here?

---

pkg/cmd/roachtest/tests/tpce.go line 61 at r1 (raw file):

func initTPCESpec(
	ctx context.Context, t test.Test, c cluster.Cluster, loadNode,
	roachNodes option.NodeListOption,

TIL this is crlfmt compliant.

---

pkg/cmd/roachtest/tests/tpce.go line 63 at r1 (raw file):

	roachNodes option.NodeListOption,
) (*tpceSpec, error) {
	t.Status("installing docker")

Suggestion: can we take a logger instance as argument and use that here instead of t.Status / t.L(). You can keep existing behavior in the your caller by passing t.L() and would make this struct friendlier to implementations that desire more fine grained control of test logging.

[msbutler]

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

---

pkg/cmd/roachtest/tests/tpce.go line 30 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Nit: make this an int if you have any intentions of really having just one "load node", or rename it to loadNodes if having multiple workload nodes is officially supported (as suggested by the current type).

Done.

---

pkg/cmd/roachtest/tests/tpce.go line 56 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

May I suggest the use of commandbuilder here?

dang this is slick!!! Sadly I had to sprintf the hosts as they're passed as duplicate flags: e.g "hosts=ip1 hosts=ip2".

---

pkg/cmd/roachtest/tests/tpce.go line 61 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

TIL this is crlfmt compliant.

🤷

crlfmt
diff -u old/pkg/cmd/roachtest/tests/tpce.go new/pkg/cmd/roachtest/tests/tpce.go
--- old/pkg/cmd/roachtest/tests/tpce.go	2023-03-30 09:16:29
+++ new/pkg/cmd/roachtest/tests/tpce.go	2023-03-30 09:16:29
@@ -13,17 +13,17 @@
 import (
 	"context"
 	"fmt"
-	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
-	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"strings"
 	"time"

 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 )
@@ -49,7 +49,10 @@
 }

 func initTPCESpec(
-	ctx context.Context, l *logger.Logger, c cluster.Cluster, loadNode int,
+	ctx context.Context,
+	l *logger.Logger,
+	c cluster.Cluster,
+	loadNode int,
 	roachNodes option.NodeListOption,
 ) (*tpceSpec, error) {
 	l.Printf("Installing docker")

---

pkg/cmd/roachtest/tests/tpce.go line 63 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Suggestion: can we take a logger instance as argument and use that here instead of t.Status / t.L(). You can keep existing behavior in the your caller by passing t.L() and would make this struct friendlier to implementations that desire more fine grained control of test logging.

Done.

[renatolabs]

Sorry, forgot about this PR. Just one small nit!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @msbutler and @smg260)

---

pkg/cmd/roachtest/tests/tpce.go line 56 at r1 (raw file):

Previously, msbutler (Michael Butler) wrote…

dang this is slick!!! Sadly I had to sprintf the hosts as they're passed as duplicate flags: e.g "hosts=ip1 hosts=ip2".

Yeah, when I saw this code I thought of how we could have some APIs in that package to support use cases like this one. I'll eventually get to it.

---

pkg/cmd/roachtest/tests/tpce.go line 86 at r2 (raw file):

// import of the data and schema creation.
func (ts *tpceSpec) init(ctx context.Context, t test.Test, c cluster.Cluster, o tpceCmdOptions) {
	cmd := ts.newCmd(o).Flag("init", "")

Nit: while the command will run fine with this (the formatting will include extra spaces due to the empty string), the API for flags/options that don't take a corresponding value is Option("init").

[msbutler]

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

---

pkg/cmd/roachtest/tests/tpce.go line 86 at r2 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Nit: while the command will run fine with this (the formatting will include extra spaces due to the empty string), the API for flags/options that don't take a corresponding value is Option("init").

Done

[msbutler]

TFTR!

bors=renatolabs

[msbutler]

bors r=renatolabs

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)