Support multiple external addresses in database inventory tables - #11114
Conversation
c22a827 to
1ed5074
Compare
- Add a new table `inv_omicron_sled_config_zone_external_ip` to store the external IP information for inventoried zones in `inv_omicron_sled_config_zone`. This lets us support multiple IPs in the table, each with a reference back to the inventory collection and zone it came from. - Migrate schema and old data. This moves the "second service IP" information to the new table, only for the zones where that IP is an external address (Nexus, External DNS, and Boundary NTP). This also drops the now-unused SNAT related columns, but leaves the `second_service_ip` column for things like internal DNS's additional underlay address. - Add model types and update a bunch of callsites and tests.
1ed5074 to
e5d565d
Compare
| inv_collection_id, sled_config_id, id, snat_ip, | ||
| NULL::INT4, snat_first_port, snat_last_port | ||
| FROM omicron.public.inv_omicron_sled_config_zone | ||
| WHERE zone_type = 'boundary_ntp' AND snat_ip IS NOT NULL; |
There was a problem hiding this comment.
Does this need an ON CONFLICT DO NOTHING to be idempotent? (up02.sql runs and commits, then Nexus crashes before we move to up03.sql - I think we'll rerun up02.sql again and try to insert the same rows?)
There was a problem hiding this comment.
I don't think so. The update_since_base_has_idempotent_up() test applies the schema migrations twice, and we don't fail here. I think that's because each schema migration, as a whole, is applied inside a BEGIN/COMMIT transaction. When we parse the update SQL files, we determine if they're transactional based on a special suffix to the filenames. This doesn't have that, so everything is run in one transaction.
There was a problem hiding this comment.
Hmm, I'm not sure.
The update_since_base_has_idempotent_up() test applies the schema migrations twice, and we don't fail here.
I don't think there's any relevant data in the tables during that test, so this INSERT wouldn't insert anything that could possible conflict. That test is really only checking that structural changes are idempotent.
I think that's because each schema migration, as a whole, is applied inside a BEGIN/COMMIT transaction. When we parse the update SQL files, we determine if they're transactional based on a special suffix to the filenames. This doesn't have that, so everything is run in one transaction.
My concern is that Nexus recording which step of the migration it's on is not in the same transaction as the migration itself, so any upXX.sql can be run multiple times (even if it already committed successfully). E.g., this sequence:
- Nexus prepares to run up01.sql.
- Nexus applies up01.sql.
- Nexus prepare up02.sql.
- Nexus applies up02.sql. This inserts all the relevant rows, transactionally.
- Nexus crashes before it prepares up03.sql.
When Nexus restarts, I believe it will not know whether up02.sql was applied or not, so it will rerun it. At this point migration will fail because trying to insert the same rows again will conflict, right?
There was a problem hiding this comment.
Good point about the test, I agree there's no data and so nothing to conflict either way.
When Nexus restarts, I believe it will not know whether up02.sql was applied or not, so it will rerun it. At this point migration will fail because trying to insert the same rows again will conflict, right?
Ah I think I confused myself into believing the whole schema migration is transactional. But it's only each step. I agree, this is a problem. I might try to add a test, but will also add an ON CONFLICT clause either way.
- Add contraints around new table's port columns - Some typos
|
Adding a specific test to catch conflicts in the data migration is essentially John's #11193, and it requires figuring out what to do about the past migrations that need an |
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?)
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?)
3 participants
inv_omicron_sled_config_zone_external_ipto store the external IP information for inventoried zones ininv_omicron_sled_config_zone. This lets us support multiple IPs in the table, each with a reference back to the inventory collection and zone it came from.second_service_ipcolumn for things like internal DNS's additional underlay address.