[hawkw]

Note

To reviewers: yes, this is kind of an enormous diff. All I can really say in my defense is that a lot of those lines are either new tests I'm adding or the huge and somewhat deranged block comments I wrote to convince myself that this all made sense. I also realize the code is somewhat unpleasantly convoluted. It might be possible to simplify some of this, and I'd love suggestions for how to. Personally, I've spent too long looking at the current implementation to have any ideas left for how to make it less unpleasant.

First, some background. When a VMM record is in the Creating state, it represents a VMM that exists in the database but which has not yet been started on a sled by a vmm_register request to a sled-agent. VMM records exist in this state if, and only if, an instance-start or instance-migrate saga is in progress for the instance that VMM is attached to, but has not yet recorded¹ the state transition that results from sending a vmm_register request to the sled.

When an instance's active VMM record is in the Creating VMM state, the instance's external API state is Starting (because the VMM is being created in order to start running the instance). When the VMM is registered with the sled-agent and the VMM record's state transitions to Starting, the instance's externally-visible state will remain Starting (because the instance is still in the process of starting). This is important to remember, because an instance which is Starting may be requested to stop by the instance_stop API endpoint. Attempting to stop an instance which is in the Starting state and has an active VMM currently always results in Nexus sending a vmm_put_state request to the sled-agent for the sled that the VMM record says the VMM is on.

Sending the stop request to the sled-agent is the correct thing to do when the VMM record is also in the Starting state, as there is an actual Propolis process running on the sled able to receive the request. However, when the sled-agent has not yet been sent the vmm_register request, if it receives a request to stop the VMM, it will return a 404 error with the NO_SUCH_INSTANCE error code, indicating that it does not have a VMM with the requested UUID. This results in the VMM being marked as Failed, and an error being returned to the caller. See #11560 for details.

The consequences of sending the stop request to the sled before the VMM was registered used to be Real Bad, but now that #11608 has merged, they are only a little bit bad. Before #11608, the start saga would continue running, and would successfully start the VMM on the sled, while the rest of the control plane believes it doesn't exist, and Nexus would attempt to restart the instance because the VMM was believed to have failed. This could result in two VMM processes running that instance, with the "zombie" VMM that the control plane believes does not exist and cannot stop being the one that successfully activated the instance's Crucible volumes. Creating a leaked zombie process that we cannot stop but which has claimed exclusive ownership of the instance's distributed disks was basically the worst possible thing which could occur under these circumstances. #11608 fixed the worst consequences of this race by making the VMM's transition to Failed cause the instance_start saga to unwind and clean up after itself, rather than leaking the VMM.

After #11608, attempts to stop an Starting instance while its active VMM is in the Creating state would (depending on how the race plays out) generally result in the instance not starting, which is much better than the previous situation. However, this would come about in a somewhat messy and unpleasant way: the active VMM would be marked as Failed, and an instance_update saga triggered by the transition to Failed would probably transition the instance to Failed as well, but we wouldn't restart it because its intended state would be Stopped. The 404 error from the sled-agent would bubble up to the caller who was trying to stop the instance. So we would more or less give you what you wanted, but only through an ugly Rube Goldberg contraption of things not quite doing what they're supposed to and which would present the caller with errors and Failed instances despite basically doing what they wanted.

This branch builds on top of #11608 by attempting to make all of this behave reasonably, at the cost of making it somewhat more complicated. While we could probably have avoided all of this by just not allowing callers to stop instances that have Creating active VMMs, I didn't love the user experience consequences of that. It seems reasonable to want to stop a Starting instance --- essentially, you might want to "cancel" starting the instance --- and if we continued to allow it to be stopped once the actual VMM state had transitioned to Starting, we would let you stop a Starting instance sometimes but not other times, which I really dislike. Therefore, I've decided to instead clean up this interaction and allow instance stop requests to cancel an ongoing start saga gracefully, regardless of whether or not the VMM has been started on the sled.

This branch therefore more or less implements the approach I described in this comment: when a stop request is received for an instance whose active VMM is Creating, we attempt to update the VMM record to a new VMM state, Cancelling, conditionally on the VMM record's generation number not having advanced since we observed it to be Creating. If this succeeds, the instance_start saga will then observe that the state has changed out from under it when it attempts to change the state to Starting, causing it to unwind, and unregister any VMM process it may have created on the sled. If this fails because the generation number has changed, it means that the VMM has advanced to Starting and now exists on the sled, and we will then send the sled-agent a stop request normally, as we did before this change.

We must introduce a new VMM state variant to represent this, rather than using the existing Stopping or SagaUnwound states. Stopping won't work because the new state must must be considered to not exist on a sled by other parts of the code such as other state change requests or the instance-watcher background task, or else we would once again see requests for that VMM being sent to the sled-agent after it has been unregistered. On the other hand, SagaUnwound also won't work because it signals the virtual provisioning records created for the VMM have already been cleaned up, which is not the case here, as the unwinding start saga will still have to do that. Thus, Cancelling. While the VMM is Cancelling, the instance will be considered Stopping in the external API.

