From e2cf3ae9ab54f7aea1354de112f7a58ece135e9e Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Tue, 25 Jan 2022 16:58:23 +0000 Subject: [PATCH] imagebackend: Add disk_info_mapping as an optional attribute of Image Change-Id: I67203a85b8030899514b80d26390d299f8619845 --- nova/tests/fixtures/libvirt_imagebackend.py | 13 +-- nova/tests/unit/virt/libvirt/test_driver.py | 29 ++++-- .../unit/virt/libvirt/test_imagebackend.py | 26 +++--- nova/virt/libvirt/driver.py | 65 +++++++++----- nova/virt/libvirt/imagebackend.py | 89 +++++++++++++------ 5 files changed, 149 insertions(+), 73 deletions(-) diff --git a/nova/tests/fixtures/libvirt_imagebackend.py b/nova/tests/fixtures/libvirt_imagebackend.py index 3d6f2e81e9d7..ea32b6b34ad1 100644 --- a/nova/tests/fixtures/libvirt_imagebackend.py +++ b/nova/tests/fixtures/libvirt_imagebackend.py @@ -154,7 +154,9 @@ class LibvirtImageBackendFixture(fixtures.Fixture): # their construction. Tests can use this to assert that disks were # created of the expected type. - def image_init(instance=None, disk_name=None, path=None): + def image_init( + instance=None, disk_name=None, path=None, disk_info_mapping=None + ): # There's nothing special about this path except that it's # predictable and unique for (instance, disk). if path is None: @@ -169,6 +171,7 @@ class LibvirtImageBackendFixture(fixtures.Fixture): # the real constructor. setattr(disk, 'path', path) setattr(disk, 'is_block_dev', mock.sentinel.is_block_dev) + setattr(disk, 'disk_info_mapping', disk_info_mapping) # Used by tests. Note that image_init is a closure over image_type. setattr(disk, 'image_type', image_type) @@ -217,16 +220,16 @@ class LibvirtImageBackendFixture(fixtures.Fixture): self.imported_files.append((local_filename, remote_filename)) def _fake_libvirt_info( - self, mock_disk, disk_info, cache_mode, extra_specs, disk_unit=None, + self, mock_disk, cache_mode, extra_specs, disk_unit=None, boot_order=None, ): # For tests in test_virt_drivers which expect libvirt_info to be # functional info = config.LibvirtConfigGuestDisk() info.source_type = 'file' - info.source_device = disk_info['type'] - info.target_bus = disk_info['bus'] - info.target_dev = disk_info['dev'] + info.source_device = mock_disk.disk_info_mapping['type'] + info.target_bus = mock_disk.disk_info_mapping['bus'] + info.target_dev = mock_disk.disk_info_mapping['dev'] info.driver_cache = cache_mode info.driver_format = 'raw' info.source_path = mock_disk.path diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 30af4e3e2ff8..af3579230d71 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -15660,10 +15660,9 @@ class LibvirtConnTestCase(test.NoDBTestCase, instance = objects.Instance(**instance_ref) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) image_meta = objects.ImageMeta.from_dict({'disk_format': 'raw'}) - disk_info = blockinfo.get_disk_info(CONF.libvirt.virt_type, - instance, - image_meta) - disk_info['mapping'].pop('disk.local') + disk_info = blockinfo.get_disk_info( + CONF.libvirt.virt_type, instance, image_meta, + block_device_info=block_device_info) with test.nested( mock.patch('oslo_concurrency.processutils.execute'), @@ -20520,8 +20519,10 @@ class LibvirtConnTestCase(test.NoDBTestCase, drvr._get_disk_config_image_type()) self.assertEqual(2, drvr.image_backend.by_name.call_count) - call1 = mock.call(instance, 'disk.config', 'rbd') - call2 = mock.call(instance, 'disk.config', 'flat') + call1 = mock.call(instance, 'disk.config', 'rbd', + disk_info_mapping=disk_mapping['disk.config']) + call2 = mock.call(instance, 'disk.config', 'flat', + disk_info_mapping=disk_mapping['disk.config']) drvr.image_backend.by_name.assert_has_calls([call1, call2]) self.assertEqual(mock.sentinel.diskconfig, diskconfig) @@ -23138,6 +23139,9 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): } instance = self._create_instance(params=inst_params) + image_meta = objects.ImageMeta.from_dict({}) + disk_info = blockinfo.get_disk_info( + CONF.libvirt.virt_type, instance, image_meta) disk_images = {'image_id': instance.image_ref} instance_dir = libvirt_utils.get_instance_path(instance) disk_path = os.path.join(instance_dir, 'disk') @@ -23157,7 +23161,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): ] drvr._create_and_inject_local_root( - self.context, instance, False, '', disk_images, None, None) + self.context, instance, disk_info['mapping'], False, '', + disk_images, None, None) mock_fetch_calls = [ mock.call(test.MatchType(nova.virt.libvirt.imagebackend.Qcow2), @@ -23240,9 +23245,13 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): # config_drive is True by default, configdrive.required_by() # returns True instance_ref = self._create_instance() + image_meta = objects.ImageMeta.from_dict({}) + disk_info = blockinfo.get_disk_info( + CONF.libvirt.virt_type, instance_ref, image_meta) disk_images = {'image_id': None} - drvr._create_and_inject_local_root(self.context, instance_ref, False, + drvr._create_and_inject_local_root(self.context, instance_ref, + disk_info['mapping'], False, '', disk_images, get_injection_info(), None) self.assertFalse(mock_inject.called) @@ -23262,6 +23271,9 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): mock_image.get.return_value = {'locations': [], 'disk_format': 'raw'} instance = self._create_instance() + image_meta = objects.ImageMeta.from_dict({}) + disk_info = blockinfo.get_disk_info( + CONF.libvirt.virt_type, instance, image_meta) disk_images = {'image_id': 'foo'} self.flags(images_type='rbd', group='libvirt') drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) @@ -23272,6 +23284,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): mock_fetch.reset_mock() drvr._create_and_inject_local_root(self.context, instance, + disk_info['mapping'], False, '', disk_images, diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index fdac09198566..34f022700ca3 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -163,7 +163,13 @@ class _ImageTestCase(object): self.assertEqual(fs.source_file, image.path) def test_libvirt_info(self): - image = self.image_class(self.INSTANCE, self.NAME) + disk_info = { + 'bus': 'virtio', + 'dev': '/dev/vda', + 'type': 'cdrom', + } + image = self.image_class( + self.INSTANCE, self.NAME, disk_info_mapping=disk_info) extra_specs = { 'quota:disk_read_bytes_sec': 10 * units.Mi, 'quota:disk_read_iops_sec': 1 * units.Ki, @@ -172,15 +178,9 @@ class _ImageTestCase(object): 'quota:disk_total_bytes_sec': 30 * units.Mi, 'quota:disk_total_iops_sec': 3 * units.Ki, } - disk_info = { - 'bus': 'virtio', - 'dev': '/dev/vda', - 'type': 'cdrom', - } disk = image.libvirt_info( - disk_info, cache_mode="none", extra_specs=extra_specs, - boot_order="1") + cache_mode="none", extra_specs=extra_specs, boot_order="1") self.assertIsInstance(disk, vconfig.LibvirtConfigGuestDisk) self.assertEqual("/dev/vda", disk.target_dev) @@ -205,16 +205,18 @@ class _ImageTestCase(object): get_disk_size.assert_called_once_with(image.path) def _test_libvirt_info_scsi_with_unit(self, disk_unit): - # The address should be set if bus is scsi and unit is set. - # Otherwise, it should not be set at all. - image = self.image_class(self.INSTANCE, self.NAME) disk_info = { 'bus': 'scsi', 'dev': '/dev/sda', 'type': 'disk', } + # The address should be set if bus is scsi and unit is set. + # Otherwise, it should not be set at all. + image = self.image_class( + self.INSTANCE, self.NAME, disk_info_mapping=disk_info) + disk = image.libvirt_info( - disk_info, cache_mode='none', extra_specs={}, disk_unit=disk_unit) + cache_mode='none', extra_specs={}, disk_unit=disk_unit) if disk_unit: self.assertEqual(0, disk.device_addr.controller) self.assertEqual(disk_unit, disk.device_addr.unit) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 8d4e31251800..2c78df529713 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -4638,12 +4638,16 @@ class LibvirtDriver(driver.ComputeDriver): ignore_bdi_for_swap=False): booted_from_volume = self._is_booted_from_volume(block_device_info) - def image(fname, image_type=CONF.libvirt.images_type): - return self.image_backend.by_name(instance, - fname + suffix, image_type) + def image( + fname, image_type=CONF.libvirt.images_type, disk_info_mapping=None + ): + return self.image_backend.by_name( + instance, fname + suffix, image_type, + disk_info_mapping=disk_info_mapping) - def raw(fname): - return image(fname, image_type='raw') + def raw(fname, disk_info_mapping=None): + return image( + fname, image_type='raw', disk_info_mapping=disk_info_mapping) created_instance_dir = True @@ -4662,8 +4666,6 @@ class LibvirtDriver(driver.ComputeDriver): flavor = instance.get_flavor() swap_mb = 0 if 'disk.swap' in disk_mapping: - mapping = disk_mapping['disk.swap'] - if ignore_bdi_for_swap: # This is a workaround to support legacy swap resizing, # which does not touch swap size specified in bdm, @@ -4677,12 +4679,17 @@ class LibvirtDriver(driver.ComputeDriver): # leaving the work with bdm only. swap_mb = flavor['swap'] else: + disk_info_mapping = disk_mapping['disk.swap'] + disk_device = disk_info_mapping['dev'] swap = driver.block_device_info_get_swap(block_device_info) if driver.swap_is_usable(swap): swap_mb = swap['swap_size'] - elif (flavor['swap'] > 0 and - not block_device.volume_in_mapping( - mapping['dev'], block_device_info)): + elif ( + flavor['swap'] > 0 and + not block_device.volume_in_mapping( + disk_device, block_device_info, + ) + ): swap_mb = flavor['swap'] if swap_mb > 0: @@ -4715,8 +4722,8 @@ class LibvirtDriver(driver.ComputeDriver): image_id=disk_images['ramdisk_id']) created_disks = self._create_and_inject_local_root( - context, instance, booted_from_volume, suffix, disk_images, - injection_info, fallback_from_host) + context, instance, disk_mapping, booted_from_volume, suffix, + disk_images, injection_info, fallback_from_host) # Lookup the filesystem type if required os_type_with_default = nova.privsep.fs.get_fs_type_for_os_type( @@ -4729,7 +4736,9 @@ class LibvirtDriver(driver.ComputeDriver): vm_mode = fields.VMMode.get_from_instance(instance) ephemeral_gb = instance.flavor.ephemeral_gb if 'disk.local' in disk_mapping: - disk_image = image('disk.local') + disk_info_mapping = disk_mapping['disk.local'] + disk_image = image( + 'disk.local', disk_info_mapping=disk_info_mapping) # Short circuit the exists() tests if we already created a disk created_disks = created_disks or not disk_image.exists() @@ -4748,7 +4757,9 @@ class LibvirtDriver(driver.ComputeDriver): for idx, eph in enumerate(driver.block_device_info_get_ephemerals( block_device_info)): - disk_image = image(blockinfo.get_eph_disk(idx)) + disk_name = blockinfo.get_eph_disk(idx) + disk_info_mapping = disk_mapping[disk_name] + disk_image = image(disk_name, disk_info_mapping=disk_info_mapping) # Short circuit the exists() tests if we already created a disk created_disks = created_disks or not disk_image.exists() @@ -4787,7 +4798,7 @@ class LibvirtDriver(driver.ComputeDriver): return (created_instance_dir, created_disks) - def _create_and_inject_local_root(self, context, instance, + def _create_and_inject_local_root(self, context, instance, disk_mapping, booted_from_volume, suffix, disk_images, injection_info, fallback_from_host): created_disks = False @@ -4804,7 +4815,10 @@ class LibvirtDriver(driver.ComputeDriver): if size == 0 or suffix == '.rescue': size = None - backend = self.image_backend.by_name(instance, 'disk' + suffix) + disk_name = 'disk' + suffix + disk_info_mapping = disk_mapping[disk_name] + backend = self.image_backend.by_name( + instance, disk_name, disk_info_mapping=disk_info_mapping) created_disks = not backend.exists() if instance.task_state == task_states.RESIZE_FINISH: @@ -5362,7 +5376,9 @@ class LibvirtDriver(driver.ComputeDriver): if image_type is None: image_type = CONF.libvirt.images_type disk_unit = None - disk = self.image_backend.by_name(instance, name, image_type) + disk_info_mapping = disk_mapping[name] + disk = self.image_backend.by_name( + instance, name, image_type, disk_info_mapping=disk_info_mapping) if (name == 'disk.config' and image_type == 'rbd' and not disk.exists()): # This is likely an older config drive that has not been migrated @@ -5371,18 +5387,21 @@ class LibvirtDriver(driver.ComputeDriver): # remove this fall back once we know all config drives are in rbd. # NOTE(vladikr): make sure that the flat image exist, otherwise # the image will be created after the domain definition. - flat_disk = self.image_backend.by_name(instance, name, 'flat') + flat_disk = self.image_backend.by_name( + instance, name, 'flat', disk_info_mapping=disk_info_mapping) if flat_disk.exists(): disk = flat_disk LOG.debug('Config drive not found in RBD, falling back to the ' 'instance directory', instance=instance) - disk_info = disk_mapping[name] - if 'unit' in disk_mapping and disk_info['bus'] == 'scsi': + # The 'unit' key is global to the disk_mapping (rather than for an + # individual disk) because it is used solely to track the incrementing + # unit number. + if 'unit' in disk_mapping and disk_info_mapping['bus'] == 'scsi': disk_unit = disk_mapping['unit'] - disk_mapping['unit'] += 1 # Increments for the next disk added + disk_mapping['unit'] += 1 # Increments for the next disk conf = disk.libvirt_info( - disk_info, self.disk_cachemode, flavor['extra_specs'], - disk_unit=disk_unit, boot_order=boot_order) + self.disk_cachemode, flavor['extra_specs'], disk_unit=disk_unit, + boot_order=boot_order) return conf def _get_guest_fs_config( diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index 155267af2317..7c93a6f6d8f2 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -82,13 +82,22 @@ class Image(metaclass=abc.ABCMeta): SUPPORTS_CLONE = False - def __init__(self, path, source_type, driver_format, is_block_dev=False): + def __init__( + self, + path, + source_type, + driver_format, + is_block_dev=False, + disk_info_mapping=None + ): """Image initialization. :param path: libvirt's representation of the path of this disk. :param source_type: block or file :param driver_format: raw or qcow2 :param is_block_dev: + :param disk_info_mapping: disk_info['mapping'][device] metadata + specific to this image generated by nova.virt.libvirt.blockinfo. """ if (CONF.ephemeral_storage_encryption.enabled and not self._supports_encryption()): @@ -105,6 +114,8 @@ class Image(metaclass=abc.ABCMeta): self.is_block_dev = is_block_dev self.preallocate = False + self.disk_info_mapping = disk_info_mapping + # NOTE(dripton): We store lines of json (path, disk_format) in this # file, for some image types, to prevent attacks based on changing the # disk_format. @@ -145,22 +156,23 @@ class Image(metaclass=abc.ABCMeta): pass def libvirt_info( - self, disk_info, cache_mode, extra_specs, boot_order=None, - disk_unit=None, + self, cache_mode, extra_specs, boot_order=None, disk_unit=None, ): """Get `LibvirtConfigGuestDisk` filled for this image. - :disk_info: Metadata generated by libvirt.blockinfo.get_disk_mapping :cache_mode: Caching mode for this image :extra_specs: Instance type extra specs dict. :boot_order: Disk device boot order """ - disk_bus = disk_info['bus'] + if self.disk_info_mapping is None: + raise AttributeError( + 'Image must have disk_info_mapping to call libvirt_info()') + disk_bus = self.disk_info_mapping['bus'] info = vconfig.LibvirtConfigGuestDisk() info.source_type = self.source_type - info.source_device = disk_info['type'] + info.source_device = self.disk_info_mapping['type'] info.target_bus = disk_bus - info.target_dev = disk_info['dev'] + info.target_dev = self.disk_info_mapping['dev'] info.driver_cache = cache_mode info.driver_discard = self.discard_mode info.driver_io = self.driver_io @@ -522,11 +534,16 @@ class Flat(Image): when creating a disk from a qcow2 if force_raw_images is not set in config. """ - def __init__(self, instance=None, disk_name=None, path=None): + def __init__( + self, instance=None, disk_name=None, path=None, disk_info_mapping=None + ): self.disk_name = disk_name path = (path or os.path.join(libvirt_utils.get_instance_path(instance), disk_name)) - super(Flat, self).__init__(path, "file", "raw", is_block_dev=False) + super().__init__( + path, "file", "raw", is_block_dev=False, + disk_info_mapping=disk_info_mapping + ) self.preallocate = ( strutils.to_slug(CONF.preallocate_images) == 'space') @@ -614,10 +631,15 @@ class Flat(Image): class Qcow2(Image): - def __init__(self, instance=None, disk_name=None, path=None): + def __init__( + self, instance=None, disk_name=None, path=None, disk_info_mapping=None + ): path = (path or os.path.join(libvirt_utils.get_instance_path(instance), disk_name)) - super(Qcow2, self).__init__(path, "file", "qcow2", is_block_dev=False) + super().__init__( + path, "file", "qcow2", is_block_dev=False, + disk_info_mapping=disk_info_mapping + ) self.preallocate = ( strutils.to_slug(CONF.preallocate_images) == 'space') @@ -695,7 +717,10 @@ class Lvm(Image): def escape(filename): return filename.replace('_', '__') - def __init__(self, instance=None, disk_name=None, path=None): + def __init__( + self, instance=None, disk_name=None, path=None, + disk_info_mapping=None + ): self.ephemeral_key_uuid = instance.get('ephemeral_key_uuid') if self.ephemeral_key_uuid is not None: @@ -724,7 +749,10 @@ class Lvm(Image): self.lv_path = os.path.join('/dev', self.vg, self.lv) path = '/dev/mapper/' + dmcrypt.volume_name(self.lv) - super(Lvm, self).__init__(path, "block", "raw", is_block_dev=True) + super(Lvm, self).__init__( + path, "block", "raw", is_block_dev=True, + disk_info_mapping=disk_info_mapping + ) # TODO(sbauza): Remove the config option usage and default the # LVM logical volume creation to preallocate the full size only. @@ -832,7 +860,9 @@ class Rbd(Image): SUPPORTS_CLONE = True - def __init__(self, instance=None, disk_name=None, path=None): + def __init__( + self, instance=None, disk_name=None, path=None, disk_info_mapping=None + ): if not CONF.libvirt.images_rbd_pool: raise RuntimeError(_('You should specify' ' images_rbd_pool' @@ -854,31 +884,32 @@ class Rbd(Image): if self.driver.ceph_conf: path += ':conf=' + self.driver.ceph_conf - super(Rbd, self).__init__(path, "block", "rbd", is_block_dev=False) + super().__init__( + path, "block", "rbd", is_block_dev=False, + disk_info_mapping=disk_info_mapping + ) self.discard_mode = CONF.libvirt.hw_disk_discard def libvirt_info( - self, disk_info, cache_mode, extra_specs, boot_order=None, - disk_unit=None, + self, cache_mode, extra_specs, boot_order=None, disk_unit=None ): """Get `LibvirtConfigGuestDisk` filled for this image. - :disk_info: Metadata generated by libvirt.blockinfo.get_disk_mapping :cache_mode: Caching mode for this image :extra_specs: Instance type extra specs dict. :boot_order: Disk device boot order """ info = vconfig.LibvirtConfigGuestDisk() - disk_bus = disk_info['bus'] + disk_bus = self.disk_info_mapping['bus'] hosts, ports = self.driver.get_mon_addrs() - info.source_device = disk_info['type'] + info.source_device = self.disk_info_mapping['type'] info.driver_format = 'raw' info.driver_cache = cache_mode info.driver_discard = self.discard_mode info.target_bus = disk_bus - info.target_dev = disk_info['dev'] + info.target_dev = self.disk_info_mapping['dev'] info.source_type = 'network' info.source_protocol = 'rbd' info.source_name = '%s/%s' % (self.driver.pool, self.rbd_name) @@ -1195,10 +1226,15 @@ class Rbd(Image): class Ploop(Image): - def __init__(self, instance=None, disk_name=None, path=None): + def __init__( + self, instance=None, disk_name=None, path=None, disk_info_mapping=None + ): path = (path or os.path.join(libvirt_utils.get_instance_path(instance), disk_name)) - super(Ploop, self).__init__(path, "file", "ploop", is_block_dev=False) + super().__init__( + path, "file", "ploop", is_block_dev=False, + disk_info_mapping=disk_info_mapping + ) self.resolve_driver_format() @@ -1301,13 +1337,14 @@ class Backend(object): raise RuntimeError(_('Unknown image_type=%s') % image_type) return image - def by_name(self, instance, name, image_type=None): + def by_name(self, instance, name, image_type=None, disk_info_mapping=None): """Return an Image object for a disk with the given name. :param instance: the instance which owns this disk :param name: The name of the disk :param image_type: (Optional) Image type. Default is CONF.libvirt.images_type. + :param disk_info_mapping: (Optional) Disk info mapping dict :return: An Image object for the disk with given name and instance. :rtype: Image """ @@ -1316,7 +1353,9 @@ class Backend(object): # default inline in the method, and not in the kwarg declaration. image_type = image_type or CONF.libvirt.images_type backend = self.backend(image_type) - return backend(instance=instance, disk_name=name) + return backend( + instance=instance, disk_name=name, + disk_info_mapping=disk_info_mapping) def by_libvirt_path(self, instance, path, image_type=None): """Return an Image object for a disk with the given libvirt path.