From ee0f363378a039a3d5e179ed5a22b83c9dcae0fb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Dec 2023 11:07:03 -0600 Subject: [PATCH] clarify and clean up migrations, remove a couple of TODOs --- nexus/db-queries/src/db/datastore/ip_pool.rs | 2 +- nexus/db-queries/src/db/datastore/rack.rs | 8 ++---- schema/crdb/21.0.0/up4.sql | 26 +++++++++++--------- schema/crdb/21.0.0/up5.sql | 8 +++--- schema/crdb/21.0.0/up6.sql | 10 +++++--- 5 files changed, 30 insertions(+), 24 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index b98e171989..fc47db9f09 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/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()), ), ), }) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index ca50607b29..50bae03c2d 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/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); diff --git a/schema/crdb/21.0.0/up4.sql b/schema/crdb/21.0.0/up4.sql index a5ba739755..51cdaaa4f3 100644 --- a/schema/crdb/21.0.0/up4.sql +++ b/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; diff --git a/schema/crdb/21.0.0/up5.sql b/schema/crdb/21.0.0/up5.sql index d5f86a8ca7..037274b015 100644 --- a/schema/crdb/21.0.0/up5.sql +++ b/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; diff --git a/schema/crdb/21.0.0/up6.sql b/schema/crdb/21.0.0/up6.sql index 570f3dba7b..f8b2e3ddf9 100644 --- a/schema/crdb/21.0.0/up6.sql +++ b/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;