From 442bf251c281b11f3ce9ca184874716234f539d5 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 1 Oct 2024 13:03:57 -0700 Subject: [PATCH 1/2] [spr] changes to main this commit is based on Created using spr 1.3.6-beta.1 [skip ci] --- .../planning/src/blueprint_builder/builder.rs | 132 +++++---- nexus/reconfigurator/planning/src/planner.rs | 53 +--- .../types/src/deployment/network_resources.rs | 13 +- nexus/types/src/deployment/tri_map.rs | 279 +++++++++++++++++- 4 files changed, 374 insertions(+), 103 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 35512a50f3f..fd4b987ad9c 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -1699,7 +1699,6 @@ pub mod test { use omicron_test_utils::dev::test_setup_log; use slog_error_chain::InlineErrorChain; use std::collections::BTreeSet; - use std::mem; pub const DEFAULT_N_SLEDS: usize = 3; @@ -2073,14 +2072,22 @@ pub mod test { let logctx = test_setup_log(TEST_NAME); let (collection, input, _) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); - let input = { - // Clear out the external networking records from `input`, since - // we're building an empty blueprint. - let mut builder = input.into_builder(); - *builder.network_resources_mut() = - OmicronZoneNetworkResources::new(); - builder.build() - }; + + // Previously, we would clear out network resources from the planning + // input here. However, currently, while constructing the example + // system, network resources do not make their way into the planning + // input. So that operation was a no-op. + // + // For now, we just check that the network resources are empty. + // + // TODO: This is arguably a bug in how planning inputs are generated, + // and will hopefully be addressed in the future. + assert_eq!( + input.network_resources(), + &OmicronZoneNetworkResources::new(), + "input network resources should be empty -- \ + has the ExampleSystem logic been updated to populate them?" + ); // Start with an empty blueprint (sleds with no zones). let parent = BlueprintBuilder::build_empty_with_sleds_seeded( @@ -2155,14 +2162,23 @@ pub mod test { // Discard the example blueprint and start with an empty one. let (collection, input, _) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); - let input = { - // Clear out the external networking records from `input`, since - // we're building an empty blueprint. - let mut builder = input.into_builder(); - *builder.network_resources_mut() = - OmicronZoneNetworkResources::new(); - builder.build() - }; + + // Previously, we would clear out network resources from the planning + // input here. However, currently, while constructing the example + // system, network resources do not make their way into the planning + // input! So that operation was a no-op. + // + // For now, we just check that the network resources are empty. + // + // TODO: This is arguably a bug in how planning inputs are generated, + // and will hopefully be addressed in the future. + assert_eq!( + input.network_resources(), + &OmicronZoneNetworkResources::new(), + "input network resources should be empty -- \ + has the ExampleSystem logic been updated to populate them?" + ); + let parent = BlueprintBuilder::build_empty_with_sleds_seeded( input.all_sled_ids(SledFilter::Commissioned), "test", @@ -2205,7 +2221,7 @@ pub mod test { fn test_add_nexus_error_cases() { static TEST_NAME: &str = "blueprint_builder_test_add_nexus_error_cases"; let logctx = test_setup_log(TEST_NAME); - let (mut collection, mut input, mut parent) = + let (mut collection, input, mut parent) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); // Remove the Nexus zone from one of the sleds so that @@ -2219,50 +2235,35 @@ pub mod test { if zones.zones.zones.len() < nzones_before_retain { selected_sled_id = Some(*sled_id); // Also remove this zone from the blueprint. - let mut removed_nexus = None; parent .blueprint_zones .get_mut(sled_id) .expect("missing sled") .zones .retain(|z| match &z.zone_type { - BlueprintZoneType::Nexus(z) => { - removed_nexus = Some(z.clone()); - false - } + BlueprintZoneType::Nexus(_) => false, _ => true, }); - let removed_nexus = - removed_nexus.expect("removed Nexus from blueprint"); - - // Also remove this Nexus's external networking resources - // from `input`. - let mut builder = input.into_builder(); - let mut new_network_resources = - OmicronZoneNetworkResources::new(); - let old_network_resources = builder.network_resources_mut(); - for ip in old_network_resources.omicron_zone_external_ips() - { - if ip.ip.id() != removed_nexus.external_ip.id { - new_network_resources - .add_external_ip(ip.zone_id, ip.ip) - .expect("copied IP to new input"); - } - } - for nic in old_network_resources.omicron_zone_nics() { - if nic.nic.id.into_untyped_uuid() - != removed_nexus.nic.id - { - new_network_resources - .add_nic(nic.zone_id, nic.nic) - .expect("copied NIC to new input"); - } - } - mem::swap( - old_network_resources, - &mut new_network_resources, + + // Previously, we would clear out network resources from + // the planning input assigned to the removed zone. + // However, currently, while constructing the example + // system, network resources do not make their way into the + // planning input. So that operation was a no-op. + // + // For now, we just check that the network resources are + // empty. + // + // TODO: This is arguably a bug in how planning inputs are + // generated, and will hopefully be addressed in the + // future. + assert_eq!( + input.network_resources(), + &OmicronZoneNetworkResources::new(), + "input network resources should be empty -- \ + has the ExampleSystem logic been updated to \ + populate them?" ); - input = builder.build(); break; } @@ -2540,14 +2541,23 @@ pub mod test { // Discard the example blueprint and start with an empty one. let (collection, input, _) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); - let input = { - // Clear out the external networking records from `input`, since - // we're building an empty blueprint. - let mut builder = input.into_builder(); - *builder.network_resources_mut() = - OmicronZoneNetworkResources::new(); - builder.build() - }; + + // Previously, we would clear out network resources from the planning + // input here. However, currently, while constructing the example + // system, network resources do not make their way into the planning + // input. So that operation was a no-op. + // + // For now, we just check that the network resources are empty. + // + // TODO: This is arguably a bug in how planning inputs are generated, + // and will hopefully be addressed in the future. + assert_eq!( + input.network_resources(), + &OmicronZoneNetworkResources::new(), + "input network resources should be empty -- \ + has the ExampleSystem logic changed?" + ); + let parent = BlueprintBuilder::build_empty_with_sleds_seeded( input.all_sled_ids(SledFilter::Commissioned), "test", diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index f02d8ec3def..3cf006b3b43 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -769,12 +769,10 @@ mod test { use omicron_common::api::external::Generation; use omicron_common::disk::DiskIdentity; use omicron_test_utils::dev::test_setup_log; - use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; use std::collections::HashMap; - use std::mem; use std::net::IpAddr; use typed_rng::TypedUuidRng; @@ -983,41 +981,22 @@ mod test { blueprint.blueprint_zones.retain(|k, _v| keep_sled_id == *k); blueprint.blueprint_disks.retain(|k, _v| keep_sled_id == *k); - // Also remove all the networking resources for the zones we just - // stripped out; i.e., only keep those for `keep_sled_id`. - let mut new_network_resources = OmicronZoneNetworkResources::new(); - let old_network_resources = builder.network_resources_mut(); - for old_ip in old_network_resources.omicron_zone_external_ips() { - if blueprint.all_omicron_zones(BlueprintZoneFilter::All).any( - |(_, zone)| { - zone.zone_type - .external_networking() - .map(|(ip, _nic)| ip.id() == old_ip.ip.id()) - .unwrap_or(false) - }, - ) { - new_network_resources - .add_external_ip(old_ip.zone_id, old_ip.ip) - .expect("copied IP to new input"); - } - } - for old_nic in old_network_resources.omicron_zone_nics() { - if blueprint.all_omicron_zones(BlueprintZoneFilter::All).any( - |(_, zone)| { - zone.zone_type - .external_networking() - .map(|(_ip, nic)| { - nic.id == old_nic.nic.id.into_untyped_uuid() - }) - .unwrap_or(false) - }, - ) { - new_network_resources - .add_nic(old_nic.zone_id, old_nic.nic) - .expect("copied NIC to new input"); - } - } - mem::swap(old_network_resources, &mut &mut new_network_resources); + // Previously, we would clear out any network resources from the + // planning input that were assigned to sleds other than + // `keep_sled_id`. However, currently, while constructing the + // example system, network resources do not make their way into the + // planning input! So that operation was a no-op. + // + // For now, we just check that the network resources are empty. + // + // TODO: This is arguably a bug in how ExampleSystem is generated, + // and will hopefully be addressed in the future. + assert_eq!( + builder.network_resources_mut(), + &OmicronZoneNetworkResources::new(), + "input network resources should be empty -- \ + has the ExampleSystem logic been updated to populate them?" + ); (keep_sled_id, blueprint, collection, builder.build()) }; diff --git a/nexus/types/src/deployment/network_resources.rs b/nexus/types/src/deployment/network_resources.rs index 4afec0989b5..d8725743497 100644 --- a/nexus/types/src/deployment/network_resources.rs +++ b/nexus/types/src/deployment/network_resources.rs @@ -42,7 +42,7 @@ use thiserror::Error; /// /// So we use two separate maps for now. But a single map is always a /// possibility in the future, if required. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] pub struct OmicronZoneNetworkResources { /// external IPs allocated to Omicron zones omicron_zone_external_ips: TriMap, @@ -59,6 +59,11 @@ impl OmicronZoneNetworkResources { } } + pub fn is_empty(&self) -> bool { + self.omicron_zone_external_ips.is_empty() + && self.omicron_zone_nics.is_empty() + } + pub fn omicron_zone_external_ips( &self, ) -> impl Iterator + '_ { @@ -233,7 +238,7 @@ pub struct OmicronZoneExternalSnatIp { /// /// This is a slimmer `nexus_db_model::ServiceNetworkInterface` that only stores /// the fields necessary for blueprint planning. -#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub struct OmicronZoneNic { pub id: VnicUuid, pub mac: MacAddr, @@ -245,7 +250,7 @@ pub struct OmicronZoneNic { /// A pair of an Omicron zone ID and an external IP. /// /// Part of [`OmicronZoneNetworkResources`]. -#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)] pub struct OmicronZoneExternalIpEntry { pub zone_id: OmicronZoneUuid, pub ip: OmicronZoneExternalIp, @@ -276,7 +281,7 @@ impl TriMapEntry for OmicronZoneExternalIpEntry { /// A pair of an Omicron zone ID and a network interface. /// /// Part of [`OmicronZoneNetworkResources`]. -#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)] pub struct OmicronZoneNicEntry { pub zone_id: OmicronZoneUuid, pub nic: OmicronZoneNic, diff --git a/nexus/types/src/deployment/tri_map.rs b/nexus/types/src/deployment/tri_map.rs index e4ef320b4fd..82d16b90ef9 100644 --- a/nexus/types/src/deployment/tri_map.rs +++ b/nexus/types/src/deployment/tri_map.rs @@ -22,13 +22,76 @@ use serde::{Deserialize, Serialize, Serializer}; #[derive_where(Clone, Debug, Default)] pub(crate) struct TriMap { entries: Vec, - // Invariant: the value (usize) in these maps are valid indexes into + // Invariant: the values (usize) in these maps are valid indexes into // `entries`, and are a 1:1 mapping. k1_to_entry: HashMap, k2_to_entry: HashMap, k3_to_entry: HashMap, } +impl PartialEq for TriMap { + fn eq(&self, other: &Self) -> bool { + // Implementing PartialEq for TriMap is tricky because TriMap is not + // semantically like an IndexMap: two maps are equivalent even if their + // entries are in a different order. In other words, any permutation of + // entries is equivalent. + // + // We also can't sort the entries because they're not necessarily Ord. + // + // So we write a custom equality check that checks that each key in one + // map points to the same entry as in the other map. + + if self.entries.len() != other.entries.len() { + return false; + } + + // Walk over all the entries in the first map and check that they point + // to the same entry in the second map. + for (ix, entry) in self.entries.iter().enumerate() { + let k1 = entry.key1(); + let k2 = entry.key2(); + let k3 = entry.key3(); + + // Check that the indexes are the same in the other map. + let Some(other_ix1) = other.k1_to_entry.get(&k1).copied() else { + return false; + }; + let Some(other_ix2) = other.k2_to_entry.get(&k2).copied() else { + return false; + }; + let Some(other_ix3) = other.k3_to_entry.get(&k3).copied() else { + return false; + }; + + if other_ix1 != other_ix2 || other_ix1 != other_ix3 { + // All the keys were present but they didn't point to the same + // entry. + return false; + } + + // Check that the other map's entry is the same as this map's + // entry. (This is what we use the `PartialEq` bound on T for.) + // + // Because we've checked that other_ix1, other_ix2 and other_ix3 + // are Some(ix), we know that ix is valid and points to the + // expected entry. + let other_entry = &other.entries[other_ix1]; + if entry != other_entry { + eprintln!( + "mismatch: ix: {}, entry: {:?}, other_entry: {:?}", + ix, entry, other_entry + ); + return false; + } + } + + true + } +} + +// The Eq bound on T ensures that the TriMap forms an equivalence class. +impl Eq for TriMap {} + // Note: Eq and PartialEq are not implemented for TriMap. Implementing them // would need to be done with care, because TriMap is not semantically like an // IndexMap: two maps are equivalent even if their entries are in a different @@ -92,6 +155,10 @@ impl TriMap { } } + pub(crate) fn is_empty(&self) -> bool { + self.entries.is_empty() + } + pub(crate) fn iter(&self) -> impl Iterator { self.entries.iter() } @@ -255,6 +322,7 @@ impl std::error::Error for DuplicateEntry {} #[cfg(test)] mod tests { use super::*; + use prop::sample::SizeRange; use proptest::prelude::*; use test_strategy::{proptest, Arbitrary}; @@ -512,4 +580,213 @@ mod tests { } } } + + #[proptest(cases = 16)] + fn proptest_permutation_eq( + #[strategy(test_entry_permutation_strategy(0..256))] entries: ( + Vec, + Vec, + ), + ) { + let (entries1, entries2) = entries; + let mut map1 = TriMap::::new(); + let mut map2 = TriMap::::new(); + + for entry in entries1 { + map1.insert_no_dups(entry.clone()).unwrap(); + } + for entry in entries2 { + map2.insert_no_dups(entry.clone()).unwrap(); + } + + assert_eq_props(map1, map2); + } + + // Returns a pair of permutations of a set of unique entries. + fn test_entry_permutation_strategy( + size: impl Into, + ) -> impl Strategy, Vec)> { + prop::collection::vec(any::(), size.into()).prop_perturb( + |v, mut rng| { + // It is possible (likely even) that the input vector has + // duplicates. How can we remove them? The easiest way is to + // use the TriMap logic that already exists to check for + // duplicates. Insert all the entries one by one, then get the + // list. + let mut map = TriMap::::new(); + for entry in v { + // The error case here is expected -- we're actively + // de-duping entries right now. + _ = map.insert_no_dups(entry); + } + let v = map.entries; + + // Now shuffle the entries. This is a simple Fisher-Yates + // shuffle (Durstenfeld variant, low to high). + let mut v2 = v.clone(); + if v.len() < 2 { + return (v, v2); + } + for i in 0..v2.len() - 2 { + let j = rng.gen_range(i..v2.len()); + v2.swap(i, j); + } + + (v, v2) + }, + ) + } + + // Test various conditions for non-equality. + // + // It's somewhat hard to capture mutations in a proptest (partly because + // `TriMap` doesn't support mutating existing entries at the moment), so + // this is a small example-based test. + #[test] + fn test_permutation_eq_examples() { + let mut map1 = TriMap::::new(); + let mut map2 = TriMap::::new(); + + // Two empty maps are equal. + assert_eq!(map1, map2); + + // Insert a single entry into one map. + let entry = TestEntry { + key1: 0, + key2: 'a', + key3: "x".to_string(), + value: "v".to_string(), + }; + map1.insert_no_dups(entry.clone()).unwrap(); + + // The maps are not equal. + assert_ne_props(&map1, &map2); + + // Insert the same entry into the other map. + map2.insert_no_dups(entry.clone()).unwrap(); + + // The maps are now equal. + assert_eq_props(&map1, &map2); + + { + // Insert an entry with the same key2 and key3 but a different + // key1. + let mut map1 = map1.clone(); + map1.insert_no_dups(TestEntry { + key1: 1, + key2: 'b', + key3: "y".to_string(), + value: "v".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + + let mut map2 = map2.clone(); + map2.insert_no_dups(TestEntry { + key1: 2, + key2: 'b', + key3: "y".to_string(), + value: "v".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + } + + { + // Insert an entry with the same key1 and key3 but a different + // key2. + let mut map1 = map1.clone(); + map1.insert_no_dups(TestEntry { + key1: 1, + key2: 'b', + key3: "y".to_string(), + value: "v".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + + let mut map2 = map2.clone(); + map2.insert_no_dups(TestEntry { + key1: 1, + key2: 'c', + key3: "y".to_string(), + value: "v".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + } + + { + // Insert an entry with the same key1 and key2 but a different + // key3. + let mut map1 = map1.clone(); + map1.insert_no_dups(TestEntry { + key1: 1, + key2: 'b', + key3: "y".to_string(), + value: "v".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + + let mut map2 = map2.clone(); + map2.insert_no_dups(TestEntry { + key1: 1, + key2: 'b', + key3: "z".to_string(), + value: "v".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + } + + { + // Insert an entry where all the keys are the same, but the value is + // different. + let mut map1 = map1.clone(); + map1.insert_no_dups(TestEntry { + key1: 1, + key2: 'b', + key3: "y".to_string(), + value: "w".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + + let mut map2 = map2.clone(); + map2.insert_no_dups(TestEntry { + key1: 1, + key2: 'b', + key3: "y".to_string(), + value: "x".to_string(), + }) + .unwrap(); + assert_ne_props(&map1, &map2); + } + } + + /// Assert equality properties. + /// + /// The PartialEq algorithm is not obviously symmetric or reflexive, so we + /// must ensure in our tests that it is. + #[allow(clippy::eq_op)] + fn assert_eq_props(a: T, b: T) { + assert_eq!(a, a, "a == a"); + assert_eq!(b, b, "b == b"); + assert_eq!(a, b, "a == b"); + assert_eq!(b, a, "b == a"); + } + + /// Assert inequality properties. + /// + /// The PartialEq algorithm is not obviously symmetric or reflexive, so we + /// must ensure in our tests that it is. + #[allow(clippy::eq_op)] + fn assert_ne_props(a: T, b: T) { + // Also check reflexivity while we're here. + assert_eq!(a, a, "a == a"); + assert_eq!(b, b, "b == b"); + assert_ne!(a, b, "a != b"); + assert_ne!(b, a, "b != a"); + } } From 6b65896116403b16e4f8712692c1214b8051f106 Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 3 Oct 2024 17:35:39 -0700 Subject: [PATCH 2/2] fix clippy Created using spr 1.3.6-beta.1 --- nexus/db-model/src/inventory.rs | 9 ++++----- nexus/reconfigurator/planning/src/system.rs | 1 + schema/crdb/dbinit.sql | 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 0805c10eb7a..0c9770bc2ab 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -1173,12 +1173,11 @@ impl From for nexus_types::inventory::Dataset { } } -/// Information about a sled's Omicron zones, part of [`nexus_types::inventory::SledAgent`]. +/// Information about a sled's Omicron zones, part of +/// [`nexus_types::inventory::SledAgent`]. /// -/// -/// # Todo -/// -/// TODO: this table is vestigial and can be combined with InvSledAgent. See . +/// TODO: This table is vestigial and can be combined with `InvSledAgent`. See +/// https://github.com/oxidecomputer/omicron/issues/6770. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = inv_sled_omicron_zones)] pub struct InvSledOmicronZones { diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index e43af326e36..3bb4eb6151e 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -516,6 +516,7 @@ struct Sled { impl Sled { /// Create a `Sled` using faked-up information based on a `SledBuilder` + #[allow(clippy::too_many_arguments)] fn new_simulated( sled_id: SledUuid, sled_subnet: Ipv6Subnet, diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index a6cd9b38fed..017def5e569 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3328,6 +3328,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_dataset ( PRIMARY KEY (inv_collection_id, sled_id, name) ); +-- TODO: This table is vestigial and can be combined with `inv_sled_agent`. See +-- https://github.com/oxidecomputer/omicron/issues/6770. CREATE TABLE IF NOT EXISTS omicron.public.inv_sled_omicron_zones ( -- where this observation came from -- (foreign key into `inv_collection` table)