sled agent should terminate Propolis zones when Propolis indicates a previously-started VM has gone missing

[jordanhendricks]

An instance on rack2 appeared to be stuck because:

• propolis-server panicked
• the propolis-server service was restarted by SMF (its restarter is svc:/system/svc/restarter:default)
• sled-agent failed to notice that propolis-server had panicked (sled-agent failed to notice propolis-server panicked #3206), and the instance was still marked as running
• a new propolis-server ran, and requests from the serial console in the web console continued, but there was no instance, so the console seemed to hang

It isn't clear to me that restarting propolis-server is the right thing to do here. We should consider having the service go into maintenance and not restart when that happens.

Related: #2825

[davepacheco]

Maybe related to #4872?

[hawkw]

Because the propolis-server process owns userspace VMM state that isn't really persisted anywhere, it's more or less impossible to recover a VM in a usable state after a restart, so restarting the propolis-server process is basically never correct. When a propolis-server comes back up, it won't know about the VM it will have previously managed, and there's no way for it to resume managing that VM correctly as a bunch of state that only exists in userspace is gone. @gjcolombo and I talked it over today and we agreed that we should change this to never restart propolis-server.

[gjcolombo]

We discussed this at the 9/26/24 hypervisor huddle. The thing we want to get right here is that when a Propolis zone panics, sled agent should be able to discover as much and then should (a) move the VMM to Failed, (b) terminate the zone, and (c) ensure that a bundle is taken that includes logs and the panic core.

(b) and (c) happen naturally as part of sled agent's existing "VMM terminated" logic, and (a) is trivial once the zone is gone (or, more precisely, once sled agent stops watching for updates from the relevant propolis-server). The main issue, then, is making sure that sled agent properly detects that Propolis was terminated and restarted.

Fortunately, when a client asks Propolis to do something that requires an active VM, Propolis will return a unique "VM? what VM?" error code if no VM is present. This means that sled agent should be able to detect Propolis restarts: if it knows that it previously started a VM in a Propolis server, and now the server says there's no VM there, then the whole enterprise has failed and sled agent should clean up accordingly.

[hawkw]

Thanks @gjcolombo for renaming this one --- I think the new title is much better. I investigated what we're currently doing in sled-agent in #3238 (comment), and the TL;DR is "it's still pretty bad": the error code Propolis returns when it's been restarted is blindly passed up to Nexus (which isn't terrible, it's at least not a 500 anymore), but Nexus doesn't know what it means, and sled-agent doesn't actually clean up the zone in that case. So I'm gonna go fix that.

[hawkw]

Good news, the propolis-server InstanceStateMonitor API will also return the error codes we want from it (which, it turns out, I had added, but I forgot about): https://github.com/oxidecomputer/propolis/blob/dfcd4430f0211160c9fffd4688d1be556296c769/bin/propolis-server/src/lib/server.rs#L389-L418

Bad news, the sled-agent InstanceMonitorRunner will just bail out on receipt of any error from Propolis:

omicron/sled-agent/src/instance.rs

Lines 253 to 289 in 888f6a1

	impl InstanceMonitorRunner {
	async fn run(self) -> Result<(), anyhow::Error> {
	let mut gen = 0;
	loop {
	// State monitoring always returns the most recent state/gen pair
	// known to Propolis.
	let response = self
	.client
	.instance_state_monitor()
	.body(propolis_client::types::InstanceStateMonitorRequest {
	gen,
	})
	.send()
	.await?
	.into_inner();
	let observed_gen = response.gen;
	// Now that we have the response from Propolis' HTTP server, we
	// forward that to the InstanceRunner.
	//
	// It will decide the new state, provide that info to Nexus,
	// and possibly identify if we should terminate.
	let (tx, rx) = oneshot::channel();
	self.tx_monitor
	.send(InstanceMonitorRequest::Update { state: response, tx })
	.await?;
	if let Reaction::Terminate = rx.await? {
	return Ok(());
	}
	// Update the generation number we're asking for, to ensure the
	// Propolis will only return more recent values.
	gen = observed_gen + 1;
	}
	}
	}

We would expect this to drop the send-side of the channel it uses to communicate with the InstanceRunner, closing the channel, and hitting this select, which would correctly mark the VMM as failed and do what we want, despite eating the actual error message from propolis:

omicron/sled-agent/src/instance.rs

Lines 388 to 394 in 888f6a1

	// NOTE: This case shouldn't really happen, as we keep a copy
	// of the sender alive in "self.tx_monitor".
	None => {
	warn!(self.log, "Instance 'VMM monitor' channel closed; shutting down");
	let mark_failed = true;
	self.terminate(mark_failed).await;
	},

HOWEVER, as the comment points out, the InstanceRunner is also holding its own clone of the channel sender, keeping us from ever hitting that case:

omicron/sled-agent/src/instance.rs

Lines 308 to 309 in 888f6a1

	// Request channel on which monitor requests are made.
	tx_monitor: mpsc::Sender<InstanceMonitorRequest>,

AFAICT, this means we'll just totally give up on monitoring the instance as soon as we hit any error here, which seems...quite bad. I think that this actually means that when a Propolis process crashes unexpectedly, we'll get an error when the TCP connection closes, bail out, and then never try hitting the instance monitor endpoint ever again¹. So, we won't notice that the Propolis is actually out to lunch until we try to ask it to change state. This Is Not Great!

We should probably rework this so that the InstanceMonitorRunner will proactively send any errors which definitively indicate goneness to the InstanceRunner, and just keep trying in the face of any other errors (at least, connection errors), probably with a smallish backoff.

Footnotes

1. Although I've not actually verified that this is what happens. ↩

[hawkw]

retagging this one as "bug" because it's turned out that this is kinda bad and i'm going to make fixing it my top priority.