roachprod: roachtest: refactoring #104915

smg260 · 2023-06-14T20:21:55Z

This commit addresses some usability issues within roachprod/roachtest, and does some minor clean up.

Summary:

roachprod

c.Parallel now accepts nodes Nodes and func(ctx context.Context, node Node). Previously, these was an int n representing the first n nodes to operate on, and func(ctx context.Context, int n), where second arg would confusingly represented the index of the node being operated on, which was wholly dependent on a captured-by-reference slice at the call site. This change makes it explicit to the caller and the function, which exact node is targeted.
c.Parallel now returns []*RunResultDetails, boolean, error. The slice will contain results, successful and failed, for each node (or thus far, depending on fail-fast behaviour). The boolean is a signal to the caller that at least one of the results in the slice suffered from a command error, which can be inspected in the result itself. The error now more idiomatically represents an unrecoverable error encountered during Parallel; i.e. an un retryable error in roachprod. In the latter case, results will be nil.
All calls to execute a remote command on a node are all funnelled to c.runCmdOnSingleNode, ensuring consistency, and reducing duplicated code. This function accepts a RunCmdOptions struct to configure it's behaviour, including stdin/out
runCmdOnSingleNode : in error cases, include stdout/err output (truncated after n bytes - effectively tail). This output will show in the failure_n.log and give convenient insight. Full output is always retained in the run log.
RunResultDetails.{Stdout,Stderr,CombinedOut} are now all string to represent the most common use case, and a convenience r.Output() added to print out the contents of Stderr,Stdout,CombinedOut
Remove complex legacy pipe processing logic from roachtest (execCmd,execCmdEx), previously used when roachprod was not a library of roachtest
When showing the result of a command on each node on the CLI, explicitly display <ok> or <err> followed by output/error

roachtest

skip post-test assertions on a test failure
removal of execCmd/execCmdRes which were doing some complex pipe processing before calling roachprod.Run (not required since roachprod is a lib of roachtest)

Minor

Clean up various redundant/unused variables
Typos
Command formatting

Informs: #104312
Informs: #104316 (TBD)

Epic: none
Release note: none

cockroach-teamcity · 2023-06-14T20:22:15Z

This change is

105343: roachprod: suppress SCP retries for specific error substrings. r=herkolategan,renatolabs a=smg260 Previously, SCP file transfers would be retried regardless of the error encountered, even if it was one that would unlikley be resolved by retries. This PR adds a slice of substrings to match scp output to, and in which cases not to retry, resulting in less noisy logs. A connect timeout of 10s has also been specified. Epic: none Release note: none This is cherry-picked (and slightly amended) from #104915 Co-authored-by: Miral Gadani <miral@cockroachlabs.com>

- Funnel all ssh command calls to `runCmdOnSingleNode`. A boolean is added to control whether we prepend ROACHPROD env vars, which are required for user-supplied commands via `c.Run/RunWithDetails` - `c.ParallelE` now accepts a node list to make it clear which nodes a specified command is running on. Previously, and confusingly, this was achieved via an `int` arg and a captured-by-ref slice of nodes. The int arg referred to the first `int` nodes in the captured slice.

smg260 · 2023-07-27T15:55:02Z

TeamCity @ SELECT_PROBABILITY=0.3

renatolabs

Definitely a lot of changes and it's hard to digest it all in one sitting. Overall I do quite like the changes though, just left minor comments.

I'm looking at the run on this branch that you linked above. I was curious about the new formatting of the command output, but couldn't find anything. Some things also caught my attention and I have the feeling they are not expected:

the .failed log file seems to be missing.
The jepsen failure has an empty log file for the jepsen command.

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

pkg/cmd/roachtest/cluster.go line 589 at r2 (raw file):

		err:    err,
		stdout: stdoutString,
		stderr: stderrString,

Nice, 🔥

pkg/cmd/roachtest/cluster.go line 2207 at r2 (raw file):

	logFileFull := l.File.Name()
	if err != nil {
		createFailedFile(logFileFull)

In RunE, we call createFailedFile(logFile). Any reason this needs to be createFailedFile(logFileFull) in this function?

pkg/cmd/roachtest/cluster.go line 2226 at r2 (raw file):

func createFailedFile(logFile string) {
	if logFile == "" {
		return

Why the check? Can this actually be empty in any of the callers?

pkg/roachprod/roachprod.go line 1760 at r2 (raw file):

		res := &install.RunResultDetails{Node: node}

		//TODO: this usage may not always be correct, if the target nodes are not sequential

The current implementation of TargetNodes already guarantees ordering. I'd either remove the TODO to avoid confusion (same for TODO below)

pkg/roachprod/install/cluster_synced.go line 876 at r2 (raw file):

		res = newRunResultDetails(node, cmdErr)
		res.CombinedOut = string(out)
		//output = fmtOut(res.CombinedOut)

Can be removed?

pkg/roachprod/install/cluster_synced.go line 1134 at r2 (raw file):

		runOpts.combinedOut = false
		return c.runCmdOnSingleNode(ctx, l, n, cmd, runOpts)
		//sess := c.newSession(l, 1, cmd, withDebugName("ssh-gen-key"))

Delete?

pkg/roachprod/install/cluster_synced.go line 1148 at r2 (raw file):

		runOpts.stdin = bytes.NewReader(sshTar)
		return c.runCmdOnSingleNode(ctx, l, node, `tar xf -`, runOpts)
		//sess := c.newSession(l, node, cmd, withDebugName("ssh-dist-key"))

Delete.

pkg/roachprod/install/cluster_synced.go line 2452 at r2 (raw file):

// on the cluster.
//
// ParallelE  runs at most `concurrency` (or  `config.MaxConcurrency` if it is lower) in parallel.

Nit: extra space.

smg260

Thankyou for the review:

the .failed log file seems to be missing.
Addressed in comments below

The jepsen failure has an empty log file for the jepsen command.
The cmd log files show stdout/err only, and the jepsen command redirects both to invoke.log, hence no output.

However, I've made an update to show some info in the command logs regardless:

the full command being run
the final status ( or error)

I'll run another roachtest at 0.05 select to showcase the changes, and will update this thread when it's ready.

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

pkg/cmd/roachtest/cluster.go line 2207 at r2 (raw file):