clarify and clean up migrations, remove a couple of TODOs

nexus/db-queries/src/db/datastore/ip_pool.rs

@@ -600,7 +600,7 @@ impl DataStore {
                     // TODO: would be nice to put the actual names and/or ids in
                     // here but LookupType on each of the two silos doesn't have
                     // a nice to_string yet or a way of composing them
-                    LookupType::ByCompositeId(format!("(pool, silo)")),
+                    LookupType::ByCompositeId("(pool, silo)".to_string()),
                 ),
             ),
         })

nexus/db-queries/src/db/datastore/rack.rs

@@ -1330,9 +1330,7 @@ mod test {
         // been allocated as a part of the service IP pool.
         let (.., svc_pool) =
             datastore.ip_pools_service_lookup(&opctx).await.unwrap();
-        // TODO: do we care? we should just check that the name or ID of the
-        // pool itself matches the known name or ID of the service pool
-        // assert_eq!(svc_pool.silo_id, Some(*INTERNAL_SILO_ID));
+        assert_eq!(svc_pool.name().as_str(), "oxide-service-pool");
 
         let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await;
         assert_eq!(observed_ip_pool_ranges.len(), 1);
@@ -1534,9 +1532,7 @@ mod test {
         // allocated as a part of the service IP pool.
         let (.., svc_pool) =
             datastore.ip_pools_service_lookup(&opctx).await.unwrap();
-        // TODO: do we care? we should just check that the name or ID of the
-        // pool itself matches the known name or ID of the service pool
-        // assert_eq!(svc_pool.silo_id, Some(*INTERNAL_SILO_ID));
+        assert_eq!(svc_pool.name().as_str(), "oxide-service-pool");
 
         let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await;
         assert_eq!(observed_ip_pool_ranges.len(), 1);

schema/crdb/21.0.0/up4.sql

@@ -1,22 +1,26 @@
--- copy existing fleet associations into association table. treat all existing
--- pools as fleet-associated because that is the current behavior
+-- Fleet-scoped pools are going away, but we recreate the equivalent of a fleet
+-- link for existing fleet-scoped pools by associating them with every existing
+-- silo, i.e., inserting a row into the association table for each (pool, silo)
+-- pair.
+--
+-- Note 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 link it to S1 with is_default = false. So first, here we
+-- copy the "original" value of is_default to the link between P1 and S1. Then,
+-- in up5, we flip is_default to false on P1 when we see that P2 wants to be the
+-- default for S1.
 INSERT INTO omicron.public.ip_pool_resource (ip_pool_id, resource_type, resource_id, is_default)
 SELECT 
   p.id AS ip_pool_id,
   'silo' AS resource_type,
   s.id AS resource_id,
-  -- note problem solved by up5.sql after this regarding is_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 link
-  -- it to S1 with is_default = false. So first, here we copy the "original"
-  -- value of is_default, and then in up5 we flip is_default to false if there
-  -- is a conflicting default silo-linked pool
   p.is_default
-FROM ip_pool AS p
-CROSS JOIN silo AS s
+FROM omicron.public.ip_pool AS p
+CROSS JOIN omicron.public.silo AS s
 WHERE p.time_deleted IS null 
   AND p.silo_id IS null -- means it's a fleet pool
   AND s.time_deleted IS null
--- make this idempotent
+-- this makes it idempotent
 ON CONFLICT (ip_pool_id, resource_type, resource_id)
 DO NOTHING;

schema/crdb/21.0.0/up5.sql

@@ -1,10 +1,12 @@
--- turn any former fleet defaults into non-defaults if there's going to be a
--- silo conflicting with it
+-- Preemptively turn any former fleet defaults into non-defaults if there's
+-- going to be a silo conflicting with it after up6
 UPDATE omicron.public.ip_pool_resource AS ipr
 SET is_default = false
 FROM omicron.public.ip_pool as ip
-WHERE ipr.is_default = true
+WHERE ipr.resource_type = 'silo' -- technically unnecessary because there is only silo
+  AND ipr.is_default = true
   AND ip.is_default = true -- both being default is the conflict being resolved
+  AND ip.silo_id IS NOT NULL
   AND ip.silo_id = ipr.resource_id
   AND ip.id = ipr.ip_pool_id;
 

schema/crdb/21.0.0/up6.sql

@@ -1,9 +1,13 @@
 -- copy existing ip_pool-to-silo associations into association table
 INSERT INTO omicron.public.ip_pool_resource (ip_pool_id, resource_type, resource_id, is_default)
-SELECT id, 'silo', silo_id, is_default
-FROM ip_pool
+SELECT 
+  id as ip_pool_id, 
+  'silo' as resource_type,
+  silo_id as resource_id,
+  is_default
+FROM omicron.public.ip_pool AS ip
 WHERE silo_id IS NOT null
   AND time_deleted IS null
--- make this idempotent
+-- this makes it idempotent
 ON CONFLICT (ip_pool_id, resource_type, resource_id)
 DO NOTHING;