CmdError error messages should provide more information (#4428)

Fixes: #4419 This commit is a little invasive at various call sites so I would appreciate some feedback. Before this commit you would see: ``` sled-agent: Failed to delete all XDE devices ``` After this commit we now see: ``` sled-agent: Failed to delete all XDE devices Caused by: 0: Failure interacting with the OPTE ioctl(2) interface: command ListPorts failed: BadApiVersion { user: 25, kernel: 23 } 1: command ListPorts failed: BadApiVersion { user: 25, kernel: 23 } ``` --------- Co-authored-by: John Gallagher <[email protected]>
oxidecomputer · Nov 8, 2023 · 03c7f12 · 03c7f12
1 parent 9ff5aa3
commit 03c7f12
Show file tree

Hide file tree

Showing 11 changed files with 130 additions and 119 deletions.
diff --git a/common/src/cmd.rs b/common/src/cmd.rs
@@ -13,7 +13,7 @@ pub enum CmdError {
     /// incorrect command-line arguments
     Usage(String),
     /// all other errors
-    Failure(String),
+    Failure(anyhow::Error),
 }
 
 /// Exits the current process on a fatal error.
@@ -26,7 +26,7 @@ pub fn fatal(cmd_error: CmdError) -> ! {
         .unwrap_or("command");
     let (exit_code, message) = match cmd_error {
         CmdError::Usage(m) => (2, m),
-        CmdError::Failure(m) => (1, m),
+        CmdError::Failure(e) => (1, format!("{e:?}")),
     };
     eprintln!("{}: {}", arg0, message);
     exit(exit_code);

diff --git a/dev-tools/omicron-dev/src/bin/omicron-dev.rs b/dev-tools/omicron-dev/src/bin/omicron-dev.rs
@@ -4,8 +4,7 @@
 
 //! Developer tool for easily running bits of Omicron
 
-use anyhow::bail;
-use anyhow::Context;
+use anyhow::{bail, Context};
 use camino::Utf8Path;
 use camino::Utf8PathBuf;
 use clap::Args;
@@ -34,7 +33,7 @@ async fn main() -> Result<(), anyhow::Error> {
         OmicronDb::CertCreate { ref args } => cmd_cert_create(args).await,
     };
     if let Err(error) = result {
-        fatal(CmdError::Failure(format!("{:#}", error)));
+        fatal(CmdError::Failure(error));
     }
     Ok(())
 }

diff --git a/gateway/src/bin/mgs.rs b/gateway/src/bin/mgs.rs
@@ -4,6 +4,7 @@
 
 //! Executable program to run gateway, the management gateway service
 
+use anyhow::{anyhow, Context};
 use clap::Parser;
 use futures::StreamExt;
 use omicron_common::cmd::{fatal, CmdError};
@@ -70,26 +71,24 @@ async fn do_run() -> Result<(), CmdError> {
     let args = Args::parse();
 
     match args {
-        Args::Openapi => run_openapi().map_err(CmdError::Failure),
+        Args::Openapi => {
+            run_openapi().map_err(|e| CmdError::Failure(anyhow!(e)))
+        }
         Args::Run {
             config_file_path,
             id_and_address_from_smf,
             id,
             address,
         } => {
-            let config = Config::from_file(&config_file_path).map_err(|e| {
-                CmdError::Failure(format!(
-                    "failed to parse {}: {}",
-                    config_file_path.display(),
-                    e
-                ))
-            })?;
-
-            let mut signals = Signals::new([signal::SIGUSR1]).map_err(|e| {
-                CmdError::Failure(format!(
-                    "failed to set up signal handler: {e}"
-                ))
-            })?;
+            let config = Config::from_file(&config_file_path)
+                .with_context(|| {
+                    format!("failed to parse {}", config_file_path.display())
+                })
+                .map_err(CmdError::Failure)?;
+
+            let mut signals = Signals::new([signal::SIGUSR1])
+                .context("failed to set up signal handler")
+                .map_err(CmdError::Failure)?;
 
             let (id, addresses, rack_id) = if id_and_address_from_smf {
                 let config = read_smf_config()?;
@@ -102,35 +101,31 @@ async fn do_run() -> Result<(), CmdError> {
                 (id.unwrap(), vec![address.unwrap()], rack_id)
             };
             let args = MgsArguments { id, addresses, rack_id };
-            let mut server =
-                start_server(config, args).await.map_err(CmdError::Failure)?;
+            let mut server = start_server(config, args)
+                .await
+                .map_err(|e| CmdError::Failure(anyhow!(e)))?;
 
             loop {
                 tokio::select! {
                     signal = signals.next() => match signal {
                         Some(signal::SIGUSR1) => {
                             let new_config = read_smf_config()?;
                             if new_config.id != id {
-                                return Err(CmdError::Failure(
-                                    "cannot change server ID on refresh"
-                                        .to_string()
-                                ));
+                                return Err(CmdError::Failure(anyhow!("cannot change server ID on refresh")));
                             }
                             server.set_rack_id(new_config.rack_id);
                             server
                                 .adjust_dropshot_addresses(&new_config.addresses)
                                 .await
-                                .map_err(|err| CmdError::Failure(
-                                    format!("config refresh failed: {err}")
-                                ))?;
+                                .map_err(|err| CmdError::Failure(anyhow!("config refresh failed: {err}")))?;
                         }
                         // We only register `SIGUSR1` and never close the
                         // handle, so we never expect `None` or any other
                         // signal.
                         _ => unreachable!("invalid signal: {signal:?}"),
                     },
                     result = server.wait_for_finish() => {
-                        return result.map_err(CmdError::Failure)
+                        return result.map_err(|err| CmdError::Failure(anyhow!(err)))
                     }
                 }
             }
@@ -141,7 +136,7 @@ async fn do_run() -> Result<(), CmdError> {
 #[cfg(target_os = "illumos")]
 fn read_smf_config() -> Result<ConfigProperties, CmdError> {
     fn scf_to_cmd_err(err: illumos_utils::scf::ScfError) -> CmdError {
-        CmdError::Failure(err.to_string())
+        CmdError::Failure(anyhow!(err))
     }
 
     use illumos_utils::scf::ScfHandle;
@@ -165,25 +160,31 @@ fn read_smf_config() -> Result<ConfigProperties, CmdError> {
 
     let prop_id = config.value_as_string(PROP_ID).map_err(scf_to_cmd_err)?;
 
-    let prop_id = Uuid::try_parse(&prop_id).map_err(|err| {
-        CmdError::Failure(format!(
-            "failed to parse `{CONFIG_PG}/{PROP_ID}` \
-             ({prop_id:?}) as a UUID: {err}"
-        ))
-    })?;
+    let prop_id = Uuid::try_parse(&prop_id)
+        .with_context(|| {
+            format!(
+                "failed to parse `{CONFIG_PG}/{PROP_ID}` ({prop_id:?}) as a \
+                UUID"
+            )
+        })
+        .map_err(CmdError::Failure)?;
 
     let prop_rack_id =
         config.value_as_string(PROP_RACK_ID).map_err(scf_to_cmd_err)?;
 
     let rack_id = if prop_rack_id == "unknown" {
         None
     } else {
-        Some(Uuid::try_parse(&prop_rack_id).map_err(|err| {
-            CmdError::Failure(format!(
-                "failed to parse `{CONFIG_PG}/{PROP_RACK_ID}` \
-                 ({prop_rack_id:?}) as a UUID: {err}"
-            ))
-        })?)
+        Some(
+            Uuid::try_parse(&prop_rack_id)
+                .with_context(|| {
+                    format!(
+                        "failed to parse `{CONFIG_PG}/{PROP_RACK_ID}` \
+                        ({prop_rack_id:?}) as a UUID"
+                    )
+                })
+                .map_err(CmdError::Failure)?,
+        )
     };
 
     let prop_addr =
@@ -192,16 +193,20 @@ fn read_smf_config() -> Result<ConfigProperties, CmdError> {
     let mut addresses = Vec::with_capacity(prop_addr.len());
 
     for addr in prop_addr {
-        addresses.push(addr.parse().map_err(|err| {
-            CmdError::Failure(format!(
-                "failed to parse `{CONFIG_PG}/{PROP_ADDR}` \
-             ({addr:?}) as a socket address: {err}"
-            ))
-        })?);
+        addresses.push(
+            addr.parse()
+                .with_context(|| {
+                    format!(
+                        "failed to parse `{CONFIG_PG}/{PROP_ADDR}` ({addr:?}) \
+                        as a socket address"
+                    )
+                })
+                .map_err(CmdError::Failure)?,
+        );
     }
 
     if addresses.is_empty() {
-        Err(CmdError::Failure(format!(
+        Err(CmdError::Failure(anyhow!(
             "no addresses specified by `{CONFIG_PG}/{PROP_ADDR}`"
         )))
     } else {
@@ -211,7 +216,7 @@ fn read_smf_config() -> Result<ConfigProperties, CmdError> {
 
 #[cfg(not(target_os = "illumos"))]
 fn read_smf_config() -> Result<ConfigProperties, CmdError> {
-    Err(CmdError::Failure(
-        "SMF configuration only available on illumos".to_string(),
-    ))
+    Err(CmdError::Failure(anyhow!(
+        "SMF configuration only available on illumos"
+    )))
 }
diff --git a/installinator-artifactd/src/bin/installinator-artifactd.rs b/installinator-artifactd/src/bin/installinator-artifactd.rs
@@ -29,10 +29,9 @@ fn do_run() -> Result<(), CmdError> {
     match args {
         Args::Openapi => {
             installinator_artifactd::run_openapi().map_err(|error| {
-                CmdError::Failure(format!(
-                    "failed to generate OpenAPI spec: {:?}",
-                    error
-                ))
+                CmdError::Failure(
+                    error.context("failed to generate OpenAPI spec"),
+                )
             })
         }
     }

diff --git a/nexus/src/bin/nexus.rs b/nexus/src/bin/nexus.rs
@@ -10,6 +10,7 @@
 // - General networking and runtime tuning for availability and security: see
 //   omicron#2184, omicron#2414.
 
+use anyhow::anyhow;
 use clap::Parser;
 use omicron_common::cmd::fatal;
 use omicron_common::cmd::CmdError;
@@ -53,13 +54,13 @@ async fn do_run() -> Result<(), CmdError> {
     let args = Args::parse();
 
     let config = Config::from_file(args.config_file_path)
-        .map_err(|e| CmdError::Failure(e.to_string()))?;
+        .map_err(|e| CmdError::Failure(anyhow!(e)))?;
 
     if args.openapi {
-        run_openapi_external().map_err(CmdError::Failure)
+        run_openapi_external().map_err(|err| CmdError::Failure(anyhow!(err)))
     } else if args.openapi_internal {
-        run_openapi_internal().map_err(CmdError::Failure)
+        run_openapi_internal().map_err(|err| CmdError::Failure(anyhow!(err)))
     } else {
-        run_server(&config).await.map_err(CmdError::Failure)
+        run_server(&config).await.map_err(|err| CmdError::Failure(anyhow!(err)))
     }
 }
diff --git a/nexus/tests/integration_tests/commands.rs b/nexus/tests/integration_tests/commands.rs
@@ -56,10 +56,9 @@ fn test_nexus_bad_config() {
     let (exit_status, stdout_text, stderr_text) = run_command(exec);
     assert_exit_code(exit_status, EXIT_FAILURE, &stderr_text);
     assert_contents("tests/output/cmd-nexus-badconfig-stdout", &stdout_text);
-    assert_eq!(
-        stderr_text,
-        format!("nexus: read \"nonexistent\": {}\n", error_for_enoent())
-    );
+    let expected_err =
+        format!("nexus: read \"nonexistent\": {}\n", error_for_enoent());
+    assert!(&stderr_text.starts_with(&expected_err));
 }
 
 #[test]
@@ -73,13 +72,11 @@ fn test_nexus_invalid_config() {
         "tests/output/cmd-nexus-invalidconfig-stdout",
         &stdout_text,
     );
-    assert_eq!(
-        stderr_text,
-        format!(
-            "nexus: parse \"{}\": missing field `deployment`\n",
-            config_path.display()
-        ),
+    let expected_err = format!(
+        "nexus: parse \"{}\": missing field `deployment`\n",
+        config_path.display()
     );
+    assert!(&stderr_text.starts_with(&expected_err));
 }
 
 #[track_caller]

diff --git a/oximeter/collector/src/bin/oximeter.rs b/oximeter/collector/src/bin/oximeter.rs
@@ -6,6 +6,7 @@
 
 // Copyright 2023 Oxide Computer Company
 
+use anyhow::{anyhow, Context};
 use clap::Parser;
 use omicron_common::cmd::fatal;
 use omicron_common::cmd::CmdError;
@@ -132,7 +133,9 @@ async fn main() {
 async fn do_run() -> Result<(), CmdError> {
     let args = Args::parse();
     match args {
-        Args::Openapi => run_openapi().map_err(CmdError::Failure),
+        Args::Openapi => {
+            run_openapi().map_err(|err| CmdError::Failure(anyhow!(err)))
+        }
         Args::Run { config_file, id, address } => {
             let config = Config::from_file(config_file).unwrap();
             let args = OximeterArguments { id, address };
@@ -141,13 +144,15 @@ async fn do_run() -> Result<(), CmdError> {
                 .unwrap()
                 .serve_forever()
                 .await
-                .map_err(|e| CmdError::Failure(e.to_string()))
+                .context("Failed to create oximeter")
+                .map_err(CmdError::Failure)
         }
         Args::Standalone { id, address, nexus, clickhouse, log_level } => {
             // Start the standalone Nexus server, for registration of both the
             // collector and producers.
             let nexus_server = StandaloneNexus::new(nexus.into(), log_level)
-                .map_err(|e| CmdError::Failure(e.to_string()))?;
+                .context("Failed to create nexus")
+                .map_err(CmdError::Failure)?;
             let args = OximeterArguments { id, address };
             Oximeter::new_standalone(
                 nexus_server.log(),
@@ -159,10 +164,10 @@ async fn do_run() -> Result<(), CmdError> {
             .unwrap()
             .serve_forever()
             .await
-            .map_err(|e| CmdError::Failure(e.to_string()))
-        }
-        Args::StandaloneOpenapi => {
-            run_standalone_openapi().map_err(CmdError::Failure)
+            .context("Failed to create standalone oximeter")
+            .map_err(CmdError::Failure)
         }
+        Args::StandaloneOpenapi => run_standalone_openapi()
+            .map_err(|err| CmdError::Failure(anyhow!(err))),
     }
 }
diff --git a/sled-agent/src/bin/sled-agent-sim.rs b/sled-agent/src/bin/sled-agent-sim.rs
@@ -6,7 +6,7 @@
 
 // TODO see the TODO for nexus.
 
-use anyhow::Context;
+use anyhow::{anyhow, Context};
 use camino::Utf8PathBuf;
 use clap::Parser;
 use dropshot::ConfigDropshot;
@@ -96,7 +96,7 @@ async fn do_run() -> Result<(), CmdError> {
     let args = Args::parse();
 
     let tmp = camino_tempfile::tempdir()
-        .map_err(|e| CmdError::Failure(e.to_string()))?;
+        .map_err(|e| CmdError::Failure(anyhow!(e)))?;
     let config = Config {
         id: args.uuid,
         sim_mode: args.sim_mode,
@@ -125,10 +125,10 @@ async fn do_run() -> Result<(), CmdError> {
         (Some(cert_path), Some(key_path)) => {
             let cert_bytes = std::fs::read_to_string(&cert_path)
                 .with_context(|| format!("read {:?}", &cert_path))
-                .map_err(|e| CmdError::Failure(e.to_string()))?;
+                .map_err(CmdError::Failure)?;
             let key_bytes = std::fs::read_to_string(&key_path)
                 .with_context(|| format!("read {:?}", &key_path))
-                .map_err(|e| CmdError::Failure(e.to_string()))?;
+                .map_err(CmdError::Failure)?;
             Some(NexusTypes::Certificate { cert: cert_bytes, key: key_bytes })
         }
         _ => {
@@ -145,7 +145,5 @@ async fn do_run() -> Result<(), CmdError> {
         tls_certificate,
     };
 
-    run_standalone_server(&config, &rss_args)
-        .await
-        .map_err(|e| CmdError::Failure(e.to_string()))
+    run_standalone_server(&config, &rss_args).await.map_err(CmdError::Failure)
 }