roachtest: string match for transient errors as a fallback #132702

DarrylWong · 2024-10-15T20:07:15Z

The require package is commonly used through roachtest to assert that no error occured. i.e. require.NoError(t, err) However, this function does not preserve the error object. This causes our transient error flake detection to not work. Since require is an upstream dependency, we cannot easily change this.

This change adds a fallback to our flake detection that string matches for the TRANSIENT_ERROR message we add. If found it will mark the error as a flake to reduce noise.

However, we have seen other cases where we do not preserve the error object but the code lives somewhere that is easily changeable for us. In those cases, we ideally should fix the code instead of resorting to this fallback.

To make sure we still do that, the fallback also explicity checks for a message that require.NoError prepends to all errors. If we find additional cases that require this fallback, we can review and add them on a case by case basis.

Fixes: #131094
Epic: none
Release note: none

cockroach-teamcity · 2024-10-15T20:07:24Z

This change is

blathers-crl · 2024-10-17T17:29:00Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

DarrylWong · 2024-10-17T17:34:48Z

First two commits are part of #132795 as the newly added tests build off of it. The core logic behind this PR is in transientErrorOwnershipFallback and should be reviewable without knowing the context of the other PR.

pkg/cmd/roachtest/test_impl.go

srosenberg

It would be good to add a (mocked) roachtest using the require package s.t. we have e2e coverage. Otherwise, LGTM!

DarrylWong · 2024-10-24T22:29:32Z

It would be good to add a (mocked) roachtest using the require package s.t. we have e2e coverage.

Good idea, added.

The `require` package is commonly used through roachtest to assert that no error occured. i.e. `require.NoError(t, err)` However, this function does not preserve the error object. This causes our transient error flake detection to not work. Since `require` is an upstream dependency, we cannot easily change this. This change adds a fallback to our flake detection that string matches for the `TRANSIENT_ERROR` message we add. If found it will mark the error as a flake to reduce noise. However, we have seen other cases where we do not preserve the error object but the code lives somewhere that is easily changeable for us. In those cases, we ideally should fix the code instead of resorting to this fallback. To make sure we still do that, the fallback also explicity checks for a message that `require.NoError` prepends to all errors. If we find additional cases that require this fallback, we can review and add them on a case by case basis.

srosenberg · 2024-10-25T00:36:45Z

pkg/cmd/roachtest/test_test.go

+	})
+
+	// Now test that if the transient error is not handled by the `require` package,
+	// but similarly lost due to casting to a string, the test runner *won't* mark


srosenberg

LGTM

DarrylWong · 2024-10-25T03:37:47Z

TFTR!

bors r=srosenberg

craig · 2024-10-25T04:05:20Z

Build succeeded:

DarrylWong force-pushed the transient-error-string-match branch 3 times, most recently from a7042cd to e2f6f96 Compare October 17, 2024 17:28

DarrylWong force-pushed the transient-error-string-match branch from e2f6f96 to 1e3f6ff Compare October 17, 2024 17:31

DarrylWong force-pushed the transient-error-string-match branch from 1e3f6ff to cbc9880 Compare October 17, 2024 21:26

DarrylWong marked this pull request as ready for review October 17, 2024 21:31

DarrylWong requested review from a team as code owners October 17, 2024 21:31

DarrylWong requested review from srosenberg and vidit-bhat and removed request for a team October 17, 2024 21:31