[mgs] Use slog-error-chain to clean up error types and logging (#4717)

These are all pretty mechanical changes:

* Use `#[source]` and `#[from]` correctly
* Add a bit of context to a few error cases (e.g., including the
`SpIdentifier` when returning an `SpCommunicationFailed` error)
* Use `InlineErrorChain` instead of `anyhow` to convert error chains
into strings (avoiding the intermediate `anyhow::Error` heap allocation)
* Switch to `Utf8PathBuf` for command line args and related errors

Cargo.toml

@@ -339,6 +339,7 @@ slog = { version = "2.7", features = [ "dynamic-keys", "max_level_trace", "relea
 slog-async = "2.8"
 slog-dtrace = "0.2"
 slog-envlogger = "2.2"
+slog-error-chain = { git = "https://github.com/oxidecomputer/slog-error-chain", branch = "main", features = ["derive"] }
 slog-term = "2.9"
 smf = "0.2"
 snafu = "0.7"

gateway/Cargo.toml

@@ -7,6 +7,7 @@ license = "MPL-2.0"
 [dependencies]
 anyhow.workspace = true
 base64.workspace = true
+camino.workspace = true
 clap.workspace = true
 dropshot.workspace = true
 futures.workspace = true
@@ -25,6 +26,7 @@ signal-hook.workspace = true
 signal-hook-tokio.workspace = true
 slog.workspace = true
 slog-dtrace.workspace = true
+slog-error-chain.workspace = true
 thiserror.workspace = true
 tokio = { workspace = true, features = ["full"] }
 tokio-stream.workspace = true

gateway/src/bin/mgs.rs

@@ -5,14 +5,14 @@
 //! Executable program to run gateway, the management gateway service
 
 use anyhow::{anyhow, Context};
+use camino::Utf8PathBuf;
 use clap::Parser;
 use futures::StreamExt;
 use omicron_common::cmd::{fatal, CmdError};
 use omicron_gateway::{run_openapi, start_server, Config, MgsArguments};
 use signal_hook::consts::signal;
 use signal_hook_tokio::Signals;
 use std::net::SocketAddrV6;
-use std::path::PathBuf;
 use uuid::Uuid;
 
 #[derive(Debug, Parser)]
@@ -24,7 +24,7 @@ enum Args {
     /// Start an MGS server
     Run {
         #[clap(name = "CONFIG_FILE_PATH", action)]
-        config_file_path: PathBuf,
+        config_file_path: Utf8PathBuf,
 
         /// Read server ID and address(es) for dropshot server from our SMF
         /// properties (only valid when running as a service on illumos)
@@ -81,9 +81,7 @@ async fn do_run() -> Result<(), CmdError> {
             address,
         } => {
             let config = Config::from_file(&config_file_path)
-                .with_context(|| {
-                    format!("failed to parse {}", config_file_path.display())
-                })
+                .map_err(anyhow::Error::new)
                 .map_err(CmdError::Failure)?;
 
             let mut signals = Signals::new([signal::SIGUSR1])

gateway/src/config.rs

@@ -6,10 +6,11 @@
 //! configuration
 
 use crate::management_switch::SwitchConfig;
+use camino::Utf8Path;
+use camino::Utf8PathBuf;
 use dropshot::ConfigLogging;
 use serde::{Deserialize, Serialize};
-use std::path::Path;
-use std::path::PathBuf;
+use slog_error_chain::SlogInlineError;
 use thiserror::Error;
 
 /// Configuration for a gateway server
@@ -30,13 +31,11 @@ impl Config {
     /// Load a `Config` from the given TOML file
     ///
     /// This config object can then be used to create a new gateway server.
-    // The format is described in the README. // TODO add a README
-    pub fn from_file<P: AsRef<Path>>(path: P) -> Result<Config, LoadError> {
-        let path = path.as_ref();
+    pub fn from_file(path: &Utf8Path) -> Result<Config, LoadError> {
         let file_contents = std::fs::read_to_string(path)
-            .map_err(|e| (path.to_path_buf(), e))?;
+            .map_err(|err| LoadError::Io { path: path.into(), err })?;
         let config_parsed: Config = toml::from_str(&file_contents)
-            .map_err(|e| (path.to_path_buf(), e))?;
+            .map_err(|err| LoadError::Parse { path: path.into(), err })?;
         Ok(config_parsed)
     }
 }
@@ -46,32 +45,18 @@ pub struct PartialDropshotConfig {
     pub request_body_max_bytes: usize,
 }
 
-#[derive(Debug, Error)]
+#[derive(Debug, Error, SlogInlineError)]
 pub enum LoadError {
-    #[error("error reading \"{}\": {}", path.display(), err)]
-    Io { path: PathBuf, err: std::io::Error },
-    #[error("error parsing \"{}\": {}", path.display(), err)]
-    Parse { path: PathBuf, err: toml::de::Error },
-}
-
-impl From<(PathBuf, std::io::Error)> for LoadError {
-    fn from((path, err): (PathBuf, std::io::Error)) -> Self {
-        LoadError::Io { path, err }
-    }
-}
-
-impl From<(PathBuf, toml::de::Error)> for LoadError {
-    fn from((path, err): (PathBuf, toml::de::Error)) -> Self {
-        LoadError::Parse { path, err }
-    }
-}
-
-impl std::cmp::PartialEq<std::io::Error> for LoadError {
-    fn eq(&self, other: &std::io::Error) -> bool {
-        if let LoadError::Io { err, .. } = self {
-            err.kind() == other.kind()
-        } else {
-            false
-        }
-    }
+    #[error("error reading \"{path}\"")]
+    Io {
+        path: Utf8PathBuf,
+        #[source]
+        err: std::io::Error,
+    },
+    #[error("error parsing \"{path}\"")]
+    Parse {
+        path: Utf8PathBuf,
+        #[source]
+        err: toml::de::Error,
+    },
 }

gateway/src/error.rs

@@ -5,16 +5,17 @@
 //! Error handling facilities for the management gateway.
 
 use crate::management_switch::SpIdentifier;
-use anyhow::anyhow;
 use dropshot::HttpError;
 use gateway_messages::SpError;
 pub use gateway_sp_comms::error::CommunicationError;
 use gateway_sp_comms::error::UpdateError;
 use gateway_sp_comms::BindError;
+use slog_error_chain::InlineErrorChain;
+use slog_error_chain::SlogInlineError;
 use std::time::Duration;
 use thiserror::Error;
 
-#[derive(Debug, Error)]
+#[derive(Debug, Error, SlogInlineError)]
 pub enum StartupError {
     #[error("invalid configuration file: {}", .reasons.join(", "))]
     InvalidConfig { reasons: Vec<String> },
@@ -23,116 +24,137 @@ pub enum StartupError {
     BindError(#[from] BindError),
 }
 
-#[derive(Debug, Error)]
+#[derive(Debug, Error, SlogInlineError)]
 pub enum SpCommsError {
     #[error("discovery process not yet complete")]
     DiscoveryNotYetComplete,
     #[error("location discovery failed: {reason}")]
     DiscoveryFailed { reason: String },
-    #[error("nonexistent SP (type {:?}, slot {})", .0.typ, .0.slot)]
+    #[error("nonexistent SP {0:?}")]
     SpDoesNotExist(SpIdentifier),
-    #[error(
-        "unknown socket address for SP (type {:?}, slot {})",
-        .0.typ,
-        .0.slot,
-    )]
+    #[error("unknown socket address for SP {0:?}")]
     SpAddressUnknown(SpIdentifier),
     #[error(
         "timeout ({timeout:?}) elapsed communicating with {sp:?} on port {port}"
     )]
     Timeout { timeout: Duration, port: usize, sp: Option<SpIdentifier> },
-    #[error("error communicating with SP: {0}")]
-    SpCommunicationFailed(#[from] CommunicationError),
-    #[error("updating SP failed: {0}")]
-    UpdateFailed(#[from] UpdateError),
+    #[error("error communicating with SP {sp:?}")]
+    SpCommunicationFailed {
+        sp: SpIdentifier,
+        #[source]
+        err: CommunicationError,
+    },
+    #[error("updating SP {sp:?} failed")]
+    UpdateFailed {
+        sp: SpIdentifier,
+        #[source]
+        err: UpdateError,
+    },
 }
 
 impl From<SpCommsError> for HttpError {
-    fn from(err: SpCommsError) -> Self {
-        match err {
+    fn from(error: SpCommsError) -> Self {
+        match error {
             SpCommsError::SpDoesNotExist(_) => HttpError::for_bad_request(
                 Some("InvalidSp".to_string()),
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::SpCommunicationFailed(
-                CommunicationError::SpError(
-                    SpError::SerialConsoleAlreadyAttached,
-                ),
-            ) => HttpError::for_bad_request(
+            SpCommsError::SpCommunicationFailed {
+                err:
+                    CommunicationError::SpError(
+                        SpError::SerialConsoleAlreadyAttached,
+                    ),
+                ..
+            } => HttpError::for_bad_request(
                 Some("SerialConsoleAttached".to_string()),
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::SpCommunicationFailed(
-                CommunicationError::SpError(SpError::RequestUnsupportedForSp),
-            ) => HttpError::for_bad_request(
+            SpCommsError::SpCommunicationFailed {
+                err:
+                    CommunicationError::SpError(SpError::RequestUnsupportedForSp),
+                ..
+            } => HttpError::for_bad_request(
                 Some("RequestUnsupportedForSp".to_string()),
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::SpCommunicationFailed(
-                CommunicationError::SpError(
-                    SpError::RequestUnsupportedForComponent,
-                ),
-            ) => HttpError::for_bad_request(
+            SpCommsError::SpCommunicationFailed {
+                err:
+                    CommunicationError::SpError(
+                        SpError::RequestUnsupportedForComponent,
+                    ),
+                ..
+            } => HttpError::for_bad_request(
                 Some("RequestUnsupportedForComponent".to_string()),
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::SpCommunicationFailed(
-                CommunicationError::SpError(SpError::InvalidSlotForComponent),
-            ) => HttpError::for_bad_request(
+            SpCommsError::SpCommunicationFailed {
+                err:
+                    CommunicationError::SpError(SpError::InvalidSlotForComponent),
+                ..
+            } => HttpError::for_bad_request(
                 Some("InvalidSlotForComponent".to_string()),
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::UpdateFailed(UpdateError::ImageTooLarge) => {
-                HttpError::for_bad_request(
-                    Some("ImageTooLarge".to_string()),
-                    format!("{:#}", anyhow!(err)),
-                )
-            }
-            SpCommsError::UpdateFailed(UpdateError::Communication(
-                CommunicationError::SpError(SpError::UpdateSlotBusy),
-            )) => http_err_with_message(
+            SpCommsError::UpdateFailed {
+                err: UpdateError::ImageTooLarge,
+                ..
+            } => HttpError::for_bad_request(
+                Some("ImageTooLarge".to_string()),
+                InlineErrorChain::new(&error).to_string(),
+            ),
+            SpCommsError::UpdateFailed {
+                err:
+                    UpdateError::Communication(CommunicationError::SpError(
+                        SpError::UpdateSlotBusy,
+                    )),
+                ..
+            } => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "UpdateSlotBusy",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::UpdateFailed(UpdateError::Communication(
-                CommunicationError::SpError(SpError::UpdateInProgress {
-                    ..
-                }),
-            )) => http_err_with_message(
+            SpCommsError::UpdateFailed {
+                err:
+                    UpdateError::Communication(CommunicationError::SpError(
+                        SpError::UpdateInProgress { .. },
+                    )),
+                ..
+            } => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "UpdateInProgress",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
             SpCommsError::DiscoveryNotYetComplete => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "DiscoveryNotYetComplete",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
             SpCommsError::SpAddressUnknown(_) => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "SpAddressUnknown",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
             SpCommsError::DiscoveryFailed { .. } => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "DiscoveryFailed ",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
             SpCommsError::Timeout { .. } => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "Timeout ",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
-            SpCommsError::SpCommunicationFailed(_) => http_err_with_message(
-                http::StatusCode::SERVICE_UNAVAILABLE,
-                "SpCommunicationFailed",
-                format!("{:#}", anyhow!(err)),
-            ),
-            SpCommsError::UpdateFailed(_) => http_err_with_message(
+            SpCommsError::SpCommunicationFailed { .. } => {
+                http_err_with_message(
+                    http::StatusCode::SERVICE_UNAVAILABLE,
+                    "SpCommunicationFailed",
+                    InlineErrorChain::new(&error).to_string(),
+                )
+            }
+            SpCommsError::UpdateFailed { .. } => http_err_with_message(
                 http::StatusCode::SERVICE_UNAVAILABLE,
                 "UpdateFailed",
-                format!("{:#}", anyhow!(err)),
+                InlineErrorChain::new(&error).to_string(),
             ),
         }
     }