openstack/deb-nova
Code Issues Proposed changes

Include CellMapping in InstanceMapping object

Browse Source 
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
This commit is contained in:
Andrew Laski
2016-02-29 14:39:20 -05:00
committed by Dan Smith
parent b63177470f
commit cabe2df804
5 changed files with 131 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
nova/db/sqlalchemy/api_models.py
View File

@@ -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):

45
nova/objects/instance_mapping.py
View File

@@ -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):

29
nova/tests/functional/db/test_instance_mapping.py
View File

@@ -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()

75
nova/tests/unit/objects/test_instance_mapping.py
View File

@@ -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,

2
nova/tests/unit/objects/test_objects.py
View File

@@ -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',