From f4acad0ee8cf8a02a1f5663c37daa568e5627480 Mon Sep 17 00:00:00 2001 From: Rain Date: Sun, 13 Oct 2024 20:27:18 -0700 Subject: [PATCH] [spr] initial version Created using spr 1.3.6-beta.1 --- Cargo.lock | 1 + dev-tools/reconfigurator-cli/src/main.rs | 4 +- dns-server/Cargo.toml | 1 + dns-server/src/bin/dnsadm.rs | 4 +- dns-server/src/storage.rs | 126 ++++++++++++------ dns-server/tests/basic_test.rs | 2 +- internal-dns/resolver/src/resolver.rs | 10 +- internal-dns/types/src/config.rs | 9 +- nexus/db-queries/src/db/datastore/dns.rs | 48 +++---- nexus/db-queries/src/db/datastore/rack.rs | 8 +- nexus/reconfigurator/execution/src/dns.rs | 49 +++---- nexus/src/app/background/init.rs | 13 +- nexus/src/app/background/tasks/dns_config.rs | 41 ++++-- .../app/background/tasks/dns_propagation.rs | 5 +- nexus/test-utils/src/lib.rs | 5 +- openapi/dns-server.json | 14 +- openapi/nexus-internal.json | 4 +- sled-agent/src/rack_setup/service.rs | 17 +-- sled-agent/src/sim/server.rs | 3 +- 19 files changed, 207 insertions(+), 157 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c0a5563939..4523059ca1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2339,6 +2339,7 @@ dependencies = [ "hickory-server", "http 1.1.0", "internal-dns-types", + "omicron-common", "omicron-test-utils", "omicron-workspace-hack", "openapi-lint", diff --git a/dev-tools/reconfigurator-cli/src/main.rs b/dev-tools/reconfigurator-cli/src/main.rs index f70ac2fc23..b1eefe7d1b 100644 --- a/dev-tools/reconfigurator-cli/src/main.rs +++ b/dev-tools/reconfigurator-cli/src/main.rs @@ -1392,7 +1392,7 @@ fn cmd_load_example( sim.internal_dns.insert( blueprint.internal_dns_version, DnsConfigParams { - generation: blueprint.internal_dns_version.into(), + generation: blueprint.internal_dns_version, time_created: Utc::now(), zones: vec![internal_dns], }, @@ -1400,7 +1400,7 @@ fn cmd_load_example( sim.external_dns.insert( blueprint.external_dns_version, DnsConfigParams { - generation: blueprint.external_dns_version.into(), + generation: blueprint.external_dns_version, time_created: Utc::now(), zones: vec![external_dns], }, diff --git a/dns-server/Cargo.toml b/dns-server/Cargo.toml index b3e7839162..c0f25700f6 100644 --- a/dns-server/Cargo.toml +++ b/dns-server/Cargo.toml @@ -21,6 +21,7 @@ hickory-resolver.workspace = true hickory-server.workspace = true http.workspace = true internal-dns-types.workspace = true +omicron-common.workspace = true pretty-hex.workspace = true schemars.workspace = true serde.workspace = true diff --git a/dns-server/src/bin/dnsadm.rs b/dns-server/src/bin/dnsadm.rs index 1c6a446124..372a810968 100644 --- a/dns-server/src/bin/dnsadm.rs +++ b/dns-server/src/bin/dnsadm.rs @@ -220,7 +220,7 @@ async fn main() -> Result<()> { .collect(); let new_config = DnsConfigParams { - generation: old_config.generation + 1, + generation: old_config.generation.next(), time_created: chrono::Utc::now(), zones, }; @@ -275,7 +275,7 @@ fn add_record( our_kv.1.push(record); Ok(DnsConfigParams { - generation: generation + 1, + generation: generation.next(), time_created: chrono::Utc::now(), zones: other_zones .into_iter() diff --git a/dns-server/src/storage.rs b/dns-server/src/storage.rs index 6c58af4978..2d20a4d0b2 100644 --- a/dns-server/src/storage.rs +++ b/dns-server/src/storage.rs @@ -99,6 +99,7 @@ use hickory_resolver::Name; use internal_dns_types::config::{ DnsConfig, DnsConfigParams, DnsConfigZone, DnsRecord, }; +use omicron_common::api::external::Generation; use serde::{Deserialize, Serialize}; use sled::transaction::ConflictableTransactionError; use slog::{debug, error, info, o, warn}; @@ -132,7 +133,7 @@ pub struct Store { #[derive(Debug, Deserialize, Eq, PartialEq, Serialize)] struct CurrentConfig { - generation: u64, + generation: Generation, zones: Vec, time_created: chrono::DateTime, time_applied: chrono::DateTime, @@ -144,7 +145,10 @@ pub enum UpdateError { "unsupported attempt to update to generation \ {attempted_generation} while at generation {current_generation}" )] - BadUpdateGeneration { current_generation: u64, attempted_generation: u64 }, + BadUpdateGeneration { + current_generation: Generation, + attempted_generation: Generation, + }, #[error( "update already in progress (from req_id {req_id:?}, \ @@ -153,7 +157,7 @@ pub enum UpdateError { UpdateInProgress { start_time: chrono::DateTime, elapsed: chrono::Duration, - generation: u64, + generation: Generation, req_id: String, }, @@ -193,7 +197,7 @@ impl Store { if store.read_config_optional()?.is_none() { let now = chrono::Utc::now(); let initial_config_bytes = serde_json::to_vec(&CurrentConfig { - generation: 0, + generation: 0.into(), zones: vec![], time_created: now, time_applied: now, @@ -234,7 +238,7 @@ impl Store { .transpose() } - fn tree_name_for_zone(zone_name: &str, generation: u64) -> String { + fn tree_name_for_zone(zone_name: &str, generation: Generation) -> String { format!("generation_{}_zone_{}", generation, zone_name) } @@ -302,7 +306,7 @@ impl Store { async fn begin_update<'a, 'b>( &'a self, req_id: &'b str, - generation: u64, + generation: Generation, ) -> Result, UpdateError> { if self.poisoned.load(Ordering::SeqCst) { panic!( @@ -344,7 +348,7 @@ impl Store { ) -> Result<(), UpdateError> { let log = &self.log.new(o!( "req_id" => req_id.to_owned(), - "new_generation" => config.generation + "new_generation" => u64::from(config.generation), )); // Lock out concurrent updates. We must not return until we've released @@ -512,7 +516,7 @@ impl Store { self.prune_trees(trees_to_prune, "too new"); } - fn all_name_trees(&self) -> impl Iterator { + fn all_name_trees(&self) -> impl Iterator { self.db.tree_names().into_iter().filter_map(|tree_name_bytes| { let tree_name = std::str::from_utf8(&tree_name_bytes).ok()?; let parts = tree_name.splitn(4, '_').collect::>(); @@ -523,7 +527,8 @@ impl Store { return None; } - let gen_num = parts[1].parse::().ok()?; + let gen_num = + Generation::try_from(parts[1].parse::().ok()?).ok()?; Some((gen_num, tree_name.to_owned())) }) } @@ -694,7 +699,7 @@ pub(crate) enum QueryError { struct UpdateInfo { start_time: chrono::DateTime, start_instant: std::time::Instant, - generation: u64, + generation: Generation, req_id: String, } @@ -788,6 +793,7 @@ mod test { use internal_dns_types::config::DnsConfigParams; use internal_dns_types::config::DnsConfigZone; use internal_dns_types::config::DnsRecord; + use omicron_common::api::external::Generation; use omicron_test_utils::dev::test_setup_log; use std::collections::BTreeSet; use std::collections::HashMap; @@ -874,11 +880,11 @@ mod test { /// Returns an ordered list of the generation numbers that have trees in /// the underlying Store's database. This is used to verify the /// behavior around pruning trees. - fn generations_with_trees(store: &Store) -> Vec { + fn generations_with_trees(store: &Store) -> Vec { store .all_name_trees() .map(|(gen, _)| gen) - .collect::>() + .collect::>() .into_iter() .collect() } @@ -890,7 +896,7 @@ mod test { // Verify the initial configuration. assert!(generations_with_trees(&tc.store).is_empty()); let config = tc.store.dns_config().await.unwrap(); - assert_eq!(config.generation, 0); + assert_eq!(config.generation, Generation::from_u32(0)); assert!(config.zones.is_empty()); expect(&tc.store, "gen1_name.zone1.internal", Expect::NoZone); expect(&tc.store, "Gen1_name.zone1.internal", Expect::NoZone); @@ -902,7 +908,7 @@ mod test { let dummy_record = DnsRecord::Aaaa(Ipv6Addr::LOCALHOST); let update1 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 1, + generation: Generation::from_u32(1), zones: vec![DnsConfigZone { zone_name: "zone1.internal".to_string(), records: HashMap::from([ @@ -913,7 +919,10 @@ mod test { }; tc.store.dns_config_update(&update1, "my request id").await.unwrap(); - assert_eq!(vec![1], generations_with_trees(&tc.store)); + assert_eq!( + vec![Generation::from_u32(1)], + generations_with_trees(&tc.store) + ); expect( &tc.store, "gen1_name.zone1.internal", @@ -942,7 +951,7 @@ mod test { // one of the names from the existing zone. let update2 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 2, + generation: Generation::from_u32(2), zones: vec![ DnsConfigZone { zone_name: "zone1.internal".to_string(), @@ -961,7 +970,10 @@ mod test { ], }; tc.store.dns_config_update(&update2, "my request id").await.unwrap(); - assert_eq!(vec![1, 2], generations_with_trees(&tc.store)); + assert_eq!( + vec![Generation::from_u32(1), Generation::from_u32(2)], + generations_with_trees(&tc.store) + ); expect(&tc.store, "gen1_name.zone1.internal", Expect::NoName); expect(&tc.store, "gen1_name.ZONE1.internal", Expect::NoName); expect(&tc.store, "Gen1_name.zone1.internal", Expect::NoName); @@ -980,7 +992,7 @@ mod test { // Do another update, but this time, skip several generation numbers. let update8 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 8, + generation: Generation::from_u32(8), zones: vec![DnsConfigZone { zone_name: "zone8.internal".to_string(), records: HashMap::from([( @@ -990,7 +1002,14 @@ mod test { }], }; tc.store.dns_config_update(&update8, "my request id").await.unwrap(); - assert_eq!(vec![1, 2, 8], generations_with_trees(&tc.store)); + assert_eq!( + vec![ + Generation::from_u32(1), + Generation::from_u32(2), + Generation::from_u32(8) + ], + generations_with_trees(&tc.store) + ); expect(&tc.store, "gen1_name.zone1.internal", Expect::NoZone); expect(&tc.store, "shared_name.zone1.internal", Expect::NoZone); expect(&tc.store, "gen2_name.zone2.internal", Expect::NoZone); @@ -1003,7 +1022,14 @@ mod test { // Updating to generation 8 again should be a no-op. It should succeed // and show the same behavior. tc.store.dns_config_update(&update8, "my request id").await.unwrap(); - assert_eq!(vec![1, 2, 8], generations_with_trees(&tc.store)); + assert_eq!( + vec![ + Generation::from_u32(1), + Generation::from_u32(2), + Generation::from_u32(8) + ], + generations_with_trees(&tc.store) + ); expect(&tc.store, "gen1_name.zone1.internal", Expect::NoZone); expect(&tc.store, "shared_name.zone1.internal", Expect::NoZone); expect(&tc.store, "gen2_name.zone2.internal", Expect::NoZone); @@ -1024,9 +1050,13 @@ mod test { println!("{:?}", error); match &error { UpdateError::BadUpdateGeneration { - current_generation: 8, - attempted_generation: 2, - } => (), + current_generation, + attempted_generation, + } if *current_generation == Generation::from_u32(8) + && *attempted_generation == Generation::from_u32(2) => + { + () + } e => panic!("unexpected failure to update: {:#}", e), }; assert_eq!( @@ -1042,7 +1072,7 @@ mod test { // generation 2), not the last three integers. let update9 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 9, + generation: Generation::from_u32(9), zones: vec![DnsConfigZone { zone_name: "zone8.internal".to_string(), records: HashMap::from([( @@ -1052,7 +1082,14 @@ mod test { }], }; tc.store.dns_config_update(&update9, "my request id").await.unwrap(); - assert_eq!(vec![2, 8, 9], generations_with_trees(&tc.store)); + assert_eq!( + vec![ + Generation::from_u32(2), + Generation::from_u32(8), + Generation::from_u32(9) + ], + generations_with_trees(&tc.store) + ); tc.cleanup_successful(); } @@ -1064,14 +1101,14 @@ mod test { // Initial configuration. assert!(generations_with_trees(&tc.store).is_empty()); let config = tc.store.dns_config().await.unwrap(); - assert_eq!(config.generation, 0); + assert_eq!(config.generation, Generation::from_u32(0)); assert!(config.zones.is_empty()); // Make one normal update. let dummy_record = DnsRecord::Aaaa(Ipv6Addr::LOCALHOST); let update1 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 1, + generation: Generation::from_u32(1), zones: vec![DnsConfigZone { zone_name: "zone1.internal".to_string(), records: HashMap::from([( @@ -1082,7 +1119,10 @@ mod test { }; tc.store.dns_config_update(&update1, "my request id").await.unwrap(); - assert_eq!(vec![1], generations_with_trees(&tc.store)); + assert_eq!( + vec![Generation::from_u32(1)], + generations_with_trees(&tc.store) + ); // Now make an update to generation 2. We're going to do this like // normal, examine the state, and then we're going to unwind the very @@ -1096,7 +1136,7 @@ mod test { // it's better to test some of this behavior than none. let update2 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 2, + generation: Generation::from_u32(2), zones: vec![DnsConfigZone { zone_name: "zone2.internal".to_string(), records: HashMap::from([( @@ -1107,7 +1147,7 @@ mod test { }; let gen1_config = tc.store.read_config().unwrap(); - assert_eq!(1, gen1_config.generation); + assert_eq!(Generation::from_u32(1), gen1_config.generation); expect( &tc.store, "gen1_name.zone1.internal", @@ -1116,9 +1156,12 @@ mod test { expect(&tc.store, "gen2_name.zone2.internal", Expect::NoZone); tc.store.dns_config_update(&update2, "my request id").await.unwrap(); - assert_eq!(vec![1, 2], generations_with_trees(&tc.store)); + assert_eq!( + vec![Generation::from_u32(1), Generation::from_u32(2)], + generations_with_trees(&tc.store) + ); let gen2_config = tc.store.read_config().unwrap(); - assert_eq!(2, gen2_config.generation); + assert_eq!(Generation::from_u32(2), gen2_config.generation); expect(&tc.store, "gen1_name.zone1.internal", Expect::NoZone); expect( &tc.store, @@ -1155,7 +1198,10 @@ mod test { let config = store.read_config().unwrap(); assert_eq!(gen1_config, config); // We ought to have pruned the tree associated with generation 2. - assert_eq!(vec![1], generations_with_trees(&store)); + assert_eq!( + vec![Generation::from_u32(1)], + generations_with_trees(&store) + ); // The rest of the behavior ought to be like generation 1. expect( &store, @@ -1166,9 +1212,12 @@ mod test { // Now we can do another update to generation 2. store.dns_config_update(&update2, "my request id").await.unwrap(); - assert_eq!(vec![1, 2], generations_with_trees(&store)); + assert_eq!( + vec![Generation::from_u32(1), Generation::from_u32(2)], + generations_with_trees(&store) + ); let gen2_config = store.read_config().unwrap(); - assert_eq!(2, gen2_config.generation); + assert_eq!(Generation::from_u32(2), gen2_config.generation); expect(&store, "gen1_name.zone1.internal", Expect::NoZone); expect( &store, @@ -1186,14 +1235,15 @@ mod test { // Begin an update. let before = chrono::Utc::now(); - let update1 = tc.store.begin_update("my req id", 3).await.unwrap(); + let update1 = + tc.store.begin_update("my req id", 3.into()).await.unwrap(); let after = chrono::Utc::now(); // Concurrently attempt another update. let dummy_record = DnsRecord::Aaaa(Ipv6Addr::LOCALHOST); let update2 = DnsConfigParams { time_created: chrono::Utc::now(), - generation: 1, + generation: 1.into(), zones: vec![DnsConfigZone { zone_name: "zone1.internal".to_string(), records: HashMap::from([( @@ -1225,7 +1275,7 @@ mod test { req_id, } if *start_time >= before && *start_time <= after - && *generation == 3 + && *generation == Generation::from_u32(3) && *req_id == "my req id" => { () diff --git a/dns-server/tests/basic_test.rs b/dns-server/tests/basic_test.rs index c72bb4b3ac..b555b82a80 100644 --- a/dns-server/tests/basic_test.rs +++ b/dns-server/tests/basic_test.rs @@ -458,7 +458,7 @@ async fn dns_records_create( let zones = other_zones.into_iter().chain(std::iter::once(new_zone)).collect(); let after = DnsConfigParams { - generation: before.generation + 1, + generation: before.generation.next(), zones, time_created: chrono::Utc::now(), }; diff --git a/internal-dns/resolver/src/resolver.rs b/internal-dns/resolver/src/resolver.rs index 2378e62c65..afee118124 100644 --- a/internal-dns/resolver/src/resolver.rs +++ b/internal-dns/resolver/src/resolver.rs @@ -686,7 +686,7 @@ mod test { // If we deploy a new generation that removes all records, then we don't // find anything any more. - dns_config.generation += 1; + dns_config.generation = dns_config.generation.next(); dns_config.zones[0].records = HashMap::new(); dns_server.update(&dns_config).await.unwrap(); @@ -707,7 +707,7 @@ mod test { // If we remove the zone altogether, we'll get a different resolution // error because the DNS server is no longer authoritative for this // zone. - dns_config.generation += 1; + dns_config.generation = dns_config.generation.next(); dns_config.zones = Vec::new(); dns_server.update(&dns_config).await.unwrap(); @@ -746,7 +746,7 @@ mod test { dns_builder.service_backend_zone(srv_crdb, &zone, 54321).unwrap(); let mut dns_config = dns_builder.build_full_config_for_initial_generation(); - dns_config.generation += 1; + dns_config.generation = dns_config.generation.next(); dns_server.update(&dns_config).await.unwrap(); let found_addr = resolver .lookup_socket_v6(ServiceName::Cockroach) @@ -1024,7 +1024,7 @@ mod test { // Now let's remove one of the AAAA records for a zone/target. // The lookup should still succeed and return the other address. - dns_config.generation += 1; + dns_config.generation = dns_config.generation.next(); let root = dns_config .zones .iter_mut() @@ -1066,7 +1066,7 @@ mod test { assert_eq!(targets, expected_targets); // Finally, let's remove the last AAAA record as well - dns_config.generation += 1; + dns_config.generation = dns_config.generation.next(); let root = dns_config .zones .iter_mut() diff --git a/internal-dns/types/src/config.rs b/internal-dns/types/src/config.rs index a7f223caee..a40e9de8a6 100644 --- a/internal-dns/types/src/config.rs +++ b/internal-dns/types/src/config.rs @@ -572,7 +572,7 @@ impl DnsConfigBuilder { pub fn build_full_config_for_initial_generation(self) -> DnsConfigParams { let zone = self.build_zone(); DnsConfigParams { - generation: u64::from(Generation::new()), + generation: Generation::new(), time_created: chrono::Utc::now(), zones: vec![zone], } @@ -581,7 +581,7 @@ impl DnsConfigBuilder { #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] pub struct DnsConfigParams { - pub generation: u64, + pub generation: Generation, pub time_created: chrono::DateTime, pub zones: Vec, } @@ -606,7 +606,7 @@ impl DnsConfigParams { #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] pub struct DnsConfig { - pub generation: u64, + pub generation: Generation, pub time_created: chrono::DateTime, pub time_applied: chrono::DateTime, pub zones: Vec, @@ -688,6 +688,7 @@ pub struct Srv { mod test { use super::{DnsConfigBuilder, Host, ServiceName}; use crate::{config::Zone, names::DNS_ZONE}; + use omicron_common::api::external::Generation; use omicron_uuid_kinds::{OmicronZoneUuid, SledUuid}; use std::{collections::BTreeMap, io::Write, net::Ipv6Addr}; @@ -838,7 +839,7 @@ mod test { ("non_trivial", builder_non_trivial), ] { let config = builder.build_full_config_for_initial_generation(); - assert_eq!(config.generation, 1); + assert_eq!(config.generation, Generation::from(1)); assert_eq!(config.zones.len(), 1); assert_eq!(config.zones[0].zone_name, DNS_ZONE); write!(&mut output, "builder: {:?}\n", label).unwrap(); diff --git a/nexus/db-queries/src/db/datastore/dns.rs b/nexus/db-queries/src/db/datastore/dns.rs index 6fe524686d..f37e8d9e34 100644 --- a/nexus/db-queries/src/db/datastore/dns.rs +++ b/nexus/db-queries/src/db/datastore/dns.rs @@ -282,21 +282,13 @@ impl DataStore { } } - let generation = - u64::try_from(i64::from(&version.version.0)).map_err(|e| { - Error::internal_error(&format!( - "unsupported generation number: {:#}", - e - )) - })?; - debug!(log, "read DNS config"; "version" => i64::from(&version.version.0), "nzones" => zones.len() ); Ok(DnsConfigParams { - generation, + generation: version.version.0, time_created: version.time_created, zones, }) @@ -816,7 +808,7 @@ mod test { .await .expect("failed to read DNS config"); println!("found config: {:?}", dns_config); - assert_eq!(dns_config.generation, 1); + assert_eq!(u64::from(dns_config.generation), 1); // A round-trip through the database reduces the precision of the // "time_created" value. assert_eq!( @@ -876,7 +868,7 @@ mod test { .await .expect("failed to read DNS config"); println!("found config: {:?}", dns_config); - assert_eq!(dns_config.generation, 1); + assert_eq!(u64::from(dns_config.generation), 1); assert!(dns_config.time_created >= before); assert!(dns_config.time_created <= after); assert_eq!(dns_config.zones.len(), 0); @@ -916,7 +908,7 @@ mod test { .await .expect("failed to read DNS config"); println!("found config: {:?}", dns_config); - assert_eq!(dns_config.generation, 1); + assert_eq!(u64::from(dns_config.generation), 1); assert!(dns_config.time_created >= before); assert!(dns_config.time_created <= after); assert_eq!(dns_config.zones.len(), 1); @@ -1216,7 +1208,7 @@ mod test { .await .unwrap(); println!("dns_config_v1: {:?}", dns_config_v1); - assert_eq!(dns_config_v1.generation, 1); + assert_eq!(u64::from(dns_config_v1.generation), 1); assert_eq!(dns_config_v1.zones.len(), 2); assert_eq!(dns_config_v1.zones[0].zone_name, "z1.foo"); assert_eq!( @@ -1238,7 +1230,7 @@ mod test { .await .unwrap(); println!("dns_config_v2: {:?}", dns_config_v2); - assert_eq!(dns_config_v2.generation, 2); + assert_eq!(u64::from(dns_config_v2.generation), 2); assert_eq!(dns_config_v2.zones.len(), 3); assert_eq!(dns_config_v2.zones[0].zone_name, "z1.foo"); assert_eq!( @@ -1271,7 +1263,7 @@ mod test { .await .unwrap(); println!("dns_config_v3: {:?}", dns_config_v3); - assert_eq!(dns_config_v3.generation, 3); + assert_eq!(u64::from(dns_config_v3.generation), 3); assert_eq!(dns_config_v3.zones.len(), 2); assert_eq!(dns_config_v3.zones[0].zone_name, "z2.foo"); assert_eq!( @@ -1301,7 +1293,7 @@ mod test { .await .unwrap(); println!("internal dns_config_v1: {:?}", internal_dns_config_v1); - assert_eq!(internal_dns_config_v1.generation, 1); + assert_eq!(u64::from(internal_dns_config_v1.generation), 1); assert_eq!(internal_dns_config_v1.zones.len(), 0); // Verify internal version 2. @@ -1310,7 +1302,7 @@ mod test { .await .unwrap(); println!("internal dns_config_v2: {:?}", internal_dns_config_v2); - assert_eq!(internal_dns_config_v2.generation, 2); + assert_eq!(u64::from(internal_dns_config_v2.generation), 2); assert_eq!(internal_dns_config_v2.zones.len(), 1); assert_eq!(internal_dns_config_v2.zones[0].zone_name, "z1.foo"); assert_eq!( @@ -1574,7 +1566,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 1); + assert_eq!(u64::from(dns_config.generation), 1); assert_eq!(dns_config.zones.len(), 0); // Add a few DNS names. @@ -1605,7 +1597,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 2); + assert_eq!(u64::from(dns_config.generation), 2); assert_eq!(dns_config.zones.len(), 2); assert_eq!(dns_config.zones[0].zone_name, "oxide1.test"); assert_eq!( @@ -1640,7 +1632,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 3); + assert_eq!(u64::from(dns_config.generation), 3); assert_eq!(dns_config.zones.len(), 2); assert_eq!(dns_config.zones[0].zone_name, "oxide1.test"); assert_eq!( @@ -1673,7 +1665,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 4); + assert_eq!(u64::from(dns_config.generation), 4); assert_eq!(dns_config.zones.len(), 2); assert_eq!(dns_config.zones[0].zone_name, "oxide1.test"); assert_eq!( @@ -1703,7 +1695,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 5); + assert_eq!(u64::from(dns_config.generation), 5); assert_eq!(dns_config.zones.len(), 2); assert_eq!(dns_config.zones[0].zone_name, "oxide1.test"); assert_eq!( @@ -1780,7 +1772,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 6); + assert_eq!(u64::from(dns_config.generation), 6); assert_eq!(dns_config.zones.len(), 2); assert_eq!(dns_config.zones[0].zone_name, "oxide1.test"); assert_eq!( @@ -1793,7 +1785,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::Internal) .await .unwrap(); - assert_eq!(dns_config.generation, 2); + assert_eq!(u64::from(dns_config.generation), 2); assert_eq!(dns_config.zones.len(), 1); assert_eq!(dns_config.zones[0].zone_name, "oxide3.test"); assert_eq!( @@ -1830,7 +1822,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 6); + assert_eq!(u64::from(dns_config.generation), 6); // Failure case: cannot add a name that already exists. { @@ -1860,7 +1852,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config.generation, 6); + assert_eq!(u64::from(dns_config.generation), 6); assert_eq!(dns_config.zones.len(), 2); assert_eq!(dns_config.zones[0].zone_name, "oxide1.test"); assert_eq!( @@ -1958,7 +1950,7 @@ mod test { .await .expect("failed to read config"); let gen2 = nexus_db_model::Generation(gen1.next()); - assert_eq!(u64::from(*gen2), config.generation); + assert_eq!(gen2.0, config.generation); assert_eq!(1, config.zones.len()); let records = &config.zones[0].records; assert!(records.contains_key("nelson")); @@ -1975,7 +1967,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::Internal) .await .expect("failed to read config"); - assert_eq!(u64::from(gen2.next()), config.generation); + assert_eq!(gen2.next(), config.generation); assert_eq!(1, config.zones.len()); let records = &config.zones[0].records; assert!(records.contains_key("nelson")); diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 534854d2df..27b64eed2f 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1151,7 +1151,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::Internal) .await .unwrap(); - assert_eq!(dns_internal.generation, 1); + assert_eq!(u64::from(dns_internal.generation), 1); assert!(dns_internal.time_created >= before); assert!(dns_internal.time_created <= after); assert_eq!(dns_internal.zones.len(), 0); @@ -1162,7 +1162,7 @@ mod test { .unwrap(); // The external DNS zone has an extra update due to the initial Silo // creation. - assert_eq!(dns_internal.generation + 1, dns_external.generation); + assert_eq!(dns_internal.generation.next(), dns_external.generation); assert_eq!(dns_internal.zones, dns_external.zones); // Verify the details about the initial Silo. @@ -1923,7 +1923,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::Internal) .await .unwrap(); - assert_eq!(dns_config_internal.generation, 1); + assert_eq!(u64::from(dns_config_internal.generation), 1); assert_eq!(dns_config_internal.zones.len(), 1); assert_eq!(dns_config_internal.zones[0].zone_name, DNS_ZONE); assert_eq!( @@ -1935,7 +1935,7 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .unwrap(); - assert_eq!(dns_config_external.generation, 2); + assert_eq!(u64::from(dns_config_external.generation), 2); assert_eq!(dns_config_external.zones.len(), 1); assert_eq!( dns_config_external.zones[0].zone_name, diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index d3c75fd673..37f62253c5 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -19,7 +19,6 @@ use nexus_types::identity::Resource; use nexus_types::internal_api::params::DnsConfigParams; use nexus_types::internal_api::params::DnsConfigZone; use omicron_common::api::external::Error; -use omicron_common::api::external::Generation; use omicron_common::api::external::InternalContext; use omicron_common::bail_unless; use omicron_uuid_kinds::SledUuid; @@ -219,7 +218,7 @@ pub(crate) async fn deploy_dns_one( let dns_config_blueprint = DnsConfigParams { zones: vec![dns_zone_blueprint], time_created: chrono::Utc::now(), - generation: u64::from(blueprint_generation.next()), + generation: blueprint_generation.next(), }; info!( @@ -228,16 +227,13 @@ pub(crate) async fn deploy_dns_one( dns_config_current.generation, dns_config_blueprint.generation, ); - let generation_u32 = - u32::try_from(dns_config_current.generation).map_err(|e| { - Error::internal_error(&format!( - "DNS generation got too large: {}", - e, - )) - })?; - let generation = - nexus_db_model::Generation::from(Generation::from(generation_u32)); - datastore.dns_update_from_version(opctx, update, generation).await + datastore + .dns_update_from_version( + opctx, + update, + dns_config_current.generation.into(), + ) + .await } fn dns_compute_update( @@ -386,7 +382,7 @@ mod test { fn dns_config_empty() -> DnsConfigParams { DnsConfigParams { - generation: 1, + generation: Generation::new(), time_created: chrono::Utc::now(), zones: vec![DnsConfigZone { zone_name: String::from("internal"), @@ -660,8 +656,7 @@ mod test { } let dns_empty = dns_config_empty(); - let initial_dns_generation = - Generation::from(u32::try_from(dns_empty.generation).unwrap()); + let initial_dns_generation = dns_empty.generation; let mut blueprint = Blueprint { id: Uuid::new_v4(), blueprint_zones, @@ -1355,14 +1350,8 @@ mod test { sled_rows: &sled_rows, zpool_rows: &zpool_rows, ip_pool_range_rows: &ip_pool_range_rows, - internal_dns_version: Generation::from( - u32::try_from(dns_initial_internal.generation).unwrap(), - ) - .into(), - external_dns_version: Generation::from( - u32::try_from(dns_latest_external.generation).unwrap(), - ) - .into(), + internal_dns_version: dns_initial_internal.generation.into(), + external_dns_version: dns_latest_external.generation.into(), // These are not used because we're not actually going through // the planner. cockroachdb_settings: &CockroachDbSettings::empty(), @@ -1457,7 +1446,7 @@ mod test { assert_eq!( dns_latest_internal.generation, - dns_initial_internal.generation + 1, + dns_initial_internal.generation.next(), ); let diff = diff_sole_zones(&dns_initial_internal, &dns_latest_internal); @@ -1495,7 +1484,7 @@ mod test { .expect("fetching latest external DNS"); assert_eq!( dns_latest_external.generation, - dns_previous_external.generation + 1, + dns_previous_external.generation.next(), ); let diff = diff_sole_zones(&dns_previous_external, &dns_latest_external); @@ -1621,7 +1610,10 @@ mod test { .dns_config_read(&opctx, DnsGroup::External) .await .expect("fetching latest external DNS"); - assert_eq!(old_external.generation + 1, dns_latest_external.generation); + assert_eq!( + old_external.generation.next(), + dns_latest_external.generation + ); // Specifically, there should be one new name (for the new Silo). let diff = diff_sole_zones(&old_external, &dns_latest_external); @@ -1651,7 +1643,10 @@ mod test { .await .expect("fetching latest external DNS"); assert_eq!(old_internal.generation, dns_latest_internal.generation); - assert_eq!(old_external.generation + 1, dns_latest_external.generation); + assert_eq!( + old_external.generation.next(), + dns_latest_external.generation + ); dns_latest_external } diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index fdd2fb7c90..dd1333715a 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -958,6 +958,7 @@ pub mod test { use nexus_types::internal_api::params as nexus_params; use nexus_types::internal_api::params::DnsRecord; use omicron_common::api::external::Error; + use omicron_common::api::external::Generation; use omicron_test_utils::dev::poll; use std::net::SocketAddr; use std::sync::atomic::AtomicU64; @@ -1057,7 +1058,7 @@ pub mod test { .dns_config_get() .await .expect("failed to get initial DNS server config"); - assert_eq!(config.generation, 1); + assert_eq!(config.generation, Generation::from_u32(1)); let internal_dns_srv_name = ServiceName::InternalDns.dns_name(); @@ -1167,7 +1168,7 @@ pub mod test { &cptestctx.logctx.log, "initial", initial_dns_dropshot_server.local_addr(), - 2, + 2.into(), ) .await; @@ -1180,7 +1181,7 @@ pub mod test { &cptestctx.logctx.log, "new", new_dns_dropshot_server.local_addr(), - 2, + 2.into(), ) .await; @@ -1198,7 +1199,7 @@ pub mod test { &cptestctx.logctx.log, "initial", initial_dns_dropshot_server.local_addr(), - 3, + Generation::from_u32(3), ) .await; @@ -1206,7 +1207,7 @@ pub mod test { &cptestctx.logctx.log, "new", new_dns_dropshot_server.local_addr(), - 3, + Generation::from_u32(3), ) .await; } @@ -1216,7 +1217,7 @@ pub mod test { log: &slog::Logger, label: &str, addr: SocketAddr, - generation: u64, + generation: Generation, ) { println!( "waiting for propagation of generation {generation} to {label} \ diff --git a/nexus/src/app/background/tasks/dns_config.rs b/nexus/src/app/background/tasks/dns_config.rs index 192724a89c..44158b25bb 100644 --- a/nexus/src/app/background/tasks/dns_config.rs +++ b/nexus/src/app/background/tasks/dns_config.rs @@ -53,7 +53,7 @@ impl BackgroundTask for DnsConfigWatcher { let log = match &self.last { None => opctx.log.clone(), Some(old) => opctx.log.new(o!( - "current_generation" => old.generation, + "current_generation" => u64::from(old.generation), "current_time_created" => old.time_created.to_string(), )), }; @@ -87,7 +87,7 @@ impl BackgroundTask for DnsConfigWatcher { info!( &log, "found latest generation (first find)"; - "generation" => new_config.generation + "generation" => u64::from(new_config.generation), ); self.last = Some(new_config.clone()); self.tx.send_replace(Some(new_config)); @@ -128,7 +128,7 @@ impl BackgroundTask for DnsConfigWatcher { debug!( &log, "found latest DNS generation (unchanged)"; - "generation" => new.generation, + "generation" => u64::from(new.generation), ); json!({ "generation": new.generation }) } @@ -139,9 +139,9 @@ impl BackgroundTask for DnsConfigWatcher { info!( &log, "found latest DNS generation (newer than we had)"; - "generation" => new.generation, + "generation" => u64::from(new.generation), "time_created" => new.time_created.to_string(), - "old_generation" => old.generation, + "old_generation" => u64::from(old.generation), "old_time_created" => old.time_created.to_string(), ); self.last = Some(new.clone()); @@ -168,6 +168,7 @@ mod test { use nexus_db_model::DnsGroup; use nexus_db_queries::context::OpContext; use nexus_test_utils_macros::nexus_test; + use omicron_common::api::external::Generation; use serde_json::json; type ControlPlaneTestContext = @@ -191,20 +192,32 @@ mod test { // The datastore from the ControlPlaneTestContext is initialized with a // DNS config with generation 1. let value = task.activate(&opctx).await; - assert_eq!(watcher.borrow().as_ref().unwrap().generation, 1); + assert_eq!( + watcher.borrow().as_ref().unwrap().generation, + Generation::from_u32(1) + ); assert_eq!(value, json!({ "generation": 1 })); // Now write generation 2, activate again, and verify that the update // was sent to the watcher. write_test_dns_generation(&opctx, &datastore).await; - assert_eq!(watcher.borrow().as_ref().unwrap().generation, 1); + assert_eq!( + watcher.borrow().as_ref().unwrap().generation, + Generation::from_u32(1) + ); let value = task.activate(&opctx).await; - assert_eq!(watcher.borrow().as_ref().unwrap().generation, 2); + assert_eq!( + watcher.borrow().as_ref().unwrap().generation, + Generation::from_u32(2), + ); assert_eq!(value, json!({ "generation": 2 })); // Activate again and make sure it does nothing. let value = task.activate(&opctx).await; - assert_eq!(watcher.borrow().as_ref().unwrap().generation, 2); + assert_eq!( + watcher.borrow().as_ref().unwrap().generation, + Generation::from_u32(2), + ); assert_eq!(value, json!({ "generation": 2 })); // Simulate the configuration going backwards. This should not be @@ -219,7 +232,10 @@ mod test { .unwrap(); } let value = task.activate(&opctx).await; - assert_eq!(watcher.borrow().as_ref().unwrap().generation, 2); + assert_eq!( + watcher.borrow().as_ref().unwrap().generation, + Generation::from_u32(2), + ); assert_eq!( value, json!({ @@ -263,7 +279,10 @@ mod test { .unwrap(); let _ = task.activate(&opctx).await; - assert_eq!(watcher.borrow().as_ref().unwrap().generation, 2); + assert_eq!( + watcher.borrow().as_ref().unwrap().generation, + Generation::from_u32(2), + ); // Verify that a new watcher also handles this okay. (i.e., that we can // come up with no state in the database). diff --git a/nexus/src/app/background/tasks/dns_propagation.rs b/nexus/src/app/background/tasks/dns_propagation.rs index 9dd698fa37..df78860912 100644 --- a/nexus/src/app/background/tasks/dns_propagation.rs +++ b/nexus/src/app/background/tasks/dns_propagation.rs @@ -86,7 +86,7 @@ impl BackgroundTask for DnsPropagator { // Set up a logger for this activation that includes metadata about // the current generation and servers. let log = opctx.log.new(o!( - "generation" => dns_config.generation, + "generation" => u64::from(dns_config.generation), "servers" => format!("{:?}", dns_servers), )); @@ -186,6 +186,7 @@ mod test { use nexus_db_queries::context::OpContext; use nexus_test_utils_macros::nexus_test; use nexus_types::internal_api::params::DnsConfigParams; + use omicron_common::api::external::Generation; use serde::Deserialize; use serde_json::json; use std::collections::BTreeMap; @@ -208,7 +209,7 @@ mod test { let mut task = DnsPropagator::new(config_rx, servers_rx, 3); let dns_config = DnsConfigParams { - generation: 1, + generation: Generation::from_u32(1), time_created: chrono::Utc::now(), zones: vec![], }; diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index d69abbd93e..5bd63765c4 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -828,10 +828,7 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> { blueprint_disks: BTreeMap::new(), sled_state, parent_blueprint_id: None, - internal_dns_version: dns_config - .generation - .try_into() - .expect("bad internal DNS generation"), + internal_dns_version: dns_config.generation, external_dns_version: Generation::new(), cockroachdb_fingerprint: String::new(), cockroachdb_setting_preserve_downgrade: diff --git a/openapi/dns-server.json b/openapi/dns-server.json index 0252c1538a..30bf9acf9e 100644 --- a/openapi/dns-server.json +++ b/openapi/dns-server.json @@ -64,9 +64,7 @@ "type": "object", "properties": { "generation": { - "type": "integer", - "format": "uint64", - "minimum": 0 + "$ref": "#/components/schemas/Generation" }, "time_applied": { "type": "string", @@ -94,9 +92,7 @@ "type": "object", "properties": { "generation": { - "type": "integer", - "format": "uint64", - "minimum": 0 + "$ref": "#/components/schemas/Generation" }, "time_created": { "type": "string", @@ -215,6 +211,12 @@ "request_id" ] }, + "Generation": { + "description": "Generation numbers stored in the database, used for optimistic concurrency control", + "type": "integer", + "format": "uint64", + "minimum": 0 + }, "Srv": { "type": "object", "properties": { diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 7d762ecc5b..9f424f609d 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -3065,9 +3065,7 @@ "type": "object", "properties": { "generation": { - "type": "integer", - "format": "uint64", - "minimum": 0 + "$ref": "#/components/schemas/Generation" }, "time_created": { "type": "string", diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs index 5124096c1e..a5ba8d9d7f 100644 --- a/sled-agent/src/rack_setup/service.rs +++ b/sled-agent/src/rack_setup/service.rs @@ -714,8 +714,7 @@ impl ServiceInner { let blueprint = build_initial_blueprint_from_plan( &sled_configs_by_id, service_plan, - ) - .map_err(SetupServiceError::ConvertPlanToBlueprint)?; + ); info!(self.log, "Nexus address: {}", nexus_address.to_string()); @@ -1427,17 +1426,11 @@ fn build_sled_configs_by_id( fn build_initial_blueprint_from_plan( sled_configs_by_id: &BTreeMap, service_plan: &ServicePlan, -) -> anyhow::Result { - let internal_dns_version = - Generation::try_from(service_plan.dns_config.generation) - .context("invalid internal dns version")?; - - let blueprint = build_initial_blueprint_from_sled_configs( +) -> Blueprint { + build_initial_blueprint_from_sled_configs( sled_configs_by_id, - internal_dns_version, - ); - - Ok(blueprint) + service_plan.dns_config.generation, + ) } pub(crate) fn build_initial_blueprint_from_sled_configs( diff --git a/sled-agent/src/sim/server.rs b/sled-agent/src/sim/server.rs index 5ebe56ae19..f4d6440679 100644 --- a/sled-agent/src/sim/server.rs +++ b/sled-agent/src/sim/server.rs @@ -363,8 +363,7 @@ pub async fn run_standalone_server( let dns_config = dns_config_builder.build_full_config_for_initial_generation(); dns.initialize_with_config(&log, &dns_config).await?; - let internal_dns_version = Generation::try_from(dns_config.generation) - .expect("invalid internal dns version"); + let internal_dns_version = dns_config.generation; let all_u2_zpools = server.sled_agent.get_zpools().await; let get_random_zpool = || {