Skip to content

Commit

Permalink
cleanup, programmatic diff, fix up test
Browse files Browse the repository at this point in the history
  • Loading branch information
davepacheco committed Jan 12, 2024
1 parent c39c75d commit 81842f6
Show file tree
Hide file tree
Showing 11 changed files with 410 additions and 400 deletions.
2 changes: 0 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 0 additions & 24 deletions clients/nexus-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,6 @@ progenitor::generate_api!(
}
);

impl types::OmicronZoneType {
/// Human-readable label describing what kind of zone this is
///
/// This is just use for testing and reporting.
/// Note that this function is identical to its analog in sled-agent-client.
pub fn label(&self) -> impl std::fmt::Display {
match self {
types::OmicronZoneType::BoundaryNtp { .. } => "boundary_ntp",
types::OmicronZoneType::Clickhouse { .. } => "clickhouse",
types::OmicronZoneType::ClickhouseKeeper { .. } => {
"clickhouse_keeper"
}
types::OmicronZoneType::CockroachDb { .. } => "cockroach_db",
types::OmicronZoneType::Crucible { .. } => "crucible",
types::OmicronZoneType::CruciblePantry { .. } => "crucible_pantry",
types::OmicronZoneType::ExternalDns { .. } => "external_dns",
types::OmicronZoneType::InternalDns { .. } => "internal_dns",
types::OmicronZoneType::InternalNtp { .. } => "internal_ntp",
types::OmicronZoneType::Nexus { .. } => "nexus",
types::OmicronZoneType::Oximeter { .. } => "oximeter",
}
}
}