So, that's the core idea of this change. The actual mechanics of the implementation is somewhat more complex, but my hope is that the unnecessarily wordy block comments I've written help explain that sufficiently.

As an aside, this change should make it possible for the vmm_stop_for_update background task added by @karencfv #11380 to begin stopping Creating VMMs, if (and only if) it was reworked to call Nexus::instance_update_runtime directly rather than calling the sled-agent's vmm_put_state API directly. I did not make that change here, and I think there are some tradeoffs there since the current implementation allows us to reuse the sled-agent client, but it might be worth considering eventually.

Footnotes

1. Critically, the request may have been sent, and may have been received by the sled-agent! Sending the request and writing back the resulting state change to the database is inherently non-atomic. ↩

[hawkw]

@claude review

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟡 nexus/src/app/instance.rs — A stop that loses the new CAS race and then gets a sled-agent "VMM gone" error no longer marks the VMM Failed. It stays Starting or Running until the instance-watcher notices. On error, instance_stop passes the pre-loop state.vmm() snapshot (still Creating) to mark_vmm_failed. That snapshot's transition(Failed) generation is stale, so vmm_update_runtime writes nothing. Fix: have instance_request_state return the VMM record it last acted on, or refetch it on error, and pass that record to mark_vmm_failed. This keeps the generation check meaningful. [also at: nexus/src/app/instance.rs:1194 - A stop request that loses the CAS race and then gets a fatal sled-agent error can fail to mark the VMM Failed. The VMM stays recorded as alive.]

  Why this was flagged

  The trigger is a Stop request that reads the VMM as Creating at generation g. Its DoGoofyVmmStateCasLoopDance update fails because the start saga's notify_vmm_updated already wrote Starting at generation g+1. The loop then refetches through instance_fetch_with_vmm, sees a VMM that exists_on_sled(), and sends SendToSled to that refetched VMM. If the sled-agent replies with an error that has a vmm_failure_reason(), instance_stop (instance.rs:1189-1195) calls mark_vmm_failed with the stale Creating record from before the loop. mark_vmm_failed (instance.rs:2070-2074) writes Failed at generation g+1, which vmm_update_runtime's state_generation.lt filter rejects. The VMM stays Starting/Running in the database even though the sled-agent says it is gone. On the base branch there was no refetch, so the record passed to mark_vmm_failed was always the one the request had been sent to.

  Verification: Severity: nit. It is a regression on a narrow race, and the instance-watcher recovers it later. In nexus/src/app/instance.rs, instance_stop lines 1189-1195 still use the pre-loop snapshot. On the base branch there is no retry loop, so the VMM that mark_vmm_failed targets is always the one the request was sent for, and its generation matches.

[hawkw]

@claude review

[hawkw]

@claude review

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[jmpesp]

If the vmm_register call happens some time between the VMM states Creating and Starting, it's not correct to say that the Creating state represents a state where the VMM cannot be resident on the sled, right? The record will still be in state Creating for the time between when the vmm_register call is made and when the record transitions to Starting.

For this to be true for the Creating state, you'd need an additional intermediate state between Creating and Starting, and use that to mark when you're going to send the vmm_register call, like Creating -> Registering -> Starting.

Similarly, if I understand correctly, depending on when the record transitions to Cancelling, there may be a VMM on a sled that the instance-start saga removes during its unwind after it tries to transition to Starting.

[hawkw]

So, you are technically right that SagaUnwound and Destroyed are the only states here that are guaranteed to not exist, but that isn't really how this set of states is being used. Instead, it's being used to mean "the VMM may not exist and the instance should not be transitioned to Failed if the VMM is discovered to not exist". This is mainly important because it prevents the instance-watcher task from health-checking VMMs that may not yet exist.

Perhaps we should rename the whole thing to, I dunno, DUBIOUSLY_EXISTENT_STATES or something? What it really represents is the union of the states that definitely don't exist (SagaUnwound and Destroyed) and the states that represent a VMM which may be undergoing an...existence transition (Creating and Cancelling).

Note also that I've tried to preserve the unspoken naming convention¹ here, where states that are transitional are <Verb>ing while states in which a VMM may come to rest for an arbitrary amount of time are <Verb>ed.²

Footnotes

1. Possibly due to Greg? ↩

2. With the unfortunate exception of Running, which an astute reader may note should probably really be Runned, but ... well. I'm not gonna call it that. ↩

[hawkw]

Hopefully 4f8e404 makes the documentation a bit more precise about what this state actually means?

[jmpesp]

Based on the question in the previous comment, I don't think this comment is accurate

[hawkw]

Similarly, I'm hoping e0228f6 clears this up a bit.

[jgallagher]

I've been reading this all morning and feel like I'm joining you in your descent into madness. I think I understand the immediate changes here (modulo a bunch of questions below), but have very little confidence that I really get all the ramifications of the changes. I don't really know what to do about that.

[jgallagher]

