[hawkw]

Okay, so basically, all of this code is bad, and I should feel bad about
it, but this PR makes some of it a bit better at the cost of changing a
lot of code all at once.

record scratch You're probably wondering how I got here. Basically, it
turns out that the undo actions for the sis_ensure_registered and
sim_ensure_destination_propolis nodes in the instance-start and
instance-migrate sagas, respectively, are both a bit wrong. Both of
these nodes' forward actions are responsible for sending a vmm_ensure
request to a sled-agent, to start the Propolis zone to start an instance
or for the migration destination of an instance, respectively. When
these sagas unwind, they must ensure that the corresponding VMM is
unregistered --- i.e. that the sled-agent is told to forcefully
terminate that Propolis zone --- since the control plane does not
believe that an instance is running there. If we fail to ensure that the
zone is unregistered when these sagas unwind, we run the risk of
"leaking" a Propolis zone which the control plane would believe does not
exist but which may believe itself to be running the instance (if the
start saga got far enough along before unwinding).

Presently, the unwinding actions for these nodes are wrong in several
ways. The first such way (which is not addressed in this PR) is that
they really should run regardless of whether or not the forward action
succeded: if the forward action fails and we start unwinding there, we
may still have sent an vmm_ensure request to the sled-agent, and the
VMM may still exist. That's part of what leads to the bug described in
#11560, and I had started on trying to fix this in #11608 when I
stumbled over all the other stuff that's wrong here.

The other things that are wrong are:

1. When an error is returned by the sled-agent that affirmatively
   indicates the VMM does NOT exist on that sled (i.e. a 404 with the
   error code NO_SUCH_INSTANCE), the unwinding action will...mark the
   VMM record as Failed, and then return an error and get the saga
   stuck. This is a bit weird of us to do here. While we generally
   do handle these 404 NO_SUCH_INSTANCE errors by failing the VMM, we
   probably don't want to here: that error means the Propolis zone
   definitely doesn't exist, which is precisely what we were trying
   to ensure in the first place. And, indeed, the else branch, which
   would execute on any error except for those which positively
   indicates the Propolis zone no longer exists, logs "instance already
   unregistered from sled", which is actually what the case where we
   do get the NO_SUCH_INSTANCE error represents. So this is
   basically doing the exat opposite of what it's clearly intended to
   do.¹

2. If we encounter a transient network error here, we incorrectly assume
   that it means the instance has been unregistered, as described in
   (1), and therefore do not retry. This means we will continue
   unwinding, leaving behind the VMM which we had meant to unregister,
   if we just so happen to hit a network blip while talking to the
   sled-agent.

