Skip to content

Dedupe compatible_sled_cpu_families across {Instance,Vmm}CpuPlatform #11697

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.

Sign up for GitHub

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

Open
iximeow wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Dedupe compatible_sled_cpu_families across {Instance,Vmm}CpuPlatform #11697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions nexus/db-model/src/instance_cpu_platform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,12 @@ impl_enum_type!(
);

impl InstanceCpuPlatform {
/// Returns a slice containing the set of sled CPU families that can
/// accommodate an instance with this CPU platform.
/// A convenience helper for the set of sled CPU families which support this
/// instance CPU platform, without callers having to convert to a matching
/// VMM CPU platform first.
pub fn compatible_sled_cpu_families(&self) -> &'static [SledCpuFamily] {
match self {
// Turin-based sleds have a superset of the features made available
// in a guest's Milan CPU platform
Self::AmdMilan => &[
SledCpuFamily::AmdMilan,
SledCpuFamily::AmdTurin,
SledCpuFamily::AmdTurinDense,
],
Self::AmdTurin => {
&[SledCpuFamily::AmdTurin, SledCpuFamily::AmdTurinDense]
}
Self::AmdTurinV2 => {
&[SledCpuFamily::AmdTurin, SledCpuFamily::AmdTurinDense]
}
}
let vmm_platform: crate::VmmCpuPlatform = (*self).into();
vmm_platform.compatible_sled_cpu_families()
}
}

Expand Down
31 changes: 21 additions & 10 deletions nexus/db-model/src/vmm_cpu_platform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,36 +30,47 @@ impl_enum_type!(
);

impl VmmCpuPlatform {
/// If this VMM has a well-known CPU platform, returns a `Some` containing
/// the set of sled CPU families that can host that the VMM. Returns `None`
/// if there is insufficient information to determine what CPU families
/// could host this VMM.
pub fn compatible_sled_cpu_families(&self) -> Option<&[SledCpuFamily]> {
/// If this VMM has a well-known CPU platform, returns a set of sled CPU
/// families that can host the VMM. Otherwise, returns an empty set to
/// indicate that a VMM CPU platform of "the target sled's defaults" cannot
/// be generalized to any `SledCpuFamily`.
pub fn compatible_sled_cpu_families(&self) -> &'static [SledCpuFamily] {
match self {
// Milan-based instances can run on both Milan and Turin processors.
// Turin and Turin Dense are equally viable from a features
// perspective.
Self::AmdMilan => Some(&[
Self::AmdMilan => &[
SledCpuFamily::AmdMilan,
SledCpuFamily::AmdTurin,
SledCpuFamily::AmdTurinDense,
]),
],
Self::AmdTurin => {
Some(&[SledCpuFamily::AmdTurin, SledCpuFamily::AmdTurinDense])
&[SledCpuFamily::AmdTurin, SledCpuFamily::AmdTurinDense]
}
Self::AmdTurinV2 => {
Some(&[SledCpuFamily::AmdTurin, SledCpuFamily::AmdTurinDense])
&[SledCpuFamily::AmdTurin, SledCpuFamily::AmdTurinDense]
}

// VMMs get the "sled default" CPU platform when an instance starts
// up on a sled that hasn't reported a well-known CPU family. Assume
// that nothing is known about the VM's compatible CPU platforms in
// this case.
Self::SledDefault => None,
Self::SledDefault => &[],
}
}
}

// An InstanceCpuPlatform is an instance's request for how it wants vCPUs to
// look, whereas a VmmCpuPlatform is the actual chosen platform at the point
// that instance was incarnated. A critical difference is that VmmCpuPlatform
// has an escape hatch in the form of `SledDefault` if an instance is to be run
// on a kind of hardware the control plane does not yet know about - this is
// only a possible value if both the instance did not have an explicit CPU
// platform *and* it was placed on a sled whose family we don't know.
//
// While this means we can convert between Instance and VMM CPU platforms, in
// practice Instance->VMM is the only useful direction. Since that is expected
// to be injective, `From` suffices.
Comment on lines +63 to +73

@hawkw hawkw Nov 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for this note, this is great. as a very tiny note, it turns out you can put doc comments on impls, so this could be

Suggested change
// An InstanceCpuPlatform is an instance's request for how it wants vCPUs to
// look, whereas a VmmCpuPlatform is the actual chosen platform at the point
// that instance was incarnated. A critical difference is that VmmCpuPlatform
// has an escape hatch in the form of `SledDefault` if an instance is to be run
// on a kind of hardware the control plane does not yet know about - this is
// only a possible value if both the instance did not have an explicit CPU
// platform *and* it was placed on a sled whose family we don't know.
//
// While this means we can convert between Instance and VMM CPU platforms, in
// practice Instance->VMM is the only useful direction. Since that is expected
// to be injective, `From` suffices.
/// An InstanceCpuPlatform is an instance's request for how it wants vCPUs to
/// look, whereas a VmmCpuPlatform is the actual chosen platform at the point
/// that instance was incarnated. A critical difference is that VmmCpuPlatform
/// has an escape hatch in the form of `SledDefault` if an instance is to be run
/// on a kind of hardware the control plane does not yet know about - this is
/// only a possible value if both the instance did not have an explicit CPU
/// platform *and* it was placed on a sled whose family we don't know.
///
/// While this means we can convert between Instance and VMM CPU platforms, in
/// practice Instance->VMM is the only useful direction. Since that is expected
/// to be injective, `From` suffices.

if you want it to be...

impl From<InstanceCpuPlatform> for VmmCpuPlatform {
fn from(value: InstanceCpuPlatform) -> Self {
match value {
Expand Down
6 changes: 3 additions & 3 deletions nexus/src/app/sagas/instance_migrate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,9 @@ async fn sim_reserve_sled_resources(
let params = sagactx.saga_params::<Params>()?;
let propolis_id = sagactx.lookup::<PropolisUuid>("dst_propolis_id")?;

let Some(compatible_sled_families) =
params.src_vmm.cpu_platform.compatible_sled_cpu_families()
else {
let compatible_sled_families =
params.src_vmm.cpu_platform.compatible_sled_cpu_families();
if compatible_sled_families.is_empty() {
// If we're here there are no compatible sled families for the VMM's CPU
// platform. That means the VMM's CPU platform is "SledDefault" and we
// just don't know what sleds might be compatible with the CPU platform
Expand Down
Loading