Roachtest redirect SSH flakes to test-eng

[smg260]

See second commit note at the bottom

This PR inspects the failure output of a roachtest, and if it sees an SSH_PROBLEM, overrides the owning team to test-eng when reporting the github issue.

Currently errors are classified as an SSH error by roachprod if the exit code is 255 with an accompanying message prefixed with SSH_PROBLEM [1]. The errors are stringified and saved into t.mu.output|failureMsg. Thus in the test_runner at the call site of issue posting, we can check t.mu.output for SSH_PROBLEM and override the team and issue name accordingly.

Resolves: #82398

Release justification: test-only change
Release note: none

[cockroach-teamcity]

This change is 

[tbg]

We can also build on top of #88556 for this, I have a DM going with Miral.

[tbg]

@smg260 (correct me if I'm wrong please) is getting #88556 in first and then re-working this to build on top of that PR.

I'm unassigning for review so that it doesn't show up in my queue in the meantime.

[smg260]

@tbg Yep. Marking as draft for now.

[smg260]

This is rebased on top of #88556 which introduces structured errors.

Detecting the SSH flake via substring still works and keeps this PR simple.

This might be a good place to introduce some retry logic for running the SSH commands

[srosenberg]

Detecting the SSH flake via substring still works and keeps this PR simple.

A chance of a false positive (via substring) is probably very low, but structured errors should make it cleaner and pave the way for more cases. I am thinking of a switch statement on something like errCategory with cluster creation and ssh errors injecting the corresponding category at the site?

This might be a good place to introduce some retry logic for running the SSH commands

There is a separate issue to deal with SSH retries: #73542

[srosenberg]

I recently spotted another issue... seeing this error message quite often during cluster teardown,

teardown: 16:53:25 cluster.go:1149: failed to fetch logs: cluster.Get: get logs failed

The above error message originates in [1]. The problem is that the actual error message is swallowed [2] because l.File is usually non-nil, when invoked from roachtest. (The logger is essentially the root logger and l.File is test.log.) l believe the same issue applies to Put.

[1] https://github.com/cockroachdb/cockroach/blob/master/pkg/roachprod/install/cluster_synced.go#L2034
[2] https://github.com/cockroachdb/cockroach/blob/master/pkg/roachprod/install/cluster_synced.go#L2028

[renatolabs]

structured errors should make it cleaner and pave the way for more cases

I agree with this, I think structured errors open the door for us to do things based on the error types. I think that's cleaner and also wouldn't break if someone changed how errors.SSH is formatted.

As general food for thought for everyone: IMO, errors like cluster creation and SSH problems are better reported in a more concise way other than a GitHub issue. We just want to be notified about them, but there's little (nothing?) we can do about them other than acknowledge and close the issue when it's opened.

One idea is to post something on Slack (in the existing test-eng-ops) channel with a summary of a roachtest nightly run, including how many tests were skipped due to cluster creation errors, SSH flakes, etc.

[renatolabs]

@smg260 I pointed a couple of misplaced comments on #88556. Maybe we could fix those here?

[smg260]

A chance of a false positive (via substring) is probably very low, but structured errors should make it cleaner and pave the way for more cases. I am thinking of a switch statement on something like errCategory with cluster creation and ssh errors injecting the corresponding category at the site?

That's definitely the goal, and my thoughts were to introduce that when we switch to cockroachdb/errors. We'd then be able to annotate errors using something like errors.Mark(err, SSHerr) or `errors.Mark(err, ClusterCreationErr) at the point we detect it. (tbg's idea).

So we can keep this PR tight in scope and address just the SSH flakes, or expand it to more generically handle different error types. The PR does at least currently constrain the logic to to determine the owning team in githib.go (as it should).

There is a separate issue to deal with SSH retries: #73542

👍 This was getting a bit gnarly to implement in my test branch

[smg260]

As general food for thought for everyone: IMO, errors like cluster creation and SSH problems are better reported in a more concise way other than a GitHub issue. We just want to be notified about them, but there's little (nothing?) we can do about them other than acknowledge and close the issue when it's opened.

One idea is to post something on Slack (in the existing test-eng-ops) channel with a summary of a roachtest nightly run, including how many tests were skipped due to cluster creation errors, SSH flakes, etc.

I agree that some errors are better reported via other mediums such as Slack, but having the same error reported a shared github issue (or similar) provides a better system of record. Ultimately, where an issue is posting should be agnostic to the caller and the posting code could drive which channels are notified via some coniguration

[smg260]

The second commit switches to using errors/marker api to mark an SSH flake where it is detected in roachprod. This allows us to look for a marker exception and act on it accordingly.

Summary of changes:

1. switch from []error to []failure in t.mu where failure is a struct containing all errors which were passed as args. The impetus for this was to preserve secondary+ exceptions which are passed into the t.Error/Fatal methods. Currently when t.Error(someErr) is invoked, a new error is created, moving someErr to a secondary error, out of the primary error chain. This makes is very hard to search for the marker which would have been inserted at someErr but is now not visible to errors.Is(..)
2. Add ErrSSH255 and errClusterProvisioningFailed reference errors (the marker errors). We control what we post in the issue according to if we find these
3. remove unused members of t.mu
4. remove t.argsToErr in favour of errors.NewWithDepthf

[tbg]

Reviewed 4 of 4 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @smg260, and @srosenberg)

---

pkg/cmd/roachtest/github.go line 137 at r1 (raw file):

Previously, smg260 (Miral Gadani) wrote…

It probably makes sense to keep SSH flakes together, instead of having a new one created for each incidence.

We can prepend the test name to the message body like cluster creation does?

I agree that this isn't excessively pretty, but will it maybe get the job done just fine? For example, the cluster_creation messages mention the test name sufficiently prominently:

#89810

Not sure if this is the case here too (since there won't be a .Skip()) but the message also contains the test name right at the top: #90045 (comment)

We can also go through the refactor and add a title override that we can use instead of the test name. I'm just not sure it's worth the time.

---

pkg/cmd/roachtest/github.go line 40 at r2 (raw file):

	clusterCreationErr issueCategory = iota
	sshErr
	otherErr

tiny nit: since otherErr is the catch-all, it should be represented by the zero value.

---

pkg/cmd/roachtest/github.go line 162 at r3 (raw file):

	//TODO: perhaps remove category completely and move this to
	// issuePoster?

The poster has multiple callers and so I think this should continue to live in roachtest.

---

pkg/cmd/roachtest/test_impl.go line 45 at r3 (raw file):

	// errors are all the errors passed to a single invokation of
	// `addFailure`
	errors []error

Add some color here, why are we stashing these errors? It's because they might be structured and we want to preserve that (which fmt.Sprintf(...) wouldn't do). Maybe given an example that t.Fatalf("foo %s %s %s", "hello", err1, err2) would lead to []error{err1,err2}.

---

pkg/cmd/roachtest/test_impl.go line 280 at r3 (raw file):

// from a test's closure. The test runner itself should never call this.
func (t *testImpl) Fatal(args ...interface{}) {
	t.Fatalf("%v", args...)

This will print t.Fatal("hi") as ["hi"] which isn't what we want. I think you want something like

t.Fatalf(strings.Join(strings.Repeat(len(args), " %v"))[1:], args...)

---

pkg/cmd/roachtest/test_impl.go line 287 at r3 (raw file):

	errs := []error{errors.NewWithDepthf(1, format, args...)}
	errs = append(errs, collectErrors(args...)...)
	t.addFailure(newFailure(errs...))

The symmetry in newFailure is a little misleading then since the first error is special, how about

type failure struct {
  squashedErr error
  wrapped []error
}

or something like that?

---

pkg/cmd/roachtest/test_impl.go line 298 at r3 (raw file):

func (t *testImpl) Error(args ...interface{}) {
	t.Errorf("%v", args...)

ditto

---

pkg/roachprod/errors/errors.go line 123 at r3 (raw file):

	if exitErr, ok := asExitError(err); ok {
		if exitErr.ExitCode() == 255 {
			return SSH{errors.Mark(err, ErrSSH255)}

Does this work? Does SSH{} implement the necessary Unwrap() method?

[smg260]

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

---

pkg/cmd/roachtest/github.go line 105 at r1 (raw file):

Previously, smg260 (Miral Gadani) wrote…

that's a good catch - it would have been breaking on master too but we only override for cluster creation there

Done

---

pkg/cmd/roachtest/github.go line 162 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The poster has multiple callers and so I think this should continue to live in roachtest.

Yep sorry - unclear TODO. Was a thought to myself to remove the idea of a category, and instead depend completely on reference errors. issuePoster here refers to the createPostRequest function in roachtest/github.go

---

pkg/cmd/roachtest/test_impl.go line 280 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This will print t.Fatal("hi") as ["hi"] which isn't what we want. I think you want something like

t.Fatalf(strings.Join(strings.Repeat(len(args), " %v"))[1:], args...)

Done. Thank you. Got lost in the variadic args being passed from func to func.

---

pkg/cmd/roachtest/test_impl.go line 287 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The symmetry in newFailure is a little misleading then since the first error is special, how about

type failure struct {
  squashedErr error
  wrapped []error
}

or something like that?

Agreed. That is much better.

---

pkg/roachprod/errors/errors.go line 123 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Does this work? Does SSH{} implement the necessary Unwrap() method?

Yep . In roachprod/errors.go

Code snippet:

func (e SSH) Unwrap() error {
	return e.Err
}

[tbg]

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

---

-- commits line 39 at r6:
💌

---

pkg/cmd/roachtest/test_impl.go line 48 at r6 (raw file):

// being captured in the squashedErr
type failure struct {
	// this is the single error created from variadic args passed to t.Error(f)/t.Fatal(f)

general comment, our style guide asks for complete sentences incl punctuation and capitalization (except for inline comments where it's the opposite), i.e.

// This is the single error created from variadic args passed to t.{Fatal,Error}{,f}.

I usually don't point out isolated incidents of this but it looks like it's happening throughout here, it will save you lots of nits down the road to get the muscle memory right. :-)

---

pkg/cmd/roachtest/test_impl.go line 286 at r6 (raw file):

// from a test's closure. The test runner itself should never call this.
func (t *testImpl) Fatal(args ...interface{}) {
	t.Error(args...)

ditto

---

pkg/cmd/roachtest/test_impl.go line 292 at r6 (raw file):

// Fatalf is like Fatal, but takes a format string.
func (t *testImpl) Fatalf(format string, args ...interface{}) {
	t.Errorf(format, args...)

Yo zneed to call addFailure because you'l

---

pkg/cmd/roachtest/test_impl.go line 298 at r6 (raw file):

// FailNow implements the TestingT interface.
func (t *testImpl) FailNow() {
	t.addFailure("FailNow called", nil)

passing nil here doesn't seem right, FailNow called doesn't have a verb in it and besides a nil doesn't add any info.

---

pkg/cmd/roachtest/test_impl.go line 400 at r6 (raw file):

// failureContainsError returns true if any of the errors in a given failure
// matches the reference error
func failureContainsError(refError error, f failure) bool {

nit: switch the order of the arguments, to reflect "failure contains error", not "error contains failure".

---

pkg/testutils/lint/passes/fmtsafe/functions.go line 96 at r6 (raw file):

	// Both of these signatures need to be included for the linter to not flag
	// roachtest testImpl.addFailure since it is in the main package
	// This could be a bug in nogo

Did you mean to leave this? If so, mind linking an issue?

[smg260]

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

---

pkg/cmd/roachtest/test_impl.go line 48 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

general comment, our style guide asks for complete sentences incl punctuation and capitalization (except for inline comments where it's the opposite), i.e.

// This is the single error created from variadic args passed to t.{Fatal,Error}{,f}.

I usually don't point out isolated incidents of this but it looks like it's happening throughout here, it will save you lots of nits down the road to get the muscle memory right. :-)

👍 Thanks, missed the capitalisation. I don't think it's happened anywhere else though

---

pkg/cmd/roachtest/test_impl.go line 286 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

ditto

What is missing here?

---

pkg/cmd/roachtest/test_impl.go line 292 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Yo zneed to call addFailure because you'l

Cut off comment, but addFailure is being called via t.Errorf

[smg260]

Test name will be prepended to issue body in the case of cluster creation AND ssh flakes.

Other conversations resolved. Will merge after TC builds are green.

[smg260]

bors r=tbg

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[smg260]

bors retry

[craig]

Already running a review

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)