From bfeef067aac7572bb914396db4553da5c013403f Mon Sep 17 00:00:00 2001
From: Julia Kreger <juliaashleykreger@gmail.com>
Date: Wed, 22 Apr 2020 08:42:29 -0700
Subject: [PATCH] Fix agent_client handling of embedded errors

The agent_client may get a general "command_error" field
returned to it upon commands and the agent may not properly
sinal that the command failed because we are reliant upon
the same data elsewhere in the ironic/agent interaction.

This resulted in the case where the embedded error signaled that
the error was method compatability as opposed to the actual method
command error.

This could cause higher level failures and prevent fallback logic
from detecting that we could try a different command.

We now consider the error type, and raise the appropriate exception
to signal that the issue may be an API compatability issue.

Change-Id: Ia2f63bd853632e1d7138901cf23fde1e261fc4d6
---
 ironic/drivers/modules/agent_client.py        | 17 ++++++++++++++-
 .../unit/drivers/modules/test_agent_client.py | 21 +++++++++++++++++++
 ...dded_ipa_error_codes-c8fdfaa9e6a1ed06.yaml |  9 ++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/consider_embedded_ipa_error_codes-c8fdfaa9e6a1ed06.yaml

diff --git a/ironic/drivers/modules/agent_client.py b/ironic/drivers/modules/agent_client.py
index 4c17403b96..feb4ebc88d 100644
--- a/ironic/drivers/modules/agent_client.py
+++ b/ironic/drivers/modules/agent_client.py
@@ -127,11 +127,18 @@ class AgentClient(object):
             LOG.error(msg)
             raise exception.IronicException(msg)
 
+        error = result.get('command_error')
+        exc_type = None
+        if error:
+            # if an error, we should see if a type field exists. This type
+            # field may signal an exception that is compatability based.
+            exc_type = error.get('type')
+
         LOG.debug('Agent command %(method)s for node %(node)s returned '
                   'result %(res)s, error %(error)s, HTTP status code %(code)d',
                   {'node': node.uuid, 'method': method,
                    'res': result.get('command_result'),
-                   'error': result.get('command_error'),
+                   'error': error,
                    'code': response.status_code})
 
         if response.status_code >= http_client.BAD_REQUEST:
@@ -142,6 +149,14 @@ class AgentClient(object):
             raise exception.AgentAPIError(node=node.uuid,
                                           status=response.status_code,
                                           error=result.get('faultstring'))
+        if exc_type == 'TypeError':
+            LOG.error('Agent command %(method)s for node %(node)s failed. '
+                      'Internal %(exc_type)s error detected: Error %(error)s',
+                      {'method': method, 'node': node.uuid,
+                       'exc_type': exc_type, 'error': error})
+            raise exception.AgentAPIError(node=node.uuid,
+                                          status=error.get('code'),
+                                          error=result.get('faultstring'))
 
         return result
 
diff --git a/ironic/tests/unit/drivers/modules/test_agent_client.py b/ironic/tests/unit/drivers/modules/test_agent_client.py
index 65bb58172d..1bea741880 100644
--- a/ironic/tests/unit/drivers/modules/test_agent_client.py
+++ b/ironic/tests/unit/drivers/modules/test_agent_client.py
@@ -178,6 +178,27 @@ class TestAgentClient(base.TestCase):
             params={'wait': 'false'},
             timeout=60)
 
+    def test__command_error_code_okay_error_typeerror_embedded(self):
+        response_text = ('{"faultstring": "you dun goofd", '
+                         '"command_error": {"type": "TypeError"}}')
+        self.client.session.post.return_value = MockResponse(
+            response_text)
+        method = 'standby.run_image'
+        image_info = {'image_id': 'test_image'}
+        params = {'image_info': image_info}
+
+        url = self.client._get_command_url(self.node)
+        body = self.client._get_command_body(method, params)
+
+        self.assertRaises(exception.AgentAPIError,
+                          self.client._command,
+                          self.node, method, params)
+        self.client.session.post.assert_called_once_with(
+            url,
+            data=body,
+            params={'wait': 'false'},
+            timeout=60)
+
     def test_get_commands_status(self):
         with mock.patch.object(self.client.session, 'get',
                                autospec=True) as mock_get:
diff --git a/releasenotes/notes/consider_embedded_ipa_error_codes-c8fdfaa9e6a1ed06.yaml b/releasenotes/notes/consider_embedded_ipa_error_codes-c8fdfaa9e6a1ed06.yaml
new file mode 100644
index 0000000000..0f4ee2d512
--- /dev/null
+++ b/releasenotes/notes/consider_embedded_ipa_error_codes-c8fdfaa9e6a1ed06.yaml
@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Fixes an issue in the ``ironic-python-agent`` client code
+    where a command exception may not be captured in the interaction
+    with the agent rest API. The client code would return the resulting
+    error message and a static error code. We now look with-in the error
+    to detect if the error may be a compatability error to raise the
+    appropriate exception for fallback logic to engage.