Skip to content

Commit

Permalink
[nexus-types] more validation for PlanningInput (#5644)
Browse files Browse the repository at this point in the history

While reconciling the PlanningInput with the blueprint, I was a bit concerned
about getting nonsensical inputs (e.g. multiple zones get the same IPs) and the
system going haywire. In particular, I was concerned about the Serialize and
Deserialize impls which were seemingly added for reconfigurator-cli. However,
they were liabilities that didn't check for internal invariants.

To address this, this PR introduces the notion of a `TriMap`. A `TriMap` is
a 1:1:1 map which can be looked up by any one of three keys. Instead of storing
just a regular map, the `PlanningInput` now stores a couple of `TriMap`
instances via an intermediate `OmicronZoneNetworkResources` struct.

A `TriMap` is implemented as a vector with three indexes. It's a pretty
straightforward implementation, and I've also added property-based tests
to ensure that a `TriMap` is always valid (including always deserializing
to a valid structure).

At the moment, a `TriMap` does not allow removing entries. If necessary,
removals can be implemented by just marking the entry as dead and removing
the keys from the indexes. This is fine for our use case, since `TriMap`s
aren't long-lived.

I've also factored out the omicron zone IP and NIC code into an
`OmicronZoneNetworkResources` struct. In the future, we'll make the
`BlueprintBuilder` use this struct, so that blueprints are validated
whenever they're being mutated. We can also add this to the general code
that validates blueprints.
  • Loading branch information
sunshowers authored May 14, 2024
1 parent 372d980 commit 949c6ab
Show file tree
Hide file tree
Showing 13 changed files with 982 additions and 235 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock

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

5 changes: 4 additions & 1 deletion dev-tools/reconfigurator-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ use nexus_types::inventory::SledRole;
use omicron_common::api::external::Generation;
use omicron_common::api::external::Name;
use omicron_uuid_kinds::CollectionUuid;
use omicron_uuid_kinds::GenericUuid;
use omicron_uuid_kinds::SledUuid;
use omicron_uuid_kinds::VnicUuid;
use reedline::{Reedline, Signal};
use std::collections::BTreeMap;
use std::io::BufRead;
Expand Down Expand Up @@ -146,7 +148,8 @@ impl ReconfiguratorSim {
.add_omicron_zone_external_ip(zone.id, external_ip)
.context("adding omicron zone external IP")?;
let nic = OmicronZoneNic {
id: nic.id,
// TODO-cleanup use `TypedUuid` everywhere
id: VnicUuid::from_untyped_uuid(nic.id),
mac: nic.mac,
ip: nic.ip,
slot: nic.slot,
Expand Down
4 changes: 3 additions & 1 deletion nexus/db-model/src/network_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ use ipnetwork::NetworkSize;
use nexus_types::external_api::params;
use nexus_types::identity::Resource;
use omicron_common::api::{external, internal};
use omicron_uuid_kinds::GenericUuid;
use omicron_uuid_kinds::OmicronZoneUuid;
use omicron_uuid_kinds::VnicUuid;
use sled_agent_client::ZoneKind;
use uuid::Uuid;

Expand Down Expand Up @@ -207,7 +209,7 @@ impl TryFrom<&'_ ServiceNetworkInterface>
});
}
Ok(Self {
id: nic.id(),
id: VnicUuid::from_untyped_uuid(nic.id()),
mac: *nic.mac,
ip: nic.ip.ip(),
slot: *nic.slot,
Expand Down
5 changes: 3 additions & 2 deletions nexus/db-queries/src/db/datastore/deployment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1346,15 +1346,16 @@ mod tests {
use omicron_uuid_kinds::PhysicalDiskUuid;
use omicron_uuid_kinds::SledUuid;
use omicron_uuid_kinds::ZpoolUuid;
use once_cell::sync::Lazy;
use pretty_assertions::assert_eq;
use rand::thread_rng;
use rand::Rng;
use slog::Logger;
use std::mem;
use std::net::Ipv6Addr;

static EMPTY_PLANNING_INPUT: PlanningInput =
PlanningInputBuilder::empty_input();
static EMPTY_PLANNING_INPUT: Lazy<PlanningInput> =
Lazy::new(|| PlanningInputBuilder::empty_input());

// This is a not-super-future-maintainer-friendly helper to check that all
// the subtables related to blueprints have been pruned of a specific
Expand Down
5 changes: 4 additions & 1 deletion nexus/reconfigurator/planning/src/example.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ use nexus_types::deployment::OmicronZoneNic;
use nexus_types::deployment::PlanningInput;
use nexus_types::deployment::SledFilter;
use nexus_types::inventory::Collection;
use omicron_uuid_kinds::GenericUuid;
use omicron_uuid_kinds::SledKind;
use omicron_uuid_kinds::VnicUuid;
use typed_rng::TypedUuidRng;

pub struct ExampleSystem {
Expand Down Expand Up @@ -105,7 +107,8 @@ impl ExampleSystem {
.add_omicron_zone_nic(
service_id,
OmicronZoneNic {
id: nic.id,
// TODO-cleanup use `TypedUuid` everywhere
id: VnicUuid::from_untyped_uuid(nic.id),
mac: nic.mac,
ip: nic.ip,
slot: nic.slot,
Expand Down
5 changes: 5 additions & 0 deletions nexus/types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ anyhow.workspace = true
chrono.workspace = true
clap.workspace = true
base64.workspace = true
derive-where.workspace = true
futures.workspace = true
humantime.workspace = true
ipnetwork.workspace = true
Expand All @@ -38,3 +39,7 @@ omicron-common.workspace = true
omicron-passwords.workspace = true
omicron-workspace-hack.workspace = true
sled-agent-client.workspace = true

[dev-dependencies]
proptest.workspace = true
test-strategy.workspace = true
7 changes: 7 additions & 0 deletions nexus/types/proptest-regressions/deployment/tri_map.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Seeds for failure cases proptest has generated in the past. It is
# automatically read and these particular cases re-run before any
# novel cases are generated.
#
# It is recommended to check this file in to source control so that
# everyone who runs the test benefits from these saved cases.
cc bafcbc817cff65814a6f3233f1ef3d6c36f75c37ad35175d17d1c8484a734034 # shrinks to input = _ProptestOpsArgs { initial: {(0, '$', ""): "", (0, ' ', ""): ""}, ops: [] }
17 changes: 12 additions & 5 deletions nexus/types/src/deployment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,22 @@ use strum::IntoEnumIterator;
use thiserror::Error;
use uuid::Uuid;

mod network_resources;
mod planning_input;
mod tri_map;
mod zone_type;

pub use network_resources::AddNetworkResourceError;
pub use network_resources::OmicronZoneExternalFloatingAddr;
pub use network_resources::OmicronZoneExternalFloatingIp;
pub use network_resources::OmicronZoneExternalIp;
pub use network_resources::OmicronZoneExternalIpEntry;
pub use network_resources::OmicronZoneExternalIpKey;
pub use network_resources::OmicronZoneExternalSnatIp;
pub use network_resources::OmicronZoneNetworkResources;
pub use network_resources::OmicronZoneNic;
pub use network_resources::OmicronZoneNicEntry;
pub use planning_input::DiskFilter;
pub use planning_input::OmicronZoneExternalFloatingAddr;
pub use planning_input::OmicronZoneExternalFloatingIp;
pub use planning_input::OmicronZoneExternalIp;
pub use planning_input::OmicronZoneExternalSnatIp;
pub use planning_input::OmicronZoneNic;
pub use planning_input::PlanningInput;
pub use planning_input::PlanningInputBuildError;
pub use planning_input::PlanningInputBuilder;
Expand Down
Loading

0 comments on commit 949c6ab

Please sign in to comment.