[sled agent] Add "rebooting" as enum variant, update sled agent state-handling by smklein · Pull Request #88 · oxidecomputer/omicron

smklein · 2021-05-12T22:09:15Z

This patch consists of two major changes: (1) Updating the API for instance state, and (2) a refactor of the sled agent's state-handling logic.

Updating the API for the instance state:

BEFORE:

ApiInstanceState was separate from the concept of "rebooting" - reboot_in_progress was a separate bool, which means it could be toggled regardless of state. This usage of types presented a slightly imprecise view of the world: rebooting as a concept only makes sense within the "stopped" and "stopping" states.

AFTER:

reboot_in_progress removed from ApiInstanceRuntimeState, replaced by an enum variant rebooting within the Stopped and Stopping varaints.
reboot_wanted removed from ApiInstanceRuntimeStateRequested, replaced by ApiInstanceStateRequested::Reboot.

These changes make it more difficult for clients to make requests which are invalid (such as requesting a change to "failed", or requesting a change to "destroyed + reboot_wanted").

Note on the Database...

This aforementioned change has a trickle-effect throughout storage, translation, etc, as it modifies a structure stored within CockroachDB.

Changes necessary to update a struct stored in the DB...

[model.rs] Change the structure itself (add rebooting: bool as an enum variant). This required...
[dbinit.sql] Update the database schema.
[schema.rs] Update the database schema.
serialization:
- [conversions.rs] Add SqlSerialize implementation for ApiInstanceState.
- [conversions.rs] Update SqlSerialize implementation for ApiInstanceRuntimeState, which itself contains ApiInstanceState.
- [datastore.rs] Update storage of ApiInstanceState within the SqlValueSet within project_delete_instance.
deserialization:
- [model.rs] Remove try_from(&str) -> Result<ApiInstanceState> which relied on deserialization from JSON, as it is not intended to be stored as a JSON string.
- [model.rs] Add try_from((&str, bool)) -> Result<ApiInstanceState>, which instead reconstructs the enum from a (variant, rebooting) tuple.
- [model_db.rs] Add try_from(tokio_postgres::Row) -> Result<ApiInstanceState>, which itself invokes the (&str, bool) variant after extracting values from the SQL row.
- [model_db.rs] Update try_from(tokio_postgres::Row) -> Result<ApiInstanceRuntimeState>, which itself relied on the deserialization of the instance state.

(anyway, here's footage of me making all these changes, still insisting that this is a small and reasonable update)

Refactoring the Sled Agent's state-handling Logic

This mostly impacted next_state_for_new_target. In effect, rather than using multiple matches of conditional logic to try to match all cases, this updates the format to an actual match statement, to make state transitions a bit more obvious.

… impl

davepacheco

Thanks for doing this! When you started working on this, I didn't anticipate how soon you'd be diving into the half-baked abstractions in the database code you'd be diving into, or I'd have documented it better and maybe suggested talking through some of it. Sorry about that, and I hope that wasn't too painful!

davepacheco · 2021-05-13T16:24:59Z

omicron-common/src/model.rs

+    ) -> Result<Self, Self::Error> {
+        println!("ApiInstanceState::try_from: ({}, {})", variant, rebooting);
+        let r = match variant {
+            "creating" => ApiInstanceState::Creating,


What do you think about making the the "rebooting" column in the database a nullable boolean? That seems to me a more precise analog to the Rust data type here, where there's no boolean value present in many of the variants.

This would have some small impacts in a bunch of places, but I think those changes would be clarifying, too. In this function, you'd be accepting an Option<bool>, and we could validate that it's None if the variant is not Stopping or Stopped, similar to what we do for the attached instance id in ApiDiskState. Another example: it might make more sense for is_rebooting() to return Option<bool> (because it seems weird to me to say that a Destroyed instance is not rebooting -- I'd expect kind of a "mu" answer). That one depends on the callers and what they tend to care about, though.

Done; this involved the following:

removing the NOT NULL constraint

updating all sql_row_value invocations to explicitly return an Option<bool>

updating the try_from impl to act on an optional bool

This does mean that SqlSerialize is now doing some shenanigans to try to ensure this field is truly none if we're in any non-stopping states, which means injecting None into the SqlValueSet.

davepacheco · 2021-05-13T16:32:48Z

omicron-common/src/model.rs

+    fn try_from(
+        (variant, rebooting): (&str, bool),
+    ) -> Result<Self, Self::Error> {
+        println!("ApiInstanceState::try_from: ({}, {})", variant, rebooting);


This looks like a debug println left in.

davepacheco · 2021-05-13T16:48:53Z

omicron-common/src/sql/dbinit.sql

    -- instance_state omicron.public.InstanceState NOT NULL, // TODO see above
    instance_state TEXT NOT NULL,
-    reboot_in_progress BOOL NOT NULL,
+    rebooting BOOL NOT NULL,


Was this name change semantically meaningful? (This is fine -- they seem equivalent to me -- I'm just curious if by changing this you intended some distinction that I'm missing.)

ahhhh no, this was just a weird by-product of my progression, which was...

Add the "rebooting" variant to "instance_state" (which I thought could still be converted to TEXT via json)

Remove the separate "reboot_in_progress" variable (which meant deleting it here)

Realize that I should not be storing the instance object as JSON, so add this auxiliary data back here (as rebooting).

TBH, I actually think instance_state_rebooting ends up being clearer, since it makes it more obvious that it's associated with the instance_state object. Updating now.

(I left instance_state as-is, but I'd actually be willing to change it to instance_state_variant...)

davepacheco · 2021-05-13T16:56:19Z

omicron-nexus/src/db/datastore.rs

+        let p1 = cond_sql.next_param(&stopped);
+        let failed = ApiInstanceState::Failed.to_string();
+        let p2 = cond_sql.next_param(&failed);
        cond_sql.push_str(&format!("instance_state in ({}, {})", p1, p2));


This feels kind of weirder now but I'm not sure if we should do anything about it or what we would do about it.

What's weird is that at L394 we're constructing a value for Stopped {rebooting: false} but at L396 we're expecting it to match equal to rows that would really be represented as Stopped { rebooting: true }.

I guess a more precise translation from the Rust type would be something like this:

(instance_state, run_state) IN (("stopped", false), ("stopped", true), ("failed", NULL))

I'm not sure you can do that in SQL per se, but I think the meaning is clear. You might have to decompose that into a more complex boolean expression. (This also assumes we made "rebooting" a nullable column.)

The question here is: what level of abstraction are these functions operating at? The answer in the new code is that they're pretty close-to-the-SQL: they know the underlying representation of a type like ApiInstanceState, and the code is within its rights to say: "I know this Rust type maps to two SQL columns, but I only care about the instance_state part, so I'm going to construct a variant with a dummy "rebooting" value in order to get its instance_state part [and throw away the other part]".

The other answer would be to say: these functions aren't allowed to assume knowledge of the SQL representation and are instead expected to rely on the various traits to serialize values to/from SQL. This was my original thought here. But looking at this example, it seems like that would require quite a lot more work, and I'm not sure what the abstraction would look like.

Anyway, I don't think there's much actionable here. This just seemed like a bigger change in abstraction than it looks and worth thinking about for the future.

Yeah, it's a good point (I'll at least include a comment to make this more explicit).

(instance_state, run_state) IN (("stopped", false), ("stopped", true), ("failed", NULL))

I'm hesitant with this example - if the stored value was an integer, not a bool, I wouldn't want to exhaustively list all integers! The truth is just that I simply don't care about it here - I want an analog for "this enum matches this variant, but we can discard the content in this context".

davepacheco · 2021-05-13T17:11:05Z

omicron-nexus/src/nexus.rs

        /*
         * To implement reboot, we issue a call to the sled agent to set a
-         * runtime state with "reboot_wanted".  We cannot simply stop the
+         * runtime state with "rebooting = true".  We cannot simply stop the


I think this should be updated to say that we're setting the runtime state to "Reboot" (not "with "rebooting = true"")?

davepacheco · 2021-05-13T17:12:18Z

omicron-nexus/src/nexus.rs

         * we might leave it stopped.
         *
-         * When an instance is rebooted, the "reboot_in_progress" remains set on
+         * When an instance is rebooted, the "rebooting" remains set on


'the "rebooting" flag', maybe?

davepacheco · 2021-05-13T17:15:26Z

omicron-sled-agent/src/sim/collection.rs

+    }
+
+    /**
+     * Tests SimObject in general when backed by a SimDisk in particular, for


Should that say "SimInstance in particular"? (I think this was there before your change but I just noticed it.)

Ah, yeah, bad comment migration from L419 in the old version. I'll just re-phrase this entirely.

smklein added 14 commits May 8, 2021 16:37

Sled Agent refactor: Move simulated impl, make room for non-simulated…

32ac278

… impl

Oops, that README was a little *too* updated. Roll it back

4479130

whitespace

a813bbe

Split InstanceState from InstanceStateRequested

6e4e50b

Format

0b1cf73

WIP

8a87e94

Update dockerfile

608254b

Merge branch 'sa-refactor' into ensure-api

3f3a9df

Fix tests

6027ad6

Merge

e4df39e

Add 'rebooting' requestable state, move rebooting flag into state enum

e901c89

Omg the tests pass

6dabce4

Less hacky bits

72aa7f0

Sacrifice an '.into()' to appease Clippy

937a2ef

smklein mentioned this pull request May 12, 2021

[sled agent] Split "ApiInstanceState" from "ApiInstanceStateRequested" #87

Merged

smklein requested a review from davepacheco May 12, 2021 22:51

smklein changed the title ~~[sled agent] Add "rebooting" indicator under a subset of states~~ [sled agent] Add "rebooting" as enum variant, update sled agent state-handling May 12, 2021

smklein added 2 commits May 12, 2021 20:07

Implement Copy on the instance state objects (credit to bnaecker)

aae6104

Merge branch 'main' into sa-refactor

8f1f4ca

davepacheco reviewed May 13, 2021

View reviewed changes

smklein added 5 commits May 14, 2021 17:27

Merge branch 'main' into sa-refactor

d27183c

Merge branch 'sa-refactor' into ensure-api

0638198

merge

624a027

Code review feedback

b173745

Format

45d6544

Base automatically changed from ensure-api to main May 14, 2021 23:04

smklein added 4 commits May 17, 2021 09:57

Merge branch 'main' into sa-refactor

3e513d0

Merge branch 'sa-refactor' into ensure-api

a128b08

Merge branch 'ensure-api' into reboot-states

dc299b9

Merge branch 'main' into reboot-states

231a2b7

smklein merged commit c5f23b7 into main May 25, 2021

smklein deleted the reboot-states branch May 25, 2021 15:34

david-crespo mentioned this pull request Jun 7, 2021

Problem with structs in ApiInstanceState enum #117

Closed

smklein mentioned this pull request Jun 8, 2021

ApiInstanceState: Make a separate state for "rebooting". #118

Merged

citrus-it added a commit to citrus-it/omicron that referenced this pull request Oct 24, 2025

image: reduce recovery image content (oxidecomputer#88)

3f03882

Conversation

smklein commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updating the API for the instance state:

Note on the Database...

Refactoring the Sled Agent's state-handling Logic

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

smklein commented May 12, 2021 •

edited

Loading