[hawkw]

Currently stacked atop #11606 as I want to use that to write a test
for the thing I'm eventually aiming to fix here.

Unwinding instance_start and instance_migrate sagas call the
sled-agent vmm_unregister endpoint, in order to ensure that a Propolis
zone that the unwinding saga may have created is destroyed. This is
necessary because if we do not ensure any such zones that may have been
created by a failed start or migration are removed, we may leak a
running Propolis process that the control plane does not believe exists,
and which cannot be stopped normally.

While working on a fix for #11560, which involves changes to how
instance_start sagas unwind, I noticed that the current code for
ensuring a registered VMM is unregistered is a bit...sketchy. In
particular:

• The VMM is unregistered using the
  Nexus::instance_ensure_unregistered function. This looks up the
  sled-agent client, returns an error if no sled with that ID is found
  in the database, and then calls the vmm_unregister API through that
  client. If the sled record has been hard-deleted, the saga will get
  stuck (in this branch)

• Nexus::instance_ensure_unregistered function wraps the response from
  the sled-agent in a SledAgentInstanceError. That error type is used
  to detect a 404 error with the NO_SUCH_INSTANCE error code, which
  some sled-agent endpoints use to indicate that a requested Propolis
  zone no longer exists, and the instance record should be marked as
  Failed. The vmm_unregister endpoint, however, will never return
  that error code: if the requested Propolis ID was never ensured, or
  has already been destroyed, it just returns 200 OK.

  If this were not the case, however, and it did return a 404
  NO_SUCH_INSTANCE if the VMM didn't exist on the sled, we would be
  handling that error incorrectly: the sis_ensure_registered_undo undo
  action currently attempts to mark the VMM as Failed and then
  returns an error. This would be incorrect if it ever occurred,
  since we would have just been told that the VMM didn't exist on the
  sled (which is what we are trying to ensure), and we would have
  responded to it by returning an error and getting the unwinding saga
  stuck. Instead of doing that, the correct behavior would be to just
  keep unwinding in that case.

• Any other error communicating with the sled-agent is taken to
  mean that the VMM no longer exists. This is incorrect: if we
  encountered a network error or a transient "channel full" from the
  sled-agent, we would just assume the VMM is gone and continue
  unwinding, potentially leaving behind a running Propolis zone.

• The sled the VMM was on being expunged is not considered as a
  possibility by any of this code.

Instead of this, the correct behavior for ensuring a Propolis zone is
unregistered in an unwinding saga should be to retry indefinitely until
we either receive a 200 OK from the sled-agent (indicating the VMM was
either successfully destroyed or no longer existed), or until the sled
itself is no longer in service (and we can treat any VMMs that may have
been there as though they no longer existed).

This commit changes the unwinding actions to do this by adding a new
ensure_vmm_unregistered helper function in sagas::instance_common
that uses a progenitor_extras retry_operation_while_indefinitely
loop to continually retry calling the sled-agent's vmm_unregister
endpoint until it returns a 200 OK or the
sagas::sled_out_of_service_gone_check function indicates the sled is
no longer in service. If the attempt to look up the sled-agent client in
the database indicates that no record for that sled exists, we treat
that as equivalent to the sled having been expunged, rather than letting
the saga get stuck. Finally, I've removed the
Nexus::instance_ensure_unregistered function and its wrapping of the
error in SledAgentInstanceError, in favor of just calling the
sled-agent client's vmm_unregister method directly. This way, we no
longer have to handle the 404 NO_SUCH_INSTANCE error here at all,
because the sled-agent will not return it in this case.

[hawkw]

This part was never actually necessary: SledAgentInstanceError's job is mainly to detect the 404 NO_SUCH_INSTANCE error code and transition the VMM to Failed if we see it. However, vmm_ensure_unregistered never returns that error code, so trying to detect it just forces us to handle an error that shouldn't ever be received (see #11622 (comment)). Also, the way we used to try to handle it was wrong, anyway, and would have done a very incorrect thing if we did ever see that error (see #11622 (comment)). So, we can just excise all of this.

[hawkw]

Note that this obsoletes some of the change in #11622. I will probably try and pull out the refactoring to the error types from that branch into a separate PR, but I think this better accomplishes the goal of making this operation correctly retryable.

[jmpesp]

you'd be able to deduplicate the instance_id, propolis_id, and sled_id from the rest of the log lines in this file if you construct log here with those here under a o!(...).

[hawkw]

I didn't like that that would allocate, both once per each thing we had to to_string() and again to make the o! itself. It seemed unnecessary when everything here was in scope and we weren't passing the logger to anything else that might need to log in the same context. But, maybe going out of my way to avoid a few smallish String allocations here is kind of premature optimization-y of me...

[jmpesp]

Taking this out means that the saga's no longer marking VMMs as failed when this happens. Is there another bit of code that will move them to failed, or was that incorrect?

[hawkw]

This was both incorrect and unreachable, which I tried to explain in the PR description --- sorry if that was unclear.

Basically, what this code is doing is that if (and only if) the sled-agent returns the 404 error with the error code that indicates that it doesn't have a propolis zone with the requested Propolis UUID, we would mark the VMM as Failed. I think the reason we were previously doing that here was because every other place where we send a sled-agent a request to act on a VMM and receive an error saying the sled-agent doesn't know about that VMM, we mark the VMM as Failed. That's correct for other state change endpoints, but here, we are specifically trying to tell the sled-agent that we don't want it to have that VMM and that if it does have the VMM, please get rid of it. So if we had successfully gotten rid of it, or had never ensured it, this bit would fail instead of saying "okay that's good, we have successfully gotten rid of it." And, because that branch returned an error, it would get the saga stuck if it executed this node twice. So that's why this was wrong.

Luckily, though, it didn't actually matter that this code was wrong, because it was also unreachable. The sled-agent's vmm_unregister API never actually returns that error code. If we attempt to unregister a VMM ID that it doesn't know about, it just returns 200 OK idempotently. That's why we never actually had sagas getting stuck here when this node executes twice.

So, all of this was dead code, and if it wasn't dead code, it would have been incorrect, so I just got rid of all of it.

[jmpesp]

👍