omicron-stress can trigger sled-agent panic: "should have an active Propolis zone by now"

[jmpesp]

Running omicron-stress with the patch

diff --git a/src/actor/instance.rs b/src/actor/instance.rs
index 7c14b94..cd0593b 100644
--- a/src/actor/instance.rs
+++ b/src/actor/instance.rs
@@ -120,7 +120,7 @@ impl InstanceActor {
             ncpus: oxide::types::InstanceCpuCount(1),
             network_interfaces:
                 oxide::types::InstanceNetworkInterfaceAttachment::None,
-            start: true,
+            start: false,
             user_data: String::new(),
             ssh_public_keys: None,
         };

and the invocation:

exec cargo run --release -- \
    --num-test-instances 8 \
    --threads-per-instance 8 \
    --num-test-disks 0 \
    --threads-per-disk 0 \
    --num-test-snapshots 0 \
    --threads-per-snapshot 0 \
    --server-errors-fatal

In the Canada region, it seems I can reliably get sled-agent to panic after a few invocations:

thread 'tokio-rt-worker' (30) panicked at sled-agent/src/instance.rs:2286:9:
should have an active Propolis zone by now
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[jmpesp]

Running with the following diff in omicron:

diff --git a/sled-agent/src/common/instance.rs b/sled-agent/src/common/instance.rs
index 83c63f187..cbcb798ad 100644
--- a/sled-agent/src/common/instance.rs
+++ b/sled-agent/src/common/instance.rs
@@ -149,6 +149,7 @@ impl From<&'_ InstanceMigrationStatus> for ObservedMigrationState {
 /// a subset of the instance states Nexus knows about: the Creating and
 /// Destroyed states are reserved for Nexus to use for instances that are being
 /// created for the very first time or have been explicitly deleted.
+#[derive(Debug)]
 pub enum PublishedVmmState {
     Stopping,
     Rebooting,
diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs
index a009e7f38..87e4c40c1 100644
--- a/sled-agent/src/instance.rs
+++ b/sled-agent/src/instance.rs
@@ -2273,6 +2273,16 @@ impl InstanceRunner {
         // check that one already exists. Note that the calls that create the
         // zone also send a VM creation request to the new Propolis process, but
         // this is trickier to assert without actually calling the Propolis API.
+        if self.running_state.is_none() {
+            error!(
+                self.log,
+                "about to panic!";
+                "propolis_request" => ?propolis_request,
+                "next_published" => ?next_published,
+                "state" => ?state,
+            );
+        }
+
         assert!(
             self.running_state.is_some(),
             "should have an active Propolis zone by now"

shows

16:57:01.716Z ERRO SledAgent (InstanceManager): about to panic!
    file = sled-agent/src/instance.rs:2277
    instance_id = 76952174-2a87-490b-8ec8-bb161b9250bc
    next_published = None
    propolis_id = 5d52b32f-98bd-4f4b-9b47-ddb0ca506308
    propolis_request = None
    state = Stopped

[jmpesp]

The match for Stopped does this:

            VmmStateRequested::Stopped => {
                // If there's no running Propolis yet, unregister this instance
                // straightaway. Since nothing will ever send a state update in
                // this case, force the VMM into a terminal Destroyed state,
                // which is what it would have reached if it *had* existed and
                // then stopped in an orderly manner.
                if self.running_state.is_none() {
                    self.state.force_state_to_destroyed();
                    self.terminate().await;
                    (None, None)
                } else {
                    (
                        Some(PropolisRequest::Stop),
                        Some(PublishedVmmState::Stopping),
                    )
                }

which does not return an Err for the case of self.running_state.is_none(), like the other match branches. The very next thing after the match is the assert for self.running_state.is_some(), which fires here.

[jmpesp]

I'm now testing

diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs
index a009e7f38..042369e7d 100644
--- a/sled-agent/src/instance.rs
+++ b/sled-agent/src/instance.rs
@@ -114,6 +114,9 @@ pub enum Error {
     #[error("Propolis job with ID {0} is registered but not running")]
     VmNotRunning(PropolisUuid),
 
+    #[error("Propolis job with ID {0} is registered but not running, and stop requested")]
+    VmNotRunningAndRequestedStop(PropolisUuid),
+
     #[error("Propolis job with ID {0} already registered")]
     PropolisAlreadyRegistered(PropolisUuid),
 
@@ -2250,7 +2253,7 @@ impl InstanceRunner {
                 if self.running_state.is_none() {
                     self.state.force_state_to_destroyed();
                     self.terminate().await;
-                    (None, None)
+                    return Err(Error::VmNotRunningAndRequestedStop(self.propolis_id));
                 } else {
                     (
                         Some(PropolisRequest::Stop),

I haven't seen the sled-agent panic yet