Skip to content

Commit

Permalink
[reconfigurator] add SimRngState to track RNGs (#7019)
Browse files Browse the repository at this point in the history
Currently, each time a component like a system or collection is generated, its
RNGs live in their own world. While this is okay for tests, it is somewhat
inconvenient for the reconfigurator CLI which would really be able to generate
not just one fixed RNG but a succession of generations of RNGs.

In some cases, to do incremental work we were reusing existing RNGs (e.g. the
sled ID RNG). Unfortunately that led to situations where changing how the
example system was generated would also change these downstream IDs.

Restructure how the RNGs are managed -- each builder now has its own RNG struct
which can be initialized separately (we pass these around rather than `impl
Hash` instances for type safety). We also now have a stateful RNG creator which
tracks RNG generation numbers and can be used to build downstream RNGs.

In an upcoming PR we're going to use this stateful RNG creator in
reconfigurator-sim.

I've tried to minimize disruption to existing tests, especially those with
checked-in fixtures. The way to generate additional sleds does change, though,
and because the physical disk and zpool IDs are generated as pure functions of
the sled ID, those change too.
  • Loading branch information
sunshowers authored Nov 8, 2024
1 parent 3f6cd90 commit 15305ae
Show file tree
Hide file tree
Showing 9 changed files with 358 additions and 191 deletions.
42 changes: 31 additions & 11 deletions nexus/inventory/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,28 @@ impl std::fmt::Display for CollectorBug {
}
}

/// Random generator of UUIDs for a [`CollectionBuilder`].
#[derive(Debug, Clone)]
pub struct CollectionBuilderRng {
// We just generate one UUID for each collection.
id_rng: TypedUuidRng<CollectionKind>,
}

impl CollectionBuilderRng {
pub fn from_entropy() -> Self {
CollectionBuilderRng { id_rng: TypedUuidRng::from_entropy() }
}

pub fn from_seed<H: Hash>(seed: H) -> 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";
CollectionBuilderRng {
id_rng: TypedUuidRng::from_seed(seed, SEED_EXTRA),
}
}
}

/// Build an inventory [`Collection`]
///
/// This interface is oriented around the interfaces used by an actual
Expand All @@ -92,9 +114,10 @@ pub struct CollectionBuilder {
sleds: BTreeMap<SledUuid, SledAgent>,
clickhouse_keeper_cluster_membership:
BTreeSet<ClickhouseKeeperClusterMembership>,

// We just generate one UUID for each collection.
id_rng: TypedUuidRng<CollectionKind>,
// CollectionBuilderRng is taken by value, rather than passed in as a
// mutable ref, to encourage a tree-like structure where each RNG is
// generally independent.
rng: CollectionBuilderRng,
}

impl CollectionBuilder {
Expand All @@ -120,7 +143,7 @@ impl CollectionBuilder {
rot_pages_found: BTreeMap::new(),
sleds: BTreeMap::new(),
clickhouse_keeper_cluster_membership: BTreeSet::new(),
id_rng: TypedUuidRng::from_entropy(),
rng: CollectionBuilderRng::from_entropy(),
}
}

Expand All @@ -133,7 +156,7 @@ impl CollectionBuilder {
}

Collection {
id: self.id_rng.next(),
id: self.rng.id_rng.next(),
errors: self.errors.into_iter().map(|e| e.to_string()).collect(),
time_started: self.time_started,
time_done: now_db_precision(),
Expand All @@ -151,15 +174,12 @@ impl CollectionBuilder {
}
}

/// Within tests, set a seeded RNG for deterministic results.
/// Within tests, set an 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<H: Hash>(&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);
pub fn set_rng(&mut self, rng: CollectionBuilderRng) -> &mut Self {
self.rng = rng;
self
}

Expand Down
1 change: 1 addition & 0 deletions nexus/inventory/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ mod sled_agent_enumerator;

// only exposed for test code to construct collections
pub use builder::CollectionBuilder;
pub use builder::CollectionBuilderRng;
pub use builder::CollectorBug;
pub use builder::InventoryError;

Expand Down
37 changes: 23 additions & 14 deletions nexus/reconfigurator/planning/src/blueprint_builder/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ use std::collections::BTreeMap;
use std::collections::BTreeSet;
use std::collections::HashSet;
use std::fmt;
use std::hash::Hash;
use std::net::IpAddr;
use std::net::Ipv6Addr;
use std::net::SocketAddr;
Expand Down Expand Up @@ -301,18 +300,20 @@ impl<'a> BlueprintBuilder<'a> {
sled_ids: impl Iterator<Item = SledUuid>,
creator: &str,
) -> Blueprint {
Self::build_empty_with_sleds_impl(sled_ids, creator, PlannerRng::new())
Self::build_empty_with_sleds_impl(
sled_ids,
creator,
PlannerRng::from_entropy(),
)
}

