roachtest: streamline debug collection #36562
Conversation
The case in which a test hit an error and the case in which a test times out were handled differently and I had little confidence that it was working as intended. This simplifies by not running the test in the main goroutine, but reserving the main goroutine for defering the debug and cleanup actions while the test itself is now running in a child goroutine. I tested this running `tpcc/nodes=3/w=headroom` locally (where it runs with one warehouse and only for a minute) in the passing case and in the case in which it fails with an artificially low timeout. Release note: None
|
This change is |
| } | ||
| // NB: c.destroyed is nil for cloned clusters (i.e. in subtests). | ||
| if !debugEnabled && c.destroyed != nil { | ||
| c.Destroy(ctx) |
There was a problem hiding this comment.
PS I'm unclear why there's this random c.Destroy call that only fires on a timeout. Perhaps someone can educate me. I thought destroying would be left to a higher power. Perhaps we can just stop in all cases and don't need to track the bool?
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)
pkg/cmd/roachtest/test.go, line 1190 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
PS I'm unclear why there's this random
c.Destroycall that only fires on a timeout. Perhaps someone can educate me. I thought destroying would be left to a higher power. Perhaps we can just stop in all cases and don't need to track the bool?
As I've mentioned to @andreimatei before, this code has evolved into the current mess and deserves to be rewritten, but there is a high probability of fallout from any changes.
pkg/cmd/roachtest/test.go, line 1230 at r1 (raw file):
t.mu.Unlock() // Run the test itself in a goroutine. The main goroutine is in charge
Does this muck with the test.runner stacktrace magic? See test.decorate. Also, test.runnerID is supposed to be the ID of the goroutine running the test. See test.Status, and test.Progress. I suspect switching to running the test in a goroutine is going to have a bunch of fallout.
There was a problem hiding this comment.
Let's not change the existing code any more. I'm trying to get my rewrite merged asap.
Reviewable status:
Inspired by 1.
The case in which a test hit an error and the case in which a test
times out were handled differently and I had little confidence that
it was working as intended.
This simplifies by not running the test in the main goroutine, but
reserving the main goroutine for defering the debug and cleanup
actions while the test itself is now running in a child goroutine.
I tested this running
tpcc/nodes=3/w=headroomlocally (where itruns with one warehouse and only for a minute) in the passing case
and in the case in which it fails with an artificially low timeout.
Release note: None