server: separate statuses of inbound and outbound migrations #661

gjcolombo · 2024-03-13T17:14:03Z

N.B. This is a breaking change to the Propolis API that will need a corresponding Omicron change before it can be merged there.

Change the way the server reports migration statuses:

Instead of asking clients to identify a specific migration to query, create a migration-status endpoint that reports on all migrations currently known to the server. (The server wasn't keeping any history of prior migration attempts anyway, so specifying an ID wasn't buying very much; if the client cares about a specific ID they can compare it to the IDs the server now returns.)
When reporting migration status (either through the migration status endpoint or the instance monitor endpoint), report the ID and status of both the inbound migration (if there was one) and the most recent attempted outbound migration.

This should allow us to simplify a few things in sled agent. Today:

Sled agent has to handle mismatched migration IDs carefully to cover cases such as this one:
- Propolis was initialized via incoming migration with ID X
- Sled agent knows about outgoing migration with ID Y
- Propolis reports an instance state change that still bears ID X This sort of case is easier to reason about (or at least to make fewer assumptions about) when the outbound and inbound migrations are properly distinguished.
If Propolis reports a status of "destroyed, migration finished," it's ambiguous whether the VM was migrated into and then stopped or was migrated out of. With these changes it's obvious that a successful migration-out occurred.

Additional fixes/improvements:

The pending_migration_id field in the VM controller wasn't being used for anything; now it is.
Add a log line to the code path that publishes a new externally-visible instance state.

Tests:

cargo test
local PHD run (migration-from-base tests are broken because of the breaking API change)
ran an ad hoc live migration with propolis-cli to test both that the CLI works properly and that the right messages appear in the Propolis logs

Fixes #508.

gjcolombo · 2024-03-13T17:15:03Z

This should be OK to review as-is, but I'm going to try to cook up the corresponding sled agent change before sending actual review requests here.

gjcolombo · 2024-03-13T17:18:47Z

This should be OK to review as-is

falcon Failing after 4m — Failure!

Oops. I think this is just an out-of-date OpenAPI doc--will work on the fix.

crates/propolis-api-types/src/lib.rs

**N.B.** This is a breaking change to the Propolis API that will need a corresponding Omicron change before it can be merged there. Change the way the server reports migration statuses: - Instead of asking clients to identify a specific migration to query, create a `migration-status` endpoint that reports on all migrations currently known to the server. (The server wasn't keeping any history of prior migration attempts anyway, so specifying an ID wasn't buying very much; if the client cares about a specific ID they can compare it to the IDs the server now returns.) - When reporting migration status (either through the migration status endpoint or the instance monitor endpoint), report the ID and status of both the inbound migration (if there was one) and the most recent attempted outbound migration. This should allow us to simplify a few things in sled agent. Today: - Sled agent has to handle mismatched migration IDs carefully to cover cases such as this one: - Propolis was initialized via incoming migration with ID X - Sled agent knows about outgoing migration with ID Y - Propolis reports an instance state change that still bears ID X This sort of case is easier to reason about (or at least to make fewer assumptions about) when the outbound and inbound migrations are properly distinguished. - If Propolis reports a status of "destroyed, migration finished," it's ambiguous whether the VM was migrated into and then stopped or was migrated out of. With these changes it's obvious that a successful migration-out occurred. Additional fixes/improvements: - The `pending_migration_id` field in the VM controller wasn't being used for anything; now it is. - Add a log line to the code path that publishes a new externally-visible instance state. Tests: - cargo test - local PHD run (migration-from-base tests are broken because of the breaking API change) - ran an ad hoc live migration with propolis-cli to test both that the CLI works properly and that the right messages appear in the Propolis logs

hawkw

Excited to get this merged! I just had a couple fairly minor nits.

hawkw · 2024-06-14T22:39:14Z

bin/propolis-cli/src/main.rs

+                let Some(migration) = migration else {
+                    if role == "dst" {
+                        anyhow::bail!(
+                            "{role} instance's migration ID wasn't set"


considered nitpicky: since this is inside an if role == "dst", do we really need to format it here?

hawkw · 2024-06-14T22:42:23Z

crates/propolis-api-types/src/lib.rs

+/// direction. This means it's possible for both the inbound and outbound
+/// migration statuses to be set, e.g. if a Propolis was initialized via
+/// migration in and is now the source of a migration out.


It might be nice if this comment discussed the motivation behind reporting both migrations here, rather than e.g. just an enum MigrationRole or similar?

hawkw

Excited to get this one merged! I had some very unimportant nits, but feel free to ignore them --- I'd like to get this wired through to sled-agent as soon as possible. :)

hawkw · 2024-06-17T16:45:43Z

crates/propolis-api-types/src/lib.rs

-    pub migration_id: Uuid,
-    pub state: MigrationState,
+    /// The status of the most recent attempt to initialize the current instance
+    /// via migration in, or None if the instance has never been a migration


extremely nitpicky (I don't actually care about this):

Suggested change

/// via migration in, or None if the instance has never been a migration

/// via migration in, or `None` if the instance has never been a migration

hawkw · 2024-06-17T16:45:55Z

crates/propolis-api-types/src/lib.rs

+    /// target.
+    pub migration_in: Option<InstanceMigrationStatus>,
+    /// The status of the most recent attempt to migrate out of the current
+    /// instance, or None if the instance has never been a migration source.


similarly:

Suggested change

/// instance, or None if the instance has never been a migration source.

/// instance, or `None` if the instance has never been a migration source.

hawkw · 2024-06-17T16:47:57Z

bin/propolis-server/src/lib/vm/state_driver.rs

+    ///
+    /// # Panics
+    ///
+    /// Panics if both `instance_state` and `migration_state` are `None`.


this is a bit of a bummer, I kind of wonder if we should instead have an

enum PublishedState { Instance(ApiInstanceState), Migration(PublishedMigrationState), Both(ApiInstanceState, PublishedMigrationState), }

or similar? On the other hand, if that's too annoying to work with, I don't think it's a huge deal...

This was bothering me too on self-review. I'll have a fix in my next push.

hawkw · 2024-06-17T17:04:46Z

Looks like the OpenAPI specs need to be regenerated: https://github.com/oxidecomputer/propolis/actions/runs/9551503512/job/26325948032?pr=661

gjcolombo · 2024-06-17T17:11:52Z

Once more with feeling!

hawkw self-requested a review March 13, 2024 17:27

gjcolombo commented Mar 13, 2024

View reviewed changes

crates/propolis-api-types/src/lib.rs Outdated Show resolved Hide resolved

gjcolombo added 3 commits June 14, 2024 22:03

falcon openapi update

261a4a4

add doc comments

d65ae06

hawkw approved these changes Jun 14, 2024

View reviewed changes

merge fix

8e44af6

gjcolombo force-pushed the gjcolombo/two-migration-statuses branch from c1ad061 to 8e44af6 Compare June 14, 2024 23:37

PR feedback

da9f216

hawkw approved these changes Jun 17, 2024

View reviewed changes

more cleanup + openapi fixes

08f441c

gjcolombo merged commit 50cb28f into master Jun 17, 2024
9 of 10 checks passed

gjcolombo deleted the gjcolombo/two-migration-statuses branch June 17, 2024 17:27

hawkw mentioned this pull request Jul 30, 2024

Perform instance state transitions in instance-update saga oxidecomputer/omicron#5749

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: separate statuses of inbound and outbound migrations #661

server: separate statuses of inbound and outbound migrations #661

gjcolombo commented Mar 13, 2024

gjcolombo commented Mar 13, 2024

gjcolombo commented Mar 13, 2024

hawkw left a comment

hawkw Jun 14, 2024

hawkw Jun 14, 2024

hawkw left a comment

hawkw Jun 17, 2024

hawkw Jun 17, 2024

hawkw Jun 17, 2024

gjcolombo Jun 17, 2024

hawkw commented Jun 17, 2024

gjcolombo commented Jun 17, 2024

	/// via migration in, or None if the instance has never been a migration
	/// via migration in, or `None` if the instance has never been a migration

	/// instance, or None if the instance has never been a migration source.
	/// instance, or `None` if the instance has never been a migration source.

server: separate statuses of inbound and outbound migrations #661

server: separate statuses of inbound and outbound migrations #661

Conversation

gjcolombo commented Mar 13, 2024

gjcolombo commented Mar 13, 2024

gjcolombo commented Mar 13, 2024

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jun 14, 2024

Choose a reason for hiding this comment

hawkw Jun 14, 2024

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jun 17, 2024

Choose a reason for hiding this comment

hawkw Jun 17, 2024

Choose a reason for hiding this comment

hawkw Jun 17, 2024

Choose a reason for hiding this comment

gjcolombo Jun 17, 2024

Choose a reason for hiding this comment

hawkw commented Jun 17, 2024

gjcolombo commented Jun 17, 2024