Fix how the Cells API is returning ComputeNode objects

As we're expecting ComputeNode objects on the compute.api.HostAPI, we need
to convert the Cells API to return ComputeNode objects by using the
ComputeNodeProxy object.

Co-Authored-By: Andrew Laski <andrew.laski@rackspace.com>

Closes-Bug: #1423237

Change-Id: I60995554d119735c42f6bf3ee2c7e78ba8f91b28

nova/cells/manager.py

@@ -372,7 +372,7 @@ class CellsManager(manager.Manager):
         response = self.msg_runner.compute_node_get(ctxt, cell_name,
                                                     compute_id)
         node = response.value_or_raise()
-        cells_utils.add_cell_to_compute_node(node, cell_name)
+        node = cells_utils.add_cell_to_compute_node(node, cell_name)
         return node
 
     def compute_node_get_all(self, ctxt, hypervisor_match=None):
@@ -385,8 +385,8 @@ class CellsManager(manager.Manager):
         for response in responses:
             nodes = response.value_or_raise()
             for node in nodes:
-                cells_utils.add_cell_to_compute_node(node,
-                                                     response.cell_name)
+                node = cells_utils.add_cell_to_compute_node(node,
+                                                            response.cell_name)
                 ret_nodes.append(node)
         return ret_nodes
 

nova/cells/messaging.py

@@ -773,12 +773,7 @@ class _TargetedMessageMethods(_BaseMessageMethods):
 
     def compute_node_get(self, message, compute_id):
         """Get compute node by ID."""
-        compute_node = objects.ComputeNode.get_by_id(message.ctxt, compute_id)
-        # NOTE(sbauza): Cells Manager is calling
-        # cells_utils.add_cell_to_compute_node with this result so we need
-        # to convert it back to a dict so as to prevent the coercion to an
-        # integer for the ID field (which then becomes a string)
-        return objects_base.obj_to_primitive(compute_node)
+        return objects.ComputeNode.get_by_id(message.ctxt, compute_id)
 
     def actions_get(self, message, instance_uuid):
         actions = self.db.actions_get(message.ctxt, instance_uuid)
@@ -1162,15 +1157,9 @@ class _BroadcastMessageMethods(_BaseMessageMethods):
     def compute_node_get_all(self, message, hypervisor_match):
         """Return compute nodes in this cell."""
         if hypervisor_match is not None:
-            nodes = objects.ComputeNodeList.get_by_hypervisor(message.ctxt,
-                                                              hypervisor_match)
-        else:
-            nodes = objects.ComputeNodeList.get_all(message.ctxt)
-        # NOTE(sbauza): Cells Manager is calling
-        # cells_utils.add_cell_to_compute_node with this result so we need
-        # to convert it back to a dict so as to prevent the coercion to an
-        # integer for the ID field (which then becomes a string)
-        return objects_base.obj_to_primitive(nodes)
+            return objects.ComputeNodeList.get_by_hypervisor(message.ctxt,
+                                                             hypervisor_match)
+        return objects.ComputeNodeList.get_all(message.ctxt)
 
     def compute_node_stats(self, message):
         """Return compute node stats from this cell."""

nova/cells/rpcapi.py

@@ -122,7 +122,10 @@ class CellsAPI(object):
         target = messaging.Target(topic=CONF.cells.topic, version='1.0')
         version_cap = self.VERSION_ALIASES.get(CONF.upgrade_levels.cells,
                                                CONF.upgrade_levels.cells)
-        serializer = objects_base.NovaObjectSerializer()
+        # NOTE(sbauza): Yes, this is ugly but cells_utils is calling cells.db
+        # which itself calls cells.rpcapi... You meant import cycling ? Gah.
+        from nova.cells import utils as cells_utils
+        serializer = cells_utils.ProxyObjectSerializer()
         self.client = rpc.get_client(target,
                                      version_cap=version_cap,
                                      serializer=serializer)

nova/cells/utils.py

