oxidecomputer / oxidecomputer/omicron
Don't allow continually failing instance start/migrate sagas to block instance update sagas
@hawkw is already working on this.
Since Aug 12, 2024.
- 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
SagaUnwoundmust 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 anInstanceStatevariant 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 theInstanceState::Failedfor 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
InstanceStatevariants...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 toFailedthere, 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.