[inventory] Add svcs enabled not online to DB by karencfv · Pull Request #10195 · oxidecomputer/omicron

karencfv · 2026-03-31T04:03:22Z

Follow up to #9977
Closes: #9516

Manual testing with no data from services

$ ./target/debug/omdb db inventory collections show latest --db-url "postgresql://root@[::1]:57066/omicron?sslmode=disable"
<...>
SMF SERVICES STATUS
    no data on SMF services has been collected
<...>

Happy system

$ ./target/debug/omdb db inventory collections show latest --db-url "postgresql://root@[::1]:57066/omicron?sslmode=disable"
<...>
SMF SERVICES STATUS
    0 SMF services enabled but not online at 2026-04-01T18:39:24.057Z
<...>

With failed services

$ ./target/debug/omdb db inventory collections show latest --db-url "postgresql://root@[::1]:57066/omicron?sslmode=disable"
<...>
SMF SERVICES STATUS
    3 SMF services enabled but not online at 2026-04-01T17:54:42.261Z
        FMRI                                ZONE       STATE       
        svc:/site/fake-service:default      global     maintenance 
        svc:/site/fake-service3:default     global     offline     
        svc:/site/fake-service4:default     global     degraded    
<...>

With a command error

$ ./target/debug/omdb db inventory collections show latest --db-url "postgresql://root@[::1]:57066/omicron?sslmode=disable"
<...>
SMF SERVICES STATUS
    failed to retrieve SMF services: command failure: Command [/usr/bin/svcs -Za -FAKE -o state,fmri,zone] executed and failed with status: exit status: 2  stdout:   stderr: /usr/bin/svcs: illegal option -- F
    Usage: svcs [-aHpv] [-o col[,col ... ]] [-R restarter] [-sS col] [-Z | -z zone ]
                [<service> ...]
           svcs -d | -D [-Hpv] [-o col[,col ... ]] [-sS col] [-Z | -z zone ]
                [<service> ...]
           svcs [-l | -L] [-Z | -z zone] <service> ...
           svcs -x [-v] [-Z | -z zone] [<service> ...]
           svcs -?
<...>

nexus/db-queries/src/db/datastore/inventory.rs

sled-agent/types/versions/src/impls/inventory.rs

jgallagher · 2026-04-08T15:42:35Z

schema/crdb/dbinit.sql

+    id UUID NOT NULL,
+    fmri TEXT NOT NULL,
+    zone TEXT NOT NULL,
+    state omicron.public.inv_svc_state_enum NOT NULL,


This maybe gets at a design issue with the types on the Rust side, but - what would it mean to have a row in a table named ..._not_online_service with the state online?

I see where you're getting at 🤔 . What about using an enum on the DB side (inv_svc_not_online_state ?) that does not include the online state?

I think not including the online state is the right call, if the intent of this is to store "not online" statuses. But we probably need to remove it on the Rust side too, right? For a couple reasons: the same question applies there (what would it mean for a SvcsEnabledNotOnline to contain a service in the state SvcState::Online), and what would you do at runtime if you had that and needed to store it in the db?

That does make the type names awkward; it probably needs to be something like SvcState becomes NotOnlineSvcState and Svc becomes NotOnlineSvc? I don't love that but it does seem like we probably don't want the generic-sounding Svc and SvcState types to not be able to represent online services.

Ha! Looks like this was already on my mind 😄
https://github.com/oxidecomputer/omicron/blob/main/sled-agent/types/versions/src/modify_services_in_inventory/inventory.rs#L56-L58

In the end I decided to keep the SvcState enum for the initial parse() method. It seemed just wrong not have a complete enum of all of the possible states as part of the svcs module. I did add additional enums SvcEnabledNotOnlineState and InvSvcEnabledNotOnlineState for the "enabled not online" filtering functionality.

Thanks, I think that makes perfect sense. 👍

schema/crdb/dbinit.sql

nexus/db-model/src/inventory.rs

nexus/db-queries/src/db/datastore/inventory.rs

nexus/inventory/src/examples.rs

schema/crdb/dbinit.sql

nexus/db-model/src/inventory.rs

karencfv

Thanks for the review! I think I've addressed all of your comments @jgallagher

nexus/db-queries/src/db/datastore/inventory.rs

nexus/db-model/src/inventory.rs

nexus/db-queries/src/db/datastore/inventory.rs

schema/crdb/dbinit.sql

