oxidecomputer / oxidecomputer/omicron

Don't allow continually failing instance start/migrate sagas to block instance update sagas

Open
#6,293 2 comments 0 reactions 1 assignee View on GitHub

@hawkw is already working on this.

Since Aug 12, 2024.

enhancement nexus
Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

In this comment, @gjcolumbo points out that, since failing instance-start and instance-migrate sagas increment an instance's generation number, an instance that is continually failing to start or to migrate can prevent instance-update sagas from completing. This is because incrementing the instance's state generation number invalidates any instance-update saga that started from a previous state generation. Although failed start and migrate sagas unwinding doesn't bump the state generation, as they just transition the VMMs they create to SagaUnwound, the forward actions for those sagas do, since they set new active VMM/target VMM/migration IDs.

As Greg put it:

There's one case buried in the design doc that I'm not quite sure is covered here. I think we should file an issue for it and take care of it in a follow-up PR. The text I have in mind is

If start and migrate sagas continually fail, they can “lock out” updates by continually changing the instance’s generation number. One way to deal with this is to force an instance into a “faulted” state after several consecutive failed attempts to start or migrate it, which state can only be cleared by a successful update.

On a second reading, I'm not sure this is too big of a concern for start sagas: any start saga that's setting SagaUnwound must have tried to start a new VMM for the instance, which implies it didn't have one before, which means that the updater must not have had anything to do. But for migration I think this is still a little dangerous: repeatedly trying and failing to migrate will cause the instance's state generation to go up every time this query executes, which can keep the update saga from committing anything.

We should come up with a plan to solve this, but because we're not migrating instances right this second I think it's OK to take care of this in a follow-up PR.

Originally posted by @gjcolombo in https://github.com/oxidecomputer/omicron/pull/5749#discussion_r1702384155

At a glance, I think this would probably involve adding counts of failed start and migrate sagas to the instance record, and incrementing them when those sagas unwind, putting it in the faulted state that prevents new start/migrate sagas from starting when they get too high, and having a successful update saga clear that faulted state.

We would have to decide how the "faulted" state that prevents start/migrate sagas from starting would be implemented. As I see it, there are a few obvious options:

  • Use the values of the failure counter(s) themselves. Starting a start or migrate saga checks that the counts are below the limit, and a successful update saga clears the counters to zero, allowing new start/migrate sagas to proceed. This is probably the simplest option as it doesn't involve a complex db operation like "increment counter, check if it is over the line, and then set the state". But, it means we can't track the total number of start/migrate fields over the instance's entire lifetime, since the counters get cleared --- I don't know if we really care about that, though.

  • Represent the faulted state as an InstanceState. Alternatively, we could have an InstanceState variant that we transition the instance to when we want to block new start/migrate sagas. We could either add a new state variant or use the InstanceState::Failed for this --- I'd have to think about the implications of that.

    If we wanted to track the total number of start/migrate failures, we could never clear the counters, and instead store the number of failures at which the most recent transition to the faulted state occured in a separate field. When the faulted state transition occurs, we store the current saga failure count in that field. Subsequent faulted states are detected as the difference between the total failures and the previous count at the last transition to the faulted state.

    This solution is a bit more complex, and we'd have to work out how the faulted state interacts with other InstanceState variants...but, we probably want it to, in some way. It makes the faulted state more externally visible, if we choose to return it in external API or convert it to Failed there, which is probably either good or bad? And, it lets us have the failure counters continually go up, rather than resetting them, which we might want for debugging purposes.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.