SSHAction: Add return_result_on_error=True/False

Currently, if an SSHAction is launched, and the target script returns
with non-zero, the execution will be set to ERROR, and we can see in
the "execution output show":

"The action raised an exception
[action=<mistral.actions.std_actions.SSHAction object at 0x1234>,
action_ex_id=<UUID>, msg='Failed to execute ssh cmd
'/path/to/command param1 param2' on ['hostname']']"

That's not at all helpful for debugging, as the command stdout
and stderr aren't accessible anymore. One may want to have the ssh
output instead, and see what went wrong.

This patch adds a return_result_on_error new param to have the
feature (and to keep backward compatibility). When this is set,
when an SSH command is launched correctly, but returns non-zero,
the output will contain the stdout, stderr and return code.
Because of Mistral's design, the workflow execusion task cannot
be en ERROR, otherwise stdout/stderr/exit_code are overwritten
by Mistral exception. Therefore, if one wants to still have the
workflow execution in error, the exit code must be looked-up
later in the workflow. Lookup in the doc change included in
this patch.

Signed-off-by: Thomas Goirand <zigo@debian.org>
Change-Id: If71f5663c26457a5cf6b9eb88f15355e664b520e

doc/source/user/wf_lang_v2.rst

@@ -1239,10 +1239,61 @@ Input parameters:
    host in **<home-user-directory>/.ssh** directory or absolute path of
    the key should be provided. The file needs to be accessible
    for the user account running the executor. *Optional*.
+-  **return_result_on_error** - Boolean, defaults to false (to keep backward
+   behavior compatibility). If set to True, even if the remote command
+   returns non-zero, the task will be in SUCCESS, and the returned value
+   should be read to tell if the command succeeded.
 
 **NOTE**: Authentication using key pairs is supported, key should be
 on Mistral Executor server machine.
 
+Example with return_result_on_error=True:
+
+::
+
+    tasks:
+      execute_command:
+        action: std.ssh
+        input:
+          host: "node1.example.com"
+          username: "root"
+          private_key: |
+          -----BEGIN OPENSSH PRIVATE KEY-----
+          ...
+          -----END OPENSSH PRIVATE KEY-----
+          cmd: "/path/to/script"
+          return_result_on_error: true
+        on-complete:
+          - collect_output
+
+      collect_output:
+        action: std.noop
+        publish:
+          ssh_stdout: <% task(execute_command).result.stdout %>
+          exit_code: <% task(execute_command).result.exit_code %>
+        on-complete:
+          - decision_task
+
+      decision_task:
+        action: std.echo
+        input:
+          output: "Checking exit code..."
+        on-success:
+          - log_completion: <% $.exit_code = 0 %>
+          - fail_workflow: <% $.exit_code != 0 %>
+
+      fail_workflow:
+        action: std.fail
+
+    log_completion:
+      action: std.noop
+
+
+Here, the workflow will be in error if /path/to/script returns non-zero.
+However, it will still be possible to read the script output by reading
+the "execute_command" task, with a "openstack task execution result show"
+command.
+
 std.echo
 ''''''''
 

mistral/actions/std_actions.py

@@ -417,7 +417,8 @@ class SSHAction(actions.Action):
         return ssh_utils.execute_command
 
     def __init__(self, cmd, host, username,
-                 password="", private_key_filename=None, private_key=None):
+                 password="", private_key_filename=None, private_key=None,
+                 return_result_on_error=False):
         super(SSHAction, self).__init__()
 
         self.cmd = cmd
@@ -426,6 +427,7 @@ class SSHAction(actions.Action):
         self.password = password
         self.private_key_filename = private_key_filename
         self.private_key = private_key
+        self.return_result_on_error = return_result_on_error
 
         self.params = {
             'cmd': self.cmd,
@@ -437,7 +439,7 @@ class SSHAction(actions.Action):
         }
 
     def run(self, context):
-        def raise_exc(parent_exc=None):
+        def raise_exc(parent_exc=None, result=None):
             message = ("Failed to execute ssh cmd "
                        "'%s' on %s" % (self.cmd, self.host))
             # We suppress the actual parent error messages in favor of
@@ -445,6 +447,9 @@ class SSHAction(actions.Action):
             if parent_exc:
                 # The full error message needs to be logged regardless
                 LOG.exception(message + " Exception: %s", str(parent_exc))
+            # New behavior: return result even on error if flag is set
+            if self.return_result_on_error and result is not None:
+                return result
             raise exc.ActionException(message)
 
         try:
@@ -456,13 +461,29 @@ class SSHAction(actions.Action):
             for host_name in self.host:
                 self.params['host'] = host_name
 
-                status_code, result = self._execute_cmd_method(**self.params)
+                if self.return_result_on_error:
+                    # Execute command - always get the result data and stderr
+                    status_code, stdout, stderr = self._execute_cmd_method(
+                        raise_when_error=False, get_stderr=True, **self.params)
+
+                    # Always build consistent result structure
+                    result = {
+                        'stdout': stdout,
+                        'stderr': stderr,
+                        'exit_code': status_code
+                    }
 
-                if status_code > 0:
-                    return raise_exc()
-                else:
                     results.append(result)
 
+                else:
+                    status_code, result = self._execute_cmd_method(
+                        **self.params)
+
+                    if status_code > 0:
+                        return raise_exc()
+                    results.append(result)
+
+            # return result(s) as a dict if host was a dict
             if len(results) > 1:
                 return results
 

mistral/tests/unit/actions/test_std_ssh_action.py

@@ -63,18 +63,163 @@ class SSHActionTest(base.BaseTest):
         )
 
     @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
-    def test_ssh_action_with_stderr(self, mocked_method):
-        mocked_method.return_value = (1, 'Error expected')
+    def test_with_return_result_on_error_success(self, mocked_method):
+        # Test with return_result_on_error=True and success
+        mocked_method.return_value = (0, 'success stdout', 'success stderr')
         cmd = "echo -n ok"
         host = "localhost"
         username = "mistral"
-        action = std.SSHAction(cmd, host, username)
+        action = std.SSHAction(
+            cmd, host, username,
+            return_result_on_error=True
+        )
 
         mock_ctx = None
 
