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: make crdb crash on span-use-after-Finish #73941

Merged

Conversation

andreimatei
Copy link
Contributor

This patch makes roachtest pass an env var to crdb asking it to panic on
mis-use of tracing spans. I've been battling such bugs, which become
more problematic as I'm trying to introduce span reuse. In production
we'll probably continue tolerating such bugs for the time being, but I
want tests to yell. Unit tests are already running with this
use-after-Finish detection, and so far so good. I've done a manual run
of all the roachtests in this configuration and nothing crashed, so I
don't expect a tragedy.

Release note: None

@andreimatei andreimatei requested a review from tbg December 16, 2021 20:59
@andreimatei andreimatei requested review from a team as code owners December 16, 2021 20:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Dec 20, 2021

Can you run a manual nightly with this change first? We'll want to avoid a wave of nightly failures blanketing the various teams.

Also, the commit seems to be doing various things besides setting the env var the commit message mentions, mind either splitting up the commits + updating the PR message to describe all changes, or removing the stuff that doesn't apply to this commit?

@andreimatei andreimatei force-pushed the tracing.use-after-finish-roachtest branch from 7f34b48 to 84ff073 Compare December 20, 2021 15:58
This patch makes roachtest pass an env var to crdb asking it to panic on
mis-use of tracing spans. I've been battling such bugs, which become
more problematic as I'm trying to introduce span reuse. In production
we'll probably continue tolerating such bugs for the time being, but I
want tests to yell. Unit tests are already running with this
use-after-Finish detection, and so far so good. I've done a manual run
of all the roachtests in this configuration and nothing crashed, so I
don't expect a tragedy.

Release note: None
@andreimatei andreimatei force-pushed the tracing.use-after-finish-roachtest branch from 84ff073 to d38e4a6 Compare December 20, 2021 15:59
Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

Can you run a manual nightly with this change first? We'll want to avoid a wave of nightly failures blanketing the various teams.

I did (mentioned in the commit msg).

Also, the commit seems to be doing various things besides setting the env var the commit message mentions

Like what? The only thing that makes sense to split is adding a comment to COCKROACH_ENABLE_RPC_COMPRESSION. I split that one.
Perhaps you're referring to copy the flag defaults from roachprod to roachtest - but that's directly related to the wanting to pass one more env variable.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

@tbg
Copy link
Member

tbg commented Dec 20, 2021

I did (mentioned in the commit msg).

Oops, sorry.

Perhaps you're referring to copy the flag defaults from roachprod to roachtest - but that's directly related to the wanting to pass one more env variable.

Ah, I understand now. Thanks!

LGTM

Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

TFTR!

Did another run of all the roachtests, came out clean.

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@craig
Copy link
Contributor

craig bot commented Dec 21, 2021

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

@craig craig bot merged commit 299abc8 into cockroachdb:master Dec 21, 2021
@craig
Copy link
Contributor

craig bot commented Dec 21, 2021

Build succeeded:

@andreimatei andreimatei deleted the tracing.use-after-finish-roachtest branch January 26, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants