From f5d8e09637886afefcdc7b1f54f9d4739d03e2b8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 28 Aug 2023 19:05:41 -0500 Subject: [PATCH] [nexus] Silo IP pools schema change (#3981) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is quite small, but I'm pretty confident it's a sufficient basis for the rest of the work in #3926, so we might as well review and merge already. A couple of minor pain points I ran into while doing this: * Needed guidance on version number (3.0.0 it is) — it will be more obvious when there are more, but I added a line to the readme anyway * Diff output for [`dbinit_equals_sum_of_all_up`](https://github.com/oxidecomputer/omicron/blob/fb16870de8c0dba92d0868a984dd715749141b73/nexus/tests/integration_tests/schema.rs#L601) is terrible — thousands of lines with only a few relevant ones. It turns out the column order matters, so if you add columns to a table in a migration, you have to add them to the _end_ of the `create table` in `dbinit.sql`. --- nexus/db-model/src/ip_pool.rs | 16 ++++++++++++++++ nexus/db-model/src/schema.rs | 4 +++- schema/crdb/3.0.0/up.sql | 36 +++++++++++++++++++++++++++++++++++ schema/crdb/README.adoc | 1 + schema/crdb/dbinit.sql | 24 ++++++++++++++++++++--- 5 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 schema/crdb/3.0.0/up.sql diff --git a/nexus/db-model/src/ip_pool.rs b/nexus/db-model/src/ip_pool.rs index 29c5f323f4..ca1d1d55fe 100644 --- a/nexus/db-model/src/ip_pool.rs +++ b/nexus/db-model/src/ip_pool.rs @@ -22,6 +22,10 @@ use std::net::IpAddr; use uuid::Uuid; /// An IP Pool is a collection of IP addresses external to the rack. +/// +/// IP pools can be external or internal. External IP pools can be associated +/// with a silo or project so that instance IP allocation draws from that pool +/// instead of a system pool. #[derive(Queryable, Insertable, Selectable, Clone, Debug, Resource)] #[diesel(table_name = ip_pool)] pub struct IpPool { @@ -37,6 +41,16 @@ pub struct IpPool { /// Child resource generation number, for optimistic concurrency control of /// the contained ranges. pub rcgen: i64, + + /// Silo, if IP pool is associated with a particular silo. Must be null + /// if internal is true. Must be non-null if project_id is non-null. When + /// project_id is non-null, silo_id will (naturally) be the ID of the + /// project's silo. + pub silo_id: Option, + + /// Project, if IP pool is associated with a particular project. Must be + /// null if internal is true. + pub project_id: Option, } impl IpPool { @@ -51,6 +65,8 @@ impl IpPool { ), internal, rcgen: 0, + silo_id: None, + project_id: None, } } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index d5364cae87..e81722d3be 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -454,6 +454,8 @@ table! { time_deleted -> Nullable, internal -> Bool, rcgen -> Int8, + silo_id -> Nullable, + project_id -> Nullable, } } @@ -1129,7 +1131,7 @@ table! { /// /// This should be updated whenever the schema is changed. For more details, /// refer to: schema/crdb/README.adoc -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(2, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(3, 0, 0); allow_tables_to_appear_in_same_query!( system_update, diff --git a/schema/crdb/3.0.0/up.sql b/schema/crdb/3.0.0/up.sql new file mode 100644 index 0000000000..b2883a933a --- /dev/null +++ b/schema/crdb/3.0.0/up.sql @@ -0,0 +1,36 @@ +-- CRDB documentation recommends the following: +-- "Execute schema changes either as single statements (as an implicit transaction), +-- or in an explicit transaction consisting of the single schema change statement." +-- +-- For each schema change, we transactionally: +-- 1. Check the current version +-- 2. Apply the idempotent update + +BEGIN; + +SELECT CAST( + IF( + ( + SELECT version = '2.0.0' and target_version = '3.0.0' + FROM omicron.public.db_metadata WHERE singleton = true + ), + 'true', + 'Invalid starting version for schema change' + ) AS BOOL +); + +ALTER TABLE omicron.public.ip_pool + ADD COLUMN IF NOT EXISTS silo_ID UUID, + ADD COLUMN IF NOT EXISTS project_id UUID, + + -- if silo_id is null, then project_id must be null + ADD CONSTRAINT IF NOT EXISTS project_implies_silo CHECK ( + NOT ((silo_id IS NULL) AND (project_id IS NOT NULL)) + ), + + -- if internal = true, non-null silo_id and project_id are not allowed + ADD CONSTRAINT IF NOT EXISTS internal_pools_have_null_silo_and_project CHECK ( + NOT (INTERNAL AND ((silo_id IS NOT NULL) OR (project_id IS NOT NULL))) + ); + +COMMIT; diff --git a/schema/crdb/README.adoc b/schema/crdb/README.adoc index d33a70e8f7..997170469d 100644 --- a/schema/crdb/README.adoc +++ b/schema/crdb/README.adoc @@ -44,6 +44,7 @@ Assumptions: Process: +* Choose a `NEW_VERSION` number. This should almost certainly be a major version bump over `OLD_VERSION`. * Add a file to `schema/crdb/NEW_VERSION/up.sql` with your changes to the schema. ** This file should validate the expected current version transactionally. ** This file should only issue a single schema-modifying statement per transaction. diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 249f22bfdf..91710a5c00 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1480,12 +1480,30 @@ CREATE TABLE IF NOT EXISTS omicron.public.ip_pool ( time_modified TIMESTAMPTZ NOT NULL, time_deleted TIMESTAMPTZ, - /* Identifies if the IP Pool is dedicated to Control Plane services */ internal BOOL NOT NULL, /* The collection's child-resource generation number */ - rcgen INT8 NOT NULL + rcgen INT8 NOT NULL, + + /* + * Fields representating association with a silo or project. silo_id must be + * non-null if project_id is non-null. When project_id is non-null, silo_id + * will (naturally) be the ID of the project's silo. Both must be null if + * internal is true, i.e., internal IP pools must be fleet-level pools. + */ + silo_id UUID, + project_id UUID, + + -- if silo_id is null, then project_id must be null + CONSTRAINT project_implies_silo CHECK ( + NOT ((silo_id IS NULL) AND (project_id IS NOT NULL)) + ), + + -- if internal = true, non-null silo_id and project_id are not allowed + CONSTRAINT internal_pools_have_null_silo_and_project CHECK ( + NOT (INTERNAL AND ((silo_id IS NOT NULL) OR (project_id IS NOT NULL))) + ) ); /* @@ -2544,7 +2562,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '2.0.0', NULL) + ( TRUE, NOW(), NOW(), '3.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;