Skip to content
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.

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

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

3 changes: 1 addition & 2 deletions dev-tools/reconfigurator-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,8 +547,7 @@ fn cmd_blueprint_diff_inventory(
.get(&blueprint_id)
.ok_or_else(|| anyhow!("no such blueprint: {}", blueprint_id))?;

let zones = collection.all_omicron_zones().map(|z| z.id).collect();
let diff = blueprint.diff_sleds_from_collection(&collection, &zones);
let diff = blueprint.diff_sleds_from_collection(&collection);
Ok(Some(diff.display().to_string()))
}

Expand Down
313 changes: 0 additions & 313 deletions dev-tools/reconfigurator-cli/tests/output/cmd-complex-stdout

This file was deleted.

22 changes: 14 additions & 8 deletions nexus/db-model/src/deployment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ use crate::{ipv6, Generation, MacAddr, Name, SqlU16, SqlU32, SqlU8};
use chrono::{DateTime, Utc};
use ipnetwork::IpNetwork;
use nexus_types::deployment::BlueprintTarget;
use nexus_types::deployment::BlueprintZoneConfig;
use nexus_types::deployment::BlueprintZonePolicy;
use nexus_types::deployment::BlueprintZonesConfig;
use omicron_common::api::internal::shared::NetworkInterface;
use uuid::Uuid;