karencfv · 2026-04-08T22:59:34Z

schema/crdb/dbinit.sql

+    id UUID NOT NULL,
+    fmri TEXT NOT NULL,
+    zone TEXT NOT NULL,
+    state omicron.public.inv_svc_state_enum NOT NULL,


I see where you're getting at 🤔 . What about using an enum on the DB side (inv_svc_not_online_state ?) that does not include the online state?

sled-agent/types/versions/src/impls/inventory.rs

sled-agent/health-monitor/src/health_checks.rs

jgallagher · 2026-04-09T14:14:27Z

Changes look good - just one nit with the error string conversion, and then the somewhat-painful "what do we do about online services" question in #10195 (comment).

jgallagher · 2026-04-13T13:16:04Z

sled-agent/types/versions/src/impls/inventory.rs

+            "offline" => SvcEnabledNotOnlineState::Offline,
+            "degraded" => SvcEnabledNotOnlineState::Degraded,
+            "maintenance" => SvcEnabledNotOnlineState::Maintenance,
+            _ => SvcEnabledNotOnlineState::Unknown,


This doesn't seem right - I could call SvcEnabledNotOnlineState::from("sled 23") and get back SvcEnabledNotOnlineState::Unknown, which I guess it kind of is an unknown state but it doesn't seem like we should infallibly convert any string at all into a service state. I'm also not sure we should have an Unknown state variant. It looks like the only use of this is converting from SvcState::Unknown. I think SvcState::Unknown has the same problems: we shouldn't have a blanket From<&str> impl for it, and I'm not sure we actually need an Unknown variant. The only use of that I'm seeing is inside SvcsResult::parse(), where if we get Unknown, we skip the state and don't include it in the result at all.

Could we do this instead:

Remove both From<&str> impls

Remove both ::Unknown variants

Put a private parse_svc_state(&str) -> Option<SvcState> inside illumos-utils/src/svcs.rs, and use None where we're currently using SvcState::Unknown to skip invalid states

?

jgallagher · 2026-04-13T14:23:19Z

nexus/db-queries/src/db/datastore/inventory.rs

+        let mut svcs_enabled_not_online_errors = Vec::new();
+        for sled_agent in &collection.sled_agents {
+            match &sled_agent.smf_services_enabled_not_online {
+                SvcsEnabledNotOnlineResult::SvcsEnabledNotOnline(svcs) => {


Tiny nit - the comments on lines 233 and 244 are the same and maybe not quite right. I wonder if it would be more future-proof to destructure svcs here:

let SvcsEnabledNotOnline { services, errors, time_of_status, } = svcs;

and then we wouldn't really need the comments as much - we'd be forced to use all the fields to avoid warnings?

karencfv added 4 commits March 30, 2026 20:57

create types and diesel tables

6802b3c

add schema changes

8c6d126

display

4ef14e9

add some testing

6814a3f

karencfv commented Apr 1, 2026

View reviewed changes

nexus/db-queries/src/db/datastore/inventory.rs Outdated Show resolved Hide resolved

karencfv marked this pull request as ready for review April 1, 2026 18:40

karencfv requested review from davepacheco and jgallagher April 1, 2026 18:40

jgallagher reviewed Apr 8, 2026

View reviewed changes

karencfv added 6 commits April 9, 2026 09:15

merge main

dd8b467

Adress comments

2e1be88

change how command errors are handled

d8cdc9f

simplify collection insertion

6fc6ab4

collection result checks

bb0b5f2

rename table

1d03292

karencfv commented Apr 9, 2026

View reviewed changes

clean up

7b78992

jgallagher reviewed Apr 9, 2026

View reviewed changes

sled-agent/health-monitor/src/health_checks.rs Outdated Show resolved Hide resolved

karencfv added 3 commits April 10, 2026 09:30

Generate testing file

4e52231

change state enum

8101090

clean up

c79398a

karencfv requested a review from jgallagher April 10, 2026 03:21

karencfv added 3 commits April 10, 2026 15:29

merge main

1c8fe02

generate openAPI spec

b140109

clean up naming

5a60d3a

jgallagher reviewed Apr 13, 2026

View reviewed changes

Conversation

karencfv commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

karencfv Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jgallagher commented Apr 9, 2026

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

karencfv commented Mar 31, 2026 •

edited

Loading

karencfv Apr 10, 2026 •

edited

Loading