Skip to content

Remove ability to generate a Blueprint from an inventory collection #5583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 20, 2024
Merged

Remove ability to generate a Blueprint from an inventory collection #5583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0727514
wip - removing blueprint from collection
jgallagher Apr 19, 2024
4cb403d
wip - removing blueprint from collection
jgallagher Apr 19, 2024
0fc4c6d
wip - removing blueprint from collection
jgallagher Apr 19, 2024
e41b2a2
remove build_initial_from_collection from DNS tests
jgallagher Apr 19, 2024
7d6784b
update nexus internal openapi
jgallagher Apr 19, 2024
46bd51a
fix broken rustdoc link
jgallagher Apr 19, 2024
0dfbfdd
Merge remote-tracking branch 'origin/main' into john/remove-blueprint…
jgallagher Apr 19, 2024
152cfac
remove stray dbg!
jgallagher Apr 19, 2024
da9d7e4
rustdoc fixes
jgallagher Apr 19, 2024
692d55a
Merge branch 'main' into john/remove-blueprint-from-collection
jgallagher Apr 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions dev-tools/omdb/src/bin/omdb/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,6 @@ enum BlueprintsCommands {
Delete(BlueprintIdArgs),
/// Interact with the current target blueprint
Target(BlueprintsTargetArgs),
/// Generate an initial blueprint from a specific inventory collection
GenerateFromCollection(CollectionIdArgs),
/// Generate a new blueprint
Regenerate,
/// Import a blueprint
Expand Down Expand Up @@ -361,15 +359,6 @@ impl NexusArgs {
let token = omdb.check_allow_destructive()?;
cmd_nexus_blueprints_regenerate(&client, token).await
}
NexusCommands::Blueprints(BlueprintsArgs {
command: BlueprintsCommands::GenerateFromCollection(args),
}) => {
let token = omdb.check_allow_destructive()?;
cmd_nexus_blueprints_generate_from_collection(
&client, args, token,
)
.await
}
NexusCommands::Blueprints(BlueprintsArgs {
command: BlueprintsCommands::Import(args),
}) => {
Expand Down Expand Up @@ -1134,26 +1123,6 @@ async fn cmd_nexus_blueprints_target_set_enabled(
Ok(())
}

async fn cmd_nexus_blueprints_generate_from_collection(
client: &nexus_client::Client,
args: &CollectionIdArgs,
_destruction_token: DestructiveOperationToken,
) -> Result<(), anyhow::Error> {
let blueprint = client
.blueprint_generate_from_collection(
&nexus_client::types::CollectionId {
collection_id: args.collection_id,
},
)
.await
.context("creating blueprint from collection id")?;
eprintln!(
"created blueprint {} from collection id {}",
blueprint.id, args.collection_id
);
Ok(())
}

async fn cmd_nexus_blueprints_regenerate(
client: &nexus_client::Client,
_destruction_token: DestructiveOperationToken,
Expand Down
37 changes: 0 additions & 37 deletions dev-tools/reconfigurator-cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,6 @@ fn process_entry(sim: &mut ReconfiguratorSim, entry: String) -> LoopResult {
Commands::InventoryList => cmd_inventory_list(sim),
Commands::InventoryGenerate => cmd_inventory_generate(sim),
Commands::BlueprintList => cmd_blueprint_list(sim),
Commands::BlueprintFromInventory(args) => {
cmd_blueprint_from_inventory(sim, args)
}
Commands::BlueprintEdit(args) => cmd_blueprint_edit(sim, args),
Commands::BlueprintPlan(args) => cmd_blueprint_plan(sim, args),
Commands::BlueprintShow(args) => cmd_blueprint_show(sim, args),
Expand Down Expand Up @@ -374,8 +371,6 @@ enum Commands {

/// list all blueprints
BlueprintList,
/// generate a blueprint that represents the contents of an inventory
BlueprintFromInventory(InventoryArgs),
/// run planner to generate a new blueprint
BlueprintPlan(BlueprintPlanArgs),
/// edit contents of a blueprint directly
Expand Down Expand Up @@ -718,38 +713,6 @@ fn cmd_blueprint_list(
Ok(Some(table))
}

fn cmd_blueprint_from_inventory(
sim: &mut ReconfiguratorSim,
args: InventoryArgs,
) -> anyhow::Result<Option<String>> {
let collection_id = args.collection_id;
let collection = sim
.collections
.get(&collection_id)
.ok_or_else(|| anyhow!("no such collection: {}", collection_id))?;
let dns_version = Generation::new();
let planning_input = sim
.system
.to_planning_input_builder()
.context("generating planning_input builder")?
.build();
let creator = "reconfigurator-sim";
let blueprint = BlueprintBuilder::build_initial_from_collection(
collection,
dns_version,
dns_version,
planning_input.all_sled_ids(SledFilter::All),
creator,
)
.context("building collection")?;
let rv = format!(
"generated blueprint {} from inventory collection {}",
blueprint.id, collection_id
);
sim.blueprint_insert_new(blueprint);
Ok(Some(rv))
}

fn cmd_blueprint_plan(
sim: &mut ReconfiguratorSim,
args: BlueprintPlanArgs,
Expand Down
136 changes: 50 additions & 86 deletions nexus/db-queries/src/db/datastore/deployment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1263,12 +1263,12 @@ mod tests {
use nexus_inventory::now_db_precision;
use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder;
use nexus_reconfigurator_planning::blueprint_builder::Ensure;
use nexus_reconfigurator_planning::example::example;
use nexus_test_utils::db::test_setup_database;
use nexus_types::deployment::BlueprintZoneDisposition;
use nexus_types::deployment::BlueprintZoneFilter;
use nexus_types::deployment::PlanningInput;
use nexus_types::deployment::PlanningInputBuilder;
use nexus_types::deployment::Policy;
use nexus_types::deployment::SledDetails;
use nexus_types::deployment::SledDisk;
use nexus_types::deployment::SledFilter;
Expand All @@ -1279,7 +1279,6 @@ mod tests {
use nexus_types::external_api::views::SledState;
use nexus_types::inventory::Collection;
use omicron_common::address::Ipv6Subnet;
use omicron_common::api::external::Generation;
use omicron_common::disk::DiskIdentity;
use omicron_test_utils::dev;
use omicron_uuid_kinds::PhysicalDiskUuid;
Expand All @@ -1288,6 +1287,7 @@ mod tests {
use pretty_assertions::assert_eq;
use rand::thread_rng;
use rand::Rng;
use slog::Logger;
use std::mem;
use std::net::Ipv6Addr;

Expand Down Expand Up @@ -1359,65 +1359,32 @@ mod tests {
}
}

// Create a `Policy` that contains all the sleds found in `collection`
fn policy_from_collection(collection: &Collection) -> Policy {
Policy {
service_ip_pool_ranges: Vec::new(),
target_nexus_zone_count: collection
.all_omicron_zones()
.filter(|z| z.zone_type.is_nexus())
.count(),
}
}
fn representative(
log: &Logger,
test_name: &str,
) -> (Collection, PlanningInput, Blueprint) {
// We'll start with an example system.
let (mut base_collection, planning_input, mut blueprint) =
example(log, test_name, 3);

fn representative() -> (Collection, PlanningInput, Blueprint) {
// We'll start with a representative collection...
// Take a more thorough collection representative (includes SPs,
// etc.)...
let mut collection =
nexus_inventory::examples::representative().builder.build();

// ...and then mutate it such that the omicron zones it reports match
// the sled agent IDs it reports. Steal the sled agent info and drop the
// fake sled-agent IDs:
let mut empty_map = BTreeMap::new();
mem::swap(&mut empty_map, &mut collection.sled_agents);
let mut sled_agents = empty_map.into_values().collect::<Vec<_>>();

// Now reinsert them with IDs pulled from the omicron zones. This
// assumes we have more fake sled agents than omicron zones, which is
// currently true for the representative collection.
for &sled_id in collection.omicron_zones.keys() {
let some_sled_agent = sled_agents.pop().expect(
"fewer representative sled agents than \
representative omicron zones sleds",
);
collection.sled_agents.insert(sled_id, some_sled_agent);
}
// ... and replace its sled agent and Omicron zones with those from our
// example system.
mem::swap(
&mut collection.sled_agents,
&mut base_collection.sled_agents,
);
mem::swap(
&mut collection.omicron_zones,
&mut base_collection.omicron_zones,
);

let policy = policy_from_collection(&collection);
let planning_input = {
let mut builder = PlanningInputBuilder::new(
policy,
Generation::new(),
Generation::new(),
);
for (sled_id, agent) in &collection.sled_agents {
builder
.add_sled(
*sled_id,
fake_sled_details(Some(*agent.sled_agent_address.ip())),
)
.expect("failed to add sled to representative");
}
builder.build()
};
let blueprint = BlueprintBuilder::build_initial_from_collection(
&collection,
Generation::new(),
Generation::new(),
planning_input.all_sled_ids(SledFilter::All),
"test",
)
.unwrap();
// Treat this blueprint as the initial blueprint for the system.
blueprint.parent_blueprint_id = None;

(collection, planning_input, blueprint)
}
Expand All @@ -1442,17 +1409,11 @@ mod tests {
let mut db = test_setup_database(&logctx.log).await;
let (opctx, datastore) = datastore_test(&logctx, &db).await;

// Create an empty collection and a blueprint from it
let collection =
nexus_inventory::CollectionBuilder::new("test").build();
let blueprint1 = BlueprintBuilder::build_initial_from_collection(
&collection,
Generation::new(),
Generation::new(),
// Create an empty blueprint from it
let blueprint1 = BlueprintBuilder::build_empty_with_sleds(
std::iter::empty(),
"test",
)
.unwrap();
);
let authz_blueprint = authz_blueprint_from_id(blueprint1.id);

// Trying to read it from the database should fail with the relevant
Expand All @@ -1471,7 +1432,7 @@ mod tests {
let blueprint_read = datastore
.blueprint_read(&opctx, &authz_blueprint)
.await
.expect("failed to read collection back");
.expect("failed to read blueprint back");
assert_eq!(blueprint1, blueprint_read);
assert_eq!(
blueprint_list_all_ids(&opctx, &datastore).await,
Expand Down Expand Up @@ -1501,13 +1462,15 @@ mod tests {

#[tokio::test]
async fn test_representative_blueprint() {
const TEST_NAME: &str = "test_representative_blueprint";
// Setup
let logctx = dev::test_setup_log("test_representative_blueprint");
let logctx = dev::test_setup_log(TEST_NAME);
let mut db = test_setup_database(&logctx.log).await;
let (opctx, datastore) = datastore_test(&logctx, &db).await;

// Create a cohesive representative collection/policy/blueprint
let (collection, planning_input, blueprint1) = representative();
let (collection, planning_input, blueprint1) =
representative(&logctx.log, TEST_NAME);
let authz_blueprint1 = authz_blueprint_from_id(blueprint1.id);

// Write it to the database and read it back.
Expand Down Expand Up @@ -1632,10 +1595,23 @@ mod tests {
let blueprint2 = builder.build();
let authz_blueprint2 = authz_blueprint_from_id(blueprint2.id);

let diff = blueprint2.diff_since_blueprint(&blueprint1).unwrap();
println!("b1 -> b2: {}", diff.display());
println!("b1 disks: {:?}", blueprint1.blueprint_disks);
println!("b2 disks: {:?}", blueprint2.blueprint_disks);
// Check that we added the new sled, as well as its disks and zones.
assert_eq!(
blueprint1.blueprint_disks.len() + new_sled_zpools.len(),
blueprint2.blueprint_disks.len(),
blueprint1
.blueprint_disks
.values()
.map(|c| c.disks.len())
.sum::<usize>()
+ new_sled_zpools.len(),
blueprint2
.blueprint_disks
.values()
.map(|c| c.disks.len())
.sum::<usize>()
Copy link
Contributor Author

@jgallagher jgallagher Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion change is, I believe, a fix for a test that was passing incorrectly. blueprint_disks is a BTreeMap<SledUuid, PhysicalDisksConfig>, which means blueprint_disks.len() reported "for how many sleds do we have a PhysicalDisksConfig?", not "how many disks are in the blueprint" as the check seemed to mean. I think this was passing purely by coincidence:

  • blueprint1.blueprint_disks was empty, because a blueprint created from an inventory collection didn't populate it at all
  • new_sled_zpools was 4, because we added 4 disks
  • blueprint2.blueprint_disks.len() was 4, because there are 4 sleds in blueprint2, and going through the builder populates blueprint_disks

so we were previously passing because 0 + 4 == 4, even though the units of these values made the comparison nonsensical.

);
assert_eq!(
blueprint1.blueprint_zones.len() + 1,
Expand Down Expand Up @@ -1757,16 +1733,10 @@ mod tests {
// Create three blueprints:
// * `blueprint1` has no parent
// * `blueprint2` and `blueprint3` both have `blueprint1` as parent
let collection =
nexus_inventory::CollectionBuilder::new("test").build();
let blueprint1 = BlueprintBuilder::build_initial_from_collection(
&collection,
Generation::new(),
Generation::new(),
let blueprint1 = BlueprintBuilder::build_empty_with_sleds(
std::iter::empty(),
"test1",
)
.unwrap();
);
let blueprint2 = BlueprintBuilder::new_based_on(
&logctx.log,
&blueprint1,
Expand Down Expand Up @@ -1911,16 +1881,10 @@ mod tests {
let (opctx, datastore) = datastore_test(&logctx, &db).await;

// Create an initial blueprint and a child.
let collection =
nexus_inventory::CollectionBuilder::new("test").build();
let blueprint1 = BlueprintBuilder::build_initial_from_collection(
&collection,
Generation::new(),
Generation::new(),
let blueprint1 = BlueprintBuilder::build_empty_with_sleds(
std::iter::empty(),
"test1",
)
.unwrap();
);
let blueprint2 = BlueprintBuilder::new_based_on(
&logctx.log,
&blueprint1,
Expand Down
Loading