Merge pull request #3079 from didier-wenzek/feat/improve-reporting-of…

…-workflow-definition-errors

feat: report to the cloud workflow definition errors

crates/core/tedge_agent/src/operation_workflows/mod.rs

@@ -3,6 +3,7 @@ use camino::Utf8PathBuf;
 use log::error;
 use std::ffi::OsStr;
 use std::path::Path;
+use tedge_api::workflow::IllFormedOperationWorkflow;
 use tedge_api::workflow::OperationWorkflow;
 use tedge_api::workflow::WorkflowSupervisor;
 use tracing::info;
@@ -35,7 +36,7 @@ pub async fn load_operation_workflows(
                     );
                 }
                 Err(err) => {
-                    error!("Ignoring operation workflow definition from {file:?}: {err:?}")
+                    error!("Ignoring {file:?}: {err:?}")
                 }
             };
         }
@@ -44,12 +45,19 @@ pub async fn load_operation_workflows(
 }
 
 async fn read_operation_workflow(path: &Path) -> Result<OperationWorkflow, anyhow::Error> {
-    let bytes = tokio::fs::read(path)
-        .await
-        .context("Reading file content")?;
-    let input = std::str::from_utf8(&bytes).context("Expecting UTF8 content")?;
-    let workflow = toml::from_str::<OperationWorkflow>(input).context("Parsing TOML content")?;
-    Ok(workflow)
+    let bytes = tokio::fs::read(path).await.context("Fail to read file")?;
+    let input = std::str::from_utf8(&bytes).context("Fail to extract UTF8 content")?;
+
+    toml::from_str::<OperationWorkflow>(input)
+        .context("Fail to parse TOML")
+        .or_else(|err| {
+            error!("Ill-formed operation workflow definition from {path:?}: {err:?}");
+            let workflow = toml::from_str::<IllFormedOperationWorkflow>(input)
+                .context("Extracting operation name")?;
+
+            let reason = format!("Invalid operation workflow definition {path:?}: {err:?}");
+            Ok(OperationWorkflow::ill_formed(workflow.operation, reason))
+        })
 }
 
 fn load_operation_workflow(

crates/core/tedge_api/src/workflow/error.rs

@@ -1,3 +1,5 @@
+use serde::Deserialize;
+
 /// Error preventing a workflow to be registered
 #[derive(thiserror::Error, Debug, Eq, PartialEq)]
 pub enum WorkflowDefinitionError {
@@ -83,3 +85,9 @@ pub enum WorkflowExecutionError {
     #[error("No such step is defined for {operation}: {step}")]
     UnknownStep { operation: String, step: String },
 }
+
+/// Struct used to recover the bare minimum information from an ill-formed workflow TOML file.
+#[derive(Deserialize)]
+pub struct IllFormedOperationWorkflow {
+    pub operation: String,
+}

crates/core/tedge_api/src/workflow/mod.rs

@@ -55,7 +55,7 @@ pub enum OperationAction {
     /// action = "proceed"
     /// on_success = "<state>"
     /// ```
-    MoveTo(StateName),
+    MoveTo(GenericStateUpdate),
 
     /// The built-in behavior is used
     ///
@@ -213,7 +213,7 @@ impl OperationWorkflow {
     /// Create a built-in operation workflow
     pub fn built_in(operation: OperationType) -> Self {
         let states = [
-            ("init", OperationAction::MoveTo("scheduled".to_string())),
+            ("init", OperationAction::MoveTo("scheduled".into())),
             ("scheduled", OperationAction::BuiltIn),
             ("executing", OperationAction::BuiltIn),
             ("successful", OperationAction::Clear),
@@ -231,6 +231,31 @@ impl OperationWorkflow {
         }
     }
 
+    /// Create a workflow that systematically fail any command with a static error
+    ///
+    /// The point is to raise an error to the user when a workflow definition cannot be parsed,
+    /// instead of silently ignoring the commands.
+    pub fn ill_formed(operation: String, reason: String) -> Self {
+        let states = [
+            ("init", OperationAction::MoveTo("executing".into())),
+            (
+                "executing",
+                OperationAction::MoveTo(GenericStateUpdate::failed(reason)),
+            ),
+            ("failed", OperationAction::Clear),
+        ]
+        .into_iter()
+        .map(|(state, action)| (state.to_string(), action))
+        .collect();
+
+        OperationWorkflow {
+            built_in: true,
+            operation: operation.as_str().into(),
+            handlers: DefaultHandlers::default(),
+            states,
+        }
+    }
+
     /// Return the MQTT message to register support for the operation described by this workflow
     pub fn capability_message(
         &self,

crates/core/tedge_api/src/workflow/state.rs

@@ -207,9 +207,14 @@ impl GenericCommandState {
     }
 
     /// Update the command state with a new status describing the next state
-    pub fn move_to(mut self, status: String) -> Self {
+    pub fn move_to(mut self, update: GenericStateUpdate) -> Self {
+        let status = update.status;
         GenericCommandState::inject_text_property(&mut self.payload, STATUS, &status);
 
+        if let Some(reason) = update.reason {
+            GenericCommandState::inject_text_property(&mut self.payload, REASON, &reason);
+        }
+
         GenericCommandState { status, ..self }
     }
 
@@ -511,6 +516,12 @@ impl From<&str> for GenericStateUpdate {
     }
 }
 
+impl Display for GenericStateUpdate {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.status.fmt(f)
+    }
+}
+
 impl From<GenericStateUpdate> for Value {
     fn from(update: GenericStateUpdate) -> Self {
         match update.reason {
@@ -674,7 +685,7 @@ mod tests {
             }
         );
 
-        let update_cmd = cmd.move_to("executing".to_string());
+        let update_cmd = cmd.move_to("executing".into());
         assert_eq!(
             update_cmd,
             GenericCommandState {

crates/core/tedge_api/src/workflow/toml_config.rs

@@ -159,8 +159,8 @@ impl TryFrom<(TomlOperationState, DefaultHandlers)> for OperationAction {
                         .handlers
                         .on_success
                         .map(|u| u.into())
-                        .unwrap_or_else(|| "successful".to_string().into());
-                    Ok(OperationAction::MoveTo(on_success.status))
+                        .unwrap_or_else(GenericStateUpdate::successful);
+                    Ok(OperationAction::MoveTo(on_success))
                 }
                 "await-agent-restart" => {
                     let handlers = TryInto::<AwaitHandlers>::try_into((input.handlers, defaults))?;

tests/RobotFramework/tests/tedge_agent/workflows/custom_operation.robot

@@ -113,6 +113,13 @@ Command steps should not be executed twice
         Should Be Equal    ${workflow_log}    ${expected_log}
     END
 
+Placeholder workflow created for ill-defined operations
+    Execute Command    tedge mqtt pub --retain te/device/main///cmd/issue-3079/test-1 '{"status":"init"}'
+    ${cmd_messages}    Should Have MQTT Messages    te/device/main///cmd/issue-3079/test-1    message_pattern=.*Invalid operation workflow definition.*
+    Execute Command    tedge mqtt pub --retain te/device/main///cmd/issue-3079/test-1 ''
+    ${workflow_log}=   Execute Command    cat /var/log/tedge/agent/workflow-issue-3079-test-1.log
+    Should Contain     ${workflow_log}    item=TOML parse error
+
 *** Keywords ***
 
 Custom Test Setup
@@ -148,3 +155,4 @@ Copy Configuration Files
     ThinEdgeIO.Transfer To Device    ${CURDIR}/lite_software_update.toml          /etc/tedge/operations/
     ThinEdgeIO.Transfer To Device    ${CURDIR}/lite_device_profile.example.txt    /etc/tedge/operations/
     ThinEdgeIO.Transfer To Device    ${CURDIR}/issue-2896.toml                    /etc/tedge/operations/
+    ThinEdgeIO.Transfer To Device    ${CURDIR}/issue-3079.toml                    /etc/tedge/operations/

tests/RobotFramework/tests/tedge_agent/workflows/issue-3079.toml

@@ -0,0 +1,16 @@
+# An operation workflow with a typo
+operation = "issue-3079"
+
+[init]
+action = "proceed"
+on_success = "executing"
+
+[executing]          
+# Oops: the action is missing!
+on_success = "successful"
+
+[successful]
+action = "cleanup"
+
+[failed]
+action = "cleanup"