[update-status] Handle svcs in-transition, unrecognized states and report in-maintenance only

[karencfv]

svcs has two additional "states" that are not in the list of official states for SMF service instances. From man svcs

Absent or unrecognized states are denoted by a question mark
(?) character. An asterisk (*) is appended for instances in
transition, unless the NSTA or NSTATE column is also being
displayed.

This PR adds two additional variants to SvcState (and related parsing) to account for these additional "states": Unrecognized and InTransition. Additionally, only the Unrecognized variant is added to SvcEnabledNotOnlineState. An unrecognised state should raise an alarm, whereas an "in-transition" state should not.

This should probably be merged until the release branch is cut? It's be nice to give this a little bit of time on the dogfood rack.

Fixes: #10997

[karencfv]

This PR looks a bit big, but it really isn't. Most of it is just API migration code.

[davepacheco]

This seems okay for what we're trying to do with this consumer, but it doesn't seem great for a general-purpose layer because it doesn't tell you anything about what state it's currently in or what it's going to. It means you lose all information about its state while it's transitioning.

I believe that at the SMF layer (in the database, visible with svcprop), this information is exposed as state and next_state, and the asterisk gets appended if next_state is not NULL:

$ svcprop -p restarter ssh
restarter/logfile astring /var/svc/log/network-ssh:default.log
restarter/contract count 71
restarter/start_pid count 432
restarter/start_method_timestamp time 1780716452.355913000
restarter/start_method_waitstatus integer 0
restarter/auxiliary_state astring dependencies_satisfied
restarter/next_state astring none
restarter/state astring online
restarter/state_timestamp time 1780716452.357390000

We could similarly expose both here. Or we could add the current and next state to the Transitioning variant?

Or maybe it would also be okay to simply ignore the asterisk? On the grounds that if it's offline*, then it is offline, even though it's transitioning. But that seems likely to lead to false positives while things are starting up. I think it matters to our consumer whether something is offline or offline* because the first is a problem and the second isn't.

Or might we also have false positives today if something is offline and not transitioning yet because its dependencies are still being started? In which case we just need to treat this at a higher level as transient. Or report the state_timestamp too, and only consider something broken if its in one of our broken states and its state hasn't changed recently (as a form of hysteresis)?

As I write that, I wonder if we're going to keep playing whack-a-mole with false positives unless we do something like that.

[karencfv]

Or might we also have false positives today if something is offline and not transitioning yet because its dependencies are still being started? In which case we just need to treat this at a higher level as transient. Or report the state_timestamp too, and only consider something broken if its in one of our broken states and its state hasn't changed recently (as a form of hysteresis)?

At this point it's starting to feel like we should completely port the svcs -x logic with a few tweaks for our specific use case using scuffle to directly interact with the property groups. Then we wouldn't have to shell out to svcs and do our best to wrangle the output. I'm thinking that this could also be useful for the FM project.

From what I can understand from the explain.c file is that the only time it cares whether there is a state in transition is in the OFFLINE case https://github.com/illumos/illumos-gate/blob/043d968df0a5ae9a27ccc0e22250576f37ecc044/usr/src/cmd/svc/svcs/explain.c#L1824-L1839 . And even there it only cares whether it's starting, otherwise it just calls it DC_TRANSITION regardless of what state it's going to. This is close to what we want but probably not?

Do you think it's worth the effort to do this work? Frankly, it's starting to feel just wrong to continue trying to get so much out of a command line output.

[karencfv]

On the other hand, it could be a lot of work, especially if we want to include information about why things failed, dependencies etc. Where do you think this fits into the current priorities we have?

[davepacheco]

I'd be interested for @jgallagher's opinion. I don't think I'd rewrite all this now to solve this problem. I'm still feeling like we need some kind of hysteresis regardless (so that transient situations don't raise the alarm), and that once we have that, a lot of these details aren't so critical right now.

[jgallagher]

scuffle's API was definitely written with "I want to configure a specific service" in mind, so I'd be surprised if it already exposes everything we'd need to port this work. (E.g., I don't think it exposes a "list all services".) So if we wanted to go that route, it'd also mean work on scuffle to support it. Probably straightforward work, but it is more work.

From the quoted manpage in the PR description: what if we added nstate to the list of columns we want as output and parse that? IIUC, that removes the possibility of * entirely, and transitioning is reported via the nstate column (- for "not transitioning", or the state it's transitioning to)? That would let us include both states in the Transitioning variant. (Although at that point the type becomes recursive, so maybe it'd be easier to have a higher-level enum for "transitioning or not" that contains 1 or 2 instances of the state enum?)

That seems like an easy-ish change to make, but I'm not sure how much it helps the false positive story given

Or might we also have false positives today if something is offline and not transitioning yet because its dependencies are still being started? In which case we just need to treat this at a higher level as transient. Or report the state_timestamp too, and only consider something broken if its in one of our broken states and its state hasn't changed recently (as a form of hysteresis)?

If we had a full fidelity Transitioning state without timestamps, what would the contact_support logic be?

[karencfv]

Note: We spoke about this in the update watercooler, and decided to not spend too much time deciding what to do with the offline case and when a service "in-transition" would be considered problematic. This contact_support field is not the end solution (FM is) so, for now we will only be reporting on services in maintenance.

[karencfv]

@davepacheco and @jgallagher, thanks for the discussion around this functionality. In the end I went for the simplest route of just considering a service to be in the state it reports; asterisk or not. This was mainly due to this information also being in inventory, and basically not wanting to end up changing a lot of the code there.

Let me know what you think!

[davepacheco]

To summarize this change as I understand it, from the bottom up:

• svcs parsing explicitly handles two new cases:
  • services in transition are reported as in their current state (rather than failing to parse it)
  • services in states that SMF doesn't understand are reported as being in state Unrecognized (rather than failing to parse it)
• the inventory API from sled agent is extended to reflect the new unrecognized state
• the database representation of inventory is extended to reflect the new unrecognized state
• in determining the contact_support flag, we now only report SMF-related problems for non-Propolis services in maintenance. (This is a behavior change, one we discussed, just not obvious from the PR synopsis.)

This all looks fine to me aside from one comment that I think is just stale but want to confirm.

[davepacheco]

Suggested change

	// official `svcs`` state and is only displayed temporarily
	// official `svcs` state and is only displayed temporarily

[davepacheco]

This comment seems wrong now since we don't see "in transition" as its own thing any more, right?

[karencfv]

I meant to leave it in case someone knew about svcs' "*" meaning, but now I realise it's just confusing. Removed!

[karencfv]

Thanks for taking a look @davepacheco. Your summary is correct. I can change the name of the PR to reflect the actual changes happening here.

Note: I'll merge main once I get an "approve" to avoid more version merge conflicts than necessary

[karencfv]

I meant to leave it in case someone knew about svcs' "*" meaning, but now I realise it's just confusing. Removed!