-        self.assertRaisesWithMessageContaining(
-            exc.ActionException,
-            "Failed to execute ssh cmd 'echo -n ok' on ['localhost']",
-            action.run,
-            mock_ctx
+        result = action.run(mock_ctx)
+
+        self.assertIsInstance(result, dict)
+        self.assertEqual('success stdout', result['stdout'])
+        self.assertEqual('success stderr', result['stderr'])
+        self.assertEqual(0, result['exit_code'])
+
+        mocked_method.assert_called_with(
+            raise_when_error=False,
+            get_stderr=True,
+            cmd=cmd,
+            host=host,
+            username=username,
+            password='',
+            private_key_filename=None,
+            private_key=None
         )
+
+    @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
+    def test_with_return_result_on_error_failure(self, mocked_method):
+        # Test with return_result_on_error=True and command failure (should not
+        # raise exception)
+        mocked_method.return_value = (1, 'error stdout', 'error stderr')
+        cmd = "echo -n ok"
+        host = "localhost"
+        username = "mistral"
+        action = std.SSHAction(
+            cmd, host, username,
+            return_result_on_error=True
+        )
+
+        mock_ctx = None
+
+        result = action.run(mock_ctx)
+
+        # Should NOT raise exception, should return structured result
+        self.assertIsInstance(result, dict)
+        self.assertEqual('error stdout', result['stdout'])
+        self.assertEqual('error stderr', result['stderr'])
+        self.assertEqual(1, result['exit_code'])
+
+    @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
+    def test_with_return_result_on_error_exception_handling(self,
+                                                            mocked_method):
+        # Test SSH connection failure handling
+        # SHOULD still raise exception
+        mocked_method.side_effect = Exception("SSH connection failed")
+        cmd = "echo -n ok"
+        host = "localhost"
+        username = "mistral"
+        action = std.SSHAction(
+            cmd, host, username,
+            return_result_on_error=True
+        )
+
+        mock_ctx = None
+
+        # SSH connection failures should still raise exceptions
+        # even with return_result_on_error=True
+        self.assertRaises(exc.ActionException, action.run, mock_ctx)
+
+    @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
+    def test_default_behavior_unchanged(self, mocked_method):
+        # Test that default behavior (return_result_on_error=False)
+        # is unchanged
+        mocked_method.return_value = (0, 'ok')
+        cmd = "echo -n ok"
+        host = "localhost"
+        username = "mistral"
+        # return_result_on_error defaults to False
+        action = std.SSHAction(cmd, host, username)
+        mock_ctx = None
+
+        stdout = action.run(mock_ctx)
+
+        self.assertEqual('ok', stdout)
+        mocked_method.assert_called_with(
+            cmd=cmd,
+            host=host,
+            username=username,
+            password='',
+            private_key_filename=None,
+            private_key=None
+        )
+
+    @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
+    def test_default_behavior_raises_on_failure(self, mocked_method):
+        # Test that default behavior raises exception on failure
+        mocked_method.return_value = (1, 'error')
+        cmd = "echo -n ok"
+        host = "localhost"
+        username = "mistral"
+        # return_result_on_error defaults to False
+        action = std.SSHAction(cmd, host, username)
+
+        mock_ctx = None
+        self.assertRaises(exc.ActionException, action.run, mock_ctx)
+
+    @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
+    def test_multiple_hosts_with_return_result_on_error(self, mocked_method):
+        # Test multiple hosts with return_result_on_error=True
+        mocked_method.side_effect = [
+            (0, 'host1 stdout', 'host1 stderr'),
+            (1, 'host2 stdout', 'host2 stderr')
+        ]
+        cmd = "echo -n ok"
+        host = ["host1", "host2"]
+        username = "mistral"
+        action = std.SSHAction(
+            cmd, host, username,
+            return_result_on_error=True
+        )
+
+        mock_ctx = None
+
+        result = action.run(mock_ctx)
+
+        # Should return list of results
+        self.assertIsInstance(result, list)
+        self.assertEqual(2, len(result))
+        self.assertEqual('host1 stdout', result[0]['stdout'])
+        self.assertEqual(0, result[0]['exit_code'])
+        self.assertEqual('host2 stdout', result[1]['stdout'])
+        self.assertEqual(1, result[1]['exit_code'])
+
+    @mock.patch.object(mistral.utils.ssh_utils, 'execute_command')
+    def test_single_host_returns_dict_not_list(self, mocked_method):
+        # Test that single host returns dict, not list
+        mocked_method.return_value = (0, 'stdout', 'stderr')
+        cmd = "echo -n ok"
+        host = "localhost"
+        username = "mistral"
+        action = std.SSHAction(
+            cmd, host, username,
+            return_result_on_error=True
+        )
+
+        mock_ctx = None
+
+        result = action.run(mock_ctx)
+
+        # Should return single dict, not list
+        self.assertIsInstance(result, dict)
+        self.assertNotIsInstance(result, list)
+        self.assertEqual('stdout', result['stdout'])
+        self.assertEqual('stderr', result['stderr'])
+        self.assertEqual(0, result['exit_code'])

releasenotes/notes/SSHAction-return_result_on_error-631427c7af28ced7.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    The `std.ssh` action has a new `return_result_on_error` parameter. When it
+    is set, the std.ssh action also returns with SUCCESS state when the remote
+    command returns non-zero. Users of the std.ssh action should then use the
+    return value structure to decide if the workflow should fail.