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: guard against nil log file reference #112343

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

renatolabs
Copy link
Contributor

These checks prevent panics in the roachtes test runner, which could jeopardize an entire test run; if the checks fail, we might not create ".failed" files for commands that fail.

Example panic:

https://teamcity.cockroachdb.com/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=12162051&_focus=7888

Epic: none

Release note: None

These checks prevent panics in the roachtes test runner, which could
jeopardize an entire test run; if the checks fail, we might not create
".failed" files for commands that fail.

Example panic:

https://teamcity.cockroachdb.com/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=12162051&_focus=7888

Epic: none

Release note: None
@renatolabs renatolabs requested a review from a team as a code owner October 13, 2023 20:01
@renatolabs renatolabs requested review from srosenberg and DarrylWong and removed request for a team October 13, 2023 20:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@renatolabs
Copy link
Contributor Author

Similar changes have been made in the past: #93124. I don't completely understand the exact circumstances that cause the (*clusterImpl).l instance to be missing a file handle. Ideally, we would fix that bug instead. In practice, figuring out the reason and making sure we apply the correct change is going to be tricky given the state of test_runner so, given the low stakes at play (missing .failed file), this quick fix is probably a good trade-off.

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

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

I seem to recall chasing the root cause for these but not succeeding... let's leave it as an exercise for our next intern; kidding. The workaround is sensible, eventually we'll find the culprit.

@renatolabs renatolabs added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Oct 13, 2023
@renatolabs
Copy link
Contributor Author

TFTR!

bors r=srosenberg

@craig
Copy link
Contributor

craig bot commented Oct 13, 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 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants