data migration tests: run steps twice to check for idempotency - #11193
data migration tests: run steps twice to check for idempotency#11193jgallagher wants to merge 3 commits into
Conversation
|
Maybe obvious, but this will only catch data-modifying steps that are accidentally non-idempotent if we've written data migration tests for them. |
This is definitely viable: the newest migration affected here (v210) shipped in R18. |
There are docs for this in https://github.com/oxidecomputer/omicron/tree/main/schema/crdb#21-updating-the-baseline-after-a-release I've had success one-shotting this (it's mechanical) using claude via: |
| } | ||
|
|
||
| apply_update(log, &crdb, version, 1).await; | ||
| apply_update(log, &crdb, version, 2).await; |
There was a problem hiding this comment.
This makes total sense, but could use a comment to explain "why".
(it makes sense that our data-less schema changes tests, probing at idempotency, don't see failure when operating on zero rows)
bnaecker
left a comment
bnaecker
left a comment
There was a problem hiding this comment.
This seems like a good change overall.
I'm not sure about changing past migrations. On the one hand, I don't they could have succeeded if there were conflicts. On the other, we're changing them at all, which is a little sketchy.
Moving the "base" version forward past these is also fine, as long as we confirm there aren't any impacted customers. I know we still had some on R17 just a few weeks ago.
This came up in #11114 (comment); if I run this change against that branch in its initial state, we do see a test failure as described in that conversation: ``` thread 'integration_tests::schema::validate_data_migrations' (1556916) panicked at nexus/tests/integration_tests/schema.rs:71:9: Failed to execute update step up02.sql: db error: ERROR: duplicate key value violates unique constraint "inv_omicron_sled_config_zone_external_ip_pkey" DETAIL: Key (inv_collection_id,sled_config_id,zone_id,ip)=('d29f7b65-9b2b-4650-9b80-9ba968468083','1fda886f-b3e4-4068-9e20-b29d7d4c9190','ab1d1dc9-2737-4c60-8aeb-f1da4ac92d93','192.0.2.1') already exists. ``` Unfortunately, this also causes some already-shipped migrations to fail. I patched those up by adding `ON CONFLICT DO NOTHING`; retroactively changing migrations seems quite spicy, so I welcome other suggestions. (We could trim our start point up past these two instead?)
48bfe80 to
820f148
Compare
Yeah it is definitely sketchy. I've rebased onto #11195, so we no longer need to do this.
I believe everyone is now on at least R20. (Although: I think this would have still been fine, since R18-R22 still contained all the migrations needed to upgrade from R17, and we don't allow skipping major versions.) |
smklein
left a comment
smklein
left a comment
There was a problem hiding this comment.
LGTM, with the approach of "move the base" rather than re-writing history
3 participants
This came up in #11114 (comment); if I run this change against that branch in its initial state, we do see a test failure as described in that conversation:
Unfortunately, this also causes some already-shipped migrations to fail. I patched those up by adding
ON CONFLICT DO NOTHING; retroactively changing migrations seems quite spicy, so I welcome other suggestions. (We could trim our start point up past these two instead?)