Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CmdError error messages should provide more information #4428

Merged
merged 8 commits into from
Nov 8, 2023
Merged

CmdError error messages should provide more information #4428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f5a7964
CmdError error messages should provide more information
papertigers Nov 3, 2023
32082dd
Fix non illumos platforms
papertigers Nov 6, 2023
f2a4fe5
Apply jgallagher suggestions from code review
papertigers Nov 6, 2023
ad8e5d6
Prefer anyhow!(err) over err.into()
papertigers Nov 6, 2023
cafa924
Missed one variant of error.into()
papertigers Nov 6, 2023
b00b142
Cleanup string formatting
papertigers Nov 6, 2023
84537ae
Remove useless conversions
papertigers Nov 6, 2023
339753b
Burned by RUST_BACKTRACE=1
papertigers Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/src/cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion dev-tools/omicron-dev/src/bin/omicron-dev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,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.into()));
}
Ok(())
}
Expand Down
93 changes: 49 additions & 44 deletions gateway/src/bin/mgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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}")))
papertigers marked this conversation as resolved.
Show resolved Hide resolved
}
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()?;
Expand All @@ -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}")))
papertigers marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand All @@ -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(err.into())
}

use illumos_utils::scf::ScfHandle;
Expand All @@ -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"
papertigers marked this conversation as resolved.
Show resolved Hide resolved
)
})
.map_err(CmdError::Failure)?,
)
};

let prop_addr =
Expand All @@ -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 {
Expand Down
7 changes: 3 additions & 4 deletions installinator-artifactd/src/bin/installinator-artifactd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
)
})
}
}
Expand Down
9 changes: 5 additions & 4 deletions nexus/src/bin/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(e.into()))?;

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)))
}
}
19 changes: 12 additions & 7 deletions oximeter/collector/src/bin/oximeter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}")))
papertigers marked this conversation as resolved.
Show resolved Hide resolved
}
Args::Run { config_file, id, address } => {
let config = Config::from_file(config_file).unwrap();
let args = OximeterArguments { id, address };
Expand All @@ -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(),
Expand All @@ -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))),
}
}
10 changes: 5 additions & 5 deletions sled-agent/src/bin/sled-agent-sim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ async fn main() {
async fn do_run() -> Result<(), CmdError> {
let args = Args::parse();

let tmp = camino_tempfile::tempdir()
.map_err(|e| CmdError::Failure(e.to_string()))?;
let tmp =
camino_tempfile::tempdir().map_err(|e| CmdError::Failure(e.into()))?;
let config = Config {
id: args.uuid,
sim_mode: args.sim_mode,
Expand Down Expand Up @@ -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(|e| CmdError::Failure(e.into()))?;
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(|e| CmdError::Failure(e.into()))?;
Some(NexusTypes::Certificate { cert: cert_bytes, key: key_bytes })
}
_ => {
Expand All @@ -147,5 +147,5 @@ async fn do_run() -> Result<(), CmdError> {

run_standalone_server(&config, &rss_args)
.await
.map_err(|e| CmdError::Failure(e.to_string()))
.map_err(|e| CmdError::Failure(e.into()))
}
24 changes: 13 additions & 11 deletions sled-agent/src/bin/sled-agent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

//! Executable program to run the sled agent

use anyhow::anyhow;
use camino::Utf8PathBuf;
use clap::{Parser, Subcommand};
use omicron_common::cmd::fatal;
Expand Down Expand Up @@ -51,16 +52,14 @@ async fn do_run() -> Result<(), CmdError> {

match args {
Args::Openapi(flavor) => match flavor {
OpenapiFlavor::Sled => {
sled_server::run_openapi().map_err(CmdError::Failure)
}
OpenapiFlavor::Bootstrap => {
bootstrap_server::run_openapi().map_err(CmdError::Failure)
}
OpenapiFlavor::Sled => sled_server::run_openapi()
.map_err(|err| CmdError::Failure(anyhow!("{err}"))),
papertigers marked this conversation as resolved.
Show resolved Hide resolved
OpenapiFlavor::Bootstrap => bootstrap_server::run_openapi()
.map_err(|err| CmdError::Failure(anyhow!("{err}"))),
papertigers marked this conversation as resolved.
Show resolved Hide resolved
},
Args::Run { config_path } => {
let config = SledConfig::from_file(&config_path)
.map_err(|e| CmdError::Failure(e.to_string()))?;
.map_err(|e| CmdError::Failure(e.into()))?;

// - Sled agent starts with the normal config file - typically
// called "config.toml".
Expand All @@ -83,15 +82,15 @@ async fn do_run() -> Result<(), CmdError> {
let rss_config = if rss_config_path.exists() {
Some(
RssConfig::from_file(rss_config_path)
.map_err(|e| CmdError::Failure(e.to_string()))?,
.map_err(|e| CmdError::Failure(e.into()))?,
)
} else {
None
};

let server = bootstrap_server::Server::start(config)
.await
.map_err(|err| CmdError::Failure(format!("{err:#}")))?;
.map_err(|err| CmdError::Failure(err.into()))?;

// If requested, automatically supply the RSS configuration.
//
Expand All @@ -103,12 +102,15 @@ async fn do_run() -> Result<(), CmdError> {
// abandon the server.
Ok(_) | Err(RssAccessError::AlreadyInitialized) => {}
Err(e) => {
return Err(CmdError::Failure(e.to_string()));
return Err(CmdError::Failure(e.into()));
}
}
}

server.wait_for_finish().await.map_err(CmdError::Failure)?;
server
.wait_for_finish()
.await
.map_err(|err| CmdError::Failure(anyhow!("{err}")))?;
papertigers marked this conversation as resolved.
Show resolved Hide resolved

Ok(())
}
Expand Down
Loading
Loading