impl omicron_common::api::external::ClientError for types::Error {
fn message(&self) -> String {
self.message.clone()
Expand Down
1 change: 0 additions & 1 deletion clients/sled-agent-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ impl types::OmicronZoneType {
/// Human-readable label describing what kind of zone this is
///
/// This is just use for testing and reporting.
/// Note that this function is identical to its analog in nexus-client.
pub fn label(&self) -> impl std::fmt::Display {
match self {
types::OmicronZoneType::BoundaryNtp { .. } => "boundary_ntp",
Expand Down
2 changes: 0 additions & 2 deletions nexus/deployment/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ uuid.workspace = true
omicron-workspace-hack.workspace = true

[dev-dependencies]
expectorate.workspace = true
nexus-inventory.workspace = true
omicron-test-utils.workspace = true
rand.workspace = true
sled-agent-client.workspace = true
6 changes: 5 additions & 1 deletion nexus/deployment/src/blueprint_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,12 @@ impl<'a> BlueprintBuilder<'a> {
zones: old_sled_zones.zones.clone(),
}
} else {
// The first generation is reserved to mean the one
// containing no zones. See
// OMICRON_ZONES_CONFIG_INITIAL_GENERATION. So we start
// with the next one.
OmicronZonesConfig {
generation: Generation::new(),
generation: Generation::new().next(),
zones: vec![],
}
}
Expand Down
97 changes: 49 additions & 48 deletions nexus/deployment/src/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,12 @@ mod test {
use omicron_common::api::external::ByteCount;
use omicron_common::api::external::Generation;
use omicron_test_utils::dev::test_setup_log;
use rand::Fill;
use rand::SeedableRng;
use sled_agent_client::types::{
Baseboard, Inventory, OmicronZoneConfig, OmicronZoneDataset,
OmicronZoneType, OmicronZonesConfig, SledRole,
};
use std::collections::BTreeMap;
use std::collections::BTreeSet;
use std::fmt::Write;
use std::net::Ipv6Addr;
use std::net::SocketAddrV6;
use std::str::FromStr;
Expand Down Expand Up @@ -150,20 +147,6 @@ mod test {
fn example() -> (Collection, Policy) {
let mut builder = nexus_inventory::CollectionBuilder::new("test-suite");

// We want deterministic uuid generation in this test so that we get
// consistent output for expectorate.
// XXX-dap this is not quite good enough because the blueprint ids and
// the ids for new sleds and zones are generated elsewhere with
// Uuid::new_v4(). What to do? Pass the RNG around everywhere?
// Find/replace these uuids in the output? Add a programmatic interface
// to the differ?
let mut rng = rand::rngs::StdRng::from_seed(Default::default());
fn new_uuid(r: &mut rand::rngs::StdRng) -> Uuid {
let mut bytes = uuid::Bytes::default();
bytes.try_fill(r).unwrap();
uuid::Builder::from_random_bytes(bytes).into_uuid()
}

let sled_ids = [
"72443b6c-b8bb-4ffa-ab3a-aeaa428ed79b",
"a5f3db3a-61aa-4f90-ad3e-02833c253bf5",
Expand Down Expand Up @@ -199,7 +182,7 @@ mod test {
let zpools = &policy.sleds.get(&sled_id).unwrap().zpools;
let ip1 = sled_ip.saturating_add(1);
let zones: Vec<_> = std::iter::once(OmicronZoneConfig {
id: new_uuid(&mut rng),
id: Uuid::new_v4(),
underlay_address: sled_ip.saturating_add(1),
zone_type: OmicronZoneType::InternalNtp {
address: SocketAddrV6::new(ip1, 12345, 0, 0).to_string(),
Expand All @@ -211,7 +194,7 @@ mod test {
.chain(zpools.iter().enumerate().map(|(i, zpool_name)| {
let ip = sled_ip.saturating_add(u128::try_from(i + 2).unwrap());
OmicronZoneConfig {
id: new_uuid(&mut rng),
id: Uuid::new_v4(),
underlay_address: ip,
zone_type: OmicronZoneType::Crucible {
address: String::from("[::1]:12345"),
Expand Down Expand Up @@ -245,9 +228,6 @@ mod test {
fn test_basic_add_sled() {
let logctx = test_setup_log("planner_basic_add_sled");

// Assemble an output string that we'll check at the end.
let mut out = String::new();

// Use our example inventory collection.
let (collection, mut policy) = example();

Expand All @@ -273,12 +253,11 @@ mod test {
.plan()
.expect("failed to plan");

writeln!(
&mut out,
"1 -> 2 (expected no changes):\n{}",
blueprint1.diff(&blueprint2)
)
.unwrap();
let diff = blueprint1.diff(&blueprint2);
println!("1 -> 2 (expected no changes):\n{}", diff);
assert_eq!(diff.sleds_added().count(), 0);
assert_eq!(diff.sleds_removed().count(), 0);
assert_eq!(diff.sleds_changed().count(), 0);

// Now add a new sled.
let new_sled_id =
Expand All @@ -295,12 +274,22 @@ mod test {
.plan()
.expect("failed to plan");

writeln!(
&mut out,
"2 -> 3 (expect new NTP zone on new sled):\n{}",
blueprint2.diff(&blueprint3)
)
.unwrap();
let diff = blueprint2.diff(&blueprint3);
println!("2 -> 3 (expect new NTP zone on new sled):\n{}", diff,);
let sleds = diff.sleds_added().collect::<Vec<_>>();
let (sled_id, sled_zones) = sleds[0];
// We have defined elsewhere that the first generation contains no
// zones. So the first one with zones must be newer. See
// OMICRON_ZONES_CONFIG_INITIAL_GENERATION.
assert!(sled_zones.generation > Generation::new());
assert_eq!(sled_id, new_sled_id);
assert_eq!(sled_zones.zones.len(), 1);
assert!(matches!(
sled_zones.zones[0].zone_type,
OmicronZoneType::InternalNtp { .. }
));
assert_eq!(diff.sleds_removed().count(), 0);
assert_eq!(diff.sleds_changed().count(), 0);

// Check that the next step is to add Crucible zones
let blueprint4 = Planner::new_based_on(
Expand All @@ -312,12 +301,27 @@ mod test {
.plan()
.expect("failed to plan");

writeln!(
&mut out,
"3 -> 4 (expect Crucible zones):\n{}",
blueprint3.diff(&blueprint4)
)
.unwrap();
let diff = blueprint3.diff(&blueprint4);
println!("3 -> 4 (expect Crucible zones):\n{}", diff);
assert_eq!(diff.sleds_added().count(), 0);
assert_eq!(diff.sleds_removed().count(), 0);
let sleds = diff.sleds_changed().collect::<Vec<_>>();
assert_eq!(sleds.len(), 1);
let (sled_id, sled_changes) = &sleds[0];
assert_eq!(
sled_changes.generation_after,
sled_changes.generation_before.next()
);
assert_eq!(*sled_id, new_sled_id);
assert_eq!(sled_changes.zones_removed().count(), 0);
assert_eq!(sled_changes.zones_changed().count(), 0);
let zones = sled_changes.zones_added().collect::<Vec<_>>();
assert_eq!(zones.len(), 3);
for zone in &zones {
let OmicronZoneType::Crucible { .. } = zone.zone_type else {
panic!("unexpectedly added a non-Crucible zone");
};
}

// Check that there are no more steps
let blueprint5 = Planner::new_based_on(
Expand All @@ -329,14 +333,11 @@ mod test {
.plan()
.expect("failed to plan");

writeln!(
&mut out,
"4 -> 5 (expect no changes):\n{}",
blueprint4.diff(&blueprint5)
)
.unwrap();

expectorate::assert_contents("tests/output/planner_basic.out", &out);
let diff = blueprint4.diff(&blueprint5);
println!("4 -> 5 (expect no changes):\n{}", diff);
assert_eq!(diff.sleds_added().count(), 0);
assert_eq!(diff.sleds_removed().count(), 0);
assert_eq!(diff.sleds_changed().count(), 0);

logctx.cleanup_successful();
}
Expand Down
108 changes: 0 additions & 108 deletions nexus/deployment/tests/output/planner_basic.out

This file was deleted.

4 changes: 2 additions & 2 deletions nexus/src/app/deployment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use nexus_db_queries::context::OpContext;
use nexus_db_queries::db::pagination::Paginator;
use nexus_deployment::blueprint_builder::BlueprintBuilder;
use nexus_deployment::planner::Planner;
use nexus_types::deployment::params;
use nexus_types::deployment::Blueprint;
use nexus_types::deployment::BlueprintTarget;
use nexus_types::deployment::Policy;
Expand All @@ -35,6 +34,7 @@ use std::collections::BTreeSet;
use std::num::NonZeroU32;
use std::str::FromStr;
use uuid::Uuid;
use nexus_types::deployment::BlueprintTargetSet;

/// "limit" used in SQL queries that paginate through all sleds, zpools, etc.
// unsafe: `new_unchecked` is only unsound if the argument is 0.
Expand Down Expand Up @@ -159,7 +159,7 @@ impl super::Nexus {
pub async fn blueprint_target_set(
&self,
opctx: &OpContext,
params: params::BlueprintTargetSet,
params: BlueprintTargetSet,
) -> Result<BlueprintTarget, Error> {
opctx.authorize(Action::Modify, &authz::BLUEPRINT_CONFIG).await?;
let new_target_id = params.target_id;
Expand Down
Loading

0 comments on commit 81842f6

Please sign in to comment.