From 5316a8a0cdb5a121a6af268d8bb078aee3ead4c4 Mon Sep 17 00:00:00 2001
From: Matt Riedemann <mriedem.os@gmail.com>
Date: Mon, 22 Oct 2018 12:37:37 -0400
Subject: [PATCH] Fix jsonutils.to_primitive UserWarning

The CheatingSerializer fixture used in nova tests keeps
the RequestContext.db_connection set on the context object
which otherwise wouldn't normally happen. The context object
can show up in error notification payloads because of how
nova.exception_wrapper.wrap_exception works. That payload
is eventually serialized for notifications and since the
cheated RequestContext.db_connection is set and cannot be
serialized, it results in a UserWarning from the
jsonutils.to_primitive method (called via JsonPayloadSerializer).

This will eventually result in failures when that UserWarning
is made into an error.

To fix this, we can pass a fallback method to to_primitive()
which will serialize a RequestContext object the same way that
RequestContextSerializer serializes a context - by simply
converting it to dict form.

Since this only affects test runs, because of using the
CheatingSerializer fixture, it should have no impact on
runtime serializations.

Error logging is added to the FakeNotifier since it's hard
to know what is wrong in the payload unless it is logged.

Also, the WarningsFixture is updated to make sure we don't
introduce new UserWarnings for the serialization issue.

The jsonutils.to_primitive() fallback method was added to
oslo.serialization via commit cdb2f60d26e3b65b6370f87b2e9864045651c117
in 2.21.1 so we have to bump our minimum required version
of that library as well.

Change-Id: Id9f960a0c7c8897dbb9edf53b4723154341412d6
Closes-Bug: #1799249
---
 lower-constraints.txt            |  2 +-
 nova/rpc.py                      | 23 +++++++++++++++++++++--
 nova/tests/fixtures.py           |  9 +++++++++
 nova/tests/unit/fake_notifier.py | 10 +++++++++-
 nova/tests/unit/test_rpc.py      | 15 +++++++++++++--
 requirements.txt                 |  2 +-
 6 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/lower-constraints.txt b/lower-constraints.txt
index ba0e7d8d8d09..292a9d5e65d7 100644
--- a/lower-constraints.txt
+++ b/lower-constraints.txt
@@ -87,7 +87,7 @@ oslo.policy==1.35.0
 oslo.privsep==1.23.0
 oslo.reports==1.18.0
 oslo.rootwrap==5.8.0
-oslo.serialization==2.18.0
+oslo.serialization==2.21.1
 oslo.service==1.33.0
 oslo.upgradecheck==0.1.1
 oslo.utils==3.37.0
diff --git a/nova/rpc.py b/nova/rpc.py
index 6d7de3d67690..472698330533 100644
--- a/nova/rpc.py
+++ b/nova/rpc.py
@@ -33,6 +33,7 @@ from oslo_messaging.rpc import dispatcher
 from oslo_serialization import jsonutils
 from oslo_service import periodic_task
 from oslo_utils import importutils
+import six
 
 import nova.conf
 import nova.context
@@ -118,9 +119,27 @@ def get_allowed_exmods():
 
 
 class JsonPayloadSerializer(messaging.NoOpSerializer):
+
     @staticmethod
-    def serialize_entity(context, entity):
-        return jsonutils.to_primitive(entity, convert_instances=True)
+    def fallback(obj):
+        """Serializer fallback
+
+        This method is used to serialize an object which jsonutils.to_primitive
+        does not otherwise know how to handle.
+
+        This is mostly only needed in tests because of the use of the nova
+        CheatingSerializer fixture which keeps some non-serializable fields
+        on the RequestContext, like db_connection.
+        """
+        if isinstance(obj, nova.context.RequestContext):
+            # This matches RequestContextSerializer.serialize_context().
+            return obj.to_dict()
+        # The default fallback in jsonutils.to_primitive() is six.text_type.
+        return six.text_type(obj)
+
+    def serialize_entity(self, context, entity):
+        return jsonutils.to_primitive(entity, convert_instances=True,
+                                      fallback=self.fallback)
 
 
 class RequestContextSerializer(messaging.Serializer):
diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py
index 6fc09b4cae5b..53e5a795124f 100644
--- a/nova/tests/fixtures.py
+++ b/nova/tests/fixtures.py
@@ -826,6 +826,15 @@ class WarningsFixture(fixtures.Fixture):
         # prevent adding violations.
         warnings.filterwarnings('error', message=".*invalid UUID.*")
 
+        # NOTE(mriedem): Avoid adding anything which tries to convert an
+        # object to a primitive which jsonutils.to_primitive() does not know
+        # how to handle (or isn't given a fallback callback).
+        warnings.filterwarnings(
+            'error',
+            message="Cannot convert <oslo_db.sqlalchemy.enginefacade"
+                    "._Default object at ",
+            category=UserWarning)
+
         self.addCleanup(warnings.resetwarnings)
 
 
diff --git a/nova/tests/unit/fake_notifier.py b/nova/tests/unit/fake_notifier.py
index 526845c85ca9..62c2fa8bb557 100644
--- a/nova/tests/unit/fake_notifier.py
+++ b/nova/tests/unit/fake_notifier.py
@@ -17,12 +17,16 @@ import functools
 import pprint
 import threading
 
+from oslo_log import log as logging
 import oslo_messaging as messaging
 from oslo_serialization import jsonutils
+from oslo_utils import excutils
 from oslo_utils import timeutils
 
 from nova import rpc
 
+LOG = logging.getLogger(__name__)
+
 
 class _Sub(object):
     """Allow a subscriber to efficiently wait for an event to occur, and
@@ -96,7 +100,11 @@ class FakeNotifier(object):
                               serializer=self._serializer)
 
     def _notify(self, priority, ctxt, event_type, payload):
-        payload = self._serializer.serialize_entity(ctxt, payload)
+        try:
+            payload = self._serializer.serialize_entity(ctxt, payload)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                LOG.error('Error serializing payload: %s', payload)
         # NOTE(sileht): simulate the kombu serializer
         # this permit to raise an exception if something have not
         # been serialized correctly
diff --git a/nova/tests/unit/test_rpc.py b/nova/tests/unit/test_rpc.py
index ef89921829be..6cbccd245f05 100644
--- a/nova/tests/unit/test_rpc.py
+++ b/nova/tests/unit/test_rpc.py
@@ -18,6 +18,7 @@ import mock
 import oslo_messaging as messaging
 from oslo_messaging.rpc import dispatcher
 from oslo_serialization import jsonutils
+import six
 
 from nova import context
 from nova import rpc
@@ -366,10 +367,20 @@ class TestRPC(test.NoDBTestCase):
 
 class TestJsonPayloadSerializer(test.NoDBTestCase):
     def test_serialize_entity(self):
+        serializer = rpc.JsonPayloadSerializer()
         with mock.patch.object(jsonutils, 'to_primitive') as mock_prim:
-            rpc.JsonPayloadSerializer.serialize_entity('context', 'entity')
+            serializer.serialize_entity('context', 'entity')
 
-        mock_prim.assert_called_once_with('entity', convert_instances=True)
+        mock_prim.assert_called_once_with('entity', convert_instances=True,
+                                          fallback=serializer.fallback)
+
+    def test_fallback(self):
+        # Convert RequestContext, should get a dict.
+        primitive = rpc.JsonPayloadSerializer.fallback(context.get_context())
+        self.assertIsInstance(primitive, dict)
+        # Convert anything else, should get a string.
+        primitive = rpc.JsonPayloadSerializer.fallback(mock.sentinel.entity)
+        self.assertIsInstance(primitive, six.text_type)
 
 
 class TestRequestContextSerializer(test.NoDBTestCase):
diff --git a/requirements.txt b/requirements.txt
index 2d22340d97fb..c782f8dd1107 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -41,7 +41,7 @@ oslo.config>=6.1.0 # Apache-2.0
 oslo.context>=2.19.2 # Apache-2.0
 oslo.log>=3.36.0 # Apache-2.0
 oslo.reports>=1.18.0 # Apache-2.0
-oslo.serialization!=2.19.1,>=2.18.0 # Apache-2.0
+oslo.serialization!=2.19.1,>=2.21.1 # Apache-2.0
 oslo.upgradecheck>=0.1.1
 oslo.utils>=3.37.0 # Apache-2.0
 oslo.db>=4.40.0 # Apache-2.0