Upstairs state machine refactoring (3/3) #1577

mkeeter · 2024-11-26T22:46:31Z

(staged on top of #1570)

This PR refactors the DsState type into more specific, data-bearing types. Here's a quick look at the new types:

pub enum DsState {
    /// New connection
    Connecting {
        state: NegotiationState,
        mode: ConnectionMode,
    },

    /// Ready for and/or currently receiving IO
    Active,

    /// This downstairs is undergoing LiveRepair
    LiveRepair,

    /// The IO task for the client is being stopped
    Stopping(ClientStopReason),
}

pub enum ConnectionMode {
    Offline,
    New,
    Faulted,
    Replaced,
}

pub enum NegotiationState {
    /// Initial state, waiting to hear `YesItsMe` from the client
    ///
    /// Once this message is heard, transitions to either `WaitActive` (if
    /// `auto_promote` is `false`) or `WaitQuorum` (if `auto_promote` is `true`)
    Start {
        auto_promote: bool,
    },

    /// Waiting for activation by the guest
    WaitActive,

    /// Waiting for the minimum number of downstairs to be present.
    WaitQuorum,

    WaitForPromote,
    WaitForRegionInfo,
    GetLastFlush,
    GetExtentVersions,

    /// Initial startup, downstairs are repairing from each other.
    Reconcile,

    /// Waiting for live-repair to begin
    LiveRepairReady,
}

Many data members of the DownstairsClient – which were only valid for a single state – are now data within a specific variant, e.g. auto_promote and negotiation_state. This makes the code both more precise and harder to mess up.

+400 of this change is updating the OpenAPI spec, which includes DsState. I don't think anyone else is expecting it to be a particular shape, but let me know if I'm wrong (also, it already changed in #1570, so warn me there as well!).

Remaining work:

Answer XXX questions, which are places that I wasn't sure about existing behavior
Figure out what to do about checked_state_transition, which I just deleted for the moment.
- Manually specifying valid state transitions and manually writing a bunch of good + bad test cases is a lot of toil; could we be smarter about this?
- How do we handle nested state machines (negotiation_state within DsState::Connecting?)
Update state machine diagrams

leftwo

So far, just questions to help me understand.
I reviewed up to is_state_transition_valid() in upstairs/src/client.rs
I wanted to post this much so it would be recorded and I'll continue and either
post more later, or post that I'm done.

leftwo · 2024-12-14T01:08:53Z

cmon/src/main.rs

-        DsState::WaitActive => "WAC".to_string(),
-        DsState::WaitQuorum => "WAQ".to_string(),
-        DsState::Reconcile => "REC".to_string(),
+        DsState::Connecting {


I was thinking about how to present this, but what you have done here is fine for now and if I come up with something I like better, I'll put out a PR for it

leftwo · 2024-12-24T17:38:04Z

upstairs/src/client.rs

-        self.state = DsState::Reconcile;
+        info!(self.log, "Transition from {:?} to Reconcile", self.state);
+        let DsState::Connecting { state, mode } = &mut self.state else {
+            panic!("invalid state {:?}", self.state);


This was there before, but could we add the client ID to the panic string here?
Sometimes the log right before a panic does not make it out, and it would be nice to
know which client was in the invalid state.

leftwo · 2024-12-24T18:13:09Z

upstairs/src/client.rs

+                // Otherwise, use live-repair; `ConnectionMode::Replaced`
+                // indicates that the address is allowed to change.
+                UpstairsState::Active | UpstairsState::Deactivating { .. } => {
+                    ConnectionMode::Replaced


If we are deactivating here, could we just go to New? Or we need the replaced state to let ourselves know when we restart this client that we should expect a new downstairs?

leftwo · 2024-12-24T18:54:51Z

upstairs/src/client.rs

-            DsState::Active | DsState::Offline if !can_replay => {
-                Some(DsState::Faulted)
-            }
+        let new_mode = match current {


This is still tough to track what happens here. I don't know how to make it
any easier to follow though. The new states do help somewhat, but we still
have the can_replay adding into the layers here.

leftwo · 2024-12-24T18:58:01Z

upstairs/src/client.rs

-            DsState::Active => Some(DsState::Offline),
+            // If the Downstairs has spontaneously stopped, we will attempt to
+            // replay jobs when reconnecting
+            DsState::Active => ConnectionMode::Offline,


I might put this right after line 553 with a comment like you have on line 552 about that covering the other cases for DsState::Connecting. This way you have all the cases covered for Active and Connecting in the same area. I was initially confused when I saw DsState::Active again here after seeing it at line 544.

leftwo · 2024-12-24T19:25:52Z

upstairs/src/client.rs

-            DsState::Stopping(ClientStopReason::Replacing) => {
-                Some(DsState::Replaced)
+                // XXX NegotiationFailed could also be hit during reconnection,
+                // in which case we shouldn't use `ConnectionMode::New` (?)


I guess the question is, what path would we take that is different if we see NegotiationFailed on initial startup (before we are active) vs. seeing it after we are active.

Looking at all the reasons we might set NegotiationFailed, they appear to be something that will fail if we try again (version mismatch), or receiving some message out of expected order (programmer bug??). If we go to New for this client, as long as we have to go back through the negotiation again I think our downstairs will end up in the right place.

leftwo · 2024-12-24T19:54:25Z

upstairs/src/client.rs

@@ -920,9 +869,10 @@ impl DownstairsClient {
    ) -> EnqueueResult {
        match self.state {
            // We never send jobs if we're in certain inactive states
-            DsState::Faulted
-            | DsState::Replaced
-            | DsState::LiveRepairReady


Where does having a downstairs DsState::Connecting with state: NegotiationState::LiveRepairReady end up? In the panic below?

leftwo · 2024-12-24T19:56:41Z

upstairs/src/client.rs

-                    | DsState::Offline => {} // Okay
-
-                    DsState::LiveRepairReady if self.cfg.read_only => {} // Okay
+                "invalid state transition from {:?} -> {:?} \


We should keep the client ID and upstairs ID in the panic message. And, maybe even add the session_id, as a propolis instance with multiple Upstairs could panic here and we might have trouble figuring out which piece hit this panic.

mkeeter requested review from faithanalog, jmpesp and leftwo November 26, 2024 22:46

mkeeter force-pushed the mkeeter/lean-mean-state-machine branch from 79652b6 to 93a6788 Compare December 2, 2024 14:44

mkeeter changed the title ~~Upstairs state machine refactoring~~ Upstairs state machine refactoring (3/3) Dec 2, 2024

mkeeter force-pushed the mkeeter/lean-mean-state-machine branch 3 times, most recently from d23dd68 to 3835f2a Compare December 9, 2024 15:09

mkeeter mentioned this pull request Dec 9, 2024

Add early rejection of IOs if too many Downstairs are inactive #1565

Open

mkeeter force-pushed the mkeeter/lean-mean-state-machine branch from 3835f2a to 43a1e30 Compare December 10, 2024 16:39

mkeeter changed the base branch from main to mkeeter/explicit-stop-state December 11, 2024 14:53

mkeeter mentioned this pull request Dec 11, 2024

Add explicit Stopping state (2/3) #1570

Open

mkeeter force-pushed the mkeeter/lean-mean-state-machine branch 2 times, most recently from 29ee56a to 448a57a Compare December 11, 2024 15:44

mkeeter added 16 commits December 11, 2024 11:03

The Grand Refactoring of DsState

93af30b

Remove needs_replay in favor of a returned value

671ab54

All tests passing

28df820

Update OpenAPI stuff

7a7a60e

Fix cmon

44ce475

All tests passing!

280c357

Allow reconciliation after replacement

2390a68

Fix valid replacement states for other DS

75d5c15

Remove duplicate ClientStopReason in ClientRunResult

26be47d

Correctly skip jobs on reinitialize

ef7bb0c

Cleaning up reinitialize

092a86f

Update comments

fc334f3

Cleaner ConnectionMode handling during replacement

1b633d6

Update OpenAPI docs

9a1ff37

Fix notify-nexus code

b27943b

Don't autopromote when a client is disabled

3ad1361

Concerns!

958627d

mkeeter force-pushed the mkeeter/lean-mean-state-machine branch from 448a57a to 958627d Compare December 11, 2024 16:03

Reimplement checked_state_transition

a1b9eee

mkeeter marked this pull request as ready for review December 12, 2024 18:03

mkeeter added 3 commits December 12, 2024 14:28

Add RUST_BACKTRACE=1

a033afb

Allow Connecting -> Connecting transition

6a1bde5

Allow Replacing -> New with auto_promote = true

26e7d50

leftwo reviewed Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstairs state machine refactoring (3/3) #1577

Upstairs state machine refactoring (3/3) #1577

mkeeter commented Nov 26, 2024 •

edited

Loading

leftwo left a comment

leftwo Dec 14, 2024

leftwo Dec 24, 2024

leftwo Dec 24, 2024

leftwo Dec 24, 2024

leftwo Dec 24, 2024

leftwo Dec 24, 2024

leftwo Dec 24, 2024

leftwo Dec 24, 2024

Upstairs state machine refactoring (3/3) #1577

Are you sure you want to change the base?

Upstairs state machine refactoring (3/3) #1577

Conversation

mkeeter commented Nov 26, 2024 • edited Loading

Remaining work:

leftwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeter commented Nov 26, 2024 •

edited

Loading