Merge pull request #3058 from jarhodes314/bug/software-update-error-d…

…etails fix: display error information for failed software update due to unrecognised type
thin-edge · Aug 19, 2024 · be7ef4c · be7ef4c
2 parents 412f491 + 2e08019
commit be7ef4c
Show file tree

Hide file tree

Showing 8 changed files with 101 additions and 37 deletions.
diff --git a/crates/core/c8y_api/src/json_c8y_deserializer.rs b/crates/core/c8y_api/src/json_c8y_deserializer.rs
@@ -248,6 +248,7 @@ impl TryFrom<C8ySoftwareUpdate> for SoftwareInfo {
                     .push(SoftwareRequestResponseSoftwareList {
                         plugin_type,
                         modules: vec![item],
+                        errors: vec![],
                     });
             }
         }

diff --git a/crates/core/plugin_sm/src/plugin_manager.rs b/crates/core/plugin_sm/src/plugin_manager.rs
@@ -1,6 +1,7 @@
 use crate::plugin::ExternalPluginCommand;
 use crate::plugin::Plugin;
 use crate::plugin::LIST;
+use std::borrow::Cow;
 use std::collections::BTreeMap;
 use std::fs;
 use std::io::ErrorKind;
@@ -34,16 +35,6 @@ pub trait Plugins {
     /// Return the plugin associated with the file extension of the module name, if any.
     fn by_file_extension(&self, module_name: &str) -> Option<&Self::Plugin>;
 
-    fn plugin(&self, software_type: &str) -> Result<&Self::Plugin, SoftwareError> {
-        let module_plugin = self.by_software_type(software_type).ok_or_else(|| {
-            SoftwareError::UnknownSoftwareType {
-                software_type: software_type.into(),
-            }
-        })?;
-
-        Ok(module_plugin)
-    }
-
     fn update_default(&mut self, new_default: &Option<SoftwareType>) -> Result<(), SoftwareError>;
 
     fn get_all_software_types(&self) -> Vec<SoftwareType>;
@@ -230,22 +221,20 @@ impl ExternalPlugins {
         mut response: SoftwareListCommand,
         mut command_log: Option<CommandLog>,
     ) -> SoftwareListCommand {
-        let mut error_count = 0;
+        let mut errors = Vec::new();
 
         if self.plugin_map.is_empty() {
             response.add_modules("".into(), vec![]);
         } else {
             for (software_type, plugin) in self.plugin_map.iter() {
                 match plugin.list(command_log.as_mut()).await {
                     Ok(software_list) => response.add_modules(software_type.clone(), software_list),
-                    Err(_) => {
-                        error_count += 1;
-                    }
+                    Err(err) => errors.push(err.to_string()),
                 }
             }
         }
 
-        if let Some(reason) = ExternalPlugins::error_message(error_count, command_log) {
+        if let Some(reason) = ExternalPlugins::error_message(errors, command_log) {
             response.with_error(reason)
         } else {
             response.with_status(CommandStatus::Successful)
@@ -259,43 +248,52 @@ impl ExternalPlugins {
         download_path: &Path,
     ) -> SoftwareUpdateCommand {
         let mut response = request.clone().with_status(CommandStatus::Executing);
-        let mut error_count = 0;
+        let mut error_messages = Vec::new();
 
         for software_type in request.modules_types() {
+            let updates = request.updates_for(&software_type);
             let errors = if let Some(plugin) = self.by_software_type(&software_type) {
-                let updates = request.updates_for(&software_type);
                 plugin
                     .apply_all(updates, command_log.as_mut(), download_path)
                     .await
             } else {
-                vec![SoftwareError::UnknownSoftwareType {
+                let error = SoftwareError::UnknownSoftwareType {
                     software_type: software_type.clone(),
-                }]
+                    updates,
+                };
+                if let Some(command_log) = &mut command_log {
+                    command_log.log_error(&error.to_string()).await;
+                }
+                vec![error]
             };
 
             if !errors.is_empty() {
-                error_count += 1;
+                let message = errors
+                    .iter()
+                    .map(|e| e.to_string())
+                    .collect::<Vec<_>>()
+                    .join(",");
+                error_messages.push(message);
                 response.add_errors(&software_type, errors);
             }
         }
 
-        if let Some(reason) = ExternalPlugins::error_message(error_count, command_log) {
+        if let Some(reason) = ExternalPlugins::error_message(error_messages, command_log) {
             response.with_error(reason)
         } else {
             response.with_status(CommandStatus::Successful)
         }
     }
 
-    fn error_message(error_count: i32, command_log: Option<CommandLog>) -> Option<String> {
-        if error_count > 0 {
-            let reason = if error_count == 1 {
-                "1 error".into()
-            } else {
-                format!("{} errors", error_count)
+    fn error_message(errors: Vec<String>, command_log: Option<CommandLog>) -> Option<String> {
+        if !errors.is_empty() {
+            let reason = match &errors[..] {
+                [single_error_message] => Cow::Borrowed(single_error_message),
+                _ => Cow::Owned(format!("{} errors", errors.len())),
             };
             let reason = command_log
                 .map(|log| format!("{}, see device log file {}", reason, log.path))
-                .unwrap_or(reason);
+                .unwrap_or_else(|| reason.into_owned());
             Some(reason)
         } else {
             None

diff --git a/crates/core/tedge_agent/src/operation_workflows/tests.rs b/crates/core/tedge_agent/src/operation_workflows/tests.rs
@@ -113,6 +113,7 @@ async fn convert_incoming_software_update_request() -> Result<(), DynError> {
     let debian_list = SoftwareRequestResponseSoftwareList {
         plugin_type: "debian".into(),
         modules: vec![debian_module1],
+        errors: vec![],
     };
 
     // The output of converter => SoftwareUpdateCommand

diff --git a/crates/core/tedge_agent/src/software_manager/tests.rs b/crates/core/tedge_agent/src/software_manager/tests.rs
@@ -88,6 +88,7 @@ async fn test_new_software_update_operation() -> Result<(), DynError> {
     let debian_list = SoftwareRequestResponseSoftwareList {
         plugin_type: "debian".into(),
         modules: vec![debian_module1],
+        errors: vec![],
     };
 
     let command = SoftwareUpdateCommand {

diff --git a/crates/core/tedge_api/src/commands.rs b/crates/core/tedge_api/src/commands.rs
@@ -468,6 +468,7 @@ impl SoftwareUpdateCommand {
                 .update_list
                 .push(SoftwareRequestResponseSoftwareList {
                     plugin_type,
+                    errors: vec![],
                     modules: vec![update.into()],
                 });
         }
@@ -478,6 +479,7 @@ impl SoftwareUpdateCommand {
             .update_list
             .push(SoftwareRequestResponseSoftwareList {
                 plugin_type: plugin_type.to_string(),
+                errors: vec![],
                 modules: updates
                     .into_iter()
                     .map(|update| update.into())
@@ -533,9 +535,10 @@ impl SoftwareUpdateCommand {
             .failures
             .push(SoftwareRequestResponseSoftwareList {
                 plugin_type: plugin_type.to_string(),
+                errors: errors.iter().map(|e| e.to_string()).collect(),
                 modules: errors
                     .into_iter()
-                    .filter_map(|update| update.into())
+                    .flat_map(Vec::<SoftwareModuleItem>::from)
                     .collect::<Vec<SoftwareModuleItem>>(),
             })
     }
@@ -559,6 +562,8 @@ pub struct SoftwareRequestResponseSoftwareList {
     #[serde(rename = "type")]
     pub plugin_type: SoftwareType,
     pub modules: Vec<SoftwareModuleItem>,
+    #[serde(skip)]
+    pub errors: Vec<String>,
 }
 
 /// Variants represent Software Operations Supported actions.
@@ -623,24 +628,44 @@ impl From<SoftwareModuleUpdate> for SoftwareModuleItem {
     }
 }
 
-impl From<SoftwareError> for Option<SoftwareModuleItem> {
+impl From<SoftwareError> for Vec<SoftwareModuleItem> {
     fn from(error: SoftwareError) -> Self {
         match error {
-            SoftwareError::Install { module, reason } => Some(SoftwareModuleItem {
+            SoftwareError::Install { module, reason } => vec![SoftwareModuleItem {
                 name: module.name,
                 version: module.version,
                 url: module.url,
                 action: Some(SoftwareModuleAction::Install),
                 reason: Some(reason),
-            }),
-            SoftwareError::Remove { module, reason } => Some(SoftwareModuleItem {
+            }],
+            SoftwareError::Remove { module, reason } => vec![SoftwareModuleItem {
                 name: module.name,
                 version: module.version,
                 url: module.url,
                 action: Some(SoftwareModuleAction::Remove),
                 reason: Some(reason),
-            }),
-            _ => None,
+            }],
+            SoftwareError::UnknownSoftwareType {
+                updates,
+                software_type,
+            } => {
+                let reason = format!("Unknown software type: {software_type:?}");
+                updates
+                    .into_iter()
+                    .map(|update| {
+                        let action = update.action();
+                        let module = update.into_module();
+                        SoftwareModuleItem {
+                            name: module.name,
+                            version: module.version,
+                            url: module.url,
+                            action: Some(action),
+                            reason: Some(reason.clone()),
+                        }
+                    })
+                    .collect()
+            }
+            _ => vec![],
         }
     }
 }
@@ -1004,6 +1029,7 @@ mod tests {
         let debian_list = SoftwareRequestResponseSoftwareList {
             plugin_type: "debian".into(),
             modules: vec![debian_module1, debian_module2],
+            errors: vec![],
         };
 
         let docker_module1 = SoftwareModuleItem {
@@ -1017,6 +1043,7 @@ mod tests {
         let docker_list = SoftwareRequestResponseSoftwareList {
             plugin_type: "docker".into(),
             modules: vec![docker_module1],
+            errors: vec![],
         };
 
         let request = SoftwareUpdateCommandPayload {

diff --git a/crates/core/tedge_api/src/error.rs b/crates/core/tedge_api/src/error.rs
@@ -1,4 +1,5 @@
 use crate::software::SoftwareModule;
+use crate::software::SoftwareModuleUpdate;
 use crate::software::SoftwareName;
 use crate::software::SoftwareType;
 use crate::software::SoftwareVersion;
@@ -73,8 +74,11 @@ pub enum SoftwareError {
         name: SoftwareName,
     },
 
-    #[error("Unknown software type: {software_type:?}")]
-    UnknownSoftwareType { software_type: SoftwareType },
+    #[error("Unknown software type: {software_type:?}. The affected packages are: {:?}", module_names(.updates))]
+    UnknownSoftwareType {
+        software_type: SoftwareType,
+        updates: Vec<SoftwareModuleUpdate>,
+    },
 
     #[error("Unexpected module type: {actual:?}, should be: {expected:?}")]
     WrongModuleType {
@@ -105,6 +109,13 @@ pub enum SoftwareError {
     RegexError { reason: String },
 }
 
+fn module_names(updates: &[SoftwareModuleUpdate]) -> Vec<String> {
+    updates
+        .iter()
+        .map(|update| update.module().name.to_owned())
+        .collect()
+}
+
 impl From<serde_json::Error> for SoftwareError {
     fn from(err: serde_json::Error) -> Self {
         SoftwareError::ParseError {

diff --git a/crates/core/tedge_api/src/software.rs b/crates/core/tedge_api/src/software.rs
@@ -4,6 +4,8 @@ use std::path::PathBuf;
 
 use download::DownloadInfo;
 
+use crate::commands::SoftwareModuleAction;
+
 pub type SoftwareType = String;
 pub type SoftwareName = String;
 pub type SoftwareVersion = String;
@@ -82,6 +84,13 @@ impl SoftwareModuleUpdate {
         SoftwareModuleUpdate::Remove { module }
     }
 
+    pub fn action(&self) -> SoftwareModuleAction {
+        match self {
+            SoftwareModuleUpdate::Install { .. } => SoftwareModuleAction::Install,
+            SoftwareModuleUpdate::Remove { .. } => SoftwareModuleAction::Remove,
+        }
+    }
+
     pub fn module(&self) -> &SoftwareModule {
         match self {
             SoftwareModuleUpdate::Install { module } | SoftwareModuleUpdate::Remove { module } => {
@@ -90,6 +99,14 @@ impl SoftwareModuleUpdate {
         }
     }
 
+    pub fn into_module(self) -> SoftwareModule {
+        match self {
+            SoftwareModuleUpdate::Install { module } | SoftwareModuleUpdate::Remove { module } => {
+                module
+            }
+        }
+    }
+
     pub fn normalize(&mut self) {
         let module = match self {
             SoftwareModuleUpdate::Install { module } | SoftwareModuleUpdate::Remove { module } => {

diff --git a/crates/core/tedge_api/src/workflow/log/command_log.rs b/crates/core/tedge_api/src/workflow/log/command_log.rs
@@ -182,6 +182,14 @@ Action:   {action}
         }
     }
 
+    pub async fn log_error(&mut self, msg: &str) {
+        error!("{msg}");
+        let line = format!("ERROR: {msg}\n");
+        if let Err(err) = self.write(&line).await {
+            error!("Fail to log to {}: {err}", self.path)
+        }
+    }
+
     pub async fn write_script_output(
         &mut self,
         command_line: &str,