Habitat: "[Err: 7] Invalid desired state ""unknown""" is a terrible message (among others)

[chefsalim]

Results from running 'sudo hab svc status' on 3 different VMs:

Get scary red error messages (different ones on each):

Machine 1: "[Err: 7] Invalid desired state ""unknown"""
Machine 2: "Connection refused (os error 61)"
Machine 3: "[Err: 4] secret key mismatch"

If the 'error' is not a real error (or may be common, eg, if no supervisor is running), we should either suppress or show friendlier message (not red). For actual errors, we should provide a better message and prescriptive guidance on what to do to fix.

[baumanj]

"[Err: 7] Invalid desired state ""unknown""" sounds like there may be communication between systems on different sides of https://github.com/habitat-sh/habitat/pull/5308. Can you give more details on how to reproduce that one.

The other two will be addressed by https://github.com/habitat-sh/habitat/issues/5169.

[chefsalim]

Unfortunately, I don't quite know how to repro the invalid desired state issue - it went away after re-booting the VM and doing a fresh install of hab 0.61

[baumanj]

I'm going to close this for now, if we see the Invalid desired state error again, we can re-open with more info, but I think it arises from an invalid configuration. The other messages are already being addressed in other issues.

[chefsalim]

The point is not that a specific issue is reproducable - we need to review the error and provide guidance to the user, not a bad message like what we are showing today.

[baumanj]

Closing per out of band discussion

[chefsalim]

Re-opening as upon review of the print_svc_status code, there appears to be at least one bug that can be easily addressed. When we receive a ServiceStatus message and parse it out, any missing values for optional fields are mapped to valid defaults. EXCEPT for DesiredState, which defaults to "unknown" which then immediately errors out in the from_str later. A fairly safe fix for this case would be to do an unwrap_or("unknown") on the from_str, which would bring the handling of this field in line with the others.

    let svc_type = status.composite.unwrap_or("standalone".to_string());
    let svc_desired_state = status
        .desired_state
        .map_or("unknown".to_string(), |s| s.to_string());
    let (svc_state, svc_pid, svc_elapsed) = {
        match status.process {
            Some(process) => (
                process.state.to_string(),
                process
                    .pid
                    .map_or_else(|| "<none>".to_string(), |p| p.to_string()),
                process.elapsed.unwrap_or_default().to_string(),
            ),
            None => (
                ProcessState::default().to_string(),
                "<none>".to_string(),
                "<none>".to_string(),
            ),
        }
    };
   ...
    write!(
        out,
        "{}\t{}\t{}\t{}\t{}\t{}\t{}\n",
        status.ident,
        svc_type,
        DesiredState::from_str(&svc_desired_state)?,   <-- unwrap_or("unknown")
        ProcessState::from_str(&svc_state)?,
        svc_elapsed,
        svc_pid,
        status.service_group,
    )?;

The other bigger problem remains - the way error handling is being done, we are printing out errors from deep in the system to the user with mostly no filtering / context.

fn main() {
    env_logger::init();
    let mut ui = UI::default_with_env();
    enable_features_from_env(&mut ui);
    thread::spawn(|| analytics::instrument_subcommand());
    if let Err(e) = start(&mut ui) {
        ui.fatal(e).unwrap();  <-- this error bubbles up all the way from FromStr for example
        std::process::exit(1)
    }
}

The interesting thing is that looking at types.rs it appears at some point we did implicitly acknowledge that these errors would be directly shown to the user and tried to make them meaningful.

            return Err(net::err(
                ErrCode::InvalidPayload,
                format!(
                    "Invalid binding \"{}\", must be of the form <NAME>:<SERVICE_GROUP> or \
                    <SERVICE_NAME>:<NAME>:<SERVICE_GROUP> where <NAME> is a service name,
                    <SERVICE_GROUP> is a valid service group, and <SERVICE_NAME> is the name of
                    a service within a composite if the given bind is for a composite service.",
                    bind_str
                ),
            ));

But we totally did not do that for DesiredState, ProcessState, BindingMode, etc. That is the higher level thing that also must be addressed.