From f408ba39b4ea68ba9af2d7299a4d31cdf0adcf26 Mon Sep 17 00:00:00 2001 From: Levon Tarver Date: Fri, 9 Feb 2024 04:12:38 +0000 Subject: [PATCH] More bugfixes The good: * Added better comments and more thorough testing. The bad: The bugfix had bugs. * We're supposed to SELECT FROM public.omicron.ipv4_nat_entry, but I wrote SELECT FROM ipv4_nat_entry. @dap explained a little while back that the fully qualified path needs to be present when the db is being initialized because we're instantiating the tables from a different namespace. This is what is breaking CI. * In the view I wrote version_added AS version twice, when the second statement should be version_removed AS version. It actually works correctly during initial dendrite startup, but will probably have issues with subsequent NAT updates. The more thorough testing catches these cases now. --- nexus/db-model/src/schema.rs | 4 +- .../src/db/datastore/ipv4_nat_entry.rs | 60 ++++++++++++++++++- schema/crdb/33.0.1/up01.sql | 1 + schema/crdb/33.0.1/up02.sql | 60 +++++++++++++++++++ schema/crdb/dbinit.sql | 28 +++++++-- 5 files changed, 145 insertions(+), 8 deletions(-) create mode 100644 schema/crdb/33.0.1/up01.sql create mode 100644 schema/crdb/33.0.1/up02.sql diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 36f7f72993..7fc4f9ae45 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -13,7 +13,7 @@ use omicron_common::api::external::SemverVersion; /// /// 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(33, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(33, 0, 1); table! { disk (id) { @@ -546,7 +546,7 @@ table! { } } -// View used for summarzing changes to ipv4_nat_entry +// View used for summarizing changes to ipv4_nat_entry table! { ipv4_nat_changes (version) { external_address -> Inet, diff --git a/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs b/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs index 39a4fb1e71..b931452c13 100644 --- a/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs +++ b/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs @@ -872,17 +872,75 @@ mod test { datastore.ipv4_nat_delete(&opctx, entry).await.unwrap(); } - // ensure that the changeset is ordered + // get the new state of all nat entries + // note that this is not the method under test + let db_records = datastore + .ipv4_nat_list_since_version(&opctx, 0, 300) + .await + .unwrap(); + + // ensure that the changeset is ordered, displaying the correct + // version numbers, and displaying the correct `deleted` status let mut version = 0; let limit = 100; let mut changes = datastore.ipv4_nat_changeset(&opctx, version, limit).await.unwrap(); while !changes.is_empty() { + // check ordering assert!(changes .windows(2) .all(|entries| entries[0].gen < entries[1].gen)); + // check deleted status and version numbers + changes.iter().for_each(|change| match change.deleted { + true => { + // version should match a deleted entry + let deleted_nat = db_records + .iter() + .find(|entry| entry.version_removed == Some(change.gen)) + .expect("did not find a deleted nat entry with a matching version number"); + + assert_eq!( + deleted_nat.external_address.ip(), + change.external_address + ); + assert_eq!( + deleted_nat.first_port, + change.first_port.into() + ); + assert_eq!(deleted_nat.last_port, change.last_port.into()); + assert_eq!( + deleted_nat.sled_address.ip(), + change.sled_address + ); + assert_eq!(*deleted_nat.mac, change.mac); + assert_eq!(deleted_nat.vni.0, change.vni); + } + false => { + // version should match an active nat entry + let added_nat = db_records + .iter() + .find(|entry| entry.version_added == change.gen) + .expect("did not find an active nat entry with a matching version number"); + + assert!(added_nat.version_removed.is_none()); + + assert_eq!( + added_nat.external_address.ip(), + change.external_address + ); + assert_eq!(added_nat.first_port, change.first_port.into()); + assert_eq!(added_nat.last_port, change.last_port.into()); + assert_eq!( + added_nat.sled_address.ip(), + change.sled_address + ); + assert_eq!(*added_nat.mac, change.mac); + assert_eq!(added_nat.vni.0, change.vni); + } + }); + version = changes.last().unwrap().gen; changes = datastore .ipv4_nat_changeset(&opctx, version, limit) diff --git a/schema/crdb/33.0.1/up01.sql b/schema/crdb/33.0.1/up01.sql new file mode 100644 index 0000000000..354480c0c9 --- /dev/null +++ b/schema/crdb/33.0.1/up01.sql @@ -0,0 +1 @@ +DROP VIEW IF EXISTS omicron.public.ipv4_nat_changes; diff --git a/schema/crdb/33.0.1/up02.sql b/schema/crdb/33.0.1/up02.sql new file mode 100644 index 0000000000..5a2a183f4c --- /dev/null +++ b/schema/crdb/33.0.1/up02.sql @@ -0,0 +1,60 @@ +/* + * A view of the ipv4 nat change history + * used to summarize changes for external viewing + */ +CREATE VIEW IF NOT EXISTS omicron.public.ipv4_nat_changes +AS +-- Subquery: +-- We need to be able to order partial changesets. ORDER BY on separate columns +-- will not accomplish this, so we'll do this by interleaving version_added +-- and version_removed (version_removed taking priority if NOT NULL) and then sorting +-- on the appropriate version numbers at call time. +WITH interleaved_versions AS ( + -- fetch all active NAT entries (entries that have not been soft deleted) + SELECT + external_address, + first_port, + last_port, + sled_address, + vni, + mac, + -- rename version_added to version + version_added AS version, + -- create a new virtual column, boolean value representing whether or not + -- the record has been soft deleted + (version_removed IS NOT NULL) as deleted + FROM omicron.public.ipv4_nat_entry + WHERE version_removed IS NULL + + -- combine the datasets, unifying the version_added and version_removed + -- columns to a single `version` column so we can interleave and sort the entries + UNION + + -- fetch all inactive NAT entries (entries that have been soft deleted) + SELECT + external_address, + first_port, + last_port, + sled_address, + vni, + mac, + -- rename version_removed to version + version_removed AS version, + -- create a new virtual column, boolean value representing whether or not + -- the record has been soft deleted + (version_removed IS NOT NULL) as deleted + FROM omicron.public.ipv4_nat_entry + WHERE version_removed IS NOT NULL +) +-- this is our new "table" +-- here we select the columns from the subquery defined above +SELECT + external_address, + first_port, + last_port, + sled_address, + vni, + mac, + version, + deleted +FROM interleaved_versions; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 880b89e108..18b1b82563 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3436,13 +3436,19 @@ STORING ( time_deleted ); -/** +/* * A view of the ipv4 nat change history * used to summarize changes for external viewing */ CREATE VIEW IF NOT EXISTS omicron.public.ipv4_nat_changes AS +-- Subquery: +-- We need to be able to order partial changesets. ORDER BY on separate columns +-- will not accomplish this, so we'll do this by interleaving version_added +-- and version_removed (version_removed taking priority if NOT NULL) and then sorting +-- on the appropriate version numbers at call time. WITH interleaved_versions AS ( + -- fetch all active NAT entries (entries that have not been soft deleted) SELECT external_address, first_port, @@ -3450,13 +3456,19 @@ WITH interleaved_versions AS ( sled_address, vni, mac, + -- rename version_added to version version_added AS version, + -- create a new virtual column, boolean value representing whether or not + -- the record has been soft deleted (version_removed IS NOT NULL) as deleted - FROM ipv4_nat_entry + FROM omicron.public.ipv4_nat_entry WHERE version_removed IS NULL + -- combine the datasets, unifying the version_added and version_removed + -- columns to a single `version` column so we can interleave and sort the entries UNION + -- fetch all inactive NAT entries (entries that have been soft deleted) SELECT external_address, first_port, @@ -3464,10 +3476,16 @@ WITH interleaved_versions AS ( sled_address, vni, mac, - version_added AS version, + -- rename version_removed to version + version_removed AS version, + -- create a new virtual column, boolean value representing whether or not + -- the record has been soft deleted (version_removed IS NOT NULL) as deleted - FROM ipv4_nat_entry WHERE version_removed IS NOT NULL + FROM omicron.public.ipv4_nat_entry + WHERE version_removed IS NOT NULL ) +-- this is our new "table" +-- here we select the columns from the subquery defined above SELECT external_address, first_port, @@ -3486,7 +3504,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '33.0.0', NULL) + ( TRUE, NOW(), NOW(), '33.0.1', NULL) ON CONFLICT DO NOTHING; COMMIT;