@@ -17,8 +17,13 @@
 Cells Utility Methods
 """
 import random
+import sys
 
 from nova import db
+from nova import exception
+from nova import objects
+from nova.objects import base as obj_base
+
 
 # Separator used between cell names for the 'full cell name' and routing
 # path
@@ -27,6 +32,66 @@ PATH_CELL_SEP = '!'
 _CELL_ITEM_SEP = '@'
 
 
+class ProxyObjectSerializer(obj_base.NovaObjectSerializer):
+    def __init__(self):
+        super(ProxyObjectSerializer, self).__init__()
+        self.serializer = super(ProxyObjectSerializer, self)
+
+    def _process_object(self, context, objprim):
+        return _CellProxy.obj_from_primitive(self.serializer, objprim, context)
+
+
+class _CellProxy(object):
+    def __init__(self, obj, cell_path):
+        self._obj = obj
+        self._cell_path = cell_path
+
+    @property
+    def id(self):
+        return cell_with_item(self._cell_path, self._obj.id)
+
+    @property
+    def host(self):
+        return cell_with_item(self._cell_path, self._obj.host)
+
+    def __getitem__(self, key):
+        if key == 'id':
+            return self.id
+        if key == 'host':
+            return self.host
+
+        return getattr(self._obj, key)
+
+    def obj_to_primitive(self):
+        obj_p = self._obj.obj_to_primitive()
+        obj_p['cell_proxy.class_name'] = self.__class__.__name__
+        obj_p['cell_proxy.cell_path'] = self._cell_path
+        return obj_p
+
+    @classmethod
+    def obj_from_primitive(cls, serializer, primitive, context=None):
+        obj_primitive = primitive.copy()
+        cell_path = obj_primitive.pop('cell_proxy.cell_path', None)
+        klass_name = obj_primitive.pop('cell_proxy.class_name', None)
+        obj = serializer._process_object(context, obj_primitive)
+        if klass_name is not None and cell_path is not None:
+            klass = getattr(sys.modules[__name__], klass_name)
+            return klass(obj, cell_path)
+        else:
+            return obj
+
+    def __getattr__(self, key):
+        return getattr(self._obj, key)
+
+
+class ComputeNodeProxy(_CellProxy):
+    pass
+
+
+class ServiceProxy(_CellProxy):
+    pass
+
+
 def get_instances_to_sync(context, updated_since=None, project_id=None,
         deleted=True, shuffle=False, uuids_only=False):
     """Return a generator that will return a list of active and
@@ -79,12 +144,20 @@ def add_cell_to_compute_node(compute_node, cell_name):
     """Fix compute_node attributes that should be unique.  Allows
     API cell to query the 'id' by cell@id.
     """
-    compute_node['id'] = cell_with_item(cell_name, compute_node['id'])
-    # Might have a 'service' backref.  But if is_primitive() was used
-    # on this and it recursed too deep, 'service' may be "?".
-    service = compute_node.get('service')
-    if isinstance(service, dict):
-        _add_cell_to_service(service, cell_name)
+    # NOTE(sbauza): As compute_node is a ComputeNode object, we need to wrap it
+    # for adding the cell_path information
+    compute_proxy = ComputeNodeProxy(compute_node, cell_name)
+    try:
+        service = compute_proxy.service
+    except exception.ServiceNotFound:
+        service = None
+    if isinstance(service, objects.Service):
+        # TODO(sbauza): Cells messaging service is still using the DB API for
+        # returning service_get() and service_get_all(). Until we fix that and
+        # converge all messsaging calls by using NovaObjects, we can't modify
+        # _add_cell_to_service to return a ServiceProxy
+        compute_proxy.service = ServiceProxy(service, cell_name)
+    return compute_proxy
 
 
 def add_cell_to_service(service, cell_name):

nova/tests/unit/cells/test_cells_manager.py

@@ -456,22 +456,31 @@ class CellsManagerClassTestCase(test.NoDBTestCase):
             cell_name = 'path!to!cell%i' % i
             compute_nodes = []
             for compute_node in FAKE_COMPUTE_NODES:
-                compute_nodes.append(copy.deepcopy(compute_node))
-                expected_compute_node = copy.deepcopy(compute_node)
-                cells_utils.add_cell_to_compute_node(expected_compute_node,
-                                                     cell_name)
-                expected_response.append(expected_compute_node)
+                fake_compute = objects.ComputeNode(**compute_node)
+                fake_compute._cached_service = None
+                compute_nodes.append(fake_compute)
+                expected_compute_node = cells_utils.ComputeNodeProxy(
+                    fake_compute, cell_name)
+                expected_response.append(
+                    (cell_name, expected_compute_node, fake_compute))
             response = messaging.Response(self.ctxt, cell_name, compute_nodes,
                                           False)
             responses.append(response)
         self.mox.StubOutWithMock(self.msg_runner,
                                  'compute_node_get_all')
+        self.mox.StubOutWithMock(cells_utils, 'add_cell_to_compute_node')
         self.msg_runner.compute_node_get_all(self.ctxt,
                 hypervisor_match='fake-match').AndReturn(responses)
+        # Calls are done by cells, so we need to sort the list by the cell name
+        expected_response.sort(key=lambda k: k[0])
+        for cell_name, compute_proxy, compute_node in expected_response:
+            cells_utils.add_cell_to_compute_node(
+                compute_node, cell_name).AndReturn(compute_proxy)
         self.mox.ReplayAll()
         response = self.cells_manager.compute_node_get_all(self.ctxt,
                 hypervisor_match='fake-match')
-        self.assertEqual(expected_response, response)
+        self.assertEqual([proxy for cell, proxy, compute in expected_response],
+                         response)
 
     def test_compute_node_stats(self):
         raw_resp1 = {'key1': 1, 'key2': 2}
@@ -491,16 +500,22 @@ class CellsManagerClassTestCase(test.NoDBTestCase):
 
     def test_compute_node_get(self):
         fake_cell = 'fake-cell'
+        fake_compute = objects.ComputeNode(**FAKE_COMPUTE_NODES[0])
+        fake_compute._cached_service = None
         fake_response = messaging.Response(self.ctxt, fake_cell,
-                                           FAKE_COMPUTE_NODES[0],
+                                           fake_compute,
                                            False)
