From adecf780d3ed4315e4ce305cb1821d493650494b Mon Sep 17 00:00:00 2001 From: Michael Still Date: Tue, 25 Nov 2014 15:42:47 +0300 Subject: [PATCH] Handle config drives being stored on rbd rbd is the only example of a currently supported image storage backend where it makes sense to put the config drive in the configured storage backend instead of local hypervisor disk. I don't think this makes sense for LVM, where we would be creating a LV for a tens of megabytes file, which seems like overkill to me. The other storage backends use local disk for their data already. This use case was covered by the now reverted changes: 228d0221763b12f11ecbacde4db38b1151f96e31 0b01e846d40f3b343da9ebe1dae89cca8bc2ac66 ecce888c469c62374a3cc43e3cede11d8aa1e799 Support this special case by moving the image to rbd once it has been created in the local instance directory on the hypervisor. I've tested this change in devstack and it works. Related-bug: #1369627 Related-bug: #1361840 Related-bug: #1246201 Co-Authored-By: Mehdi Abaakouk Co-Authored-By: Dan Smith Change-Id: Ia3ca5a18c79d62b71b9c042a612d12dd074b245e --- .../unit/virt/libvirt/storage/test_rbd.py | 15 +++++ nova/tests/unit/virt/libvirt/test_driver.py | 47 +++++++++++++++- .../unit/virt/libvirt/test_imagebackend.py | 34 ++++++++++++ nova/virt/libvirt/driver.py | 55 ++++++++++++++----- nova/virt/libvirt/imagebackend.py | 23 ++++++++ nova/virt/libvirt/storage/rbd_utils.py | 17 ++++++ 6 files changed, 176 insertions(+), 15 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/storage/test_rbd.py b/nova/tests/unit/virt/libvirt/storage/test_rbd.py index 02e945dfba10..e089ef8c1755 100644 --- a/nova/tests/unit/virt/libvirt/storage/test_rbd.py +++ b/nova/tests/unit/virt/libvirt/storage/test_rbd.py @@ -311,3 +311,18 @@ class RbdTestCase(test.NoDBTestCase): def test_cleanup_volumes_fail_other(self): self.assertRaises(test.TestingException, self._test_cleanup_exception, 'DoesNotExist') + + @mock.patch.object(rbd_utils, 'rbd') + @mock.patch.object(rbd_utils, 'rados') + @mock.patch.object(rbd_utils, 'RADOSClient') + def test_remove_image(self, mock_client, mock_rados, mock_rbd): + name = '12345_disk.config.rescue' + + rbd = mock_rbd.RBD.return_value + + client = mock_client.return_value + self.driver.remove_image(name) + rbd.remove.assert_called_once_with(client.ioctx, name) + # Make sure that we entered and exited the RADOSClient + client.__enter__.assert_called_once_with() + client.__exit__.assert_called_once_with(None, None, None) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 05c08ed2d911..e0ba8975826e 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -7381,8 +7381,9 @@ class LibvirtConnTestCase(test.NoDBTestCase): filename=ephemeral_file_name, mkfs=True) - def test_create_image_with_swap(self): + def _create_image_helper(self, callback, suffix=''): gotFiles = [] + imported_files = [] def fake_image(self, instance, name, image_type=''): class FakeImage(imagebackend.Image): @@ -7399,6 +7400,10 @@ class LibvirtConnTestCase(test.NoDBTestCase): gotFiles.append({'filename': filename, 'size': size}) + def import_file(self, instance, local_filename, + remote_filename): + imported_files.append((local_filename, remote_filename)) + def snapshot(self, name): pass @@ -7416,6 +7421,9 @@ class LibvirtConnTestCase(test.NoDBTestCase): instance_ref = self.test_instance instance_ref['image_ref'] = 1 + # NOTE(mikal): use this callback to tweak the instance to match + # what you're trying to test + callback(instance_ref) instance = objects.Instance(**instance_ref) # Turn on some swap to exercise that codepath in _create_image instance.flavor.swap = 500 @@ -7424,15 +7432,27 @@ class LibvirtConnTestCase(test.NoDBTestCase): self.stubs.Set(drvr, '_get_guest_xml', fake_none) self.stubs.Set(drvr, '_create_domain_and_network', fake_none) self.stubs.Set(drvr, 'get_info', fake_get_info) + self.stubs.Set(instance_metadata, 'InstanceMetadata', fake_none) + self.stubs.Set(nova.virt.configdrive.ConfigDriveBuilder, + 'make_drive', fake_none) image_meta = {'id': instance['image_ref']} disk_info = blockinfo.get_disk_info(CONF.libvirt.virt_type, instance, image_meta) - drvr._create_image(context, instance, disk_info['mapping']) + drvr._create_image(context, instance, disk_info['mapping'], + suffix=suffix) drvr._get_guest_xml(self.context, instance, None, disk_info, image_meta) + return gotFiles, imported_files + + def test_create_image_with_swap(self): + def enable_swap(instance_ref): + # Turn on some swap to exercise that codepath in _create_image + instance_ref['system_metadata']['instance_type_swap'] = 500 + + gotFiles, _ = self._create_image_helper(enable_swap) wantFiles = [ {'filename': '356a192b7913b04c54574d18c28d46e6395428ab', 'size': 10 * units.Gi}, @@ -7443,6 +7463,27 @@ class LibvirtConnTestCase(test.NoDBTestCase): ] self.assertEqual(gotFiles, wantFiles) + def test_create_image_with_configdrive(self): + def enable_configdrive(instance_ref): + instance_ref['config_drive'] = 'true' + + # Ensure that we create a config drive and then import it into the + # image backend store + _, imported_files = self._create_image_helper(enable_configdrive) + self.assertTrue(imported_files[0][0].endswith('/disk.config')) + self.assertEqual('disk.config', imported_files[0][1]) + + def test_create_image_with_configdrive_rescue(self): + def enable_configdrive(instance_ref): + instance_ref['config_drive'] = 'true' + + # Ensure that we create a config drive and then import it into the + # image backend store + _, imported_files = self._create_image_helper(enable_configdrive, + suffix='.rescue') + self.assertTrue(imported_files[0][0].endswith('/disk.config.rescue')) + self.assertEqual('disk.config.rescue', imported_files[0][1]) + @mock.patch.object(nova.virt.libvirt.imagebackend.Image, 'cache', side_effect=exception.ImageNotFound(image_id='fake-id')) def test_create_image_not_exist_no_fallback(self, mock_cache): @@ -12603,6 +12644,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase): ).AndReturn(fake_imagebackend.Raw()) imagebackend.Backend.image(instance, 'disk.rescue', 'default' ).AndReturn(fake_imagebackend.Raw()) + imagebackend.Backend.image(instance, 'disk.config.rescue', 'raw' + ).AndReturn(fake_imagebackend.Raw()) imagebackend.Image.cache(context=mox.IgnoreArg(), fetch_func=mox.IgnoreArg(), diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index 92c4a538b66c..924ca53fd37f 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -1362,6 +1362,40 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): ["server1:1899", "server2:1920"]), model) + def test_import_file(self): + image = self.image_class(self.INSTANCE, self.NAME) + + @mock.patch.object(image, 'check_image_exists') + @mock.patch.object(image.driver, 'remove_image') + @mock.patch.object(image.driver, 'import_image') + def _test(mock_import, mock_remove, mock_exists): + mock_exists.return_value = True + image.import_file(self.INSTANCE, mock.sentinel.file, + mock.sentinel.remote_name) + name = '%s_%s' % (self.INSTANCE.uuid, + mock.sentinel.remote_name) + mock_exists.assert_called_once_with() + mock_remove.assert_called_once_with(name) + mock_import.assert_called_once_with(mock.sentinel.file, name) + _test() + + def test_import_file_not_found(self): + image = self.image_class(self.INSTANCE, self.NAME) + + @mock.patch.object(image, 'check_image_exists') + @mock.patch.object(image.driver, 'remove_image') + @mock.patch.object(image.driver, 'import_image') + def _test(mock_import, mock_remove, mock_exists): + mock_exists.return_value = False + image.import_file(self.INSTANCE, mock.sentinel.file, + mock.sentinel.remote_name) + name = '%s_%s' % (self.INSTANCE.uuid, + mock.sentinel.remote_name) + mock_exists.assert_called_once_with() + self.assertFalse(mock_remove.called) + mock_import.assert_called_once_with(mock.sentinel.file, name) + _test() + class PloopTestCase(_ImageTestCase, test.NoDBTestCase): SIZE = 1024 diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index b89e25d647f0..1c5aa6e09478 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -2605,6 +2605,13 @@ class LibvirtDriver(driver.ComputeDriver): return os.path.join(libvirt_utils.get_instance_path(instance), 'disk.config' + suffix) + @staticmethod + def _get_disk_config_image_type(): + # TODO(mikal): there is a bug here if images_type has + # changed since creation of the instance, but I am pretty + # sure that this bug already exists. + return 'rbd' if CONF.libvirt.images_type == 'rbd' else 'raw' + def _chown_console_log_for_instance(self, instance): console_log = self._get_console_log_path(instance) if os.path.exists(console_log): @@ -2688,6 +2695,9 @@ class LibvirtDriver(driver.ComputeDriver): {'img_id': img_id, 'e': e}, instance=instance) + # NOTE(sileht): many callers of this method assume that this + # method doesn't fail if an image already exists but instead + # think that it will be reused (ie: (live)-migration/resize) def _create_image(self, context, instance, disk_mapping, suffix='', disk_images=None, network_info=None, @@ -2854,6 +2864,20 @@ class LibvirtDriver(driver.ComputeDriver): 'with error: %s'), e, instance=instance) + try: + # Tell the storage backend about the config drive + config_drive_image = self.image_backend.image( + instance, 'disk.config' + suffix, + self._get_disk_config_image_type()) + + config_drive_image.import_file( + instance, configdrive_path, 'disk.config' + suffix) + finally: + # NOTE(mikal): if the config drive was imported into RBD, then + # we no longer need the local copy + if CONF.libvirt.images_type == 'rbd': + os.unlink(configdrive_path) + # File injection only if needed elif inject_files and CONF.libvirt.inject_partition != -2: if booted_from_volume: @@ -3207,11 +3231,9 @@ class LibvirtDriver(driver.ComputeDriver): block_device.prepend_dev(diskswap.target_dev)) if 'disk.config' in disk_mapping: - diskconfig = self._get_guest_disk_config(instance, - 'disk.config', - disk_mapping, - inst_type, - 'raw') + diskconfig = self._get_guest_disk_config( + instance, 'disk.config', disk_mapping, inst_type, + self._get_disk_config_image_type()) devices.append(diskconfig) for vol in block_device.get_bdms_to_connect(block_device_mapping, @@ -5790,14 +5812,21 @@ class LibvirtDriver(driver.ComputeDriver): image_meta = utils.get_image_from_system_metadata( instance.system_metadata) - if not (is_shared_instance_path and is_shared_block_storage): - # NOTE(dims): Using config drive with iso format does not work - # because of a bug in libvirt with read only devices. However - # one can use vfat as config_drive_format which works fine. - # Please see bug/1246201 for details on the libvirt bug. - if CONF.config_drive_format != 'vfat': - if configdrive.required_by(instance): - raise exception.NoLiveMigrationForConfigDriveInLibVirt() + if configdrive.required_by(instance): + # NOTE(sileht): configdrive is stored into the block storage + # kvm is a block device, live migration will work + # NOTE(sileht): the configdrive is stored into a shared path + # kvm don't need to migrate it, live migration will work + # NOTE(dims): Using config drive with iso format does not work + # because of a bug in libvirt with read only devices. However + # one can use vfat as config_drive_format which works fine. + # Please see bug/1246201 for details on the libvirt bug. + if (is_shared_block_storage or + is_shared_instance_path or + CONF.config_drive_format == 'vfat'): + pass + else: + raise exception.NoLiveMigrationForConfigDriveInLibVirt() if not is_shared_instance_path: instance_dir = libvirt_utils.get_instance_path_at_destination( diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index 8708d29651d2..61b02cbdef59 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -386,6 +386,23 @@ class Image(object): """ raise NotImplementedError() + def import_file(self, instance, local_file, remote_name): + """Import an image from local storage into this backend. + + Import a local file into the store used by this image type. Note that + this is a noop for stores using local disk (the local file is + considered "in the store"). + + If the image already exists it will be overridden by the new file + + :param local_file: path to the file to import + :param remote_name: the name for the file in the store + """ + + # NOTE(mikal): this is a noop for now for all stores except RBD, but + # we should talk about if we want this functionality for everything. + pass + class Raw(Image): def __init__(self, instance=None, disk_name=None, path=None): @@ -808,6 +825,12 @@ class Rbd(Image): secret, servers) + def import_file(self, instance, local_file, remote_name): + name = '%s_%s' % (instance.uuid, remote_name) + if self.check_image_exists(): + self.driver.remove_image(name) + self.driver.import_image(local_file, name) + class Ploop(Image): def __init__(self, instance=None, disk_name=None, path=None): diff --git a/nova/virt/libvirt/storage/rbd_utils.py b/nova/virt/libvirt/storage/rbd_utils.py index 1f9afe4c967b..b989d888b102 100644 --- a/nova/virt/libvirt/storage/rbd_utils.py +++ b/nova/virt/libvirt/storage/rbd_utils.py @@ -239,6 +239,23 @@ class RBDDriver(object): except rbd.ImageNotFound: return False + def remove_image(self, name): + """Remove RBD volume + + :name: Name of RBD volume + """ + with RADOSClient(self, self.pool) as client: + try: + rbd.RBD().remove(client.ioctx, name) + except rbd.ImageNotFound: + LOG.warn(_LW('image %(volume)s in pool %(pool)s can not be ' + 'found, failed to remove'), + {'volume': name, 'pool': self.pool}) + except rbd.ImageHasSnapshots: + LOG.error(_LE('image %(volume)s in pool %(pool)s has ' + 'snapshots, failed to remove'), + {'volume': name, 'pool': self.pool}) + def import_image(self, base, name): """Import RBD volume from image file.