From 1e6a07c5fc8275b11044091cfbd0b580d3a0ebd3 Mon Sep 17 00:00:00 2001
From: Bence Romsics <bence.romsics@ericsson.com>
Date: Wed, 21 Nov 2018 17:12:51 +0100
Subject: [PATCH] Placement client: improve Placement 4xx exceptions

For 4xx errors Placement sends back a complex JSON object describing
the error. When turned into an exception that becomes a non-trivial
attribute of the error object. Usual ways of logging an exception (that
is LOG.exception) completely ignore that attribute, therefore the real
error message is not logged. For example we only logged the fact that
we received a BadRequest response and nothing else while Placement did
provide a whole lot more detail. Here we dig out that error detail and
re-throw a better exception with it.

Change-Id: Id97116c1c298f54f898a746d6e3c96b1f412bb49
Related-Bug: #1578989
---
 neutron_lib/exceptions/placement.py            |  4 ++++
 neutron_lib/placement/client.py                | 18 +++++++++++++++---
 .../tests/unit/placement/test_client.py        |  3 ++-
 ...ot-swallow-exceptions-c33c9a9224a27551.yaml |  8 ++++++++
 4 files changed, 29 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/placement-client-do-not-swallow-exceptions-c33c9a9224a27551.yaml

diff --git a/neutron_lib/exceptions/placement.py b/neutron_lib/exceptions/placement.py
index 7f853c170..c0b0a1dad 100644
--- a/neutron_lib/exceptions/placement.py
+++ b/neutron_lib/exceptions/placement.py
@@ -65,3 +65,7 @@ class PlacementAPIVersionIncorrect(exceptions.NotFound):
 class PlacementResourceProviderNameNotUnique(exceptions.Conflict):
     message = _("Another resource provider exists with the provided name: "
                 "%(name)s.")
+
+
+class PlacementClientError(exceptions.NeutronException):
+    message = _("Placement Client Error (4xx): %(msg)s")
diff --git a/neutron_lib/placement/client.py b/neutron_lib/placement/client.py
index b17c5fa22..fed0e3f2e 100644
--- a/neutron_lib/placement/client.py
+++ b/neutron_lib/placement/client.py
@@ -43,9 +43,21 @@ def _check_placement_api_available(f):
     """
     @functools.wraps(f)
     def wrapper(self, *a, **k):
-        if not self._client:
-            self._client = self._create_client()
-        return f(self, *a, **k)
+        try:
+            if not self._client:
+                self._client = self._create_client()
+            return f(self, *a, **k)
+        except ks_exc.http.HttpError as exc:
+            if 400 <= exc.http_status <= 499:
+                # NOTE(bence romsics): Placement has inconsistently formatted
+                # error messages. Some error response bodies are JSON
+                # formatted, seemingly machine readible objects. While others
+                # are free format text. We have to keep the whole thing
+                # to avoid losing information.
+                raise n_exc.PlacementClientError(
+                    msg=exc.response.text.replace('\n', ' '))
+            else:
+                raise
     return wrapper
 
 
diff --git a/neutron_lib/tests/unit/placement/test_client.py b/neutron_lib/tests/unit/placement/test_client.py
index a75e4cb71..508ec0e64 100644
--- a/neutron_lib/tests/unit/placement/test_client.py
+++ b/neutron_lib/tests/unit/placement/test_client.py
@@ -223,8 +223,9 @@ class TestPlacementAPIClient(base.BaseTestCase):
     def test_get_inventory_not_found(self):
         _exception = ks_exc.NotFound()
         _exception.details = "Any other exception explanation"
+        _exception.response = mock.Mock(text="Some error response body")
         self.placement_fixture.mock_get.side_effect = _exception
-        self.assertRaises(ks_exc.NotFound,
+        self.assertRaises(n_exc.PlacementClientError,
                           self.placement_api_client.get_inventory,
                           RESOURCE_PROVIDER_UUID, RESOURCE_CLASS_NAME)
 
diff --git a/releasenotes/notes/placement-client-do-not-swallow-exceptions-c33c9a9224a27551.yaml b/releasenotes/notes/placement-client-do-not-swallow-exceptions-c33c9a9224a27551.yaml
new file mode 100644
index 000000000..c22faba6a
--- /dev/null
+++ b/releasenotes/notes/placement-client-do-not-swallow-exceptions-c33c9a9224a27551.yaml
@@ -0,0 +1,8 @@
+---
+other:
+  - |
+    The Placement client previously swallowed a few exceptions (but logged
+    a warning when doing this). In order to let the user of the client choose
+    to handle or ignore the error condition the client no longer does this.
+    Also to avoid losing error information we catch and re-throw HTTP 4xx
+    exceptions with better messages.