3. On the other hand, if we encounter a (potentially equally transient)
   network error while talking to the database, either while looking
   up the VMM record in order to stop it (which is what lands us
   here), or while we are attempting to mark the VMM record as
   failed (which, as I mentioned above, we shouldn't even be doing),
   we will return an action_failed error and get the saga permanently
   stuck. Which we shouldn't do.

4. Finally, none of this code handles the fact that the sled may have
   been expunged while we were trying to do all of that, which should
   probably be treated the same as the VMM no longer existing.

Whew, okay. That was a lot. Instead of all of that nonsense, what the
undo actions really should be doing is:

• Try to tell the sled-agent to unregister the VMM.
• If that succeeds, great. We can return immediately Ok(()) and
  continue unwinding.
• If the sled-agent returns a 404 NO_SUCH_INSTANCE error, that means
  that the VMM has definitely been destroyed, either because of a
  previous idempotent execution of the undo action, because we never
  actually ensured it in the first place, or because the whole sled has
  restarted. In that case, we can return Ok(()) immediately and
  continue unwinding. We do not need to mark the VMM as Failed: in
  that case: it will be marked as SagaUnwound by a later unwinding
  action.
• If we cannot communicate with the sled-agent, we should check to see
  if the sled has been expunged. If the sled is expunged, we can safely
  assume that the VMM is gone, and return Ok(()) immediately and
  continue unwinding (you know the drill). If the sled has not been
  expunged, we must retry forever.
• If the sled record in the database has been deleted wholesale, well,
  that's weird and probably shouldn't have happened, but we can
  probably also assume that means the VMM is gone, and return Ok(())
  immediately and continue unwinding (and only feel a little bit bad
  about having done so).
• Any other error should be retried forever until the VMM is confirmed
  gone by one of the above means. We should never return an error here,
  since that gets the whole saga stuck and requires manual intervention,
  which has almost never been the correct thing for any saga to do under
  any circumstances.

Okay. So having determined that, I set out to try to make it do that.
While trying to do that , I realized that none of the other code
that will send a request to the sled-agent to operate on the instance
ever checks if the sled is expunged, either.² Upon discovering this,
I figured that we should probably make all the code that tries to
operate on a VMM handle the possibility of the sled having been expunged
similarly to the "VMM definitely gone" 404 error case, which is what
ended up turning this into such a large change.

This branch, therefore, makes the following changes:

1. Turns SledAgentInstanceError, which is currently just a little
   wrapper around a progenitor_client::Error from the
   sled-agent-client that we use to special-case the 404
   NO_SUCH_INSTANCE error, into an enum of either VmmGone,
   SledNoLongerInService, or "other".

2. Added a (somewhat awkwardly-shaped) SledAgentInstanceError::new.
   This checks if the error is an error response from the sled-agent
   indicating the VMM is gone (as we did previously). In addition, if
   the error is a communication error, it also checks if the sled has
   been expunged by consulting the database. Eventually, I would also
   kinda like to hang the "check if the sled is still in A0" code from
   [nexus] fail instances if the sled they're on isn't...on #10334 off of this type instead of making that something that only
   the instance_watcher background task knows how to do, but I'd
   rather do that in a follow-up change.

   This is kinda why we don't just use the progenitor-extras
GoneCheck stuff here, because we also want to keep the current
   handling of the NO_SUCH_INSTANCE error and the associated logic for
   constructing VmmFailureReasons from it.

3. Updated all the call sites that use SledAgentInstanceError to use
   the new thing, which is why this diff is so huge. Sorry.

4. Finally, once all that was done, added a new
   undo_instance_ensure_registered helper in sagas::instance_common
   for actually doing the ensure-unregistered undo action. This has a a
   retry loop that actually does the correct thing (as I described
   above) instead of the kind of fractally-wrong thing that the
   instance-start saga's version of this used to do. Now, both sagas
   just call this, and it does the right thing.

5. Return Ok(()) immediately and continue unwinding.

Footnotes

1. Incidentally, this is one of the only pieces of this whole mess
   that is not entirely my fault: although the code in question was
   last touched by yours truly, there is a comment above it
   explaining the rationale for the incorrect behavior. Although
   this comment is also both incorrect in describing the code's
   behavior and what it asserts that the code does would be
   incorrect if we did do that, it is exceptionally well written in
   a way where long-time fans of the show will immediately realize
   we have just stumbled across a @gjcolumbo cameo appearance (hi
   Greg!). ↩

2. With the exception of the instance-watcher background task...but
   it only checks if the sled has been expunged before trying to
   talk to the sled-agent, and doesn't check again upon encountering
   a communication error. Sigh. ↩

[hawkw]

Okay, so the missing piece here — and the reason we've not actually seen unwinding start sagas get stuck on the unregister action in practice — is that vmm_unregister always returns 200 OK for unknown VMM IDs, making it idemoptent. So, even though we were handling the NO_SUCH_INSTANCE branch in a completely incorrect way, that endpoint would never return it. Thus, I think we should really just change that bit by not wrapping that function's error in a SledAgentInstanceError, and use the progenitor-extra GoneCheck stuff there. I think the rest of the error refactoring is still worth doing independently, but we shouldn't actually see any of it in the unregistered action.

[hawkw]

Thus, I think we should really just change that bit by not wrapping that function's error in a SledAgentInstanceError, and use the progenitor-extra GoneCheck stuff there. I think the rest of the error refactoring is still worth doing independently, but we shouldn't actually see any of it in the unregistered action.

#11629 does this part. Proper commit message there to follow. Then, I'll figure out how to nicely base this whole stack of changes atop that.