-        expected_response = copy.deepcopy(FAKE_COMPUTE_NODES[0])
-        cells_utils.add_cell_to_compute_node(expected_response, fake_cell)
+
+        expected_response = cells_utils.ComputeNodeProxy(fake_compute,
+                                                         fake_cell)
         cell_and_id = cells_utils.cell_with_item(fake_cell, 'fake-id')
         self.mox.StubOutWithMock(self.msg_runner,
                                  'compute_node_get')
+        self.mox.StubOutWithMock(cells_utils, 'add_cell_to_compute_node')
         self.msg_runner.compute_node_get(self.ctxt,
                 'fake-cell', 'fake-id').AndReturn(fake_response)
+        cells_utils.add_cell_to_compute_node(
+            fake_compute, fake_cell).AndReturn(expected_response)
         self.mox.ReplayAll()
         response = self.cells_manager.compute_node_get(self.ctxt,
                 compute_id=cell_and_id)

nova/tests/unit/cells/test_cells_utils.py

@@ -16,10 +16,13 @@
 Tests For Cells Utility methods
 """
 import inspect
+import mock
 import random
 
 from nova.cells import utils as cells_utils
 from nova import db
+from nova import exception
+from nova import objects
 from nova import test
 
 
@@ -101,3 +104,64 @@ class CellsUtilsTestCase(test.NoDBTestCase):
         result_cell, result_item = cells_utils.split_cell_and_item(together)
         self.assertEqual(cell, result_cell)
         self.assertEqual(item, result_item)
+
+    @mock.patch.object(objects.Service, 'get_by_id')
+    def test_add_cell_to_compute_node_no_service(self, mock_get_by_id):
+        fake_compute = objects.ComputeNode(id=1, host='fake', service_id=1)
+        mock_get_by_id.side_effect = exception.ServiceNotFound(service_id=1)
+        cell_path = 'fake_path'
+
+        proxy = cells_utils.add_cell_to_compute_node(fake_compute, cell_path)
+
+        self.assertIsInstance(proxy, cells_utils.ComputeNodeProxy)
+        self.assertEqual(cells_utils.cell_with_item(cell_path, 1), proxy.id)
+        self.assertEqual(cells_utils.cell_with_item(cell_path, 'fake'),
+                         proxy.host)
+        self.assertRaises(exception.ServiceNotFound, getattr, proxy, 'service')
+
+    @mock.patch.object(objects.Service, 'get_by_id')
+    def test_add_cell_to_compute_node_with_service(self, mock_get_by_id):
+        fake_compute = objects.ComputeNode(id=1, host='fake', service_id=1)
+        mock_get_by_id.return_value = objects.Service(id=1, host='fake-svc')
+        cell_path = 'fake_path'
+
+        proxy = cells_utils.add_cell_to_compute_node(fake_compute, cell_path)
+
+        self.assertIsInstance(proxy, cells_utils.ComputeNodeProxy)
+        self.assertEqual(cells_utils.cell_with_item(cell_path, 1), proxy.id)
+        self.assertEqual(cells_utils.cell_with_item(cell_path, 'fake'),
+                         proxy.host)
+        self.assertIsInstance(proxy.service, cells_utils.ServiceProxy)
+        self.assertEqual(cells_utils.cell_with_item(cell_path, 1),
+                         proxy.service.id)
+        self.assertEqual(cells_utils.cell_with_item(cell_path, 'fake-svc'),
+                         proxy.service.host)
+
+    def test_proxy_object_serializer_to_primitive(self):
+        obj = objects.ComputeNode(id=1, host='fake')
+        obj_proxy = cells_utils.ComputeNodeProxy(obj, 'fake_path')
+        serializer = cells_utils.ProxyObjectSerializer()
+
+        primitive = serializer.serialize_entity('ctx', obj_proxy)
+        self.assertIsInstance(primitive, dict)
+        class_name = primitive.pop('cell_proxy.class_name')
+        cell_path = primitive.pop('cell_proxy.cell_path')
+        self.assertEqual('ComputeNodeProxy', class_name)
+        self.assertEqual('fake_path', cell_path)
+        self.assertEqual(obj.obj_to_primitive(), primitive)
+
+    def test_proxy_object_serializer_from_primitive(self):
+        obj = objects.ComputeNode(id=1, host='fake')
+        serializer = cells_utils.ProxyObjectSerializer()
+
+        # Recreating the primitive by hand to isolate the test for only
+        # the deserializing method
+        primitive = obj.obj_to_primitive()
+        primitive['cell_proxy.class_name'] = 'ComputeNodeProxy'
+        primitive['cell_proxy.cell_path'] = 'fake_path'
+
+        result = serializer.deserialize_entity('ctx', primitive)
+        self.assertIsInstance(result, cells_utils.ComputeNodeProxy)
+        self.assertEqual(obj.obj_to_primitive(),
+                         result._obj.obj_to_primitive())
+        self.assertEqual('fake_path', result._cell_path)