From c802bf3ddc1a22d6ce6f3c40fe406a6f0923fdc8 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Wed, 22 Jun 2016 14:41:09 +0100 Subject: [PATCH] libvirt: Rename Backend snapshot and image snapshot and image didn't accurately describe what the methods did. snapshot in particular suggested that the returned object was in some way specific to a snapshot, whereas snapshot was simply the only user. We also take the opportunity to rename all variables we touch in driver to refer to disks rather than images. Implements: bp/libvirt-imagebackend-refactor Change-Id: I6ab8eb3b2c8a6af4c7ad6fba1f7d13716e9e0c12 --- nova/tests/unit/virt/libvirt/test_driver.py | 66 +++++++-------- .../unit/virt/libvirt/test_imagebackend.py | 5 +- nova/virt/libvirt/driver.py | 83 +++++++++---------- nova/virt/libvirt/imagebackend.py | 30 ++++--- 4 files changed, 90 insertions(+), 94 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index ffd7a54db07b..0bffc6045ba9 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -504,8 +504,8 @@ class CacheConcurrencyTestCase(test.NoDBTestCase): wait1 = eventlet.event.Event() done1 = eventlet.event.Event() sig1 = eventlet.event.Event() - thr1 = eventlet.spawn(backend.image(self._fake_instance(uuid), - 'name').cache, + thr1 = eventlet.spawn(backend.by_name(self._fake_instance(uuid), + 'name').cache, _concurrency, 'fname', None, signal=sig1, wait=wait1, done=done1) eventlet.sleep(0) @@ -515,8 +515,8 @@ class CacheConcurrencyTestCase(test.NoDBTestCase): wait2 = eventlet.event.Event() done2 = eventlet.event.Event() sig2 = eventlet.event.Event() - thr2 = eventlet.spawn(backend.image(self._fake_instance(uuid), - 'name').cache, + thr2 = eventlet.spawn(backend.by_name(self._fake_instance(uuid), + 'name').cache, _concurrency, 'fname', None, signal=sig2, wait=wait2, done=done2) @@ -542,8 +542,8 @@ class CacheConcurrencyTestCase(test.NoDBTestCase): wait1 = eventlet.event.Event() done1 = eventlet.event.Event() sig1 = eventlet.event.Event() - thr1 = eventlet.spawn(backend.image(self._fake_instance(uuid), - 'name').cache, + thr1 = eventlet.spawn(backend.by_name(self._fake_instance(uuid), + 'name').cache, _concurrency, 'fname2', None, signal=sig1, wait=wait1, done=done1) eventlet.sleep(0) @@ -553,8 +553,8 @@ class CacheConcurrencyTestCase(test.NoDBTestCase): wait2 = eventlet.event.Event() done2 = eventlet.event.Event() sig2 = eventlet.event.Event() - thr2 = eventlet.spawn(backend.image(self._fake_instance(uuid), - 'name').cache, + thr2 = eventlet.spawn(backend.by_name(self._fake_instance(uuid), + 'name').cache, _concurrency, 'fname1', None, signal=sig2, wait=wait2, done=done2) eventlet.sleep(0) @@ -4534,7 +4534,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): drvr.image_backend = mock_image_backend mock_image = mock.MagicMock() mock_image.path = '/tmp/test.img' - drvr.image_backend.image.return_value = mock_image + drvr.image_backend.by_name.return_value = mock_image mock_setup_container.return_value = '/dev/nbd0' mock_get_info.side_effect = exception.InstanceNotFound( instance_id='foo') @@ -13576,7 +13576,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): drvr.image_backend = mock_image_backend mock_image = mock.MagicMock() mock_image.path = '/tmp/test.img' - drvr.image_backend.image.return_value = mock_image + drvr.image_backend.by_name.return_value = mock_image mock_setup_container.return_value = '/dev/nbd0' mock_get_info.return_value = hardware.InstanceInfo( state=power_state.RUNNING) @@ -13596,8 +13596,8 @@ class LibvirtConnTestCase(test.NoDBTestCase): self.assertFalse(mock_instance.called) mock_get_inst_path.assert_has_calls([mock.call(mock_instance)]) mock_ensure_tree.assert_has_calls([mock.call('/tmp/rootfs')]) - drvr.image_backend.image.assert_has_calls([mock.call(mock_instance, - 'disk')]) + drvr.image_backend.by_name.assert_has_calls([mock.call(mock_instance, + 'disk')]) setup_container_call = mock.call( mock_image.get_model(), @@ -13638,7 +13638,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): drvr.image_backend = mock_image_backend mock_image = mock.MagicMock() mock_image.path = '/tmp/test.img' - drvr.image_backend.image.return_value = mock_image + drvr.image_backend.by_name.return_value = mock_image mock_setup_container.return_value = '/dev/nbd0' mock_chown.side_effect = chown_side_effect mock_get_info.return_value = hardware.InstanceInfo( @@ -13665,8 +13665,8 @@ class LibvirtConnTestCase(test.NoDBTestCase): mock_get_inst_path.assert_has_calls([mock.call(mock_instance)]) mock_is_booted_from_volume.assert_called_once_with(mock_instance, {}) mock_ensure_tree.assert_has_calls([mock.call('/tmp/rootfs')]) - drvr.image_backend.image.assert_has_calls([mock.call(mock_instance, - 'disk')]) + drvr.image_backend.by_name.assert_has_calls([mock.call(mock_instance, + 'disk')]) setup_container_call = mock.call( mock_image.get_model(), @@ -13694,7 +13694,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): drvr.image_backend = mock_image_backend mock_image = mock.MagicMock() mock_image.path = '/tmp/test.img' - drvr.image_backend.image.return_value = mock_image + drvr.image_backend.by_name.return_value = mock_image mock_setup_container.return_value = '/dev/nbd0' mock_get_info.return_value = hardware.InstanceInfo( state=power_state.SHUTDOWN) @@ -13714,8 +13714,8 @@ class LibvirtConnTestCase(test.NoDBTestCase): self.assertFalse(mock_instance.called) mock_get_inst_path.assert_has_calls([mock.call(mock_instance)]) mock_ensure_tree.assert_has_calls([mock.call('/tmp/rootfs')]) - drvr.image_backend.image.assert_has_calls([mock.call(mock_instance, - 'disk')]) + drvr.image_backend.by_name.assert_has_calls([mock.call(mock_instance, + 'disk')]) setup_container_call = mock.call( mock_image.get_model(), @@ -14907,8 +14907,8 @@ class LibvirtConnTestCase(test.NoDBTestCase): mock_rbd_image = mock.Mock() mock_flat_image = mock.Mock() mock_flat_image.libvirt_info.return_value = mock.sentinel.diskconfig - drvr.image_backend.image.side_effect = [mock_rbd_image, - mock_flat_image] + drvr.image_backend.by_name.side_effect = [mock_rbd_image, + mock_flat_image] mock_rbd_image.exists.return_value = False instance = objects.Instance() disk_mapping = {'disk.config': {'bus': 'ide', @@ -14920,10 +14920,10 @@ class LibvirtConnTestCase(test.NoDBTestCase): instance, 'disk.config', disk_mapping, flavor, drvr._get_disk_config_image_type()) - self.assertEqual(2, drvr.image_backend.image.call_count) + self.assertEqual(2, drvr.image_backend.by_name.call_count) call1 = mock.call(instance, 'disk.config', 'rbd') call2 = mock.call(instance, 'disk.config', 'flat') - drvr.image_backend.image.assert_has_calls([call1, call2]) + drvr.image_backend.by_name.assert_has_calls([call1, call2]) self.assertEqual(mock.sentinel.diskconfig, diskconfig) def _test_prepare_domain_for_snapshot(self, live_snapshot, state): @@ -15911,7 +15911,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend context = 'fake_context' ins_ref = self._create_instance() @@ -15973,7 +15973,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): def test_finish_revert_migration_snap_backend(self): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend ins_ref = self._create_instance() with test.nested( @@ -15992,7 +15992,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): def test_finish_revert_migration_snap_backend_snapshot_not_found(self): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend ins_ref = self._create_instance() with test.nested( @@ -16012,7 +16012,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): def test_finish_revert_migration_snap_backend_image_does_not_exist(self): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend drvr.image_backend.exists.return_value = False ins_ref = self._create_instance() @@ -16050,7 +16050,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend with test.nested( mock.patch.object(os.path, 'exists'), @@ -16073,7 +16073,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend with test.nested( mock.patch.object(os.path, 'exists'), @@ -16100,7 +16100,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): ins_ref = self._create_instance({'host': CONF.host}) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend with test.nested( mock.patch.object(os.path, 'exists'), @@ -16123,7 +16123,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): ins_ref = self._create_instance({'host': CONF.host}) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() - drvr.image_backend.image.return_value = drvr.image_backend + drvr.image_backend.by_name.return_value = drvr.image_backend drvr.image_backend.exists.return_value = False with test.nested( @@ -16221,10 +16221,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase): image_backend = ImageBackend() image_backend.path = path - with mock.patch.object( - self.drvr.image_backend, - 'image', - return_value=image_backend): + with mock.patch.object(self.drvr.image_backend, 'by_name', + return_value=image_backend): self.flags(inject_partition=0, group='libvirt') self.drvr._inject_data(image_backend, **driver_params) diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index 79b2eae818af..d1a242f0e00f 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -1688,9 +1688,8 @@ class BackendTestCase(test.NoDBTestCase): self.INSTANCE['ephemeral_key_uuid'] = None def get_image(self, use_cow, image_type): - return imagebackend.Backend(use_cow).image(self.INSTANCE, - self.NAME, - image_type) + return imagebackend.Backend(use_cow).by_name(self.INSTANCE, self.NAME, + image_type) def _test_image(self, image_type, image_not_cow, image_cow): image1 = self.get_image(False, image_type) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 227c67936362..fbe07dd35af5 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1083,7 +1083,7 @@ class LibvirtDriver(driver.ComputeDriver): utils.execute('rm', '-rf', target, delay_on_retry=True, attempts=5) - root_disk = self.image_backend.image(instance, 'disk') + root_disk = self.image_backend.by_name(instance, 'disk') # TODO(nic): Set ignore_errors=False in a future release. # It is set to True here to avoid any upgrade issues surrounding # instances being in pending resize state when the software is updated; @@ -1447,7 +1447,7 @@ class LibvirtDriver(driver.ComputeDriver): # For lxc instances we won't have it either from libvirt xml # (because we just gave libvirt the mounted filesystem), or the path, # so source_type is still going to be None. In this case, - # snapshot_backend is going to default to CONF.libvirt.images_type + # root_disk is going to default to CONF.libvirt.images_type # below, which is still safe. image_format = CONF.libvirt.snapshot_image_format or source_type @@ -1499,9 +1499,8 @@ class LibvirtDriver(driver.ComputeDriver): self._prepare_domain_for_snapshot(context, live_snapshot, state, instance) - snapshot_backend = self.image_backend.snapshot(instance, - disk_path, - image_type=source_type) + root_disk = self.image_backend.by_libvirt_path( + instance, disk_path, image_type=source_type) if live_snapshot: LOG.info(_LI("Beginning live snapshot process"), @@ -1515,7 +1514,7 @@ class LibvirtDriver(driver.ComputeDriver): try: update_task_state(task_state=task_states.IMAGE_UPLOADING, expected_state=task_states.IMAGE_PENDING_UPLOAD) - metadata['location'] = snapshot_backend.direct_snapshot( + metadata['location'] = root_disk.direct_snapshot( context, snapshot_name, image_format, image_id, instance.image_ref) self._snapshot_domain(context, live_snapshot, virt_dom, state, @@ -1530,13 +1529,13 @@ class LibvirtDriver(driver.ComputeDriver): failed_snap = metadata.pop('location', None) if failed_snap: failed_snap = {'url': str(failed_snap)} - snapshot_backend.cleanup_direct_snapshot(failed_snap, - also_destroy_volume=True, - ignore_errors=True) + root_disk.cleanup_direct_snapshot(failed_snap, + also_destroy_volume=True, + ignore_errors=True) update_task_state(task_state=task_states.IMAGE_PENDING_UPLOAD, expected_state=task_states.IMAGE_UPLOADING) - # TODO(nic): possibly abstract this out to the snapshot_backend + # TODO(nic): possibly abstract this out to the root_disk if source_type == 'rbd' and live_snapshot: # Standard snapshot uses qemu-img convert from RBD which is # not safe to run with live_snapshot. @@ -1557,8 +1556,7 @@ class LibvirtDriver(driver.ComputeDriver): disk_path, out_path, source_format, image_format, instance.image_meta) else: - snapshot_backend.snapshot_extract(out_path, - image_format) + root_disk.snapshot_extract(out_path, image_format) finally: self._snapshot_domain(context, live_snapshot, virt_dom, state, instance) @@ -1579,7 +1577,7 @@ class LibvirtDriver(driver.ComputeDriver): failed_snap = metadata.pop('location', None) if failed_snap: failed_snap = {'url': str(failed_snap)} - snapshot_backend.cleanup_direct_snapshot( + root_disk.cleanup_direct_snapshot( failed_snap, also_destroy_volume=True, ignore_errors=True) @@ -2990,8 +2988,8 @@ class LibvirtDriver(driver.ComputeDriver): instance, disk_mapping) def image(fname, image_type=CONF.libvirt.images_type): - return self.image_backend.image(instance, - fname + suffix, image_type) + return self.image_backend.by_name(instance, + fname + suffix, image_type) def raw(fname): return image(fname, image_type='raw') @@ -3124,8 +3122,8 @@ class LibvirtDriver(driver.ComputeDriver): if size == 0 or suffix == '.rescue': size = None - backend = self.image_backend.image(instance, 'disk' + suffix, - CONF.libvirt.images_type) + backend = self.image_backend.by_name(instance, 'disk' + suffix, + CONF.libvirt.images_type) if instance.task_state == task_states.RESIZE_FINISH: backend.create_snap(libvirt_utils.RESIZE_SNAPSHOT_NAME) if backend.SUPPORTS_CLONE: @@ -3161,7 +3159,7 @@ class LibvirtDriver(driver.ComputeDriver): if configdrive.required_by(instance): LOG.info(_LI('Using config drive'), instance=instance) - config_drive_image = self.image_backend.image( + config_drive_image = self.image_backend.by_name( instance, 'disk.config' + suffix, self._get_disk_config_image_type()) @@ -3438,35 +3436,31 @@ class LibvirtDriver(driver.ComputeDriver): {'qemu': MIN_QEMU_DISCARD_VERSION}) raise exception.Invalid(msg) - image = self.image_backend.image(instance, - name, - image_type) + disk = self.image_backend.by_name(instance, name, image_type) if (name == 'disk.config' and image_type == 'rbd' and - not image.exists()): + not disk.exists()): # This is likely an older config drive that has not been migrated # to rbd yet. Try to fall back on 'flat' image type. # TODO(melwitt): Add online migration of some sort so we can # 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_image = self.image_backend.image(instance, name, 'flat') - if flat_image.exists(): - image = flat_image + flat_disk = self.image_backend.by_name(instance, name, 'flat') + 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] - return image.libvirt_info(disk_info['bus'], - disk_info['dev'], - disk_info['type'], - self.disk_cachemode, - inst_type['extra_specs'], - self._host.get_version()) + return disk.libvirt_info(disk_info['bus'], + disk_info['dev'], + disk_info['type'], + self.disk_cachemode, + inst_type['extra_specs'], + self._host.get_version()) def _get_guest_fs_config(self, instance, name, image_type=None): - image = self.image_backend.image(instance, - name, - image_type) - return image.libvirt_fs_info("/", "ploop") + disk = self.image_backend.by_name(instance, name, image_type) + return disk.libvirt_fs_info("/", "ploop") def _get_guest_storage_config(self, instance, image_meta, disk_info, @@ -4743,8 +4737,8 @@ class LibvirtDriver(driver.ComputeDriver): # disk is backed by a local block device. image_model = imgmodel.LocalBlockImage(disk_path) else: - image = self.image_backend.image(instance, 'disk') - image_model = image.get_model(self._conn) + root_disk = self.image_backend.by_name(instance, 'disk') + image_model = root_disk.get_model(self._conn) container_dir = os.path.join(inst_path, 'rootfs') fileutils.ensure_tree(container_dir) @@ -6706,9 +6700,8 @@ class LibvirtDriver(driver.ComputeDriver): # Creating backing file follows same way as spawning instances. cache_name = os.path.basename(info['backing_file']) - image = self.image_backend.image(instance, - instance_disk, - CONF.libvirt.images_type) + disk = self.image_backend.by_name(instance, instance_disk, + CONF.libvirt.images_type) if cache_name.startswith('ephemeral'): # The argument 'size' is used by image.cache to # validate disk size retrieved from cache against @@ -6716,7 +6709,7 @@ class LibvirtDriver(driver.ComputeDriver): # and ephemeral_size is used by _create_ephemeral # to build the image if the disk is not already # cached. - image.cache( + disk.cache( fetch_func=self._create_ephemeral, fs_label=cache_name, os_type=instance.os_type, @@ -6726,12 +6719,12 @@ class LibvirtDriver(driver.ComputeDriver): elif cache_name.startswith('swap'): inst_type = instance.get_flavor() swap_mb = inst_type.swap - image.cache(fetch_func=self._create_swap, + disk.cache(fetch_func=self._create_swap, filename="swap_%s" % swap_mb, size=swap_mb * units.Mi, swap_mb=swap_mb) else: - self._try_fetch_image_cache(image, + self._try_fetch_image_cache(disk, libvirt_utils.fetch_image, context, cache_name, instance.image_ref, @@ -6739,7 +6732,7 @@ class LibvirtDriver(driver.ComputeDriver): info['virt_disk_size'], fallback_from_host) - # if image has kernel and ramdisk, just download + # if disk has kernel and ramdisk, just download # following normal way. self._fetch_instance_kernel_ramdisk( context, instance, fallback_from_host=fallback_from_host) @@ -7408,7 +7401,7 @@ class LibvirtDriver(driver.ComputeDriver): self._cleanup_failed_migration(inst_base) utils.execute('mv', inst_base_resize, inst_base) - root_disk = self.image_backend.image(instance, 'disk') + root_disk = self.image_backend.by_name(instance, 'disk') # Once we rollback, the snapshot is no longer needed, so remove it # TODO(nic): Remove the try/except/finally in a future release # To avoid any upgrade issues surrounding instances being in pending diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index 9d4a4ccc3b59..a3ad4394c4ab 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -1095,22 +1095,28 @@ class Backend(object): raise RuntimeError(_('Unknown image_type=%s') % image_type) return image - def image(self, instance, disk_name, image_type=None): - """Constructs image for selected backend + def by_name(self, instance, name, image_type=None): + """Return an Image object for a disk with the given name. - :instance: Instance name. - :name: Image name. - :image_type: Image type. - Optional, is CONF.libvirt.images_type by default. + :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. + :return: An Image object for the disk with given name and instance. + :rtype: Image """ backend = self.backend(image_type) - return backend(instance=instance, disk_name=disk_name) + return backend(instance=instance, disk_name=name) - def snapshot(self, instance, disk_path, image_type=None): - """Returns snapshot for given image + def by_libvirt_path(self, instance, path, image_type=None): + """Return an Image object for a disk with the given libvirt path. - :path: path to image - :image_type: type of image + :param instance: The instance which owns this disk. + :param path: The libvirt representation of the image's path. + :param image_type: (Optional) Image type. + Default is CONF.libvirt.images_type. + :return: An Image object for the given libvirt path. + :rtype: Image """ backend = self.backend(image_type) - return backend(instance=instance, path=disk_path) + return backend(instance=instance, path=path)