Skip to content

Conversation

@jgallagher
Copy link
Contributor

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:

    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:

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants