From b45e3a6e8a3502146e3203d1a87a44808db75609 Mon Sep 17 00:00:00 2001 From: "Alexander V. Nikolaev" Date: Wed, 11 Dec 2024 13:53:19 +0200 Subject: [PATCH 1/2] Rewrap and propagate GRPC errors Signed-off-by: Alexander V. Nikolaev --- client/src/client.rs | 69 ++++++++++++++++++++++++++++++++------- client/src/error.rs | 27 +++++++++++++++ client/src/lib.rs | 1 + src/admin/server.rs | 16 ++++++--- src/systemd_api/client.rs | 50 ++++++++++++++++++++++++---- src/utils/tonic.rs | 18 +++++++--- 6 files changed, 153 insertions(+), 28 deletions(-) create mode 100644 client/src/error.rs diff --git a/client/src/client.rs b/client/src/client.rs index 1dd875b..298cf03 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -11,6 +11,7 @@ pub use givc_common::query::{Event, QueryResult}; use givc_common::types::*; use crate::endpoint::{EndpointConfig, TlsConfig}; +use crate::error::StatusWrapExt; type Client = pb::admin_service_client::AdminServiceClient; @@ -94,7 +95,12 @@ impl AdminClient { vm_name, args, }; - let _response = self.connect_to().await?.start_application(request).await?; + let _response = self + .connect_to() + .await? + .start_application(request) + .await + .rewrap_err()?; Ok(()) } @@ -104,7 +110,12 @@ impl AdminClient { vm_name: None, args: Vec::new(), }; - let _response = self.connect_to().await?.stop_application(request).await?; + let _response = self + .connect_to() + .await? + .stop_application(request) + .await + .rewrap_err()?; Ok(()) } @@ -114,7 +125,12 @@ impl AdminClient { vm_name: None, args: Vec::new(), }; - let _response = self.connect_to().await?.pause_application(request).await?; + let _response = self + .connect_to() + .await? + .pause_application(request) + .await + .rewrap_err()?; Ok(()) } @@ -124,31 +140,56 @@ impl AdminClient { vm_name: None, args: Vec::new(), }; - let _response = self.connect_to().await?.resume_application(request).await?; + let _response = self + .connect_to() + .await? + .resume_application(request) + .await + .rewrap_err()?; Ok(()) } pub async fn reboot(&self) -> anyhow::Result<()> { let request = pb::admin::Empty {}; - let _response = self.connect_to().await?.reboot(request).await?; + let _response = self + .connect_to() + .await? + .reboot(request) + .await + .rewrap_err()?; Ok(()) } pub async fn poweroff(&self) -> anyhow::Result<()> { let request = pb::admin::Empty {}; - let _response = self.connect_to().await?.poweroff(request).await?; + let _response = self + .connect_to() + .await? + .poweroff(request) + .await + .rewrap_err()?; Ok(()) } pub async fn suspend(&self) -> anyhow::Result<()> { let request = pb::admin::Empty {}; - let _response = self.connect_to().await?.suspend(request).await?; + let _response = self + .connect_to() + .await? + .suspend(request) + .await + .rewrap_err()?; Ok(()) } pub async fn wakeup(&self) -> anyhow::Result<()> { let request = pb::admin::Empty {}; - let _response = self.connect_to().await?.wakeup(request).await?; + let _response = self + .connect_to() + .await? + .wakeup(request) + .await + .rewrap_err()?; Ok(()) } @@ -165,7 +206,8 @@ impl AdminClient { self.connect_to() .await? .query_list(pb::admin::Empty {}) - .await? + .await + .rewrap_err()? .into_inner() .list .into_iter() @@ -177,7 +219,8 @@ impl AdminClient { self.connect_to() .await? .set_locale(pb::admin::LocaleRequest { locale }) - .await?; + .await + .rewrap_err()?; Ok(()) } @@ -185,7 +228,8 @@ impl AdminClient { self.connect_to() .await? .set_timezone(pb::admin::TimezoneRequest { timezone }) - .await?; + .await + .rewrap_err()?; Ok(()) } @@ -199,7 +243,8 @@ impl AdminClient { .connect_to() .await? .watch(pb::admin::Empty {}) - .await? + .await + .rewrap_err()? .into_inner(); let list = match watch.try_next().await? { diff --git a/client/src/error.rs b/client/src/error.rs new file mode 100644 index 0000000..28b636c --- /dev/null +++ b/client/src/error.rs @@ -0,0 +1,27 @@ +use anyhow::Error; +use tonic::Status; +use tonic_types::StatusExt; +use tracing::{debug, error}; + +fn rewrap_error(status: Status) -> Error { + let mut err = Error::msg(status.message().to_owned()); + let details = status.get_error_details(); + if let Some(debug_info) = details.debug_info() { + err = err.context(format!("Detail: {}", debug_info.detail)); + err = debug_info + .stack_entries + .iter() + .fold(err, |err, each| err.context(format!("Stack: {each}"))) + }; + err +} + +pub trait StatusWrapExt { + fn rewrap_err(self) -> Result; +} + +impl StatusWrapExt for Result { + fn rewrap_err(self) -> Result { + self.map_err(rewrap_error) + } +} diff --git a/client/src/lib.rs b/client/src/lib.rs index 5e5d3f4..4fdbec2 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -1,3 +1,4 @@ pub mod client; pub mod endpoint; +pub mod error; pub use crate::client::{AdminClient, QueryResult}; diff --git a/src/admin/server.rs b/src/admin/server.rs index a132ce4..2ec8d77 100644 --- a/src/admin/server.rs +++ b/src/admin/server.rs @@ -161,7 +161,9 @@ impl AdminServiceImpl { let vmservice = format_service_name(vmname, None); /* Return error if the vm is not registered */ - let endpoint = self.agent_endpoint(&vmservice)?; + let endpoint = self + .agent_endpoint(&vmservice) + .with_context(|| format!("{vmservice} not registered"))?; let client = SystemDClient::new(endpoint); /* Check status of the unit */ @@ -181,7 +183,7 @@ impl AdminServiceImpl { Err(anyhow!("Service {unit} is not loaded!")) } Err(e) => { - eprintln!("Error retrieving unit status: {e}"); + error!("Error retrieving unit status: {e}"); Err(e) } } @@ -194,7 +196,7 @@ impl AdminServiceImpl { let status = client .get_remote_status(name.to_string()) .await - .with_context(|| format!("cannot retrieve vm status for {name}"))?; + .with_context(|| format!("cannot retrieve vm status for {name}, host agent failed"))?; if status.load_state != "loaded" { bail!("vm {name} not loaded") @@ -224,6 +226,7 @@ impl AdminServiceImpl { match (entry.r#type.vm, entry.r#type.service) { (VmType::AppVM, ServiceType::App) => { if entry.status.is_exitted() { + debug!("Deregister exitted {}", entry.name); self.registry.deregister(&entry.name)?; } Ok(()) @@ -235,7 +238,10 @@ impl AdminServiceImpl { .with_context(|| format!("handing error, by restart VM {}", entry.name))?; Ok(()) // FIXME: should use `?` from line above, why it didn't work? } - (x, y) => bail!("Don't known how to handle_error for VM type: {x:?}:{y:?}"), + (x, y) => { + error!("Don't known how to handle_error for VM type: {x:?}:{y:?}"); + Ok(()) + } } } @@ -283,6 +289,7 @@ impl AdminServiceImpl { if let Err(err) = self.monitor_routine().await { error!("Error during watch: {err}"); } + info!("{:#?}", self.registry) } } @@ -307,6 +314,7 @@ impl AdminServiceImpl { match self.registry.by_name(&systemd_agent) { std::result::Result::Ok(e) => e, Err(_) => { + info!("Starting up VM {vm_name}"); self.start_vm(&vm_name) .await .with_context(|| format!("Starting vm for {name}"))?; diff --git a/src/systemd_api/client.rs b/src/systemd_api/client.rs index 2900ce4..9edd784 100644 --- a/src/systemd_api/client.rs +++ b/src/systemd_api/client.rs @@ -1,5 +1,6 @@ use crate::pb; use givc_client::endpoint::EndpointConfig; +use givc_client::error::StatusWrapExt; use tonic::transport::Channel; type Client = pb::systemd::unit_control_service_client::UnitControlServiceClient; @@ -24,7 +25,12 @@ impl SystemDClient { unit: String, ) -> anyhow::Result { let request = pb::systemd::UnitRequest { unit_name: unit }; - let response = self.connect().await?.get_unit_status(request).await?; + let response = self + .connect() + .await? + .get_unit_status(request) + .await + .rewrap_err()?; let status = response .into_inner() .unit_status @@ -42,35 +48,60 @@ impl SystemDClient { pub async fn start_remote(&self, unit: String) -> anyhow::Result { let request = pb::systemd::UnitRequest { unit_name: unit }; - let resp = self.connect().await?.start_unit(request).await?; + let resp = self + .connect() + .await? + .start_unit(request) + .await + .rewrap_err()?; let status = resp.into_inner(); Ok(status.cmd_status) } pub async fn stop_remote(&self, unit: String) -> anyhow::Result { let request = pb::systemd::UnitRequest { unit_name: unit }; - let resp = self.connect().await?.stop_unit(request).await?; + let resp = self + .connect() + .await? + .stop_unit(request) + .await + .rewrap_err()?; let status = resp.into_inner(); Ok(status.cmd_status) } pub async fn kill_remote(&self, unit: String) -> anyhow::Result { let request = pb::systemd::UnitRequest { unit_name: unit }; - let resp = self.connect().await?.kill_unit(request).await?; + let resp = self + .connect() + .await? + .kill_unit(request) + .await + .rewrap_err()?; let status = resp.into_inner(); Ok(status.cmd_status) } pub async fn pause_remote(&self, unit: String) -> anyhow::Result { let request = pb::systemd::UnitRequest { unit_name: unit }; - let resp = self.connect().await?.freeze_unit(request).await?; + let resp = self + .connect() + .await? + .freeze_unit(request) + .await + .rewrap_err()?; let status = resp.into_inner(); Ok(status.cmd_status) } pub async fn resume_remote(&self, unit: String) -> anyhow::Result { let request = pb::systemd::UnitRequest { unit_name: unit }; - let resp = self.connect().await?.unfreeze_unit(request).await?; + let resp = self + .connect() + .await? + .unfreeze_unit(request) + .await + .rewrap_err()?; let status = resp.into_inner(); Ok(status.cmd_status) } @@ -84,7 +115,12 @@ impl SystemDClient { unit_name: unit, args, }; - let resp = self.connect().await?.start_application(request).await?; + let resp = self + .connect() + .await? + .start_application(request) + .await + .rewrap_err()?; let status = resp.into_inner(); Ok(status.cmd_status) } diff --git a/src/utils/tonic.rs b/src/utils/tonic.rs index e8d2b0e..26638e0 100644 --- a/src/utils/tonic.rs +++ b/src/utils/tonic.rs @@ -1,9 +1,8 @@ use anyhow; use std::future::Future; -use std::result::Result; use tonic::{Code, Response, Status}; use tonic_types::{ErrorDetails, StatusExt}; -use tracing::error; +use tracing::{debug, error}; pub async fn escalate( req: tonic::Request, @@ -16,15 +15,24 @@ where let result = fun(req.into_inner()).await; match result { Ok(res) => Ok(Response::new(res)), - Err(any) => { - let err_details = ErrorDetails::new(); + Err(any_err) => { + // Convert root cause and stack to strings + let stack: Vec<_> = any_err.chain().skip(1).map(ToString::to_string).collect(); + let cause = any_err.root_cause().to_string(); + + // ...then dump them... + error!("Local error cause is {cause}"); + stack.iter().for_each(|e| error!("Local reasons is {e}")); + + // ...then pack to ErrorDetails + let err_details = ErrorDetails::with_debug_info(stack, cause); // Generate error status let status = Status::with_error_details( Code::InvalidArgument, "request contains invalid arguments", err_details, ); - error!("error handling GRPC request: {}", any); + error!("error handling GRPC request: {any_err}"); Err(status) } From 6a7506c04e57a06deb03d958e4a400c1d3deb8c6 Mon Sep 17 00:00:00 2001 From: "Alexander V. Nikolaev" Date: Wed, 11 Dec 2024 15:02:13 +0200 Subject: [PATCH 2/2] Suppress warnings Signed-off-by: Alexander V. Nikolaev --- client/src/error.rs | 1 - flake.nix | 1 - src/admin/server.rs | 4 ++-- src/utils/tonic.rs | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/client/src/error.rs b/client/src/error.rs index 28b636c..6f6b788 100644 --- a/client/src/error.rs +++ b/client/src/error.rs @@ -1,7 +1,6 @@ use anyhow::Error; use tonic::Status; use tonic_types::StatusExt; -use tracing::{debug, error}; fn rewrap_error(status: Status) -> Error { let mut err = Error::msg(status.message().to_owned()); diff --git a/flake.nix b/flake.nix index e66ec61..bf7b41b 100644 --- a/flake.nix +++ b/flake.nix @@ -23,7 +23,6 @@ }; crane = { url = "github:ipetkov/crane"; - inputs.nixpkgs.follows = "nixpkgs"; }; pre-commit-hooks-nix = { url = "github:cachix/pre-commit-hooks.nix"; diff --git a/src/admin/server.rs b/src/admin/server.rs index 2ec8d77..838de24 100644 --- a/src/admin/server.rs +++ b/src/admin/server.rs @@ -489,7 +489,7 @@ impl pb::admin_service_server::AdminService for AdminService { escalate(request, |_| async { self.inner .send_system_command(String::from("suspend.target")) - .await; + .await?; Ok(Empty {}) }) .await @@ -497,7 +497,7 @@ impl pb::admin_service_server::AdminService for AdminService { async fn wakeup( &self, - request: tonic::Request, + _request: tonic::Request, ) -> std::result::Result, tonic::Status> { println!("Not supported"); Err(Status::unimplemented("Not supported")) diff --git a/src/utils/tonic.rs b/src/utils/tonic.rs index 26638e0..0b01648 100644 --- a/src/utils/tonic.rs +++ b/src/utils/tonic.rs @@ -2,7 +2,7 @@ use anyhow; use std::future::Future; use tonic::{Code, Response, Status}; use tonic_types::{ErrorDetails, StatusExt}; -use tracing::{debug, error}; +use tracing::error; pub async fn escalate( req: tonic::Request,