Expand Down Expand Up @@ -103,7 +106,7 @@ impl BpSledOmicronZones {
pub fn new(
blueprint_id: Uuid,
sled_id: Uuid,
zones_config: &nexus_types::deployment::OmicronZonesConfig,
zones_config: &BlueprintZonesConfig,
) -> Self {
Self {
blueprint_id,
Expand Down Expand Up @@ -144,9 +147,9 @@ impl BpOmicronZone {
pub fn new(
blueprint_id: Uuid,
sled_id: Uuid,
zone: &nexus_types::inventory::OmicronZoneConfig,
zone: &BlueprintZoneConfig,
) -> Result<Self, anyhow::Error> {
let zone = OmicronZone::new(sled_id, zone)?;
let zone = OmicronZone::new(sled_id, &zone.config)?;
Ok(Self {
blueprint_id,
sled_id: zone.sled_id,
Expand All @@ -172,10 +175,11 @@ impl BpOmicronZone {
})
}

pub fn into_omicron_zone_config(
pub fn into_blueprint_zone_config(
self,
nic_row: Option<BpOmicronZoneNic>,
) -> Result<nexus_types::inventory::OmicronZoneConfig, anyhow::Error> {
zone_policy: BlueprintZonePolicy,
) -> Result<BlueprintZoneConfig, anyhow::Error> {
let zone = OmicronZone {
sled_id: self.sled_id,
id: self.id,
Expand All @@ -198,7 +202,9 @@ impl BpOmicronZone {
snat_first_port: self.snat_first_port,
snat_last_port: self.snat_last_port,
};
zone.into_omicron_zone_config(nic_row.map(OmicronZoneNic::from))
let config =
zone.into_omicron_zone_config(nic_row.map(OmicronZoneNic::from))?;
Ok(BlueprintZoneConfig { config, zone_policy })
}
}

Expand Down Expand Up @@ -234,9 +240,9 @@ impl From<BpOmicronZoneNic> for OmicronZoneNic {
impl BpOmicronZoneNic {
pub fn new(
blueprint_id: Uuid,
zone: &nexus_types::inventory::OmicronZoneConfig,
zone: &BlueprintZoneConfig,
) -> Result<Option<BpOmicronZoneNic>, anyhow::Error> {
let zone_nic = OmicronZoneNic::new(zone)?;
let zone_nic = OmicronZoneNic::new(&zone.config)?;
Ok(zone_nic.map(|nic| Self {
blueprint_id,
id: nic.id,
Expand Down
138 changes: 74 additions & 64 deletions nexus/db-queries/src/db/datastore/deployment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ use nexus_db_model::BpTarget;
use nexus_types::deployment::Blueprint;
use nexus_types::deployment::BlueprintMetadata;
use nexus_types::deployment::BlueprintTarget;
use nexus_types::deployment::OmicronZonesConfig;
use nexus_types::deployment::BlueprintZonePolicy;
use nexus_types::deployment::BlueprintZonesConfig;
use omicron_common::api::external::DataPageParams;
use omicron_common::api::external::Error;
use omicron_common::api::external::ListResultVec;
Expand Down Expand Up @@ -105,52 +106,64 @@ impl DataStore {
// so that we can produce the `Error` type that we want here.
let row_blueprint = DbBlueprint::from(blueprint);
let blueprint_id = row_blueprint.id;

// `Blueprint` stores the policy for each zone next to the zone itself.
// This would ideally be represented as a simple column in
// bp_omicron_zone.
//
// But historically, `Blueprint` used to store the set of zones in
// service in a BTreeSet. Since most zones are expected to be in
// service, we store the set of zones NOT in service (which we expect
// to be much smaller, often empty). Build that inverted set here.
//
// This will soon be replaced with an extra column in the
// `bp_omicron_zone` table, coupled with other data migrations.
let omicron_zones_not_in_service = blueprint
.all_blueprint_zones()
.filter_map(|(_, zone)| {
// Exhaustive match so this fails if we add a new variant.
match zone.zone_policy {
BlueprintZonePolicy::InService => None,
BlueprintZonePolicy::NotInService => {
Some(BpOmicronZoneNotInService {
blueprint_id,
bp_omicron_zone_id: zone.config.id,
})
}
}
})
.collect::<Vec<_>>();

let sled_omicron_zones = blueprint
.omicron_zones
.blueprint_zones
.iter()
.map(|(sled_id, zones_config)| {
BpSledOmicronZones::new(blueprint_id, *sled_id, zones_config)
})
.collect::<Vec<_>>();
let omicron_zones = blueprint
.omicron_zones
.blueprint_zones
.iter()
.flat_map(|(sled_id, zones_config)| {
zones_config.zones.iter().map(|zone| {
zones_config.zones.iter().map(move |zone| {
BpOmicronZone::new(blueprint_id, *sled_id, zone)
.map_err(|e| Error::internal_error(&format!("{:#}", e)))
})
})
.collect::<Result<Vec<_>, Error>>()?;
let omicron_zone_nics = blueprint
.omicron_zones
.blueprint_zones
.values()
.flat_map(|zones_config| {
zones_config.zones.iter().filter_map(|zone| {
BpOmicronZoneNic::new(blueprint_id, zone)
.with_context(|| format!("zone {:?}", zone.id))
.with_context(|| format!("zone {:?}", zone.config.id))
.map_err(|e| Error::internal_error(&format!("{:#}", e)))
.transpose()
})
})
.collect::<Result<Vec<BpOmicronZoneNic>, _>>()?;

// `Blueprint` stores a set of zones in service, but in the database we
// store the set of zones NOT in service (which we expect to be much
// smaller, often empty). Build that inverted set here.
let omicron_zones_not_in_service = {
let mut zones_not_in_service = Vec::new();
for zone in &omicron_zones {
if !blueprint.zones_in_service.contains(&zone.id) {
zones_not_in_service.push(BpOmicronZoneNotInService {
blueprint_id,
bp_omicron_zone_id: zone.id,
});
}
}
zones_not_in_service
};

// This implementation inserts all records associated with the
// blueprint in one transaction. This is required: we don't want
// any planner or executor to see a half-inserted blueprint, nor do we
Expand Down Expand Up @@ -273,10 +286,10 @@ impl DataStore {
// the `OmicronZonesConfig` generation number for each sled that is a
// part of this blueprint. Construct the BTreeMap we ultimately need,
// but all the `zones` vecs will be empty until our next query below.
let mut omicron_zones: BTreeMap<Uuid, OmicronZonesConfig> = {
let mut blueprint_zones: BTreeMap<Uuid, BlueprintZonesConfig> = {
use db::schema::bp_sled_omicron_zones::dsl;

let mut omicron_zones = BTreeMap::new();
let mut blueprint_zones = BTreeMap::new();
let mut paginator = Paginator::new(SQL_BATCH_SIZE);
while let Some(p) = paginator.next() {
let batch = paginated(
Expand All @@ -295,9 +308,9 @@ impl DataStore {
paginator = p.found_batch(&batch, &|s| s.sled_id);

for s in batch {
let old = omicron_zones.insert(
let old = blueprint_zones.insert(
s.sled_id,
OmicronZonesConfig {
BlueprintZonesConfig {
generation: *s.generation,
zones: Vec::new(),
},
Expand All @@ -310,7 +323,7 @@ impl DataStore {
}
}

omicron_zones
blueprint_zones
};

// Assemble a mutable map of all the NICs found, by NIC id. As we
Expand Down Expand Up @@ -391,11 +404,6 @@ impl DataStore {
omicron_zones_not_in_service
};

// Create the in-memory list of zones _in_ service, which we'll
// calculate below as we load zones. (Any zone that isn't present in
// `omicron_zones_not_in_service` is considered in service.)
let mut zones_in_service = BTreeSet::new();

// Load all the zones for each sled.
{
use db::schema::bp_omicron_zone::dsl;
Expand Down Expand Up @@ -438,8 +446,9 @@ impl DataStore {
})
})
.transpose()?;
let sled_zones =
omicron_zones.get_mut(&z.sled_id).ok_or_else(|| {
let sled_zones = blueprint_zones
.get_mut(&z.sled_id)
.ok_or_else(|| {
// This error means that we found a row in
// bp_omicron_zone with no associated record in
// bp_sled_omicron_zones. This should be
Expand All @@ -451,8 +460,14 @@ impl DataStore {
))
})?;
let zone_id = z.id;
let zone_policy =
if omicron_zones_not_in_service.remove(&zone_id) {
BlueprintZonePolicy::NotInService
} else {
BlueprintZonePolicy::InService
};
let zone = z
.into_omicron_zone_config(nic_row)
.into_blueprint_zone_config(nic_row, zone_policy)
.with_context(|| {
format!("zone {:?}: parse from database", zone_id)
})
Expand All @@ -463,14 +478,6 @@ impl DataStore {
))
})?;
sled_zones.zones.push(zone);

// If we can remove `zone_id` from
// `omicron_zones_not_in_service`, then the zone is not in
// service. Otherwise, add it to the list of in-service
// zones.
if !omicron_zones_not_in_service.remove(&zone_id) {
zones_in_service.insert(zone_id);
}
}
}
}
Expand All @@ -488,8 +495,7 @@ impl DataStore {

Ok(Blueprint {
id: blueprint_id,
omicron_zones,
zones_in_service,
blueprint_zones,
parent_blueprint_id,
internal_dns_version,
external_dns_version,
Expand Down Expand Up @@ -1359,10 +1365,8 @@ mod tests {
[blueprint1.id]
);

// There ought to be no sleds or zones in service, and no parent
// blueprint.
assert_eq!(blueprint1.omicron_zones.len(), 0);
assert_eq!(blueprint1.zones_in_service.len(), 0);
// There ought to be no sleds or zones, and no parent blueprint.
assert_eq!(blueprint1.blueprint_zones.len(), 0);
assert_eq!(blueprint1.parent_blueprint_id, None);

// Trying to insert the same blueprint again should fail.
Expand Down Expand Up @@ -1407,20 +1411,17 @@ mod tests {
);

// Check the number of blueprint elements against our collection.
assert_eq!(blueprint1.omicron_zones.len(), policy.sleds.len());
assert_eq!(blueprint1.blueprint_zones.len(), policy.sleds.len());
assert_eq!(
blueprint1.omicron_zones.len(),
blueprint1.blueprint_zones.len(),
collection.omicron_zones.len()
);
assert_eq!(
blueprint1.all_omicron_zones().count(),
blueprint1.all_blueprint_zones().count(),
collection.all_omicron_zones().count()
);
// All zones should be in service.
assert_eq!(
blueprint1.zones_in_service.len(),
blueprint1.all_omicron_zones().count()
);
assert_all_zones_in_service(&blueprint1);
assert_eq!(blueprint1.parent_blueprint_id, None);

// Set blueprint1 as the current target, and ensure that we cannot
Expand Down Expand Up @@ -1489,19 +1490,16 @@ mod tests {

// Check that we added the new sled and its zones.
assert_eq!(
blueprint1.omicron_zones.len() + 1,
blueprint2.omicron_zones.len()
blueprint1.blueprint_zones.len() + 1,
blueprint2.blueprint_zones.len()
);
assert_eq!(
blueprint1.all_omicron_zones().count() + num_new_sled_zones,
blueprint2.all_omicron_zones().count()
blueprint1.all_blueprint_zones().count() + num_new_sled_zones,
blueprint2.all_blueprint_zones().count()
);

// All zones should be in service.
assert_eq!(
blueprint2.zones_in_service.len(),
blueprint2.all_omicron_zones().count()
);
assert_all_zones_in_service(&blueprint2);
assert_eq!(blueprint2.parent_blueprint_id, Some(blueprint1.id));

// Check that we can write it to the DB and read it back.
Expand Down Expand Up @@ -1869,4 +1867,16 @@ mod tests {
db.cleanup().await.unwrap();
logctx.cleanup_successful();
}

fn assert_all_zones_in_service(blueprint: &Blueprint) {
let not_in_service = blueprint
.all_blueprint_zones()
.filter(|(_, z)| z.zone_policy != BlueprintZonePolicy::InService)
.collect::<Vec<_>>();
assert!(
not_in_service.is_empty(),
"expected all zones to be in service, \
found these zones not in service: {not_in_service:?}"
);
}
}
5 changes: 2 additions & 3 deletions nexus/db-queries/src/db/datastore/rack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ mod test {
};
use omicron_common::api::internal::shared::SourceNatConfig;
use omicron_test_utils::dev;
use std::collections::{BTreeMap, BTreeSet, HashMap};
use std::collections::{BTreeMap, HashMap};
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV6};
use std::num::NonZeroU32;

Expand All @@ -950,8 +950,7 @@ mod test {
rack_subnet: nexus_test_utils::RACK_SUBNET.parse().unwrap(),
blueprint: Blueprint {
id: Uuid::new_v4(),
omicron_zones: BTreeMap::new(),
zones_in_service: BTreeSet::new(),
blueprint_zones: BTreeMap::new(),
parent_blueprint_id: None,
internal_dns_version: *Generation::new(),
external_dns_version: *Generation::new(),
Expand Down
Loading
Loading