Skip to content

Commit 4e6a568

Browse files
authored
Use Reconfigurator simulator in planner integration tests (#9473)
This is a followon to #9456 and finishes converting the rest of the planner tests to use the simulator. I think this is a pretty solid win overall; there's a lot less boilerplate (e.g., in just raw LoC we dropped several hundred in the integration test module). I also picked up some small places where our simulator was missing bits we needed from integration tests. The biggest boilerplate reduction is that running the planner changes from this: ```rust let blueprint = Planner::new_based_on( logctx.log.clone(), &parent, &input, "test_blueprint", &collection, PlannerRng::from_seed((TEST_NAME, "blueprint")), ) .expect("failed to create planner") .plan() .expect("failed to plan"); ``` to this: ```rust let blueprint = sim.run_planner().expect("planning succeeded"); ``` because `sim` internally is able to construct all the other args needed for planning. It always uses the latest blueprint and latest inventory collection, which has been fine in practice. (If we needed to run the planner with different ones for some reason, it'd be easy to add a `run_planner_with_...`-style method to specify different ones.) There are still some rough edges; a couple off the top of my head: * A `SystemDescription` has a set of `active` and `not_yet` Nexus zones, but those don't influence the planning runs. Instead, we have to use the `set_explicit_{active,not_yet}_nexus_zones()` methods on `SimConfigBuilder`. I haven't dug into this. * Inventory changes from outside the blueprint are kind of a pain, so the zone safety checks that rely on status from cockroach, NTP, and internal DNS have a bunch of `sim.inventory_edit_latest_low_level(...)` calls to make up various statuses. This still feels pretty boilerplate-y but doesn't seem any _worse_ than it was before. I don't know if it makes sense to grow the simulator to contain this kind of status.
1 parent de084b7 commit 4e6a568

22 files changed

+1971
-2477
lines changed

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dev-tools/reconfigurator-cli/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ mod log_capture;
8888
pub mod test_utils;
8989

9090
/// REPL state
91-
#[derive(Debug)]
91+
#[derive(Clone, Debug)]
9292
struct ReconfiguratorSim {
9393
// The simulator currently being used.
9494
sim: Simulator,

dev-tools/reconfigurator-cli/src/test_utils.rs

Lines changed: 191 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,30 @@
77
88
use crate::ReconfiguratorSim;
99
use anyhow::Context;
10+
use nexus_inventory::CollectionBuilder;
11+
use nexus_inventory::now_db_precision;
1012
use nexus_reconfigurator_blippy::Blippy;
1113
use nexus_reconfigurator_blippy::BlippyReportSortKey;
14+
use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder;
1215
use nexus_reconfigurator_planning::example::ExampleSystemBuilder;
1316
use nexus_reconfigurator_planning::system::SledBuilder;
17+
use nexus_reconfigurator_planning::system::SystemDescription;
1418
use nexus_reconfigurator_simulation::BlueprintId;
1519
use nexus_reconfigurator_simulation::CollectionId;
20+
use nexus_reconfigurator_simulation::SimState;
1621
use nexus_reconfigurator_simulation::SimStateBuilder;
1722
use nexus_reconfigurator_simulation::errors::KeyError;
1823
use nexus_types::deployment::Blueprint;
24+
use nexus_types::deployment::BlueprintSource;
25+
use nexus_types::deployment::ClickhousePolicy;
1926
use nexus_types::deployment::PlanningInput;
27+
use nexus_types::external_api::views::SledPolicy;
28+
use nexus_types::inventory::Collection;
2029
use omicron_uuid_kinds::SledUuid;
2130
use slog::Logger;
31+
use std::sync::Arc;
2232

33+
#[derive(Debug, Clone)]
2334
pub struct ReconfiguratorCliTestState {
2435
sim: ReconfiguratorSim,
2536
}
@@ -53,11 +64,21 @@ impl ReconfiguratorCliTestState {
5364
}
5465

5566
/// Get the specified blueprint.
56-
pub fn blueprint(&self, id: BlueprintId) -> Result<&Blueprint, KeyError> {
67+
pub fn blueprint(
68+
&self,
69+
id: BlueprintId,
70+
) -> Result<&Arc<Blueprint>, KeyError> {
5771
let state = self.sim.current_state();
5872
state.system().resolve_and_get_blueprint(id)
5973
}
6074

75+
/// Get the specified inventory collection.
76+
pub fn inventory(&self, id: CollectionId) -> Result<&Collection, KeyError> {
77+
let system = self.current_state().system();
78+
let id = system.resolve_collection_id(id)?;
79+
system.get_collection(&id)
80+
}
81+
6182
/// Run the blueprint planner, using the latest blueprint and inventory
6283
/// collection as input.
6384
///
@@ -74,7 +95,7 @@ impl ReconfiguratorCliTestState {
7495
/// "blippy clean" blueprints, and since this is used throughout the
7596
/// planner's tests, we want to assert that any blueprint we plan is indeed
7697
/// blippy clean.
77-
pub fn run_planner(&mut self) -> anyhow::Result<Blueprint> {
98+
pub fn run_planner(&mut self) -> anyhow::Result<Arc<Blueprint>> {
7899
let output =
79100
self.sim.run_planner(BlueprintId::Latest, CollectionId::Latest)?;
80101
println!("{output}");
@@ -103,7 +124,7 @@ impl ReconfiguratorCliTestState {
103124
///
104125
/// Panics if the latest blueprint and current planning input have any
105126
/// blippy notes.
106-
pub fn assert_latest_blueprint_is_blippy_clean(&self) -> Blueprint {
127+
pub fn assert_latest_blueprint_is_blippy_clean(&self) -> Arc<Blueprint> {
107128
let blueprint = self
108129
.blueprint(BlueprintId::Latest)
109130
.expect("always have a latest blueprint");
@@ -120,7 +141,17 @@ impl ReconfiguratorCliTestState {
120141
panic!("expected blippy report for blueprint to have no notes");
121142
}
122143

123-
blueprint.clone()
144+
Arc::clone(blueprint)
145+
}
146+
147+
/// Read the current internal simulator state.
148+
pub fn current_state(&self) -> &SimState {
149+
self.sim.current_state()
150+
}
151+
152+
/// Read the current system description.
153+
pub fn current_description(&self) -> &SystemDescription {
154+
self.current_state().system().description()
124155
}
125156

126157
/// Change the internal simulator state.
@@ -138,6 +169,20 @@ impl ReconfiguratorCliTestState {
138169
Ok(ret)
139170
}
140171

172+
/// State change helper: change only the system description.
173+
pub fn change_description<F, T>(
174+
&mut self,
175+
description: &str,
176+
f: F,
177+
) -> anyhow::Result<T>
178+
where
179+
F: FnOnce(&mut SystemDescription) -> anyhow::Result<T>,
180+
{
181+
self.change_state(description, |state| {
182+
f(state.system_mut().description_mut())
183+
})
184+
}
185+
141186
/// State change helper: generate a new inventory collection from the
142187
/// current simulator state.
143188
pub fn generate_inventory(
@@ -151,15 +196,139 @@ impl ReconfiguratorCliTestState {
151196
})
152197
}
153198

199+
/// State change helper: generate a new inventory collection from the
200+
/// current simulator state.
201+
///
202+
/// `f` provides an opportunity to customize the collection.
203+
pub fn generate_inventory_customized<F, T>(
204+
&mut self,
205+
description: &str,
206+
f: F,
207+
) -> anyhow::Result<T>
208+
where
209+
F: FnOnce(&mut CollectionBuilder) -> anyhow::Result<T>,
210+
{
211+
self.change_state(description, |state| {
212+
let mut builder = state.to_collection_builder()?;
213+
let result = f(&mut builder)?;
214+
state.system_mut().add_collection(builder.build())?;
215+
Ok(result)
216+
})
217+
}
218+
219+
/// State change helper: create a new latest inventory collection, then pass
220+
/// it to `f` which is allowed to edit it in abnormal ways that cannot be
221+
/// done via `CollectionBuilder`.
222+
pub fn inventory_edit_latest_raw<F, T>(
223+
&mut self,
224+
description: &str,
225+
f: F,
226+
) -> anyhow::Result<T>
227+
where
228+
F: FnOnce(&mut Collection) -> anyhow::Result<T>,
229+
{
230+
self.change_state(description, |state| {
231+
let mut collection = state.to_collection_builder()?.build();
232+
let result = f(&mut collection)?;
233+
state.system_mut().add_collection(collection)?;
234+
Ok(result)
235+
})
236+
}
237+
238+
/// State change helper: set the Clickhouse cluster policy.
239+
pub fn set_clickhouse_policy(
240+
&mut self,
241+
description: &str,
242+
policy: ClickhousePolicy,
243+
) {
244+
self.change_description(description, |desc| {
245+
desc.clickhouse_policy(policy);
246+
Ok(())
247+
})
248+
.expect("closure can't fail");
249+
}
250+
251+
/// State change helper: edit the latest blueprint, inserting a new latest
252+
/// blueprint.
253+
pub fn blueprint_edit_latest<F>(
254+
&mut self,
255+
description: &str,
256+
f: F,
257+
) -> anyhow::Result<Arc<Blueprint>>
258+
where
259+
F: FnOnce(&mut BlueprintBuilder<'_>) -> anyhow::Result<()>,
260+
{
261+
let log = self.sim.log.clone();
262+
self.change_state(description, |state| {
263+
let rng = state.rng_mut().next_planner_rng();
264+
let system = state.system_mut();
265+
let blueprint = system
266+
.get_blueprint(
267+
&system.resolve_blueprint_id(BlueprintId::Latest),
268+
)
269+
.expect("always have a latest blueprint");
270+
let mut builder = BlueprintBuilder::new_based_on(
271+
&log,
272+
blueprint,
273+
"ReconfiguratorCliTestState",
274+
rng,
275+
)?;
276+
f(&mut builder)?;
277+
let blueprint = Arc::new(builder.build(BlueprintSource::Test));
278+
system.add_blueprint(Arc::clone(&blueprint))?;
279+
Ok(blueprint)
280+
})
281+
}
282+
283+
/// State change helper: create a new latest blueprint that is a clone of
284+
/// the current latest blueprint (but with a new ID and correct parent ID),
285+
/// then pass it to `f` which is allowed to edit it in abnormal ways that
286+
/// cannot be done via `BlueprintBuilder`.
287+
pub fn blueprint_edit_latest_raw<F>(
288+
&mut self,
289+
description: &str,
290+
f: F,
291+
) -> anyhow::Result<Arc<Blueprint>>
292+
where
293+
F: FnOnce(&mut Blueprint) -> anyhow::Result<()>,
294+
{
295+
self.change_state(description, |state| {
296+
let mut rng = state.rng_mut().next_planner_rng();
297+
let system = state.system_mut();
298+
let parent_blueprint = system
299+
.get_blueprint(
300+
&system.resolve_blueprint_id(BlueprintId::Latest),
301+
)
302+
.expect("always have a latest blueprint");
303+
let mut blueprint = Arc::clone(parent_blueprint);
304+
305+
{
306+
let blueprint = Arc::make_mut(&mut blueprint);
307+
308+
// Update metadata fields to make this a new blueprint.
309+
blueprint.id = rng.next_blueprint();
310+
blueprint.parent_blueprint_id = Some(parent_blueprint.id);
311+
blueprint.time_created = now_db_precision();
312+
313+
// Perform whatever modifications the caller wants.
314+
f(blueprint)?;
315+
}
316+
317+
system.add_blueprint(Arc::clone(&blueprint))?;
318+
319+
Ok(blueprint)
320+
})
321+
}
322+
154323
/// State change helper: add a new sled, returning its ID.
155-
pub fn add_sled(&mut self, description: &str) -> anyhow::Result<SledUuid> {
156-
self.add_sled_customized(description, |sled| sled)
324+
pub fn sled_add(&mut self, description: &str) -> anyhow::Result<SledUuid> {
325+
self.sled_add_customized(description, |sled| sled)
157326
}
158327

159328
/// State change helper: add a new sled, returning its ID.
160329
///
161330
/// `f` provides an opportunity to customize the sled.
162-
pub fn add_sled_customized<F>(
331+
pub fn sled_add_customized<F>(
163332
&mut self,
164333
description: &str,
165334
f: F,
@@ -175,6 +344,21 @@ impl ReconfiguratorCliTestState {
175344
})
176345
}
177346

347+
/// State change helper: expunge a sled.
348+
pub fn sled_expunge(
349+
&mut self,
350+
description: &str,
351+
sled_id: SledUuid,
352+
) -> anyhow::Result<()> {
353+
self.change_state(description, |state| {
354+
state
355+
.system_mut()
356+
.description_mut()
357+
.sled_set_policy(sled_id, SledPolicy::Expunged)?;
358+
Ok(())
359+
})
360+
}
361+
178362
/// State change helper: for each active sled described by `blueprint`,
179363
/// update the simulate sled's config to match.
180364
///

nexus/reconfigurator/blippy/src/checks.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2253,10 +2253,13 @@ fn check_planning_input_network_records_appear_in_blueprint(
22532253
}
22542254
}
22552255
_ => {
2256-
blippy.push_planning_input_note(
2257-
Severity::Fatal,
2258-
PlanningInputKind::NicWithUnknownOpteSubnet(nic_entry),
2259-
);
2256+
// Ignore localhost (used by the test suite).
2257+
if !nic_entry.nic.ip.is_loopback() {
2258+
blippy.push_planning_input_note(
2259+
Severity::Fatal,
2260+
PlanningInputKind::NicWithUnknownOpteSubnet(nic_entry),
2261+
);
2262+
}
22602263
}
22612264
}
22622265
}

nexus/reconfigurator/planning/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,15 @@ uuid.workspace = true
5555
omicron-workspace-hack.workspace = true
5656

5757
[dev-dependencies]
58+
assert_matches.workspace = true
5859
dns-server.workspace = true
5960
dropshot.workspace = true
6061
expectorate.workspace = true
6162
hex-literal.workspace = true
6263
hickory-resolver.workspace = true
6364
internal-dns-types.workspace = true
6465
maplit.workspace = true
66+
nexus-reconfigurator-simulation.workspace = true
6567
omicron-common = { workspace = true, features = ["testing"] }
6668
omicron-test-utils.workspace = true
6769
proptest.workspace = true

nexus/reconfigurator/planning/src/blueprint_builder/builder.rs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -919,10 +919,7 @@ impl<'a> BlueprintBuilder<'a> {
919919
}
920920

921921
/// Expunge everything on a sled.
922-
pub(crate) fn expunge_sled(
923-
&mut self,
924-
sled_id: SledUuid,
925-
) -> Result<(), Error> {
922+
pub fn expunge_sled(&mut self, sled_id: SledUuid) -> Result<(), Error> {
926923
let editor = self.sled_editors.get_mut(&sled_id).ok_or_else(|| {
927924
Error::Planner(anyhow!("tried to expunge unknown sled {sled_id}"))
928925
})?;

0 commit comments

Comments
 (0)