Repository navigation
Turin Dense is as valid as Turin for VM placement - #11668
Conversation
One may ask, how does Nexus place instances on Dense Cosmos, before this change? `compatible_sled_cpu_families()` is only involved if an explicit CPU platform is set on the instance to be started. If no explicit CPU platform is set, `sis_create_vmm_record` implements the defaulting behavior of picking a minimum compatible platform for the sled chosen according to any other placement constraints. For all of Milan, Turin, and TurinDense, that means instances end up with the Milan CPU platform. Because of this, though, simply setting an explicit CPU platform - even `amd_milan` - would make a Dense sled ineligible as a host for that instance. Even one which it had just happily hosted with a Milan CPU platform by virtue of the minimum-compatible default above.
| Self::AmdTurin => &[SledCpuFamily::AmdTurin], | ||
| Self::AmdTurinV2 => &[SledCpuFamily::AmdTurin], |
| ); | ||
| } | ||
|
|
||
| fn instance_compute_update( |
There was a problem hiding this comment.
i would mildly prefer we called this
| fn instance_compute_update( | |
| fn instance_compute_reconfigure( |
to match the name of the API, though i will accept if you think that's a bit weirdly non-verb-y?
There was a problem hiding this comment.
I'd named this update because the struct that comes out is InstanceUpdate :(
| } | ||
|
|
||
| #[nexus_test] | ||
| async fn test_can_start_turin_dense_instance_with_cpu_platform( |
There was a problem hiding this comment.
I really don't like this test being basically a carbon copy of test_can_start_instance_with_cpu_platform with one line changed, but trying to get a bit smarter about the test seems way more prone to inappropriately passing than is worth the trade.
namely, if you simply create both Turin and TurinDense simulated sleds, you also need to somehow control the instance's placement for the test. I don't think you can do a "to this specific sled" affinity rule, so that's off the table. there could be a local disk "on the sled" to constrain placement, but that seems terrible for legibility too. the option I tried the hardest was to have a list of sled CPU families to create fake sleds for, then we could just make instances of size the whole test sled, and go through an instance and its CPU platform choices one by one, leaving test sleds fully occupied as we go through. that seemed pretty complicated too.
a more legible option is probably to enumerate CPU platforms and the sleds on which they can be placed, then step through the mechanics like this for a list of (SledCpuFamily, Vec<InstancePlatformAndExpectation>). that's probably the best way to express what we're testing without getting lost in the details, but I wanted to get a patch up here before spinning too much more on the test situation.
There was a problem hiding this comment.
Yeah, I think you're right, the repeated code in two tests is not the absolute worst thing given the risk of accidentally passing tests...
Co-authored-by: Eliza Weisman <eliza@elizas.website>
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()`.
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()`.
One may ask, how does Nexus place instances on Dense Cosmos, before this
change?
compatible_sled_cpu_families()is only involved if an explicitCPU platform is set on the instance to be started.
If no explicit CPU platform is set,
sis_create_vmm_recordimplementsthe defaulting behavior of picking a minimum compatible platform for the
sled chosen according to any other placement constraints. For all of
Milan, Turin, and TurinDense, that means instances end up with the Milan
CPU platform.
Because of this, though, simply setting an explicit CPU platform - even
amd_milan- would make a Dense sled ineligible as a host for thatinstance. Even one which it had just happily hosted with a Milan CPU
platform by virtue of the minimum-compatible default above.
In adding the new test which both demonstrates the issue without the
instance_cpu_platform.rschange, and passes with that change, the now 10k-lineintegration_tests/instances.rsis somewhat dizzying. I've included two additional commits here that kinda make the new test more legible to me.. I'd be happy to squash this all together, if we want a48c3de in one change and the test cleanup in another, that's fine too.