diff --git a/nova/objects/block_device.py b/nova/objects/block_device.py index 9563eeaa9bdf..866109d47bdf 100644 --- a/nova/objects/block_device.py +++ b/nova/objects/block_device.py @@ -12,13 +12,18 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_db import api as oslo_db_api +from oslo_db.sqlalchemy import update_match from oslo_log import log as logging +from oslo_utils import uuidutils from oslo_utils import versionutils from nova import block_device from nova.cells import opts as cells_opts from nova.cells import rpcapi as cells_rpcapi from nova import db +from nova.db.sqlalchemy import api as db_api +from nova.db.sqlalchemy import models as db_models from nova import exception from nova.i18n import _ from nova import objects @@ -63,10 +68,12 @@ class BlockDeviceMapping(base.NovaPersistentObject, base.NovaObject, # get_by_volume() and get_by_volume_and_instance() # Version 1.17: Added tag field # Version 1.18: Added attachment_id - VERSION = '1.18' + # Version 1.19: Added uuid + VERSION = '1.19' fields = { 'id': fields.IntegerField(), + 'uuid': fields.UUIDField(), 'instance_uuid': fields.UUIDField(), 'instance': fields.ObjectField('Instance', nullable=True), 'source_type': fields.BlockDeviceSourceTypeField(nullable=True), @@ -90,19 +97,64 @@ class BlockDeviceMapping(base.NovaPersistentObject, base.NovaObject, def obj_make_compatible(self, primitive, target_version): target_version = versionutils.convert_version_to_tuple(target_version) + if target_version < (1, 19) and 'uuid' in primitive: + del primitive['uuid'] if target_version < (1, 18) and 'attachment_id' in primitive: del primitive['attachment_id'] if target_version < (1, 17) and 'tag' in primitive: del primitive['tag'] @staticmethod - def _from_db_object(context, block_device_obj, + @oslo_db_api.wrap_db_retry(max_retries=1, retry_on_deadlock=True) + def _create_uuid(context, bdm_id): + # NOTE(mdbooth): This method is only required until uuid is made + # non-nullable in a future release. + + # NOTE(mdbooth): We wrap this method in a retry loop because it can + # fail (safely) on multi-master galera if concurrent updates happen on + # different masters. It will never fail on single-master. We can only + # ever need one retry. + + uuid = uuidutils.generate_uuid() + values = {'uuid': uuid} + compare = db_models.BlockDeviceMapping(id=bdm_id, uuid=None) + + # NOTE(mdbooth): We explicitly use an independent transaction context + # here so as not to fail if: + # 1. We retry. + # 2. We're in a read transaction.This is an edge case of what's + # normally a read operation. Forcing everything (transitively) + # which reads a BDM to be in a write transaction for a narrow + # temporary edge case is undesirable. + tctxt = db_api.get_context_manager(context).writer.independent + with tctxt.using(context): + query = context.session.query(db_models.BlockDeviceMapping).\ + filter_by(id=bdm_id) + + try: + query.update_on_match(compare, 'id', values) + except update_match.NoRowsMatched: + # We can only get here if we raced, and another writer already + # gave this bdm a uuid + result = query.one() + uuid = result['uuid'] + assert(uuid is not None) + + return uuid + + @classmethod + def _from_db_object(cls, context, block_device_obj, db_block_device, expected_attrs=None): if expected_attrs is None: expected_attrs = [] for key in block_device_obj.fields: if key in BLOCK_DEVICE_OPTIONAL_ATTRS: continue + if key == 'uuid' and not db_block_device.get(key): + # NOTE(danms): While the records could be nullable, + # generate a UUID on read since the object requires it + bdm_id = db_block_device['id'] + db_block_device[key] = cls._create_uuid(context, bdm_id) block_device_obj[key] = db_block_device[key] if 'instance' in expected_attrs: my_inst = objects.Instance(context) diff --git a/nova/tests/unit/fake_block_device.py b/nova/tests/unit/fake_block_device.py index 3e974a4ae96c..a09cc020afcc 100644 --- a/nova/tests/unit/fake_block_device.py +++ b/nova/tests/unit/fake_block_device.py @@ -39,6 +39,7 @@ class FakeDbBlockDeviceDict(block_device.BlockDeviceDict): def __init__(self, bdm_dict=None, anon=False, **kwargs): bdm_dict = bdm_dict or {} db_id = bdm_dict.pop('id', 1) + db_uuid = bdm_dict.pop('uuid', uuids.bdm) instance_uuid = bdm_dict.pop('instance_uuid', uuids.fake) super(FakeDbBlockDeviceDict, self).__init__(bdm_dict=bdm_dict, @@ -48,6 +49,7 @@ class FakeDbBlockDeviceDict(block_device.BlockDeviceDict): 'deleted': 0} if not anon: fake_db_fields['id'] = db_id + fake_db_fields['uuid'] = db_uuid fake_db_fields['attachment_id'] = None fake_db_fields['created_at'] = timeutils.utcnow() fake_db_fields['updated_at'] = timeutils.utcnow() diff --git a/nova/tests/unit/objects/test_block_device.py b/nova/tests/unit/objects/test_block_device.py index d917df823aea..fd6681589452 100644 --- a/nova/tests/unit/objects/test_block_device.py +++ b/nova/tests/unit/objects/test_block_device.py @@ -17,6 +17,8 @@ import mock from nova.cells import rpcapi as cells_rpcapi from nova import context from nova import db +from nova.db.sqlalchemy import api as db_api +from nova.db.sqlalchemy import models as db_models from nova import exception from nova import objects from nova.objects import block_device as block_device_obj @@ -32,6 +34,7 @@ class _TestBlockDeviceMappingObject(object): instance = instance or {} fake_bdm = fake_block_device.FakeDbBlockDeviceDict({ 'id': 123, + 'uuid': uuids.bdm, 'instance_uuid': instance.get('uuid') or uuids.instance, 'attachment_id': None, 'device_name': '/dev/sda2', @@ -406,6 +409,80 @@ class _TestBlockDeviceMappingObject(object): primitive = bdm.obj_to_primitive(target_version='1.17') self.assertNotIn('attachment_id', primitive) + def test_obj_make_compatible_pre_1_19(self): + values = {'source_type': 'volume', 'volume_id': 'fake-vol-id', + 'destination_type': 'volume', + 'instance_uuid': uuids.instance, 'uuid': uuids.bdm} + bdm = objects.BlockDeviceMapping(context=self.context, **values) + primitive = bdm.obj_to_primitive(target_version='1.18') + self.assertNotIn('uuid', primitive) + + +class TestBlockDeviceMappingUUIDMigration(test.TestCase): + def setUp(self): + super(TestBlockDeviceMappingUUIDMigration, self).setUp() + self.context = context.RequestContext('fake-user-id', + 'fake-project-id') + + self.orig_create_uuid = \ + objects.BlockDeviceMapping._create_uuid + + @staticmethod + @db_api.pick_context_manager_writer + def _create_legacy_bdm(context): + # Create a BDM with no uuid + values = {'instance_uuid': uuids.instance_uuid} + bdm_ref = db_models.BlockDeviceMapping() + bdm_ref.update(values) + bdm_ref.save(context.session) + return bdm_ref + + @mock.patch.object(objects.BlockDeviceMapping, '_create_uuid') + def test_populate_uuid(self, mock_create_uuid): + mock_create_uuid.side_effect = self.orig_create_uuid + + self._create_legacy_bdm(self.context) + bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( + self.context, uuids.instance_uuid) + + # UUID should have been populated + uuid = bdms[0].uuid + self.assertIsNotNone(uuid) + + bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( + self.context, uuids.instance_uuid) + + # UUID should not have changed + self.assertEqual(uuid, bdms[0].uuid) + self.assertEqual(1, mock_create_uuid.call_count) + + def test_create_uuid_race(self): + # If threads read a legacy BDM object concurrently, we can end up + # calling _create_uuid multiple times. Assert that calling _create_uuid + # multiple times yields the same uuid. + + # NOTE(mdbooth): _create_uuid handles all forms of race, including any + # amount of overlapping. I have not attempted to write unit tests for + # all potential execution orders. This test is sufficient to + # demonstrate that the compare-and-swap works correctly, and we trust + # the correctness of the database for the rest. + + db_bdm = self._create_legacy_bdm(self.context) + uuid1 = objects.BlockDeviceMapping._create_uuid(self.context, + db_bdm['id']) + + bdm = objects.BlockDeviceMappingList.get_by_instance_uuid( + self.context, uuids.instance_uuid)[0] + self.assertEqual(uuid1, bdm.uuid) + + # We would only ever call this twice if we raced + # This is also testing that the compare-and-swap doesn't overwrite an + # existing uuid if we hit that race. + uuid2 = objects.BlockDeviceMapping._create_uuid(self.context, + bdm['id']) + + self.assertEqual(uuid1, uuid2) + class TestBlockDeviceMappingObject(test_objects._LocalTest, _TestBlockDeviceMappingObject): diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 5502b5eca0b1..64e7c39f57ac 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1064,7 +1064,7 @@ object_data = { 'AggregateList': '1.2-fb6e19f3c3a3186b04eceb98b5dadbfa', 'BandwidthUsage': '1.2-c6e4c779c7f40f2407e3d70022e3cd1c', 'BandwidthUsageList': '1.2-5fe7475ada6fe62413cbfcc06ec70746', - 'BlockDeviceMapping': '1.18-ad87cece6f84c65f5ec21615755bc6d3', + 'BlockDeviceMapping': '1.19-407e75274f48e60a76e56283333c9dbc', 'BlockDeviceMappingList': '1.17-1e568eecb91d06d4112db9fd656de235', 'BuildRequest': '1.3-077dee42bed93f8a5b62be77657b7152', 'BuildRequestList': '1.0-cd95608eccb89fbc702c8b52f38ec738',