From cd3901c0673620794fb2240a448c9fc14b608172 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Fri, 6 Nov 2015 11:01:19 -0800 Subject: [PATCH] Add uuid column to BlockDeviceMapping This adds a uuid column to the block_device_mapping table, and makes it semi-optional for graceful upgrade of existing rows without a uuid. Part of bp local-disk-serial-numbers Co-Authored-By: Lee Yarwood Co-Authored-By: Matthew Booth Partial-Bug: #1489581 Change-Id: Ibf0db6ad5b8367fc3267ac309516c08547d47e8c --- nova/block_device.py | 2 +- nova/db/sqlalchemy/api.py | 37 ++++++++-- .../migrate_repo/versions/374_bdm_uuid.py | 35 ++++++++++ nova/db/sqlalchemy/models.py | 8 +++ nova/tests/unit/db/test_db_api.py | 67 +++++++++++++++++-- nova/tests/unit/db/test_migrations.py | 9 +++ 6 files changed, 147 insertions(+), 11 deletions(-) create mode 100644 nova/db/sqlalchemy/migrate_repo/versions/374_bdm_uuid.py diff --git a/nova/block_device.py b/nova/block_device.py index 99c13933dd8e..11644c1c684f 100644 --- a/nova/block_device.py +++ b/nova/block_device.py @@ -48,7 +48,7 @@ bdm_new_fields = set(['source_type', 'destination_type', 'connection_info', 'tag']) -bdm_db_only_fields = set(['id', 'instance_uuid', 'attachment_id']) +bdm_db_only_fields = set(['id', 'instance_uuid', 'attachment_id', 'uuid']) bdm_db_inherited_fields = set(['created_at', 'updated_at', diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 2fd4b70bed1b..86e502dd93f5 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -3708,6 +3708,19 @@ def _from_legacy_values(values, legacy, allow_updates=False): return values +def _set_or_validate_uuid(values): + uuid = values.get('uuid') + + # values doesn't contain uuid, or it's blank + if not uuid: + values['uuid'] = uuidutils.generate_uuid() + + # values contains a uuid + else: + if not uuidutils.is_uuid_like(uuid): + raise exception.InvalidUUID(uuid=uuid) + + @require_context @pick_context_manager_writer def block_device_mapping_create(context, values, legacy=True): @@ -3715,6 +3728,8 @@ def block_device_mapping_create(context, values, legacy=True): values = _from_legacy_values(values, legacy) convert_objects_related_datetimes(values) + _set_or_validate_uuid(values) + bdm_ref = models.BlockDeviceMapping() bdm_ref.update(values) bdm_ref.save(context.session) @@ -3735,14 +3750,25 @@ def block_device_mapping_update(context, bdm_id, values, legacy=True): @pick_context_manager_writer def block_device_mapping_update_or_create(context, values, legacy=True): + # TODO(mdbooth): Remove this method entirely. Callers should know whether + # they require update or create, and call the appropriate method. + _scrub_empty_str_values(values, ['volume_size']) values = _from_legacy_values(values, legacy, allow_updates=True) convert_objects_related_datetimes(values) result = None - # NOTE(xqueralt): Only update a BDM when device_name was provided. We - # allow empty device names so they will be set later by the manager. - if values['device_name']: + # NOTE(xqueralt,danms): Only update a BDM when device_name or + # uuid was provided. Prefer the uuid, if available, but fall + # back to device_name if no uuid is provided, which can happen + # for BDMs created before we had a uuid. We allow empty device + # names so they will be set later by the manager. + if 'uuid' in values: + query = _block_device_mapping_get_query(context) + result = query.filter_by(instance_uuid=values['instance_uuid'], + uuid=values['uuid']).one_or_none() + + if not result and values['device_name']: query = _block_device_mapping_get_query(context) result = query.filter_by(instance_uuid=values['instance_uuid'], device_name=values['device_name']).first() @@ -3750,8 +3776,9 @@ def block_device_mapping_update_or_create(context, values, legacy=True): if result: result.update(values) else: - # Either the device_name doesn't exist in the database yet, or no - # device_name was provided. Both cases mean creating a new BDM. + # Either the device_name or uuid doesn't exist in the database yet, or + # neither was provided. Both cases mean creating a new BDM. + _set_or_validate_uuid(values) result = models.BlockDeviceMapping(**values) result.save(context.session) diff --git a/nova/db/sqlalchemy/migrate_repo/versions/374_bdm_uuid.py b/nova/db/sqlalchemy/migrate_repo/versions/374_bdm_uuid.py new file mode 100644 index 000000000000..5af704426999 --- /dev/null +++ b/nova/db/sqlalchemy/migrate_repo/versions/374_bdm_uuid.py @@ -0,0 +1,35 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +from migrate import UniqueConstraint +from sqlalchemy import Column +from sqlalchemy import MetaData +from sqlalchemy import String +from sqlalchemy import Table + + +def upgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + for prefix in ('', 'shadow_'): + table = Table(prefix + 'block_device_mapping', meta, autoload=True) + if not hasattr(table.c, 'uuid'): + new_column = Column('uuid', String(36), nullable=True) + table.create_column(new_column) + + if prefix == '': + # Only add the constraint on the non-shadow table... + con = UniqueConstraint('uuid', table=table, + name="uniq_block_device_mapping0uuid") + con.create(migrate_engine) diff --git a/nova/db/sqlalchemy/models.py b/nova/db/sqlalchemy/models.py index 12dd040ca4d5..6997a3d85b82 100644 --- a/nova/db/sqlalchemy/models.py +++ b/nova/db/sqlalchemy/models.py @@ -583,10 +583,18 @@ class BlockDeviceMapping(BASE, NovaBase, models.SoftDeleteMixin): Index('block_device_mapping_instance_uuid_volume_id_idx', 'instance_uuid', 'volume_id'), Index('block_device_mapping_instance_uuid_idx', 'instance_uuid'), + schema.UniqueConstraint('uuid', name='uniq_block_device_mapping0uuid'), ) id = Column(Integer, primary_key=True, autoincrement=True) instance_uuid = Column(String(36), ForeignKey('instances.uuid')) + # NOTE(mdbooth): The REST API for BDMs includes a UUID field. That uuid + # refers to an image, volume, or snapshot which will be used in the + # initialisation of the BDM. It is only relevant during the API call, and + # is not persisted directly. This is the UUID of the BDM itself. + # FIXME(danms): This should eventually be non-nullable, but we need a + # transition period first. + uuid = Column(String(36)) instance = orm.relationship(Instance, backref=orm.backref('block_device_mapping'), foreign_keys=instance_uuid, diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 80da9bf9fddb..b256451966dd 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -6427,6 +6427,16 @@ class BlockDeviceMappingTestCase(test.TestCase): def test_block_device_mapping_create(self): bdm = self._create_bdm({}) self.assertIsNotNone(bdm) + self.assertTrue(uuidutils.is_uuid_like(bdm['uuid'])) + + def test_block_device_mapping_create_with_blank_uuid(self): + bdm = self._create_bdm({'uuid': ''}) + self.assertIsNotNone(bdm) + self.assertTrue(uuidutils.is_uuid_like(bdm['uuid'])) + + def test_block_device_mapping_create_with_invalid_uuid(self): + self.assertRaises(exception.InvalidUUID, + self._create_bdm, {'uuid': 'invalid-uuid'}) def test_block_device_mapping_create_with_attachment_id(self): bdm = self._create_bdm({'attachment_id': uuidsentinel.attachment_id}) @@ -6456,16 +6466,19 @@ class BlockDeviceMappingTestCase(test.TestCase): 'destination_type': 'volume' } # check create - db.block_device_mapping_update_or_create(self.ctxt, values, - legacy=False) + bdm = db.block_device_mapping_update_or_create(self.ctxt, + copy.deepcopy(values), + legacy=False) + self.assertTrue(uuidutils.is_uuid_like(bdm['uuid'])) uuid = values['instance_uuid'] bdm_real = db.block_device_mapping_get_all_by_instance(self.ctxt, uuid) self.assertEqual(len(bdm_real), 1) self.assertEqual(bdm_real[0]['device_name'], 'fake_name') # check update - values['destination_type'] = 'camelot' - db.block_device_mapping_update_or_create(self.ctxt, values, + bdm0 = copy.deepcopy(values) + bdm0['destination_type'] = 'camelot' + db.block_device_mapping_update_or_create(self.ctxt, bdm0, legacy=False) bdm_real = db.block_device_mapping_get_all_by_instance(self.ctxt, uuid) self.assertEqual(len(bdm_real), 1) @@ -6474,12 +6487,13 @@ class BlockDeviceMappingTestCase(test.TestCase): self.assertEqual(bdm_real['destination_type'], 'camelot') # check create without device_name - bdm1 = dict(values) + bdm1 = copy.deepcopy(values) bdm1['device_name'] = None db.block_device_mapping_update_or_create(self.ctxt, bdm1, legacy=False) bdms = db.block_device_mapping_get_all_by_instance(self.ctxt, uuid) with_device_name = [b for b in bdms if b['device_name'] is not None] without_device_name = [b for b in bdms if b['device_name'] is None] + self.assertEqual(2, len(bdms)) self.assertEqual(len(with_device_name), 1, 'expected 1 bdm with device_name, found %d' % len(with_device_name)) @@ -6501,6 +6515,49 @@ class BlockDeviceMappingTestCase(test.TestCase): 'expected 2 bdms without device_name, found %d' % len(without_device_name)) + def test_block_device_mapping_update_or_create_with_uuid(self): + # Test that we are able to change device_name when calling + # block_device_mapping_update_or_create with a uuid. + bdm = self._create_bdm({}) + values = { + 'uuid': bdm['uuid'], + 'instance_uuid': bdm['instance_uuid'], + 'device_name': 'foobar', + } + db.block_device_mapping_update_or_create(self.ctxt, values, + legacy=False) + real_bdms = db.block_device_mapping_get_all_by_instance( + self.ctxt, bdm['instance_uuid']) + self.assertEqual(1, len(real_bdms)) + self.assertEqual('foobar', real_bdms[0]['device_name']) + + def test_block_device_mapping_update_or_create_with_blank_uuid(self): + # Test that create with block_device_mapping_update_or_create raises an + # exception if given an invalid uuid + values = { + 'uuid': '', + 'instance_uuid': uuidsentinel.instance, + 'device_name': 'foobar', + } + db.block_device_mapping_update_or_create(self.ctxt, values) + + real_bdms = db.block_device_mapping_get_all_by_instance( + self.ctxt, uuidsentinel.instance) + self.assertEqual(1, len(real_bdms)) + self.assertTrue(uuidutils.is_uuid_like(real_bdms[0]['uuid'])) + + def test_block_device_mapping_update_or_create_with_invalid_uuid(self): + # Test that create with block_device_mapping_update_or_create raises an + # exception if given an invalid uuid + values = { + 'uuid': 'invalid-uuid', + 'instance_uuid': uuidsentinel.instance, + 'device_name': 'foobar', + } + self.assertRaises(exception.InvalidUUID, + db.block_device_mapping_update_or_create, + self.ctxt, values) + def test_block_device_mapping_update_or_create_multiple_ephemeral(self): uuid = self.instance['uuid'] values = { diff --git a/nova/tests/unit/db/test_migrations.py b/nova/tests/unit/db/test_migrations.py index 4516106c444a..bb165f83597a 100644 --- a/nova/tests/unit/db/test_migrations.py +++ b/nova/tests/unit/db/test_migrations.py @@ -968,6 +968,15 @@ class NovaMigrationsCheckers(test_migrations.ModelsMigrationsSync, def _check_373(self, engine, data): self.assertColumnExists(engine, 'migrations', 'uuid') + def _check_374(self, engine, data): + self.assertColumnExists(engine, 'block_device_mapping', 'uuid') + self.assertColumnExists(engine, 'shadow_block_device_mapping', 'uuid') + + inspector = reflection.Inspector.from_engine(engine) + constraints = inspector.get_unique_constraints('block_device_mapping') + constraint_names = [constraint['name'] for constraint in constraints] + self.assertIn('uniq_block_device_mapping0uuid', constraint_names) + class TestNovaMigrationsSQLite(NovaMigrationsCheckers, test_base.DbTestCase,