Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachtest: handle panics in mixedversion #105630

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

renatolabs
Copy link
Contributor

Previously, a panic in a user function in a roachtest using the mixedversion package would crash the entire roachtest process. This is because all steps run in a separate goroutine, so if panics are not captured, the entire process crashes.

This commit updates the test runner so that all steps (including those that are part of the test infrastructure) run with panics captured. The panic message is returned as a regular error which should lead to usual GitHub error reports. The stack trace for the panic is also logged so that we can pinpoint the exact offending line in the test.

Epic: CRDB-19321

Release note: None

@renatolabs renatolabs requested a review from a team as a code owner June 27, 2023 15:53
@renatolabs renatolabs requested review from herkolategan and srosenberg and removed request for a team June 27, 2023 15:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@renatolabs renatolabs added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jun 27, 2023
@renatolabs renatolabs force-pushed the rc/mixedversion-panics branch from 235d7ad to 96ca1cc Compare June 27, 2023 16:07
Previously, a panic in a user function in a roachtest using the
`mixedversion` package would crash the entire roachtest process. This
is because all steps run in a separate goroutine, so if panics are not
captured, the entire process crashes.

This commit updates the test runner so that all steps (including those
that are part of the test infrastructure) run with panics captured.
The panic message is returned as a regular error which should lead to
usual GitHub error reports. The stack trace for the panic is also
logged so that we can pinpoint the exact offending line in the test.

Epic: CRDB-19321

Release note: None
@renatolabs renatolabs force-pushed the rc/mixedversion-panics branch from 96ca1cc to 8ca2fc4 Compare June 27, 2023 16:54
Copy link
Contributor

@smg260 smg260 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @srosenberg)

@renatolabs
Copy link
Contributor Author

TFTR!

bors r=smg260

@craig
Copy link
Contributor

craig bot commented Jun 27, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants