Allow autoselection of destination sled for instance migration by jgallagher · Pull Request #11605 · oxidecomputer/omicron

jgallagher · 2026-10-27T17:52:47Z

The non-test changes are all in the first commit (the bulk of the PR by lines-of-code is made up of test changes and additions):

Change the Nexus lockstep API and instance migration saga from taking an explicit destination sled ID to an enum with variants for "explicit destination sled ID" and "autoselect".
Update the instance placement datastore method to allow for migration autoselection via a must_not_use_sleds argument (patterned after the existing must_use_sleds argument) - this allows us to say "don't choose the sled where the VMM is currently running", in particular, since migrating a VMM to its own sled is nonsense.

Being able to autoselect destination sleds is a prerequisite to being able to add a background task that migrates VMMs on sleds that need to be evacuated via migration. It's very possible we want to do something smarter here in the long run (e.g., preferring destination sleds that have already been updated), but I believe the current change to be correct and perfectly reasonable for now, especially since "maybe we should rework the instance placement algorithm" has also come up as a possible multirack project.

jmpesp · 2026-10-27T18:52:50Z

migrating a VMM to its own sled is nonsense

you say nonsense, I say move to a newer propolis version :)

bnaecker

This is nice, thanks for all the great test coverage! I have a few questions, but mostly for my own understanding. I don't see any blocking issues.

bnaecker · 2026-10-27T18:31:13Z

+        assert_eq!(resource.sled_id(), sleds[1].id());
+
+        let test_instance =
+            Instance::new().avoid_sled(sleds[0].id()).avoid_sled(sleds[1].id());


An avoid_many_sleds() interface accepting an iterator or something might be nice, but very non-essential.

bnaecker · 2026-10-27T18:39:27Z

+        let SledReservationTransactionError::Reservation(
+            SledReservationError::NotFound,
+        ) = err


I wonder if it would be useful for the error to indicate why nothing could be found. That is, do we care about the difference between an instance that couldn't be placed because of affinity rules, and one that we avoided for this new migration-related reason?

I'm really thinking about debugging and similar uses here. We might want to know that we couldn't place an instance for the latter reason specifically, at least for logging or similar uses.

I agree but am a little worried that would be a rather larger change. We do already have some affinity-specific variants; we pick one in pick_sled_reservation_target() (line ~400 in this file), and fall through to NotFound if we don't hit any of those specifically. This test is combining "don't use this sled, and you can't use that sled because of affinity rules"; should the error message include both? This also seems closely related to your question about the generality here - maybe if we made the VMM migration "must use" / "must not use" more specific to migration it'd be easier to give specific errors?

Lemme float this by you: what if we land this more or less as-is (subject to other folks chiming in on reviews if they'd like), and I try switching away from the general must_{not_,}use_sleds sets and to a specific-to-VMM-migration enum? I'll see if that makes error reporting more straightforward, and even if it doesn't, I can put it up as a "do we like this better from an understandability point of view" change.

what if we land this more or less as-is

Yessir, that sounds good. It's an improvement and easy to use now, and we can try to improve it next.

bnaecker · 2026-10-27T18:47:22Z

+        InstanceMigrateDestination::Autoselect => {
+            constraints.must_not_select_from(&[params.src_vmm.sled_id()])
+        }
+        InstanceMigrateDestination::ById { dst_sled_id } => {
+            constraints.must_select_from(&[dst_sled_id])


I think this highlights a vague concern I have. I'm struggling to come up with a situation in which you'd want to specify both must_select_from() and must_not_select_from(). Is this for generality, or am I missing a specific use case you can see?

You're not missing anything; this was mostly "I'm touching code I've never seen before so will extend the existing names/patterns". I think this API is the only user of must_select_from, which means in practice:

it's always one or the other, not both

if we have one of them, it always contains exactly one sled, not arbitrarily-many

I wonder if it was written this way anticipating more general uses? We did end up with a more general use (affinity/anti-affinity), but that didn't map into this structure anyway and does something else.

Maybe both of these options should be squashed into something like an Option<MigrationRequirement> where MigrationRequirement is "use exactly this sled" or "never use this sled"? It makes this whole thing less general but if we're not using that generality maybe it's more clear?

I'm not sure! I think the API you've written is pretty clear, so maybe it's OK to leave it for now until we get more experience. I would say it might help to add the data in my other comment to errors that come from using an explicitly-avoided port. We might want to log that or otherwise keep track of it, as we get that experience.

FWIW I like how general this interface is

Ok. I started drafting what this would look like taking an enum instead of these general lists, and it's not that much better IMO - even trying to get accurate error messages out is still pretty hairy. I think I'll just leave this as it is for now, and revisit if we end up needing more specific errors when we go to use this as part of updates.

jmpesp · 2026-10-27T19:06:36Z

+    Autoselect,
+
+    /// Migrate to a specific sled.
+    ById { dst_sled_id: SledUuid },


this doesn't have the schemars block above it anymore, was that intentional?

Yeah. IIUC all that does is squash the SledUuid type out of the openapi spec in favor of a generic Uuid; we do that for the external API, but this is only exposed in the internal lockstep one, which is free to use the strongly-typed IDs.

jmpesp · 2026-10-27T19:36:46Z

migrating a VMM to its own sled is nonsense

you say nonsense, I say move to a newer propolis version :)

edit: I had confused something I was doing with what's possible in the product, disregard!

karencfv · 2026-10-28T06:20:34Z

+    // Add suitable constraints for the allocator:
+    //
+    // 1. CPU families must be compatible
+    // 2. Either the specified destination sled ID must be chosen (if the caller
+    //    chose the destination), or the current sled ID must NOT be chosen (if
+    //    we choose the destination).


Do we expect to filter out sleds that are evacuating here? Or is this something that will be added to the background task?

We already do! The query that builds up targets (which the other parts of this PR then filters down to exclude the source sled) ensures we only consider available sleds:

omicron/nexus/db-queries/src/db/queries/sled_reservation.rs

Lines 166 to 170 in 77901ac

SELECT sled.id as sled_id

FROM sled

JOIN rendezvous_sled_bp_availability AS sled_bp_avail

ON sled_bp_avail.sled_id = sled.id

AND sled_bp_avail.bp_availability = 'available'

jgallagher added 3 commits October 27, 2026 13:42

instance migrate internal API: allow autoselection of destination sled

7e82346

update existing tests and add new tests for migration autoselection

aab48d2

rename find_sled_agent() -> find_sled_id_other_than()

8ce4e8f

jgallagher requested review from bnaecker, jmpesp and karencfv October 27, 2026 17:52

bnaecker approved these changes Oct 27, 2026

View reviewed changes

jmpesp approved these changes Oct 27, 2026

View reviewed changes

karencfv reviewed Oct 28, 2026

View reviewed changes

jgallagher merged commit 3a547a3 into main Oct 28, 2026
20 checks passed

jgallagher deleted the john/instance-migrate-autoselect branch October 28, 2026 13:11

	SELECT sled.id as sled_id
	FROM sled
	JOIN rendezvous_sled_bp_availability AS sled_bp_avail
	ON sled_bp_avail.sled_id = sled.id
	AND sled_bp_avail.bp_availability = 'available'

Conversation

jgallagher commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmpesp commented Oct 27, 2026

Uh oh!

bnaecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmpesp commented Oct 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jgallagher commented Oct 27, 2026 •

edited

Loading