Skip to content

Dedupe compatible_sled_cpu_families across {Instance,Vmm}CpuPlatform - #11697

Open
iximeow wants to merge 1 commit into
mainfrom
ixi/compatible_sled_families_refactor
Open

iximeow wants to merge 1 commit into
mainfrom
ixi/compatible_sled_families_refactor

Conversation

@iximeow

@iximeow iximeow commented Nov 4, 2026

Copy link
Copy Markdown
Member

Both of these had versions of compatible_sled_cpu_families(), and one was right, while the other was stale, resulting in #11668. Now and for the future an instance's CPU platform, if explicitly chosen, is represented by exactly one VMM CPU platform. Because of that, we can convert the instance's CPU platform to the VMM CPU platform for it and have a single authoritative compatible_sled_cpu_families().

#11668 was "there is clearly a bug, make it stop existing", and this is "after having thought about how the bug happened, can we make the code less bug-prone", so.. delightfully, it deletes the same code that 11668 fixed this morning.

Both of these had versions of `compatible_sled_cpu_families()`, and one
was right, while the other was stale, resulting in #11668. Now and for
the future an instance's CPU platform, if explicitly chosen, is
represented by exactly one VMM CPU platform. Because of that, we can
convert the instance's CPU platform to the VMM CPU platform for it and
have a single authoritative `compatible_sled_cpu_families()`.
Comment on lines +63 to +73
// 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.

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...

This branch has not been deployed

No deployments
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