roachtest: print global seed to test.log #113950
Conversation
DarrylWong
requested review from
herkolategan and
renatolabs
and removed request for
a team
|
This change is |
DarrylWong
added
backport-23.1.x
pkg/cmd/roachtest/test_impl.go
Outdated
| @@ -151,6 +152,7 @@ func (t *testImpl) Cockroach() string { | |||
| return t.StandardCockroach() | |||
| } | |||
| t.randomCockroachOnce.Do(func() { | |||
| t.l.Printf("GLOBAL SEED: %d", roachtestflags.GlobalSeed) | |||
There was a problem hiding this comment.
Is ROACHTEST GLOBAL SEED a better message? (Up to you)
There was a problem hiding this comment.
I don't think this is the seed we should be logging. It would be more useful to log the actual seed passed to the cockroach process (i.e., what cockroachRandomSeed() returns) so that people can copy and paste that when trying to reproduce issues.
There was a problem hiding this comment.
Yeah you're right. I'm wondering now what the use case of ever setting the global seed would be though. I guess there's still randomization that happens outside of the cockroach process that we'd occasionally like to control?
There was a problem hiding this comment.
I'm wondering now what the use case of ever setting the global seed would be though
It's now become a relic from pre-Go 1.20 days, when the seed would have a constant default.
The math/rand package now automatically seeds the global random number generator (used by top-level functions like Float64 and Int) with a random value, and the top-level Seed function has been deprecated
https://tip.golang.org/doc/go1.20
So I'd say it's probably still fine to continue setting the global default while 23.1 is in support, but we don't need to log it as it's not useful data.
I guess there's still randomization that happens outside of the cockroach process that we'd occasionally like to control?
Yeah, setting the global RNG would have no effect on cockroach processes, it would just change the random values observed by tests. Unfortunately, a lot of tests (and even roachtest itself) make use of the global RNG.
There was a problem hiding this comment.
Ahh I see, I think setting global seed affected this call that determines COCKROACH_RANDOM_SEED
seed := rand.Int63()
Which explains why setting either one was working for me in my testing. cockroachRandomSeed() is definitely the way to go though.
Quick sanity run on gceworker shows that it prints it to test.log + correctly reflects the seed passed to ROACHTEST_ASSERTIONS_ENABLED_SEED.
This change prints the cockroach random seed used by metamorphic builds to test.log for ease of debugging. Before this seed was found only in cockroach.log. Release note: none
| @@ -1936,6 +1936,7 @@ func (c *clusterImpl) StartE( | |||
| // Set the same seed on every node, to be used by builds with | |||
| // runtime assertions enabled. | |||
| setUnlessExists("COCKROACH_RANDOM_SEED", c.cockroachRandomSeed()) | |||
| l.Printf("ROACHTEST COCKROACH_RANDOM_SEED: %d", c.cockroachRandomSeed()) | |||
There was a problem hiding this comment.
Two things to consider about this approach:
- It will be logged even if we are not using a metamorphic build.
- It will be logged every time we start cockroach, which could be several times in a test (especially in upgrade tests).
Maybe the best place to put this type of message is during cluster setup (only once); it could even be done as part of your work in #113301, where there's a pretty well-defined moment where we decide if we are running with metamorphic builds or not (and that only happens once during a test's lifetime).
Thoughts?
Nit: I would also slightly change the messaging, something like to reproduce failures using the same metamorphic constants, run this test with %s=%d (roachtest env var, seed). Doesn't have to be this exactly, but it would be nice to convey that information in the log message.
There was a problem hiding this comment.
Sounds good to me
|
Closing and rolling changes into #113301 |
This change prints the global seed used by metamorphic builds to test.log for ease of debugging. Before this seed was found only in cockroach.log.
Release note: none
Epic: none