From 7484017b49024ef9a3ccaacfd3f31ac451866e8e Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 28 Mar 2024 15:24:47 -0700 Subject: [PATCH] [nexus] move UuidRng code out to a common crate, make CollectionBuilder use it (#5341) In #5270, we need determinism not just from blueprints but also collections. So move the UuidRng into a common place. As part of that, I also decided to make it its own crate and write some documentation about it, making it more generic along the way. I think this should be a pretty clean representation of what this is trying to do. --- Cargo.lock | 14 +- Cargo.toml | 4 + nexus/inventory/Cargo.toml | 1 + nexus/inventory/src/builder.rs | 19 +- nexus/reconfigurator/planning/Cargo.toml | 2 +- .../planning/src/blueprint_builder.rs | 61 +--- nexus/reconfigurator/planning/src/example.rs | 6 +- nexus/reconfigurator/planning/src/planner.rs | 2 +- typed-rng/Cargo.toml | 11 + typed-rng/src/lib.rs | 260 ++++++++++++++++++ 10 files changed, 326 insertions(+), 54 deletions(-) create mode 100644 typed-rng/Cargo.toml create mode 100644 typed-rng/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index eca65905da..63dc1cc735 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4717,6 +4717,7 @@ dependencies = [ "strum 0.26.1", "thiserror", "tokio", + "typed-rng", "uuid 1.7.0", ] @@ -4827,10 +4828,10 @@ dependencies = [ "omicron-test-utils", "omicron-workspace-hack", "rand 0.8.5", - "rand_seeder", "sled-agent-client", "slog", "thiserror", + "typed-rng", "uuid 1.7.0", ] @@ -10297,6 +10298,17 @@ version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4a90726108dab678edab76459751e1cc7c597c3484a6384d6423191255fa641b" +[[package]] +name = "typed-rng" +version = "0.1.0" +dependencies = [ + "omicron-workspace-hack", + "rand 0.8.5", + "rand_core 0.6.4", + "rand_seeder", + "uuid 1.7.0", +] + [[package]] name = "typenum" version = "1.16.0" diff --git a/Cargo.toml b/Cargo.toml index 9cfb3ed283..0d91aa076b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,6 +70,7 @@ members = [ "test-utils", "tufaceous-lib", "tufaceous", + "typed-rng", "update-common", "update-engine", "uuid-kinds", @@ -149,6 +150,7 @@ default-members = [ "test-utils", "tufaceous-lib", "tufaceous", + "typed-rng", "update-common", "update-engine", "uuid-kinds", @@ -343,6 +345,7 @@ propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev proptest = "1.4.0" quote = "1.0" rand = "0.8.5" +rand_core = "0.6.4" rand_seeder = "0.2.3" ratatui = "0.26.1" rayon = "1.9" @@ -434,6 +437,7 @@ trybuild = "1.0.89" tufaceous = { path = "tufaceous" } tufaceous-lib = { path = "tufaceous-lib" } tui-tree-widget = "0.17.0" +typed-rng = { path = "typed-rng" } unicode-width = "0.1.11" update-common = { path = "update-common" } update-engine = { path = "update-engine" } diff --git a/nexus/inventory/Cargo.toml b/nexus/inventory/Cargo.toml index 1c20e8f8b6..43041ab146 100644 --- a/nexus/inventory/Cargo.toml +++ b/nexus/inventory/Cargo.toml @@ -19,6 +19,7 @@ sled-agent-client.workspace = true slog.workspace = true strum.workspace = true thiserror.workspace = true +typed-rng.workspace = true uuid.workspace = true omicron-workspace-hack.workspace = true diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 2e482fcebf..0506e8286a 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -29,8 +29,10 @@ use nexus_types::inventory::SledAgent; use nexus_types::inventory::Zpool; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::hash::Hash; use std::sync::Arc; use thiserror::Error; +use typed_rng::UuidRng; use uuid::Uuid; /// Describes an operational error encountered during the collection process @@ -86,6 +88,8 @@ pub struct CollectionBuilder { BTreeMap, RotPageFound>>, sleds: BTreeMap, omicron_zones: BTreeMap, + // We just generate one UUID for each collection. + id_rng: UuidRng, } impl CollectionBuilder { @@ -111,6 +115,7 @@ impl CollectionBuilder { rot_pages_found: BTreeMap::new(), sleds: BTreeMap::new(), omicron_zones: BTreeMap::new(), + id_rng: UuidRng::from_entropy(), } } @@ -123,7 +128,7 @@ impl CollectionBuilder { } Collection { - id: Uuid::new_v4(), + id: self.id_rng.next(), errors: self.errors.into_iter().map(|e| e.to_string()).collect(), time_started: self.time_started, time_done: now_db_precision(), @@ -140,6 +145,18 @@ impl CollectionBuilder { } } + /// Within tests, set a seeded RNG for deterministic results. + /// + /// This will ensure that tests that use this builder will produce the same + /// results each time they are run. + pub fn set_rng_seed(&mut self, seed: H) -> &mut Self { + // Important to add some more bytes here, so that builders with the + // same seed but different purposes don't end up with the same UUIDs. + const SEED_EXTRA: &str = "collection-builder"; + self.id_rng.set_seed(seed, SEED_EXTRA); + self + } + /// Record service processor state `sp_state` reported by MGS /// /// `sp_type` and `slot` identify which SP this was. diff --git a/nexus/reconfigurator/planning/Cargo.toml b/nexus/reconfigurator/planning/Cargo.toml index 3c4ba12ee4..cb55d9aa7c 100644 --- a/nexus/reconfigurator/planning/Cargo.toml +++ b/nexus/reconfigurator/planning/Cargo.toml @@ -16,10 +16,10 @@ nexus-inventory.workspace = true nexus-types.workspace = true omicron-common.workspace = true rand.workspace = true -rand_seeder.workspace = true sled-agent-client.workspace = true slog.workspace = true thiserror.workspace = true +typed-rng.workspace = true uuid.workspace = true omicron-workspace-hack.workspace = true diff --git a/nexus/reconfigurator/planning/src/blueprint_builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder.rs index 0b0d422916..ab40f3bbb7 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder.rs @@ -38,7 +38,6 @@ use omicron_common::api::external::Vni; use omicron_common::api::internal::shared::NetworkInterface; use omicron_common::api::internal::shared::NetworkInterfaceKind; use rand::rngs::StdRng; -use rand::RngCore; use rand::SeedableRng; use slog::o; use slog::Logger; @@ -50,6 +49,7 @@ use std::net::Ipv4Addr; use std::net::Ipv6Addr; use std::net::SocketAddrV6; use thiserror::Error; +use typed_rng::UuidRng; use uuid::Uuid; /// Errors encountered while assembling blueprints @@ -223,7 +223,7 @@ impl<'a> BlueprintBuilder<'a> { }) .collect::>()?; Ok(Blueprint { - id: rng.blueprint_rng.next_uuid(), + id: rng.blueprint_rng.next(), blueprint_zones, parent_blueprint_id: None, internal_dns_version, @@ -375,7 +375,7 @@ impl<'a> BlueprintBuilder<'a> { let blueprint_zones = self.zones.into_zones_map(self.policy.sleds.keys().copied()); Blueprint { - id: self.rng.blueprint_rng.next_uuid(), + id: self.rng.blueprint_rng.next(), blueprint_zones, parent_blueprint_id: Some(self.parent_blueprint.id), internal_dns_version: self.internal_dns_version, @@ -452,7 +452,7 @@ impl<'a> BlueprintBuilder<'a> { .collect(); let zone = OmicronZoneConfig { - id: self.rng.zone_rng.next_uuid(), + id: self.rng.zone_rng.next(), underlay_address: ip, zone_type: OmicronZoneType::InternalNtp { address: ntp_address.to_string(), @@ -502,7 +502,7 @@ impl<'a> BlueprintBuilder<'a> { let port = omicron_common::address::CRUCIBLE_PORT; let address = SocketAddrV6::new(ip, port, 0, 0).to_string(); let zone = OmicronZoneConfig { - id: self.rng.zone_rng.next_uuid(), + id: self.rng.zone_rng.next(), underlay_address: ip, zone_type: OmicronZoneType::Crucible { address, @@ -589,7 +589,7 @@ impl<'a> BlueprintBuilder<'a> { }; for _ in 0..num_nexus_to_add { - let nexus_id = self.rng.zone_rng.next_uuid(); + let nexus_id = self.rng.zone_rng.next(); let external_ip = self .available_external_ips .next() @@ -617,7 +617,7 @@ impl<'a> BlueprintBuilder<'a> { .next() .ok_or(Error::NoSystemMacAddressAvailable)?; NetworkInterface { - id: self.rng.network_interface_rng.next_uuid(), + id: self.rng.network_interface_rng.next(), kind: NetworkInterfaceKind::Service { id: nexus_id }, name: format!("nexus-{nexus_id}").parse().unwrap(), ip, @@ -739,14 +739,14 @@ struct BlueprintBuilderRng { impl BlueprintBuilderRng { fn new() -> Self { - Self::new_from_rng(StdRng::from_entropy()) + Self::new_from_parent(StdRng::from_entropy()) } - fn new_from_rng(mut root_rng: StdRng) -> Self { - let blueprint_rng = UuidRng::from_root_rng(&mut root_rng, "blueprint"); - let zone_rng = UuidRng::from_root_rng(&mut root_rng, "zone"); + fn new_from_parent(mut parent: StdRng) -> Self { + let blueprint_rng = UuidRng::from_parent_rng(&mut parent, "blueprint"); + let zone_rng = UuidRng::from_parent_rng(&mut parent, "zone"); let network_interface_rng = - UuidRng::from_root_rng(&mut root_rng, "network_interface"); + UuidRng::from_parent_rng(&mut parent, "network_interface"); BlueprintBuilderRng { blueprint_rng, zone_rng, network_interface_rng } } @@ -755,40 +755,7 @@ impl BlueprintBuilderRng { // Important to add some more bytes here, so that builders with the // same seed but different purposes don't end up with the same UUIDs. const SEED_EXTRA: &str = "blueprint-builder"; - let mut seeder = rand_seeder::Seeder::from((seed, SEED_EXTRA)); - *self = Self::new_from_rng(seeder.make_rng::()); - } -} - -#[derive(Debug)] -pub(crate) struct UuidRng { - rng: StdRng, -} - -impl UuidRng { - /// Returns a new `UuidRng` generated from the root RNG. - /// - /// `extra` is a string that should be unique to the purpose of the UUIDs. - fn from_root_rng(root_rng: &mut StdRng, extra: &'static str) -> Self { - let seed = root_rng.next_u64(); - let mut seeder = rand_seeder::Seeder::from((seed, extra)); - Self { rng: seeder.make_rng::() } - } - - /// `extra` is a string that should be unique to the purpose of the UUIDs. - pub(crate) fn from_seed(seed: H, extra: &'static str) -> Self { - let mut seeder = rand_seeder::Seeder::from((seed, extra)); - Self { rng: seeder.make_rng::() } - } - - /// Returns a new UUIDv4 generated from the RNG. - pub(crate) fn next_uuid(&mut self) -> Uuid { - let mut bytes = [0; 16]; - self.rng.fill_bytes(&mut bytes); - // Builder::from_random_bytes will turn the random bytes into a valid - // UUIDv4. (Parts of the system depend on the UUID actually being valid - // v4, so it's important that we don't just use `uuid::from_bytes`.) - uuid::Builder::from_random_bytes(bytes).into_uuid() + *self = Self::new_from_parent(typed_rng::from_seed(seed, SEED_EXTRA)); } } @@ -1013,7 +980,7 @@ pub mod test { assert_eq!(diff.sleds_changed().count(), 0); // The next step is adding these zones to a new sled. - let new_sled_id = example.sled_rng.next_uuid(); + let new_sled_id = example.sled_rng.next(); let _ = example.system.sled(SledBuilder::new().id(new_sled_id)).unwrap(); let policy = example.system.to_policy().unwrap(); diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 23df35e9ae..a18e3b71cf 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -5,7 +5,6 @@ //! Example blueprints use crate::blueprint_builder::BlueprintBuilder; -use crate::blueprint_builder::UuidRng; use crate::system::SledBuilder; use crate::system::SystemDescription; use nexus_types::deployment::Blueprint; @@ -14,6 +13,7 @@ use nexus_types::deployment::Policy; use nexus_types::inventory::Collection; use omicron_common::api::external::Generation; use sled_agent_client::types::OmicronZonesConfig; +use typed_rng::UuidRng; pub struct ExampleSystem { pub system: SystemDescription, @@ -38,8 +38,7 @@ impl ExampleSystem { ) -> ExampleSystem { let mut system = SystemDescription::new(); let mut sled_rng = UuidRng::from_seed(test_name, "ExampleSystem"); - let sled_ids: Vec<_> = - (0..nsleds).map(|_| sled_rng.next_uuid()).collect(); + let sled_ids: Vec<_> = (0..nsleds).map(|_| sled_rng.next()).collect(); for sled_id in &sled_ids { let _ = system.sled(SledBuilder::new().id(*sled_id)).unwrap(); } @@ -107,6 +106,7 @@ impl ExampleSystem { let blueprint = builder.build(); let mut builder = system.to_collection_builder().expect("failed to build collection"); + builder.set_rng_seed((test_name, "ExampleSystem collection")); for sled_id in blueprint.sleds() { let Some(zones) = blueprint.blueprint_zones.get(&sled_id) else { diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 60eef225d3..84360aded9 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -402,7 +402,7 @@ mod test { verify_blueprint(&blueprint2); // Now add a new sled. - let new_sled_id = example.sled_rng.next_uuid(); + let new_sled_id = example.sled_rng.next(); let _ = example.system.sled(SledBuilder::new().id(new_sled_id)).unwrap(); let policy = example.system.to_policy().unwrap(); diff --git a/typed-rng/Cargo.toml b/typed-rng/Cargo.toml new file mode 100644 index 0000000000..b02a6b974a --- /dev/null +++ b/typed-rng/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "typed-rng" +version = "0.1.0" +edition = "2021" + +[dependencies] +omicron-workspace-hack.workspace = true +rand.workspace = true +rand_core.workspace = true +rand_seeder.workspace = true +uuid.workspace = true diff --git a/typed-rng/src/lib.rs b/typed-rng/src/lib.rs new file mode 100644 index 0000000000..5d5e4b1665 --- /dev/null +++ b/typed-rng/src/lib.rs @@ -0,0 +1,260 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Typed RNGs with support for tree-based RNGs. +//! +//! ## [`TypedRng`] +//! +//! This library contains [`TypedRng`], a simple wrapper around a random number +//! generator that generates values of a particular type. +//! +//! At the moment, it only supports stateless value creation, where the +//! `Generatable::generate` method does not have access to anything other than +//! the RNG state. It may be extended in the future with the capability to pass +//! in persistent state. +//! +//! ### Tree-based RNGs +//! +//! Many RNG models are organized in a tree structure, where a parent RNG +//! generates a child RNG. The main benefit to this kind of structure is +//! stability of output: because the different child RNGs are all independent +//! of each other, making more calls to one RNG will not affect any others. +//! +//! The `TypedRng` struct provides a method to generate a new RNG from a parent +//! RNG and a seed. This is useful when you want to generate a new RNG that is +//! independent of the parent RNG, but still deterministic. +//! +//! ### Comparison with property-based testing +//! +//! In a sense, this is a very simple version of how random values are +//! generated with property-based testing, with e.g. `Arbitrary` or proptest's +//! `Strategy`. +//! +//! But with property-based tests, the goal is for small changes in the random +//! input to result in _small_ changes in the output. High-quality libraries +//! like proptest also focus on shrinking. +//! +//! With `Generatable`, the goal is for small changes in the random input (the +//! seed used to initialize the RNG) to result in _large_ changes in output. +//! (However, it is possible to delegate the bulk of the operation to a value +//! generator from a PBT framework). There is also no need for shrinking. +//! +//! Overall, this means that `Generatable` can be used in both testing and +//! production. +//! +//! ## Other functionality +//! +//! This crate also provides two additional convenience functions: +//! +//! - [`from_seed`], which generates a new RNG from a seed. +//! - [`from_parent_and_seed`], which generates a new RNG from a parent RNG and +//! a seed. +//! +//! Both these methods are short, but more ergonomic and less prone to misuse +//! than using the underlying libraries directly. + +use std::{fmt, hash::Hash, marker::PhantomData}; + +use rand::rngs::StdRng; +use rand_core::{RngCore, SeedableRng}; +use uuid::Uuid; + +/// Returns a new RNG generated only from the given seeds `seed` and `extra`. +/// `seed` may be passed down from another caller, e.g. a test, and `extra` +/// should be a fixed value specific to the callsite. +/// +/// This takes two hashable arguments rather than one, because when one is +/// passing down a seed, it is all too easy to not include any extra +/// information. That may result in multiple different RNGs generating the same +/// random values. So we expect that callers will also provide bytes of their +/// own, specific to the callsite, and gently guide them towards doing the +/// right thing. +pub fn from_seed(seed: H, extra: H2) -> R +where + R: SeedableRng, + H: Hash, + H2: Hash, +{ + // XXX: is Hash really the right thing to use here? That's what + // rand_seeder uses, but something like https://docs.rs/stable-hash may + // be more correct. + + let mut seeder = rand_seeder::Seeder::from((seed, extra)); + seeder.make_rng::() +} + +/// Generates a new RNG from a parent RNG and a hashable seed. +pub fn from_parent_and_seed(parent_rng: &mut R2, seed: H) -> R +where + R: SeedableRng, + R2: RngCore, + H: Hash, +{ + let rng_seed = parent_rng.next_u64(); + + let mut seeder = rand_seeder::Seeder::from((rng_seed, seed)); + seeder.make_rng::() +} + +/// An RNG that can be used to generate values of a single type. +/// +/// This is a convenience wrapper around a random number generator that +/// generates values of a particular type. It works against any type that +/// implements [`Generatable`], and any RNG that implements [`RngCore`]. +pub struct TypedRng { + rng: R, + // PhantomData T> is like PhantomData, but it doesn't inherit + // Send/Sync from T. See + // https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns. + _marker: PhantomData T>, +} + +impl TypedRng { + /// Returns a new typed RNG from entropy. + pub fn from_entropy() -> Self { + Self::new(StdRng::from_entropy()) + } +} + +impl TypedRng +where + T: Generatable, + R: RngCore, +{ + /// Returns a new typed RNG from the given RNG. + pub fn new(rng: R) -> Self { + Self { rng, _marker: PhantomData } + } + + /// Returns a new typed RNG generated from the parent RNG, along with a + /// seed. + /// + /// Many RNG models are organized in a tree structure, where a parent RNG + /// generates a child RNG. The main benefit to this kind of structure is + /// stability of output: because the different child RNGs are all + /// independent of each other, making more calls to one RNG will not affect + /// any others. + pub fn from_parent_rng( + parent_rng: &mut R2, + seed: H, + ) -> Self + where + R: SeedableRng, + { + Self::new(from_parent_and_seed(parent_rng, seed)) + } + + /// Returns a new typed RNG generated only from the given seeds `seed` and + /// `extra`. + /// + /// This takes two hashable arguments rather than one, because when one is + /// passing down a seed set by e.g. a test, it is all too easy to just pass + /// down that seed here. That may result in multiple different RNGs + /// generating the same random values. So we expect that callers will also + /// provide bytes of their own, specific to the call-site, and gently guide + /// them towards doing the right thing. + pub fn from_seed(seed: H, extra: H2) -> Self + where + R: SeedableRng, + H: Hash, + H2: Hash, + { + let mut seeder = rand_seeder::Seeder::from((seed, extra)); + Self::new(seeder.make_rng::()) + } + + /// Sets the seed for this RNG to the given value. + /// + /// This takes two hashable arguments rather than one, for much the same + /// reason as [`Self::from_seed`]. + pub fn set_seed(&mut self, seed: H, extra: H2) + where + R: SeedableRng, + H: Hash, + H2: Hash, + { + let mut seeder = rand_seeder::Seeder::from((seed, extra)); + self.rng = seeder.make_rng::(); + } + + /// Returns a mutable reference to the RNG inside. + pub fn inner_mut(&mut self) -> &mut R { + &mut self.rng + } + + /// Consumes self, returning the RNG inside. + pub fn into_inner(self) -> R { + self.rng + } + + /// Returns the next value. + pub fn next(&mut self) -> T { + T::generate(&mut self.rng) + } +} + +// --- Trait impls --- +// +// These have to be done by hand to avoid a dependency on T. + +impl Clone for TypedRng { + fn clone(&self) -> Self { + Self { rng: self.rng.clone(), _marker: PhantomData } + } +} + +impl Copy for TypedRng {} + +impl Default for TypedRng { + fn default() -> Self { + Self { rng: R::default(), _marker: PhantomData } + } +} + +impl fmt::Debug for TypedRng { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.debug_struct("TypedRng").field("rng", &self.rng).finish() + } +} + +impl PartialEq for TypedRng { + fn eq(&self, other: &Self) -> bool { + self.rng == other.rng + } +} + +impl Eq for TypedRng {} + +/// Represents a value that can be generated. +/// +/// This is used to generate random values of a type in a deterministic manner, +/// given a random number generator. +pub trait Generatable { + fn generate(rng: &mut R) -> Self; +} + +impl Generatable for Uuid { + fn generate(rng: &mut R) -> Self { + let mut bytes = [0; 16]; + rng.fill_bytes(&mut bytes); + // Builder::from_random_bytes will turn the random bytes into a valid + // UUIDv4. (Parts of the system depend on the UUID actually being valid + // v4, so it's important that we don't just use `uuid::from_bytes`.) + uuid::Builder::from_random_bytes(bytes).into_uuid() + } +} + +pub type UuidRng = TypedRng; + +#[cfg(test)] +mod tests { + use super::*; + + // Test that TypedRng is Send and Sync even if T isn't. + const _: fn() = || { + fn assert_send_sync() {} + struct NotSendSync(*mut u8); + assert_send_sync::>(); + }; +}