From cabe2df80494841dab529ac9ff09453035ec650a Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Mon, 29 Feb 2016 14:39:20 -0500 Subject: [PATCH] Include CellMapping in InstanceMapping object Right now InstanceMapping exposes the cell_mappings.id db field which is just an implementation detail of the db relationship and should not be exposed in the object. It is also useless as a way to look up the actual CellMapping through that object interface. And InstanceMapping is only looked up in order to find a CellMapping. So the CellMapping should be joined during the db query and loaded on the InstanceMapping object by default. Change-Id: Ia8691b76bba310327bfe0995964525409794d1af --- nova/db/sqlalchemy/api_models.py | 4 + nova/objects/instance_mapping.py | 45 +++++++++-- .../functional/db/test_instance_mapping.py | 29 ++++++- .../unit/objects/test_instance_mapping.py | 75 ++++++++++++++++--- nova/tests/unit/objects/test_objects.py | 2 +- 5 files changed, 131 insertions(+), 24 deletions(-) diff --git a/nova/db/sqlalchemy/api_models.py b/nova/db/sqlalchemy/api_models.py index 3416b02e6d..005ffa1e5e 100644 --- a/nova/db/sqlalchemy/api_models.py +++ b/nova/db/sqlalchemy/api_models.py @@ -68,6 +68,10 @@ class InstanceMapping(API_BASE): cell_id = Column(Integer, ForeignKey('cell_mappings.id'), nullable=True) project_id = Column(String(255), nullable=False) + cell_mapping = orm.relationship('CellMapping', + backref=backref('instance_mapping', uselist=False), + foreign_keys=cell_id, + primaryjoin=('InstanceMapping.cell_id == CellMapping.id')) class HostMapping(API_BASE): diff --git a/nova/objects/instance_mapping.py b/nova/objects/instance_mapping.py index 22f1b880e6..16f92d6397 100644 --- a/nova/objects/instance_mapping.py +++ b/nova/objects/instance_mapping.py @@ -11,12 +11,14 @@ # under the License. from oslo_versionedobjects import base as ovo +from sqlalchemy.orm import joinedload from nova.db.sqlalchemy import api as db_api from nova.db.sqlalchemy import api_models from nova import exception from nova import objects from nova.objects import base +from nova.objects import cell_mapping from nova.objects import fields @@ -30,14 +32,27 @@ class InstanceMapping(base.NovaTimestampObject, base.NovaObject, fields = { 'id': fields.IntegerField(read_only=True), 'instance_uuid': fields.UUIDField(), - 'cell_id': fields.IntegerField(nullable=True), + 'cell_mapping': fields.ObjectField('CellMapping', nullable=True), 'project_id': fields.StringField(), } + def _update_with_cell_id(self, updates): + cell_mapping_obj = updates.pop("cell_mapping", None) + if cell_mapping_obj: + updates["cell_id"] = cell_mapping_obj.id + return updates + @staticmethod def _from_db_object(context, instance_mapping, db_instance_mapping): for key in instance_mapping.fields: - setattr(instance_mapping, key, db_instance_mapping[key]) + db_value = db_instance_mapping.get(key) + if key == 'cell_mapping': + # cell_mapping can be None indicating that the instance has + # not been scheduled yet. + if db_value: + db_value = cell_mapping.CellMapping._from_db_object( + context, cell_mapping.CellMapping(), db_value) + setattr(instance_mapping, key, db_value) instance_mapping.obj_reset_changes() instance_mapping._context = context return instance_mapping @@ -45,9 +60,11 @@ class InstanceMapping(base.NovaTimestampObject, base.NovaObject, @staticmethod @db_api.api_context_manager.reader def _get_by_instance_uuid_from_db(context, instance_uuid): - db_mapping = context.session.query( - api_models.InstanceMapping).filter_by( - instance_uuid=instance_uuid).first() + db_mapping = (context.session.query(api_models.InstanceMapping) + .options(joinedload('cell_mapping')) + .filter( + api_models.InstanceMapping.instance_uuid + == instance_uuid)).first() if not db_mapping: raise exception.InstanceMappingNotFound(uuid=instance_uuid) @@ -64,11 +81,18 @@ class InstanceMapping(base.NovaTimestampObject, base.NovaObject, db_mapping = api_models.InstanceMapping() db_mapping.update(updates) db_mapping.save(context.session) + # NOTE: This is done because a later access will trigger a lazy load + # outside of the db session so it will fail. We don't lazy load + # cell_mapping on the object later because we never need an + # InstanceMapping without the CellMapping. + db_mapping.cell_mapping return db_mapping @base.remotable def create(self): - db_mapping = self._create_in_db(self._context, self.obj_get_changes()) + changes = self.obj_get_changes() + changes = self._update_with_cell_id(changes) + db_mapping = self._create_in_db(self._context, changes) self._from_db_object(self._context, self, db_mapping) @staticmethod @@ -87,6 +111,7 @@ class InstanceMapping(base.NovaTimestampObject, base.NovaObject, @base.remotable def save(self): changes = self.obj_get_changes() + changes = self._update_with_cell_id(changes) db_mapping = self._save_in_db(self._context, self.instance_uuid, changes) self._from_db_object(self._context, self, db_mapping) @@ -117,8 +142,12 @@ class InstanceMappingList(base.ObjectListBase, base.NovaObject): @staticmethod @db_api.api_context_manager.reader def _get_by_project_id_from_db(context, project_id): - return context.session.query(api_models.InstanceMapping).filter_by( - project_id=project_id).all() + return (context.session.query(api_models.InstanceMapping) + .join(api_models.CellMapping) + .with_entities(api_models.InstanceMapping, + api_models.CellMapping) + .filter( + api_models.InstanceMapping.project_id == project_id)).all() @base.remotable_classmethod def get_by_project_id(cls, context, project_id): diff --git a/nova/tests/functional/db/test_instance_mapping.py b/nova/tests/functional/db/test_instance_mapping.py index fbe25cc30a..82feec4d64 100644 --- a/nova/tests/functional/db/test_instance_mapping.py +++ b/nova/tests/functional/db/test_instance_mapping.py @@ -14,6 +14,7 @@ from oslo_utils import uuidutils from nova import context from nova import exception +from nova.objects import cell_mapping from nova.objects import instance_mapping from nova import test from nova.tests import fixtures @@ -24,6 +25,22 @@ sample_mapping = {'instance_uuid': '', 'project_id': 'fake-project'} +sample_cell_mapping = {'id': 3, + 'uuid': '', + 'name': 'fake-cell', + 'transport_url': 'rabbit:///', + 'database_connection': 'mysql:///'} + + +def create_cell_mapping(**kwargs): + args = sample_cell_mapping.copy() + if 'uuid' not in kwargs: + args['uuid'] = uuidutils.generate_uuid() + args.update(kwargs) + ctxt = context.RequestContext('fake-user', 'fake-project') + return cell_mapping.CellMapping._create_in_db(ctxt, args) + + def create_mapping(**kwargs): args = sample_mapping.copy() if 'instance_uuid' not in kwargs: @@ -43,11 +60,14 @@ class InstanceMappingTestCase(test.NoDBTestCase): self.mapping_obj = instance_mapping.InstanceMapping() def test_get_by_instance_uuid(self): + cell_mapping = create_cell_mapping() mapping = create_mapping() db_mapping = self.mapping_obj._get_by_instance_uuid_from_db( self.context, mapping['instance_uuid']) - for key in self.mapping_obj.fields.keys(): + for key in [key for key in self.mapping_obj.fields.keys() + if key != 'cell_mapping']: self.assertEqual(db_mapping[key], mapping[key]) + self.assertEqual(db_mapping['cell_mapping']['id'], cell_mapping['id']) def test_get_by_instance_uuid_not_found(self): self.assertRaises(exception.InstanceMappingNotFound, @@ -56,14 +76,15 @@ class InstanceMappingTestCase(test.NoDBTestCase): def test_save_in_db(self): mapping = create_mapping() + cell_mapping = create_cell_mapping() self.mapping_obj._save_in_db(self.context, mapping['instance_uuid'], - {'cell_id': 42}) + {'cell_id': cell_mapping['id']}) db_mapping = self.mapping_obj._get_by_instance_uuid_from_db( self.context, mapping['instance_uuid']) - self.assertNotEqual(db_mapping['cell_id'], mapping['cell_id']) for key in [key for key in self.mapping_obj.fields.keys() - if key not in ['cell_id', 'updated_at']]: + if key not in ['cell_id', 'cell_mapping', 'updated_at']]: self.assertEqual(db_mapping[key], mapping[key]) + self.assertEqual(db_mapping['cell_id'], cell_mapping['id']) def test_destroy_in_db(self): mapping = create_mapping() diff --git a/nova/tests/unit/objects/test_instance_mapping.py b/nova/tests/unit/objects/test_instance_mapping.py index d3f34787fc..251817d25b 100644 --- a/nova/tests/unit/objects/test_instance_mapping.py +++ b/nova/tests/unit/objects/test_instance_mapping.py @@ -16,6 +16,7 @@ from oslo_utils import uuidutils from nova import objects from nova.objects import instance_mapping +from nova.tests.unit.objects import test_cell_mapping from nova.tests.unit.objects import test_objects @@ -23,16 +24,21 @@ def get_db_mapping(**updates): db_mapping = { 'id': 1, 'instance_uuid': uuidutils.generate_uuid(), - 'cell_id': 42, + 'cell_id': None, 'project_id': 'fake-project', 'created_at': None, 'updated_at': None, } + db_mapping["cell_mapping"] = test_cell_mapping.get_db_mapping(id=42) + db_mapping['cell_id'] = db_mapping["cell_mapping"]["id"] db_mapping.update(updates) return db_mapping class _TestInstanceMappingObject(object): + def _check_cell_map_value(self, db_val, cell_obj): + self.assertEqual(db_val, cell_obj.id) + @mock.patch.object(instance_mapping.InstanceMapping, '_get_by_instance_uuid_from_db') def test_get_by_instance_uuid(self, uuid_from_db): @@ -43,7 +49,23 @@ class _TestInstanceMappingObject(object): self.context, db_mapping['instance_uuid']) uuid_from_db.assert_called_once_with(self.context, db_mapping['instance_uuid']) - self.compare_obj(mapping_obj, db_mapping) + self.compare_obj(mapping_obj, db_mapping, + subs={'cell_mapping': 'cell_id'}, + comparators={ + 'cell_mapping': self._check_cell_map_value}) + + @mock.patch.object(instance_mapping.InstanceMapping, + '_get_by_instance_uuid_from_db') + def test_get_by_instance_uuid_cell_mapping_none(self, uuid_from_db): + db_mapping = get_db_mapping(cell_mapping=None, cell_id=None) + uuid_from_db.return_value = db_mapping + + mapping_obj = objects.InstanceMapping().get_by_instance_uuid( + self.context, db_mapping['instance_uuid']) + uuid_from_db.assert_called_once_with(self.context, + db_mapping['instance_uuid']) + self.compare_obj(mapping_obj, db_mapping, + subs={'cell_mapping': 'cell_id'}) @mock.patch.object(instance_mapping.InstanceMapping, '_create_in_db') def test_create(self, create_in_db): @@ -52,15 +74,37 @@ class _TestInstanceMappingObject(object): create_in_db.return_value = db_mapping mapping_obj = objects.InstanceMapping(self.context) mapping_obj.instance_uuid = uuid - mapping_obj.cell_id = db_mapping['cell_id'] + mapping_obj.cell_mapping = objects.CellMapping(self.context, + id=db_mapping['cell_mapping']['id']) mapping_obj.project_id = db_mapping['project_id'] mapping_obj.create() create_in_db.assert_called_once_with(self.context, {'instance_uuid': uuid, - 'cell_id': db_mapping['cell_id'], + 'cell_id': db_mapping['cell_mapping']['id'], 'project_id': db_mapping['project_id']}) - self.compare_obj(mapping_obj, db_mapping) + self.compare_obj(mapping_obj, db_mapping, + subs={'cell_mapping': 'cell_id'}, + comparators={ + 'cell_mapping': self._check_cell_map_value}) + + @mock.patch.object(instance_mapping.InstanceMapping, '_create_in_db') + def test_create_cell_mapping_none(self, create_in_db): + db_mapping = get_db_mapping(cell_mapping=None, cell_id=None) + uuid = db_mapping['instance_uuid'] + create_in_db.return_value = db_mapping + mapping_obj = objects.InstanceMapping(self.context) + mapping_obj.instance_uuid = uuid + mapping_obj.cell_mapping = None + mapping_obj.project_id = db_mapping['project_id'] + + mapping_obj.create() + create_in_db.assert_called_once_with(self.context, + {'instance_uuid': uuid, + 'project_id': db_mapping['project_id']}) + self.compare_obj(mapping_obj, db_mapping, + subs={'cell_mapping': 'cell_id'}) + self.assertIsNone(mapping_obj.cell_mapping) @mock.patch.object(instance_mapping.InstanceMapping, '_save_in_db') def test_save(self, save_in_db): @@ -69,14 +113,17 @@ class _TestInstanceMappingObject(object): save_in_db.return_value = db_mapping mapping_obj = objects.InstanceMapping(self.context) mapping_obj.instance_uuid = uuid - mapping_obj.cell_id = 3 + mapping_obj.cell_mapping = objects.CellMapping(self.context, id=42) mapping_obj.save() save_in_db.assert_called_once_with(self.context, db_mapping['instance_uuid'], - {'cell_id': 3, + {'cell_id': mapping_obj.cell_mapping.id, 'instance_uuid': uuid}) - self.compare_obj(mapping_obj, db_mapping) + self.compare_obj(mapping_obj, db_mapping, + subs={'cell_mapping': 'cell_id'}, + comparators={ + 'cell_mapping': self._check_cell_map_value}) @mock.patch.object(instance_mapping.InstanceMapping, '_destroy_in_db') def test_destroy(self, destroy_in_db): @@ -87,10 +134,10 @@ class _TestInstanceMappingObject(object): mapping_obj.destroy() destroy_in_db.assert_called_once_with(self.context, uuid) - def test_cell_id_nullable(self): + def test_cell_mapping_nullable(self): mapping_obj = objects.InstanceMapping(self.context) # Just ensure this doesn't raise an exception - mapping_obj.cell_id = None + mapping_obj.cell_mapping = None class TestInstanceMappingObject(test_objects._LocalTest, @@ -104,6 +151,9 @@ class TestRemoteInstanceMappingObject(test_objects._RemoteTest, class _TestInstanceMappingListObject(object): + def _check_cell_map_value(self, db_val, cell_obj): + self.assertEqual(db_val, cell_obj.id) + @mock.patch.object(instance_mapping.InstanceMappingList, '_get_by_project_id_from_db') def test_get_by_project_id(self, project_id_from_db): @@ -114,7 +164,10 @@ class _TestInstanceMappingListObject(object): self.context, db_mapping['project_id']) project_id_from_db.assert_called_once_with(self.context, db_mapping['project_id']) - self.compare_obj(mapping_obj.objects[0], db_mapping) + self.compare_obj(mapping_obj.objects[0], db_mapping, + subs={'cell_mapping': 'cell_id'}, + comparators={ + 'cell_mapping': self._check_cell_map_value}) class TestInstanceMappingListObject(test_objects._LocalTest, diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 199e3519dd..35163fe37d 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1139,7 +1139,7 @@ object_data = { 'InstanceGroupList': '1.7-be18078220513316abd0ae1b2d916873', 'InstanceInfoCache': '1.5-cd8b96fefe0fc8d4d337243ba0bf0e1e', 'InstanceList': '2.0-6c8ba6147cca3082b1e4643f795068bf', - 'InstanceMapping': '1.0-94bff38981ef9ce37c9fccf309b94f58', + 'InstanceMapping': '1.0-65de80c491f54d19374703c0753c4d47', 'InstanceMappingList': '1.0-9e982e3de1613b9ada85e35f69b23d47', 'InstanceNUMACell': '1.3-6991a20992c5faa57fae71a45b40241b', 'InstanceNUMATopology': '1.2-d944a7d6c21e1c773ffdf09c6d025954',