adapter: turmoil chaos test for builtin schema migrations #34110
Conversation
teskje
force-pushed
the
builtin-item-migration-turmoil
branch
2 times, most recently
from
81bd870 to
39a7377
Compare
teskje
force-pushed
the
builtin-item-migration-turmoil
branch
from
39a7377 to
c98c108
Compare
This commit reorganizes the builtin schema migration code by pulling all `Transaction` handling into the top-level `run` function. `Migration` becomes unaware of the existing of the `Transaction` and instead returns a `MigrationRunResult` that informs the caller about the changes that need to be applied. The reason for doing this is that it makes it easier to test the core migration code. In tests it's difficult to construct a `Transaction` object, and with this change we don't have to do that anymore, as long as we're invoking `Migration::run` directly.
teskje
force-pushed
the
builtin-item-migration-turmoil
branch
2 times, most recently
from
77a17d0 to
76b5841
Compare
This commit adds a turmoil-based chaos test for builtin schema migrations. It spawns a bunch of processes at different versions, defines some random builtin schema changes, and then performs migrations through all versions. For added chaos, process are randomly restarted all the time.
teskje
force-pushed
the
builtin-item-migration-turmoil
branch
from
76b5841 to
953c265
Compare
| for generation in 1..=NUM_VERSIONS { | ||
| let version = Version::new(0, generation, 0); | ||
|
|
There was a problem hiding this comment.
I wonder if it's worth adding jumps to these versions instead of having them linear?
There was a problem hiding this comment.
I don't think this would do anything! The migration logic doesn't look at the versions specifically, it only compares them, and for that it doesn't matter by how much they differ. Also note that the rng might decide to assign 0 migrations and 0 processes to a version, which effectively means skipping that version.
|
|
||
| /// Fuzz test builtin schema migration using turmoil. | ||
| #[test] // allow(test-attribute) | ||
| #[ignore = "runs forever"] |
There was a problem hiding this comment.
How does CI know to move on when this runs forever?
There was a problem hiding this comment.
It doesn't! That's why the test is ignored, so it doesn't run in CI. It's mostly useful for running locally, for fuzzing during development.
| #[cfg_attr(miri, ignore)] // too slow | ||
| fn test_builtin_schema_migration() { | ||
| const NUM_VERSIONS: u64 = 10; | ||
| const NUM_BUILTINS: u64 = 5; |
There was a problem hiding this comment.
Might be interesting to have a separate variable for the number of initial builtins to be 10x larger than the number of builtins we want to migrate. More representative of our actual system at least.
There was a problem hiding this comment.
I chose a small number of builtins to make it more likely that there are multiple migrations on the same builtin, which seems more interesting than having a single migration on many. So the values are chosen based on what I believe will find the most bugs, not what's closest to the situation in production.
The idea with these constants is that it should be easy to change them during development, to tune how much certain aspects should be stressed. If you think it would be worthwhile to have another configuration running in CI as well, lmk. We can make some of these arguments and then define different #[test]s that invoke the function with different arguments.
SangJunBak
left a comment
SangJunBak
left a comment
There was a problem hiding this comment.
Just some suggestions but otw lgtm!
|
TFTR! |
2 participants
This PR adds a turmoil-based chaos test for builtin schema migrations. It spawns a bunch of processes at different versions, defines some random builtin schema changes, and then performs migrations through all versions. For added chaos, processes are randomly restarted all the time.
The first commit adjusts the builtin schema migration code by pulling the
Transactionhandling out of the core migration logic. Turns out that it isn't easy to construct aTransactionin tests, so pulling it to the edge makes the test code simpler.Motivation
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.