-
Notifications
You must be signed in to change notification settings - Fork 63
Description
This bit of data migration from #4261 did not behave as expected on dogfood. The default pool was a fleet-level default, and there was a second pool oxide-pool associated directly with the oxide silo but as non-default. In this situation, as the comment describes, the migration ends up associating both pool default and oxide-pool with silo oxide, with default keeping is_default = true and oxide-pool getting is_default = false. When we ran the update, however, both had is_default = false, leaving silo oxide without a default pool.
omicron/schema/crdb/23.0.0/up4.sql
Lines 14 to 28 in 624fbba
| -- Special handling is required for conflicts between a fleet default and a | |
| -- silo default. If pool P1 is a fleet default and pool P2 is a silo default | |
| -- on silo S1, we cannot link both to S1 with is_default = true. What we | |
| -- really want in that case is: | |
| -- | |
| -- row 1: (P1, S1, is_default=false) | |
| -- row 2: (P2, S1, is_default=true) | |
| -- | |
| -- i.e., we want to link both, but have the silo default take precedence. The | |
| -- AND NOT EXISTS here causes is_default to be false in row 1 if there is a | |
| -- conflicting silo default pool. row 2 is inserted in up5. | |
| p.is_default AND NOT EXISTS ( | |
| SELECT 1 FROM omicron.public.ip_pool | |
| WHERE silo_id = s.id AND is_default | |
| ) |
This is despite our lovely test for this very scenario. Indeed, this is the change these tests were added for.
omicron/nexus/tests/integration_tests/schema.rs
Lines 1022 to 1026 in 624fbba
| // pool3 did not previously have a corresponding silo, so now it's associated | |
| // with both silos as a new resource in each. | |
| // | |
| // Additionally, silo1 already had a default pool (pool1), but silo2 did | |
| // not have one. As a result, pool3 becomes the new default pool for silo2. |