Skip to content

Commit

Permalink
Add disposition to blueprint disks (#7169)
Browse files Browse the repository at this point in the history 
This is the second PR related to fixing #7098.

BlueprintPhysicalDisksConfig is no longer an alias of
OmicronPhysicalDisksConfig, but instead wraps a new type
BlueprintPhysicalDiskConfig which itself wraps an
OmicronPhysicalDiskConfig and a BlueprintPhysicalDiskDisposition. This
change brings blueprint physical disks in line with blueprint datasets
and zones such that expungement is a first class notion in the
blueprint, and not implicit in the disk not being present in a
blueprint.

This change only adds the new types and makes them work with the
existing code. The underlying logic around expungement and
decommissioning has not changed. That will come in a follow up PR.
  • Loading branch information
@andrewjstone
andrewjstone authored Nov 26, 2024
1 parent fe0d0a3 commit fd50d18
Show file tree
Hide file tree
Showing 18 changed files with 366 additions and 161 deletions.
7 changes: 5 additions & 2 deletions clients/nexus-client/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ progenitor::generate_api!(
// as "blueprint" this way, but we have really useful functionality
// (e.g., diff'ing) that's implemented on our local type.
Blueprint = nexus_types::deployment::Blueprint,
BlueprintPhysicalDiskConfig = nexus_types::deployment::BlueprintPhysicalDiskConfig,
BlueprintPhysicalDisksConfig = nexus_types::deployment::BlueprintPhysicalDisksConfig,
BlueprintPhysicalDiskDisposition = nexus_types::deployment::BlueprintPhysicalDiskDisposition,
Certificate = omicron_common::api::internal::nexus::Certificate,
ClickhouseMode = nexus_types::deployment::ClickhouseMode,
ClickhousePolicy = nexus_types::deployment::ClickhousePolicy,
Expand All @@ -43,8 +46,8 @@ progenitor::generate_api!(
NetworkInterface = omicron_common::api::internal::shared::NetworkInterface,
NetworkInterfaceKind = omicron_common::api::internal::shared::NetworkInterfaceKind,
NewPasswordHash = omicron_passwords::NewPasswordHash,
OmicronPhysicalDiskConfig = nexus_types::disk::OmicronPhysicalDiskConfig,
OmicronPhysicalDisksConfig = nexus_types::disk::OmicronPhysicalDisksConfig,
OmicronPhysicalDiskConfig = omicron_common::disk::OmicronPhysicalDiskConfig,
OmicronPhysicalDisksConfig = omicron_common::disk::OmicronPhysicalDisksConfig,
RecoverySiloConfig = nexus_sled_agent_shared::recovery_silo::RecoverySiloConfig,
Srv = nexus_types::internal_api::params::Srv,
TypedUuidForCollectionKind = omicron_uuid_kinds::CollectionUuid,
Expand Down
3 changes: 3 additions & 0 deletions clients/sled-agent-client/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ progenitor::generate_api!(
DatasetsConfig = omicron_common::disk::DatasetsConfig,
DatasetKind = omicron_common::api::internal::shared::DatasetKind,
DiskIdentity = omicron_common::disk::DiskIdentity,
DisksManagementResult = omicron_common::disk::DisksManagementResult,
DiskManagementStatus = omicron_common::disk::DiskManagementStatus,
DiskManagementError = omicron_common::disk::DiskManagementError,
DiskVariant = omicron_common::disk::DiskVariant,
ExternalIpGatewayMap = omicron_common::api::internal::shared::ExternalIpGatewayMap,
Generation = omicron_common::api::external::Generation,
Expand Down
55 changes: 55 additions & 0 deletions nexus/db-model/src/deployment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use nexus_types::deployment::BlueprintDatasetConfig;
use nexus_types::deployment::BlueprintDatasetDisposition;
use nexus_types::deployment::BlueprintDatasetsConfig;
use nexus_types::deployment::BlueprintPhysicalDiskConfig;
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
use nexus_types::deployment::BlueprintPhysicalDisksConfig;
use nexus_types::deployment::BlueprintTarget;
use nexus_types::deployment::BlueprintZoneConfig;
Expand Down Expand Up @@ -146,6 +147,54 @@ pub struct BpSledState {
pub sled_state: SledState,
}

impl_enum_type!(
#[derive(Clone, SqlType, Debug, QueryId)]
#[diesel(postgres_type(name = "bp_physical_disk_disposition", schema = "public"))]
pub struct DbBpPhysicalDiskDispositionEnum;

/// This type is not actually public, because [`BlueprintPhysicalDiskDisposition`]
/// interacts with external logic.
///
/// However, it must be marked `pub` to avoid errors like `crate-private
/// type `BpPhysicalDiskDispositionEnum` in public interface`. Marking this type `pub`,
/// without actually making it public, tricks rustc in a desirable way.
#[derive(Clone, Copy, Debug, AsExpression, FromSqlRow, PartialEq)]
#[diesel(sql_type = DbBpPhysicalDiskDispositionEnum)]
pub enum DbBpPhysicalDiskDisposition;

// Enum values
InService => b"in_service"
Expunged => b"expunged"
);

/// Converts a [`BlueprintPhysicalDiskDisposition`] to a version that can be inserted
/// into a database.
pub fn to_db_bp_physical_disk_disposition(
disposition: BlueprintPhysicalDiskDisposition,
) -> DbBpPhysicalDiskDisposition {
match disposition {
BlueprintPhysicalDiskDisposition::InService => {
DbBpPhysicalDiskDisposition::InService
}
BlueprintPhysicalDiskDisposition::Expunged => {
DbBpPhysicalDiskDisposition::Expunged
}
}
}

impl From<DbBpPhysicalDiskDisposition> for BlueprintPhysicalDiskDisposition {
fn from(disposition: DbBpPhysicalDiskDisposition) -> Self {
match disposition {
DbBpPhysicalDiskDisposition::InService => {
BlueprintPhysicalDiskDisposition::InService
}
DbBpPhysicalDiskDisposition::Expunged => {
BlueprintPhysicalDiskDisposition::Expunged
}
}
}
}

/// See [`nexus_types::deployment::BlueprintPhysicalDisksConfig`].
#[derive(Queryable, Clone, Debug, Selectable, Insertable)]
#[diesel(table_name = bp_sled_omicron_physical_disks)]
Expand Down Expand Up @@ -182,6 +231,8 @@ pub struct BpOmicronPhysicalDisk {

pub id: DbTypedUuid<PhysicalDiskKind>,
pub pool_id: Uuid,

pub disposition: DbBpPhysicalDiskDisposition,
}

impl BpOmicronPhysicalDisk {
Expand All @@ -198,13 +249,17 @@ impl BpOmicronPhysicalDisk {
model: disk_config.identity.model.clone(),
id: disk_config.id.into(),
pool_id: disk_config.pool_id.into_untyped_uuid(),
disposition: to_db_bp_physical_disk_disposition(
disk_config.disposition,
),
}
}
}

impl From<BpOmicronPhysicalDisk> for BlueprintPhysicalDiskConfig {
fn from(disk: BpOmicronPhysicalDisk) -> Self {
Self {
disposition: disk.disposition.into(),
identity: DiskIdentity {
vendor: disk.vendor,
serial: disk.serial,
Expand Down
2 changes: 2 additions & 0 deletions nexus/db-model/src/schema.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1665,6 +1665,8 @@ table! {

id -> Uuid,
pool_id -> Uuid,

disposition -> crate::DbBpPhysicalDiskDispositionEnum,
}
}

Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::collections::BTreeMap;
///
/// This must be updated when you change the database schema. Refer to
/// schema/crdb/README.adoc in the root of this repository for details.
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(115, 0, 0);
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(116, 0, 0);

/// List of all past database schema versions, in *reverse* order
///
Expand All @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
// | leaving the first copy as an example for the next person.
// v
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
KnownVersion::new(116, "bp-physical-disk-disposition"),
KnownVersion::new(115, "inv-omicron-physical-disks-generation"),
KnownVersion::new(114, "crucible-ref-count-records"),
KnownVersion::new(113, "add-tx-eq"),
Expand Down
74 changes: 36 additions & 38 deletions nexus/reconfigurator/execution/src/omicron_physical_disks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,12 @@ pub(crate) async fn deploy_disks(
db_sled.sled_agent_address(),
&log,
);
let result =
client.omicron_physical_disks_put(&config).await.with_context(
|| format!("Failed to put {config:#?} to sled {sled_id}"),
);
let result = client
.omicron_physical_disks_put(&config.clone().into())
.await
.with_context(|| {
format!("Failed to put {config:#?} to sled {sled_id}")
});
match result {
Err(error) => {
warn!(log, "{error:#}");
Expand Down Expand Up @@ -146,6 +148,7 @@ mod test {
use nexus_sled_agent_shared::inventory::SledRole;
use nexus_test_utils::SLED_AGENT_UUID;
use nexus_test_utils_macros::nexus_test;
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
use nexus_types::deployment::{
Blueprint, BlueprintPhysicalDiskConfig, BlueprintPhysicalDisksConfig,
BlueprintTarget, CockroachDbPreserveDowngrade, DiskFilter,
Expand All @@ -155,6 +158,10 @@ mod test {
use omicron_common::api::external::Generation;
use omicron_common::api::internal::shared::DatasetKind;
use omicron_common::disk::DiskIdentity;
use omicron_common::disk::DiskManagementError;
use omicron_common::disk::DiskManagementStatus;
use omicron_common::disk::DisksManagementResult;
use omicron_common::disk::OmicronPhysicalDisksConfig;
use omicron_uuid_kinds::DatasetUuid;
use omicron_uuid_kinds::GenericUuid;
use omicron_uuid_kinds::PhysicalDiskUuid;
Expand Down Expand Up @@ -241,8 +248,9 @@ mod test {
// See `rack_setup::service::ServiceInner::run` for more details.
fn make_disks() -> BlueprintPhysicalDisksConfig {
BlueprintPhysicalDisksConfig {
generation: Generation::new(),
generation: Generation::new().next(),
disks: vec![BlueprintPhysicalDiskConfig {
disposition: BlueprintPhysicalDiskDisposition::InService,
identity: DiskIdentity {
vendor: "test-vendor".to_string(),
serial: "test-serial".to_string(),
Expand Down Expand Up @@ -270,18 +278,16 @@ mod test {
s.expect(
Expectation::matching(all_of![
request::method_path("PUT", "/omicron-physical-disks",),
// Our generation number should be 1 and there should
// Our generation number should be 2 and there should
// be only a single disk.
request::body(json_decoded(
|c: &BlueprintPhysicalDisksConfig| {
c.generation == 1u32.into() && c.disks.len() == 1
|c: &OmicronPhysicalDisksConfig| {
c.generation == 2u32.into() && c.disks.len() == 1
}
))
])
.respond_with(json_encoded(
sled_agent_client::types::DisksManagementResult {
status: vec![],
},
DisksManagementResult { status: vec![] },
)),
);
}
Expand All @@ -303,9 +309,7 @@ mod test {
"/omicron-physical-disks",
))
.respond_with(json_encoded(
sled_agent_client::types::DisksManagementResult {
status: vec![],
},
DisksManagementResult { status: vec![] },
)),
);
}
Expand All @@ -323,11 +327,9 @@ mod test {
"PUT",
"/omicron-physical-disks",
))
.respond_with(json_encoded(
sled_agent_client::types::DisksManagementResult {
status: vec![],
},
)),
.respond_with(json_encoded(DisksManagementResult {
status: vec![],
})),
);
s2.expect(
Expectation::matching(request::method_path(
Expand All @@ -346,7 +348,7 @@ mod test {
assert_eq!(errors.len(), 1);
assert!(errors[0]
.to_string()
.starts_with("Failed to put OmicronPhysicalDisksConfig"));
.starts_with("Failed to put BlueprintPhysicalDisksConfig"));
s1.verify_and_clear();
s2.verify_and_clear();

Expand All @@ -358,30 +360,26 @@ mod test {
"PUT",
"/omicron-physical-disks",
))
.respond_with(json_encoded(
sled_agent_client::types::DisksManagementResult {
status: vec![],
},
)),
.respond_with(json_encoded(DisksManagementResult {
status: vec![],
})),
);
s2.expect(
Expectation::matching(request::method_path(
"PUT",
"/omicron-physical-disks",
))
.respond_with(json_encoded(sled_agent_client::types::DisksManagementResult {
status: vec![
sled_agent_client::types::DiskManagementStatus {
identity: omicron_common::disk::DiskIdentity {
vendor: "v".to_string(),
serial: "s".to_string(),
model: "m".to_string(),
},

// This error could occur if a disk is removed
err: Some(sled_agent_client::types::DiskManagementError::NotFound),
}
]
.respond_with(json_encoded(DisksManagementResult {
status: vec![DiskManagementStatus {
identity: omicron_common::disk::DiskIdentity {
vendor: "v".to_string(),
serial: "s".to_string(),
model: "m".to_string(),
},

// This error could occur if a disk is removed
err: Some(DiskManagementError::NotFound),
}],
})),
);

Expand Down
2 changes: 2 additions & 0 deletions nexus/reconfigurator/planning/src/blueprint_builder/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use nexus_sled_agent_shared::inventory::ZoneKind;
use nexus_types::deployment::blueprint_zone_type;
use nexus_types::deployment::Blueprint;
use nexus_types::deployment::BlueprintPhysicalDiskConfig;
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
use nexus_types::deployment::BlueprintZoneConfig;
use nexus_types::deployment::BlueprintZoneDisposition;
use nexus_types::deployment::BlueprintZoneFilter;
Expand Down Expand Up @@ -887,6 +888,7 @@ impl<'a> BlueprintBuilder<'a> {
for (disk_id, (zpool, disk)) in database_disks {
database_disk_ids.insert(disk_id);
sled_storage.ensure_disk(BlueprintPhysicalDiskConfig {
disposition: BlueprintPhysicalDiskDisposition::InService,
identity: disk.disk_identity.clone(),
id: disk_id,
pool_id: *zpool,
Expand Down
11 changes: 8 additions & 3 deletions nexus/test-utils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ use nexus_types::deployment::Blueprint;
use nexus_types::deployment::BlueprintDatasetConfig;
use nexus_types::deployment::BlueprintDatasetDisposition;
use nexus_types::deployment::BlueprintDatasetsConfig;
use nexus_types::deployment::BlueprintPhysicalDiskConfig;
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
use nexus_types::deployment::BlueprintPhysicalDisksConfig;
use nexus_types::deployment::BlueprintZoneConfig;
use nexus_types::deployment::BlueprintZoneDisposition;
Expand Down Expand Up @@ -68,7 +70,6 @@ use omicron_common::api::internal::shared::NetworkInterface;
use omicron_common::api::internal::shared::NetworkInterfaceKind;
use omicron_common::api::internal::shared::SwitchLocation;
use omicron_common::disk::CompressionAlgorithm;
use omicron_common::disk::OmicronPhysicalDiskConfig;
use omicron_common::zpool_name::ZpoolName;
use omicron_sled_agent::sim;
use omicron_test_utils::dev;
Expand Down Expand Up @@ -831,7 +832,9 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> {
let mut datasets = BTreeMap::new();
for zone in zones {
if let Some(zpool) = &zone.filesystem_pool {
disks.push(OmicronPhysicalDiskConfig {
disks.push(BlueprintPhysicalDiskConfig {
disposition:
BlueprintPhysicalDiskDisposition::InService,
identity: omicron_common::disk::DiskIdentity {
vendor: "nexus-tests".to_string(),
model: "nexus-test-model".to_string(),
Expand Down Expand Up @@ -868,7 +871,9 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> {
// Populate extra fake disks, giving each sled 10 total.
if disks.len() < 10 {
for _ in disks.len()..10 {
disks.push(OmicronPhysicalDiskConfig {
disks.push(BlueprintPhysicalDiskConfig {
disposition:
BlueprintPhysicalDiskDisposition::InService,
identity: omicron_common::disk::DiskIdentity {
vendor: "nexus-tests".to_string(),
model: "nexus-test-model".to_string(),
Expand Down
Loading

0 comments on commit fd50d18

Please sign in to comment.