Fix service API and cells

The move to the object model broke the service API when used together
with cells. The underlying problem is how cells will overload the id
field with a cell path to uniquely identify each service in each cell.

Changing the id to a string would defeat the purpose of the object model
so this patch uses a proxy to modify the id on access to make the
difference transparent to the API.

Change-Id: I165b5d64b0c1556511eb02d179ca9fc6f101c008
Closes-bug: 1251043

nova/compute/cells_api.py

@@ -464,6 +464,25 @@ class ComputeCellsAPI(compute_api.API):
         return self.cells_rpcapi.get_migrations(context, filters)
 
 
+class ServiceProxy(object):
+    def __init__(self, obj, cell_path):
+        self._obj = obj
+        self._cell_path = cell_path
+
+    @property
+    def id(self):
+        return cells_utils.cell_with_item(self._cell_path, self._obj.id)
+
+    def __getitem__(self, key):
+        if key == 'id':
+            return self.id
+
+        return getattr(self._obj, key)
+
+    def __getattr__(self, key):
+        return getattr(self._obj, key)
+
+
 class HostAPI(compute_api.HostAPI):
     """HostAPI() class for cells.
 
@@ -505,13 +524,29 @@ class HostAPI(compute_api.HostAPI):
             if zone_filter is not None:
                 services = [s for s in services
                             if s['availability_zone'] == zone_filter]
+        # NOTE(johannes): Cells adds the cell path as a prefix to the id
+        # to uniquely identify the service amongst all cells. Unfortunately
+        # the object model makes the id an integer. Use a proxy here to
+        # work around this particular problem.
+
+        # Split out the cell path first
+        cell_paths = []
+        for service in services:
+            cell_path, id = cells_utils.split_cell_and_item(service['id'])
+            service['id'] = id
+            cell_paths.append(cell_path)
+
         # NOTE(danms): Currently cells does not support objects as
         # return values, so just convert the db-formatted service objects
         # to new-world objects here
-        return obj_base.obj_make_list(context,
-                                      service_obj.ServiceList(),
-                                      service_obj.Service,
-                                      services)
+        services = obj_base.obj_make_list(context,
+                                          service_obj.ServiceList(),
+                                          service_obj.Service,
+                                          services)
+
+        # Now wrap it in the proxy with the original cell_path
+        services = [ServiceProxy(s, c) for s, c in zip(services, cell_paths)]
+        return services
 
     def service_get_by_compute_host(self, context, host_name):
         db_service = self.cells_rpcapi.service_get_by_compute_host(context,

nova/tests/api/openstack/compute/contrib/test_services.py

@@ -15,6 +15,7 @@
 
 import calendar
 import datetime
+import iso8601
 
 import mock
 import webob.exc
@@ -22,6 +23,7 @@ import webob.exc
 from nova.api.openstack.compute.contrib import services
 from nova.api.openstack import extensions
 from nova import availability_zones
+from nova.compute import cells_api
 from nova import context
 from nova import db
 from nova import exception
@@ -92,35 +94,52 @@ class FakeRequestWithHostService(object):
         GET = {"host": "host1", "binary": "nova-compute"}
 
 
-def fake_host_api_service_get_all(context, filters=None, set_zones=False):
-    if set_zones or 'availability_zone' in filters:
-        return availability_zones.set_availability_zones(context,
-                                                         fake_services_list)
+def fake_service_get_all(services):
+    def service_get_all(context, filters=None, set_zones=False):
+        if set_zones or 'availability_zone' in filters:
+            return availability_zones.set_availability_zones(context,
+                                                             services)
+        return services
+    return service_get_all
 
 
 def fake_db_api_service_get_all(context, disabled=None):
     return fake_services_list
 
 
+def fake_db_service_get_by_host_binary(services):
+    def service_get_by_host_binary(context, host, binary):
+        for service in services:
+            if service['host'] == host and service['binary'] == binary:
+                return service
+        raise exception.HostBinaryNotFound(host=host, binary=binary)
+    return service_get_by_host_binary
+
+
 def fake_service_get_by_host_binary(context, host, binary):
-    for service in fake_services_list:
-        if service['host'] == host and service['binary'] == binary:
-            return service
-    raise exception.HostBinaryNotFound(host=host, binary=binary)
+    fake = fake_db_service_get_by_host_binary(fake_services_list)
+    return fake(context, host, binary)
 
 
-def fake_service_get_by_id(value):
-    for service in fake_services_list:
+def _service_get_by_id(services, value):
+    for service in services:
         if service['id'] == value:
             return service
     return None
 
 
+def fake_db_service_update(services):
+    def service_update(context, service_id, values):
+        service = _service_get_by_id(services, service_id)
+        if service is None:
+            raise exception.ServiceNotFound(service_id=service_id)
+        return service
+    return service_update
+
+
 def fake_service_update(context, service_id, values):
-    service = fake_service_get_by_id(service_id)
-    if service is None:
-        raise exception.ServiceNotFound(service_id=service_id)
-    return service
+    fake = fake_db_service_update(fake_services_list)
+    return fake(context, service_id, values)
 
 
 def fake_utcnow():
@@ -137,17 +156,20 @@ class ServicesTest(test.TestCase):
     def setUp(self):
         super(ServicesTest, self).setUp()
 
-        self.context = context.get_admin_context()
         self.ext_mgr = extensions.ExtensionManager()
         self.ext_mgr.extensions = {}
         self.controller = services.ServiceController(self.ext_mgr)
-        self.stubs.Set(self.controller.host_api, "service_get_all",
-                       fake_host_api_service_get_all)
+
         self.stubs.Set(timeutils, "utcnow", fake_utcnow)
         self.stubs.Set(timeutils, "utcnow_ts", fake_utcnow_ts)
+
+        self.stubs.Set(self.controller.host_api, "service_get_all",
+                       fake_service_get_all(fake_services_list))
+
         self.stubs.Set(db, "service_get_by_args",
-                       fake_service_get_by_host_binary)
-        self.stubs.Set(db, "service_update", fake_service_update)
+                       fake_db_service_get_by_host_binary(fake_services_list))
+        self.stubs.Set(db, "service_update",
+                       fake_db_service_update(fake_services_list))
 
     def test_services_list(self):
         req = FakeRequest()
@@ -485,3 +507,67 @@ class ServicesTest(test.TestCase):
         request.method = 'DELETE'
         self.assertRaises(webob.exc.HTTPMethodNotAllowed,
                           self.controller.delete, request, '300')
+
+
+class ServicesCellsTest(test.TestCase):
+    def setUp(self):
+        super(ServicesCellsTest, self).setUp()
+
+        host_api = cells_api.HostAPI()
+
+        self.ext_mgr = extensions.ExtensionManager()
+        self.ext_mgr.extensions = {}
+        self.controller = services.ServiceController(self.ext_mgr)
+        self.controller.host_api = host_api
+
+        self.stubs.Set(timeutils, "utcnow", fake_utcnow)
+        self.stubs.Set(timeutils, "utcnow_ts", fake_utcnow_ts)
+
+        services_list = []
+        for service in fake_services_list:
+            service = service.copy()
+            service['id'] = 'cell1@%d' % service['id']
+            services_list.append(service)
+
+        self.stubs.Set(host_api.cells_rpcapi, "service_get_all",
+                       fake_service_get_all(services_list))
+
+    def test_services_detail(self):
+        self.ext_mgr.extensions['os-extended-services-delete'] = True
+        req = FakeRequest()
+        res_dict = self.controller.index(req)
+        utc = iso8601.iso8601.Utc()
+        response = {'services': [
+                    {'id': 'cell1@1',
+                     'binary': 'nova-scheduler',
+                     'host': 'host1',
+                     'zone': 'internal',
+                     'status': 'disabled',
+                     'state': 'up',
+                     'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 2,
+                                                     tzinfo=utc)},
+                    {'id': 'cell1@2',
+                     'binary': 'nova-compute',
+                     'host': 'host1',
+                     'zone': 'nova',
+                     'status': 'disabled',
+                     'state': 'up',
+                     'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 5,
+                                                     tzinfo=utc)},
+                    {'id': 'cell1@3',
+                     'binary': 'nova-scheduler',
+                     'host': 'host2',
+                     'zone': 'internal',
+                     'status': 'enabled',
+                     'state': 'down',
+                     'updated_at': datetime.datetime(2012, 9, 19, 6, 55, 34,
+                                                     tzinfo=utc)},
+                    {'id': 'cell1@4',
+                     'binary': 'nova-compute',
+                     'host': 'host2',
+                     'zone': 'nova',
+                     'status': 'disabled',
+                     'state': 'down',
+                     'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38,
+                                                     tzinfo=utc)}]}
+        self.assertEqual(res_dict, response)

nova/tests/api/openstack/compute/plugins/v3/test_services.py

@@ -14,12 +14,14 @@
 
 import calendar
 import datetime
+import iso8601
 
 import mock
 import webob.exc
 
 from nova.api.openstack.compute.plugins.v3 import services
 from nova import availability_zones
+from nova.compute import cells_api
 from nova import context
 from nova import db
 from nova import exception
@@ -90,35 +92,52 @@ class FakeRequestWithHostService(object):
         GET = {"host": "host1", "binary": "nova-compute"}
 
 
-def fake_host_api_service_get_all(context, filters=None, set_zones=False):
-    if set_zones or 'availability_zone' in filters:
-        return availability_zones.set_availability_zones(context,
-                                                         fake_services_list)
+def fake_service_get_all(services):
+    def service_get_all(context, filters=None, set_zones=False):
+        if set_zones or 'availability_zone' in filters:
+            return availability_zones.set_availability_zones(context,
+                                                             services)
+        return services
+    return service_get_all
 
 
 def fake_db_api_service_get_all(context, disabled=None):
     return fake_services_list
 
 
+def fake_db_service_get_by_host_binary(services):
+    def service_get_by_host_binary(context, host, binary):
+        for service in services:
+            if service['host'] == host and service['binary'] == binary:
+                return service
+        raise exception.HostBinaryNotFound(host=host, binary=binary)
+    return service_get_by_host_binary
+
+
 def fake_service_get_by_host_binary(context, host, binary):
-    for service in fake_services_list:
-        if service['host'] == host and service['binary'] == binary:
-            return service
-    raise exception.HostBinaryNotFound(host=host, binary=binary)
+    fake = fake_db_service_get_by_host_binary(fake_services_list)
+    return fake(context, host, binary)
 
 
-def fake_service_get_by_id(value):
-    for service in fake_services_list:
+def _service_get_by_id(services, value):
+    for service in services:
         if service['id'] == value:
             return service
     return None
 
 
+def fake_db_service_update(services):
+    def service_update(context, service_id, values):
+        service = _service_get_by_id(services, service_id)
+        if service is None:
+            raise exception.ServiceNotFound(service_id=service_id)
+        return service
+    return service_update
+
+
 def fake_service_update(context, service_id, values):
-    service = fake_service_get_by_id(service_id)
-    if service is None:
-        raise exception.ServiceNotFound(service_id=service_id)
-    return service
+    fake = fake_db_service_update(fake_services_list)
+    return fake(context, service_id, values)
 
 
 def fake_utcnow():
@@ -135,15 +154,18 @@ class ServicesTest(test.TestCase):
     def setUp(self):
         super(ServicesTest, self).setUp()
 
-        self.context = context.get_admin_context()
         self.controller = services.ServiceController()
-        self.stubs.Set(self.controller.host_api, "service_get_all",
-                       fake_host_api_service_get_all)
+
         self.stubs.Set(timeutils, "utcnow", fake_utcnow)
         self.stubs.Set(timeutils, "utcnow_ts", fake_utcnow_ts)
+
+        self.stubs.Set(self.controller.host_api, "service_get_all",
+                       fake_service_get_all(fake_services_list))
+
         self.stubs.Set(db, "service_get_by_args",
-                       fake_service_get_by_host_binary)
-        self.stubs.Set(db, "service_update", fake_service_update)
+                       fake_db_service_get_by_host_binary(fake_services_list))
+        self.stubs.Set(db, "service_update",
+                       fake_db_service_update(fake_services_list))
 
     def test_services_list(self):
         req = FakeRequest()
@@ -361,3 +383,68 @@ class ServicesTest(test.TestCase):
         request.method = 'DELETE'
         self.assertRaises(webob.exc.HTTPNotFound,
                           self.controller.delete, request, 'abc')
+
+
+class ServicesCellsTest(test.TestCase):
+    def setUp(self):
+        super(ServicesCellsTest, self).setUp()
+
+        host_api = cells_api.HostAPI()
+
+        self.controller = services.ServiceController()
+        self.controller.host_api = host_api
+
+        self.stubs.Set(timeutils, "utcnow", fake_utcnow)
+        self.stubs.Set(timeutils, "utcnow_ts", fake_utcnow_ts)
+
+        services_list = []
+        for service in fake_services_list:
+            service = service.copy()
+            service['id'] = 'cell1@%d' % service['id']
+            services_list.append(service)
+
+        self.stubs.Set(host_api.cells_rpcapi, "service_get_all",
+                       fake_service_get_all(services_list))
+
+    def test_services_detail(self):
+        req = FakeRequest()
+        res_dict = self.controller.index(req)
+        utc = iso8601.iso8601.Utc()
+        response = {'services': [
+                    {'id': 'cell1@1',
+                     'binary': 'nova-scheduler',
+                     'host': 'host1',
+                     'zone': 'internal',
+                     'status': 'disabled',
+                     'state': 'up',
+                     'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 2,
+                                                     tzinfo=utc),
+                     'disabled_reason': 'test1'},
+                    {'id': 'cell1@2',
+                     'binary': 'nova-compute',
+                     'host': 'host1',
+                     'zone': 'nova',
+                     'status': 'disabled',
+                     'state': 'up',
+                     'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 5,
+                                                     tzinfo=utc),
+                     'disabled_reason': 'test2'},
+                    {'id': 'cell1@3',
+                     'binary': 'nova-scheduler',
+                     'host': 'host2',
+                     'zone': 'internal',
+                     'status': 'enabled',
+                     'state': 'down',
+                     'updated_at': datetime.datetime(2012, 9, 19, 6, 55, 34,
+                                                     tzinfo=utc),
+                     'disabled_reason': ''},
+                    {'id': 'cell1@4',
+                     'binary': 'nova-compute',
+                     'host': 'host2',
+                     'zone': 'nova',
+                     'status': 'disabled',
+                     'state': 'down',
+                     'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38,
+                                                     tzinfo=utc),
+                     'disabled_reason': 'test4'}]}
+        self.assertEqual(res_dict, response)

nova/tests/compute/test_host_api.py

@@ -349,9 +349,10 @@ class ComputeHostAPICellsTestCase(ComputeHostAPITestCase):
 
     def test_service_get_all_no_zones(self):
         services = [dict(test_service.fake_service,
-                         id=1, topic='compute', host='host1'),
+                         id='cell1@1', topic='compute', host='host1'),
                     dict(test_service.fake_service,
-                         id=2, topic='compute', host='host2')]
+                         id='cell1@2', topic='compute', host='host2')]
+        exp_services = [s.copy() for s in services]
 
         fake_filters = {'host': 'host1'}
         self.mox.StubOutWithMock(self.host_api.cells_rpcapi,
@@ -361,22 +362,21 @@ class ComputeHostAPICellsTestCase(ComputeHostAPITestCase):
         self.mox.ReplayAll()
         result = self.host_api.service_get_all(self.ctxt,
                                                filters=fake_filters)
-        self._compare_objs(result, services)
+        self._compare_objs(result, exp_services)
 
-    def test_service_get_all(self):
+    def _test_service_get_all(self, fake_filters, **kwargs):
         services = [dict(test_service.fake_service,
-                         id=1, key1='val1', key2='val2', topic='compute',
-                         host='host1'),
+                         id='cell1@1', key1='val1', key2='val2',
+                         topic='compute', host='host1'),
                     dict(test_service.fake_service,
-                         id=2, key1='val2', key3='val3', topic='compute',
-                         host='host2')]
+                         id='cell1@2', key1='val2', key3='val3',
+                         topic='compute', host='host2')]
         exp_services = []
         for service in services:
             exp_service = {}
             exp_service.update(availability_zone='nova', **service)
             exp_services.append(exp_service)
 
-        fake_filters = {'key1': 'val1'}
         self.mox.StubOutWithMock(self.host_api.cells_rpcapi,
                                  'service_get_all')
         self.host_api.cells_rpcapi.service_get_all(self.ctxt,
@@ -384,22 +384,17 @@ class ComputeHostAPICellsTestCase(ComputeHostAPITestCase):
         self.mox.ReplayAll()
         result = self.host_api.service_get_all(self.ctxt,
                                                filters=fake_filters,
-                                               set_zones=True)
+                                               **kwargs)
         self.mox.VerifyAll()
         self._compare_objs(result, exp_services)
 
-        # Test w/ zone filter but no set_zones arg.
-        self.mox.ResetAll()
+    def test_service_get_all(self):
         fake_filters = {'availability_zone': 'nova'}
-        # Zone filter is done client-size, so should be stripped
-        # from this call.
-        self.host_api.cells_rpcapi.service_get_all(self.ctxt,
-                filters={}).AndReturn(services)
-        self.mox.ReplayAll()
-        result = self.host_api.service_get_all(self.ctxt,
-                                               filters=fake_filters)
-        self.mox.VerifyAll()
-        self._compare_objs(result, exp_services)
+        self._test_service_get_all(fake_filters)
+
+    def test_service_get_all_set_zones(self):
+        fake_filters = {'key1': 'val1'}
+        self._test_service_get_all(fake_filters, set_zones=True)
 
     def test_service_get_by_compute_host(self):
         self.mox.StubOutWithMock(self.host_api.cells_rpcapi,