/// A version of [`Self::build_empty_with_sleds`] that allows the
/// blueprint ID to be generated from a random seed.
pub fn build_empty_with_sleds_seeded<H: Hash>(
/// blueprint ID to be generated from a deterministic RNG.
pub fn build_empty_with_sleds_seeded(
sled_ids: impl Iterator<Item = SledUuid>,
creator: &str,
seed: H,
rng: PlannerRng,
) -> Blueprint {
let mut rng = PlannerRng::new();
rng.set_seed(seed);
Self::build_empty_with_sleds_impl(sled_ids, creator, rng)
}

Expand Down Expand Up @@ -450,7 +451,7 @@ impl<'a> BlueprintBuilder<'a> {
creator: creator.to_owned(),
operations: Vec::new(),
comments: Vec::new(),
rng: PlannerRng::new(),
rng: PlannerRng::from_entropy(),
})
}

Expand Down Expand Up @@ -579,12 +580,12 @@ impl<'a> BlueprintBuilder<'a> {
self.sled_state.insert(sled_id, desired_state);
}

/// Within tests, set a seeded RNG for deterministic results.
/// Within tests, set an 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<H: Hash>(&mut self, seed: H) -> &mut Self {
self.rng.set_seed(seed);
pub fn set_rng(&mut self, rng: PlannerRng) -> &mut Self {
self.rng = rng;
self
}

Expand Down Expand Up @@ -2615,6 +2616,7 @@ pub mod test {
use super::*;
use crate::example::example;
use crate::example::ExampleSystemBuilder;
use crate::example::SimRngState;
use crate::system::SledBuilder;
use expectorate::assert_contents;
use nexus_inventory::CollectionBuilder;
Expand Down Expand Up @@ -2841,8 +2843,13 @@ pub mod test {
fn test_basic() {
static TEST_NAME: &str = "blueprint_builder_test_basic";
let logctx = test_setup_log(TEST_NAME);
let (mut example, blueprint1) =
ExampleSystemBuilder::new(&logctx.log, TEST_NAME).build();

let mut rng = SimRngState::from_seed(TEST_NAME);
let (mut example, blueprint1) = ExampleSystemBuilder::new_with_rng(
&logctx.log,
rng.next_system_rng(),
)
.build();
verify_blueprint(&blueprint1);

let mut builder = BlueprintBuilder::new_based_on(
Expand Down Expand Up @@ -2878,7 +2885,9 @@ pub mod test {
assert_eq!(diff.sleds_modified.len(), 0);

// The next step is adding these zones to a new sled.
let new_sled_id = example.sled_rng.next();
let mut sled_id_rng = rng.next_sled_id_rng();
let new_sled_id = sled_id_rng.next();

let _ =
example.system.sled(SledBuilder::new().id(new_sled_id)).unwrap();
let input = example.system.to_planning_input_builder().unwrap().build();
Expand Down
21 changes: 15 additions & 6 deletions nexus/reconfigurator/planning/src/blueprint_builder/zones.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ mod tests {

use crate::{
blueprint_builder::{test::verify_blueprint, BlueprintBuilder, Ensure},
example::ExampleSystemBuilder,
example::{ExampleSystemBuilder, SimRngState},
planner::rng::PlannerRng,
};

use super::*;
Expand All @@ -252,13 +253,21 @@ mod tests {
fn test_builder_zones() {
static TEST_NAME: &str = "blueprint_test_builder_zones";
let logctx = test_setup_log(TEST_NAME);
let (mut example, blueprint_initial) =
ExampleSystemBuilder::new(&logctx.log, TEST_NAME).build();

let mut rng = SimRngState::from_seed(TEST_NAME);
let (example, blueprint_initial) = ExampleSystemBuilder::new_with_rng(
&logctx.log,
rng.next_system_rng(),
)
.build();

// Add a completely bare sled to the input.
let (new_sled_id, input2) = {
let mut sled_id_rng = rng.next_sled_id_rng();
let new_sled_id = sled_id_rng.next();

let mut input = example.input.clone().into_builder();
let new_sled_id = example.sled_rng.next();

input
.add_sled(
new_sled_id,
Expand Down Expand Up @@ -308,7 +317,7 @@ mod tests {
"the_test",
)
.expect("creating blueprint builder");
builder.set_rng_seed((TEST_NAME, "bp2"));
builder.set_rng(PlannerRng::from_seed((TEST_NAME, "bp2")));

// Test adding a new sled with an NTP zone.
assert_eq!(
Expand Down Expand Up @@ -486,7 +495,7 @@ mod tests {
"the_test",
)
.expect("creating blueprint builder");
builder.set_rng_seed((TEST_NAME, "bp2"));
builder.set_rng(PlannerRng::from_seed((TEST_NAME, "bp2")));

// This call by itself shouldn't bump the generation number.
builder.zones.change_sled_zones(existing_sled_id);
Expand Down
Loading

0 comments on commit 15305ae

Please sign in to comment.