Oof, sorry. This need for this warning feels like the types involved are the wrong shape. I'm sure this is orthogonal to this PR, but do you think there's a path here where this function could take a richer enum that only described valid combinations of new states, and then internally it updated all the relevant fields? (I guess ideally in that world some of the fields of InstanceRuntimeState would themselves be private, and it'd expose methods that only gave back the richer types. This might be hard/impossible; no idea.)

[hawkw]

but do you think there's a path here where this function could take a richer enum that only described valid combinations of new states, and then internally it updated all the relevant fields

Yeah, I was thinking about that myself after I got bitten by this, but I wanted to do it in a follow-up branch. Even if it isn't practical to make all the InstanceRuntimeState fields private like you suggest, we could at least have a richer enum to represent the inputs to this function, so that its API was more misuse-resistant. I'd like to do that in a subsequent change.

[jgallagher]

This seems extremely dicey - IIUC it blindly increments whatever generation is present, which at least appears to defeat the point of being a generation number. Can the caller pass in the generation number that it knows must be here? (If not, it seems pretty hard to reason about whether this method is safe / correct to call?)

I realize the warning calls this out, but I'm not really following the justification there.

[hawkw]

Sooo...we could have the caller pass in the expected prior state and have this fail if it is not that state.

However, this executes in an unwinding action, and the whole purpose of this node is to make sure the state is set to Cancelling regardless of what it previously was, in order to ensure that nothing will try to talk to the VMM that the unwinding saga is about to unregister, so that it doesn't get transitioned to Failed incorrectly. If this returned an error if the generation number was wrong and the unwinding action just bubbled that error up, the saga would get stuck, which I was really trying to avoid.

The alternative to ?ing an error in the unwinding action and getting stuck is that the unwinding saga could essentially do a compare-and-swap loop. It could try to write the generation it thinks is the correct one, and on failure, read the actual state, checks whether the VMM is already in a nonexistent state, and if it isn't, tries to do the write again with the new generation number read from the DB. That would implement the behavior which I am pretty sure is the correct one here ("advance the state to Cancelling no matter what it was previously") in what is technically a more generation-number-respecting way. But in practice, it's exactly equivalent to what I've written here, but with more steps, so I'm not sure if it's really worth it?

[jgallagher]

While I certainly appreciate long names that draw attention to unusual situations, this seems like it's describing the mechanics of how a change is going to happen rather than what change we're trying to make. Maybe something like along the lines of CancelCreating would be clearer?

[hawkw]

Yeah...this was kind of a placeholder name, but I left it in with the hopes that it would communicate the mechanics of what I was trying to do here to reviewers, and that I might give it a less silly name later. I think CancelCreating might be clearer.

At a higher level, I don't really love the current structure of this code: this is an action that we will only perform if the requested state transition is to stop the instance, but it's a possible return value from the shared select_runtime_change_action function that's called by all the state transition requests. I think there is probably a way to restructure this so that each requested state has its own way of selecting the state change action, and this variant is only returned when the requested state change is to stop the instance. But, that felt like a much bigger refactoring and I felt like it was best done after this PR to minimize the amount of existing behaviors that change here. What do you think?

[jgallagher]

At risk of yet another level of indentation, should this be

InstanceStateChangeRequest::Stop => match vmm.state {
    // ... explicit list of all the states ...
}

both to make it more obvious that all the other states are AlreadyDone and to ensure we check that logic any time we change the variants of VmmState? (An immediate question is: the docs on this say SagaUnwound "is a Failed-like state"... so why don't we also do the same thing for Failed itself?)

[hawkw]

(An immediate question is: the docs on this say SagaUnwound "is a Failed-like state"... so why don't we also do the same thing for Failed itself?)

We do do the same thing for Failed itself, it's just that this code is extremely convoluted:

omicron/nexus/src/app/instance.rs

Lines 1387 to 1422 in e0228f6

	// Failed instances may transition to Stopped by just changing
	// the Nexus state in the database to NoVmm.
	//
	// An instance's effective state will never be Failed while it
	// is linked with a VMM. If the instance has an active VMM which
	// is Failed, the instance's effective state will be Stopping,
	// rather than Failed, until an instance-update saga has
	// unlinked the Failed VMM. We can guarantee this is the case,
	// as a database CHECK constraint will not permit an instance
	// with an active Propolis ID to be Failed. Therefore, we know
	// that a Failed instance is definitely not incarnated on a
	// sled, so all we need to do to "stop" it is to update its
	// state in the database.
	InstanceState::Failed
	if matches!(
	requested,
	InstanceStateChangeRequest::Stop
	) =>
	{
	// As discussed above, this shouldn't happen, so return an
	// internal error and complain about it in the logs.
	if vmm_state.is_some() {
	return Err(Error::internal_error(
	"an instance should not be in the Failed \
	effective state if it has an active VMM",
	));
	}
	return Ok(
	InstanceStateChangeRequestAction::UpdateRuntime(
	instance_state.runtime().with_new_nexus_state(
	db::model::InstanceState::NoVmm,
	),
	),
	);
	}

Again, this is an example of how I think this code would be a lot easier to understand if we got rid of select_runtime_state_change_action and replaced it with a separate function for each state change that might be requested, as I mentioned in #11696 (comment). I'd really prefer to do that in a separate diff though, because I'm already sketched out about changing the existing behavior of unrelated branches of this...