diff options
| -rw-r--r-- | nova/compute/api.py | 18 | ||||
| -rw-r--r-- | nova/tests/unit/compute/test_api.py | 57 | ||||
| -rw-r--r-- | nova/tests/unit/virt/disk/vfs/fakeguestfs.py | 2 | ||||
| -rw-r--r-- | nova/tests/unit/virt/libvirt/test_driver.py | 43 | ||||
| -rw-r--r-- | nova/tests/unit/virt/libvirt/test_guest.py | 42 | ||||
| -rw-r--r-- | nova/virt/disk/api.py | 4 | ||||
| -rw-r--r-- | nova/virt/disk/vfs/guestfs.py | 9 | ||||
| -rw-r--r-- | nova/virt/libvirt/driver.py | 18 | ||||
| -rw-r--r-- | nova/virt/libvirt/guest.py | 19 |
9 files changed, 173 insertions, 39 deletions
diff --git a/nova/compute/api.py b/nova/compute/api.py index 9c47d03989..f4d8900b83 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -5483,11 +5483,25 @@ class API(base.Base): bdm = self._get_bdm_by_volume_id( context, volume_id, expected_attrs=['instance']) - # We allow deleting the snapshot in any vm_state as long as there is - # no task being performed on the instance and it has a host. @check_instance_host() @check_instance_state(vm_state=None) def do_volume_snapshot_delete(self, context, instance): + # FIXME(lyarwood): Avoid bug #1919487 by rejecting the request + # to delete an intermediary volume snapshot offline as this isn't + # currently implemented within the libvirt driver and will fail. + # This should be fixed in a future release but as it is essentially + # a new feature wouldn't be something we could backport. As such + # reject the request here so n-api can respond correctly to c-vol. + if (delete_info.get('merge_target_file') is not None and + instance.vm_state != vm_states.ACTIVE + ): + raise exception.InstanceInvalidState( + attr='vm_state', + instance_uuid=instance.uuid, + state=instance.vm_state, + method='volume_snapshot_delete' + ) + self.compute_rpcapi.volume_snapshot_delete(context, instance, volume_id, snapshot_id, delete_info) diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index 2356161f69..822bfc1a15 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/nova/tests/unit/compute/test_api.py @@ -3601,23 +3601,29 @@ class _ComputeAPIUnitTestMixIn(object): 'boot_index': -1}) fake_bdm['instance'] = fake_instance.fake_db_instance( launched_at=timeutils.utcnow(), - vm_state=vm_states.STOPPED) + vm_state=vm_states.ACTIVE) fake_bdm['instance_uuid'] = fake_bdm['instance']['uuid'] fake_bdm = objects.BlockDeviceMapping._from_db_object( self.context, objects.BlockDeviceMapping(), fake_bdm, expected_attrs=['instance']) mock_get_bdm.return_value = fake_bdm + delete_info = { + 'merge_target_file': 'foo.qcow2', + } - with mock.patch.object(self.compute_api.compute_rpcapi, - 'volume_snapshot_delete') as mock_snapshot: - self.compute_api.volume_snapshot_delete(self.context, volume_id, - snapshot_id, {}) - + with mock.patch.object( + self.compute_api.compute_rpcapi, 'volume_snapshot_delete' + ) as mock_snapshot: + self.compute_api.volume_snapshot_delete( + self.context, volume_id, snapshot_id, delete_info + ) mock_get_bdm.assert_called_once_with(self.context, volume_id, expected_attrs=['instance']) mock_snapshot.assert_called_once_with( - self.context, fake_bdm['instance'], volume_id, snapshot_id, {}) + self.context, fake_bdm['instance'], volume_id, snapshot_id, + delete_info + ) @mock.patch.object( objects.BlockDeviceMapping, 'get_by_volume', @@ -3651,6 +3657,43 @@ class _ComputeAPIUnitTestMixIn(object): self.context, mock.sentinel.volume_id, mock.sentinel.snapshot_id, mock.sentinel.delete_info) + @mock.patch.object(compute_api.API, '_get_bdm_by_volume_id') + def test_volume_snapshot_delete_intermediary_commit(self, mock_get_bdm): + fake_bdm = fake_block_device.FakeDbBlockDeviceDict({ + 'id': 123, + 'device_name': '/dev/sda2', + 'source_type': 'volume', + 'destination_type': 'volume', + 'connection_info': "{'fake': 'connection_info'}", + 'volume_id': uuids.volume_id, + 'boot_index': -1 + }) + fake_bdm['instance'] = fake_instance.fake_db_instance( + launched_at=timeutils.utcnow(), + vm_state=vm_states.STOPPED) + fake_bdm['instance_uuid'] = fake_bdm['instance']['uuid'] + fake_bdm = objects.BlockDeviceMapping._from_db_object( + self.context, objects.BlockDeviceMapping(), + fake_bdm, expected_attrs=['instance']) + mock_get_bdm.return_value = fake_bdm + + # c-vol can provide delete_info with merge_target_file pointing to + # an intermediary snapshot to commit into it's base. This is only + # supported while the instance is running at present. + delete_info = { + 'merge_target_file': 'snap.img' + } + + # Assert that the request is rejected as offline commit isn't supported + self.assertRaises( + exception.InstanceInvalidState, + self.compute_api.volume_snapshot_delete, + self.context, + uuids.volume_id, + uuids.snapshot_id, + delete_info + ) + def _create_instance_with_disabled_disk_config(self, object=False): sys_meta = {"image_auto_disk_config": "Disabled"} params = {"system_metadata": sys_meta} diff --git a/nova/tests/unit/virt/disk/vfs/fakeguestfs.py b/nova/tests/unit/virt/disk/vfs/fakeguestfs.py index 76304e1f15..01715a9f7b 100644 --- a/nova/tests/unit/virt/disk/vfs/fakeguestfs.py +++ b/nova/tests/unit/virt/disk/vfs/fakeguestfs.py @@ -109,7 +109,7 @@ class GuestFS(object): "mode": 0o700 } - return self.files[path]["content"] + return bytes(self.files[path]["content"].encode()) def write(self, path, content): if path not in self.files: diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 4e052c4865..faa166f903 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -10005,9 +10005,9 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch('os_brick.encryptors.get_encryption_metadata') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') - @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._allow_native_luksv1') - def test_detach_encryptor_encrypted_volume_meta_missing(self, - mock_allow_native_luksv1, mock_get_encryptor, mock_get_metadata): + def test_detach_encryptor_encrypted_volume_meta_missing( + self, mock_get_encryptor, mock_get_metadata + ): """Assert that if missing the encryption metadata of an encrypted volume is fetched and then used to detach the encryptor for the volume. """ @@ -10016,8 +10016,12 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_get_encryptor.return_value = mock_encryptor encryption = {'provider': 'luks', 'control_location': 'front-end'} mock_get_metadata.return_value = encryption - connection_info = {'data': {'volume_id': uuids.volume_id}} - mock_allow_native_luksv1.return_value = False + connection_info = { + 'data': { + 'device_path': '/dev/foo', + 'volume_id': uuids.volume_id + } + } drvr._detach_encryptor(self.context, connection_info, None) @@ -10029,9 +10033,11 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch('os_brick.encryptors.get_encryption_metadata') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') - @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._allow_native_luksv1') - def test_detach_encryptor_encrypted_volume_meta_provided(self, - mock_allow_native_luksv1, mock_get_encryptor, mock_get_metadata): + def test_detach_encryptor_encrypted_volume_meta_provided( + self, + mock_get_encryptor, + mock_get_metadata + ): """Assert that when provided there are no further attempts to fetch the encryption metadata for the volume and that the provided metadata is then used to detach the volume. @@ -10040,8 +10046,12 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_encryptor = mock.MagicMock() mock_get_encryptor.return_value = mock_encryptor encryption = {'provider': 'luks', 'control_location': 'front-end'} - connection_info = {'data': {'volume_id': uuids.volume_id}} - mock_allow_native_luksv1.return_value = False + connection_info = { + 'data': { + 'device_path': '/dev/foo', + 'volume_id': uuids.volume_id + } + } drvr._detach_encryptor(self.context, connection_info, encryption) @@ -10051,20 +10061,19 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_encryptor.detach_volume.assert_called_once_with(**encryption) @mock.patch('nova.virt.libvirt.host.Host.find_secret') - @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._allow_native_luksv1') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') - def test_detach_encryptor_native_luks_device_path_secret_missing(self, - mock_get_encryptor, mock_allow_native_luksv1, mock_find_secret): - """Assert that the encryptor is not built when native LUKS is - available, the associated volume secret is missing and device_path is - also missing from the connection_info. + def test_detach_encryptor_native_luks_device_path_secret_missing( + self, mock_get_encryptor, mock_find_secret + ): + """Assert that the encryptor is not built when the associated volume + secret is missing and device_path is also missing from the + connection_info. """ drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) encryption = {'provider': 'luks', 'control_location': 'front-end', 'encryption_key_id': uuids.encryption_key_id} connection_info = {'data': {'volume_id': uuids.volume_id}} mock_find_secret.return_value = False - mock_allow_native_luksv1.return_value = True drvr._detach_encryptor(self.context, connection_info, encryption) diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py index d967470350..f09c8ae71d 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -377,6 +377,48 @@ class GuestTestCase(test.NoDBTestCase): error_message="device not found: disk vdb not found", supports_device_missing=True) + def test_detach_device_with_already_in_process_of_unplug_error(self): + # Assert that DeviceNotFound is raised when encountering + # https://bugzilla.redhat.com/show_bug.cgi?id=1878659 + # This is raised as QEMU returns a VIR_ERR_INTERNAL_ERROR when + # a request to device_del is made while another is about to complete. + + self.domain.isPersistent.return_value = True + conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice) + conf.to_xml.return_value = "</xml>" + + existing_unplug_exc = fakelibvirt.make_libvirtError( + fakelibvirt.libvirtError, "", + error_message='device vdb is already in the process of unplug', + error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR, + error_domain=fakelibvirt.VIR_FROM_DOMAIN + ) + device_missing_exc = fakelibvirt.make_libvirtError( + fakelibvirt.libvirtError, "", + error_message='device not found: disk vdb not found', + error_code=fakelibvirt.VIR_ERR_DEVICE_MISSING, + error_domain=fakelibvirt.VIR_FROM_DOMAIN + ) + + # Raise VIR_ERR_INTERNAL_ERROR on the second call before raising + # VIR_ERR_DEVICE_MISSING to mock the first call successfully detaching + # the device asynchronously. + self.domain.detachDeviceFlags.side_effect = [ + None, + existing_unplug_exc, + device_missing_exc + ] + + retry_detach = self.guest.detach_device_with_retry( + mock.Mock(return_value=conf), + 'vdb', + live=True, + inc_sleep_time=.01 + ) + + # Assert that we raise exception.DeviceNotFound + self.assertRaises(exception.DeviceNotFound, retry_detach) + def test_get_xml_desc(self): self.guest.get_xml_desc() self.domain.XMLDesc.assert_called_once_with(flags=0) diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py index 79d629f283..9902c0608b 100644 --- a/nova/virt/disk/api.py +++ b/nova/virt/disk/api.py @@ -615,8 +615,8 @@ def _set_passwd(username, admin_passwd, passwd_data, shadow_data): :param username: the username :param admin_passwd: the admin password - :param passwd_data: path to the passwd file - :param shadow_data: path to the shadow password file + :param passwd_data: Data from the passwd file decoded as a string + :param shadow_data: Data from the shadow file decoded as a string :returns: nothing :raises: exception.NovaException(), IOError() diff --git a/nova/virt/disk/vfs/guestfs.py b/nova/virt/disk/vfs/guestfs.py index 90586ef8f1..861ad80746 100644 --- a/nova/virt/disk/vfs/guestfs.py +++ b/nova/virt/disk/vfs/guestfs.py @@ -306,7 +306,14 @@ class VFSGuestFS(vfs.VFS): def read_file(self, path): LOG.debug("Read file path=%s", path) path = self._canonicalize_path(path) - return self.handle.read_file(path) + data = self.handle.read_file(path) + # NOTE(lyarwood): libguestfs v1.41.1 (0ee02e0117527) switched the + # return type of read_file from string to bytes and as such we need to + # handle both here, decoding and returning a string if bytes is + # provided. https://bugzilla.redhat.com/show_bug.cgi?id=1661871 + if isinstance(data, bytes): + return data.decode() + return data def has_file(self, path): LOG.debug("Has file path=%s", path) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index b5eb984fef..8ee8757e40 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1964,14 +1964,18 @@ class LibvirtDriver(driver.ComputeDriver): return self._host.delete_secret('volume', volume_id) if encryption is None: encryption = self._get_volume_encryption(context, connection_info) - # NOTE(lyarwood): Handle bug #1821696 where volume secrets have been - # removed manually by returning if a LUKS provider is being used - # and device_path is not present in the connection_info. This avoids - # VolumeEncryptionNotSupported being thrown when we incorrectly build - # the encryptor below due to the secrets not being present above. - if (encryption and self._allow_native_luksv1(encryption=encryption) and - not connection_info['data'].get('device_path')): + + # NOTE(lyarwood): Handle bugs #1821696 and #1917619 by avoiding the use + # of the os-brick encryptors if we don't have a device_path. The lack + # of a device_path here suggests the volume was natively attached to + # QEMU anyway as volumes without a device_path are not supported by + # os-brick encryptors. For volumes with a device_path the calls to + # the os-brick encryptors are safe as they are actually idempotent, + # ignoring any failures caused by the volumes actually being natively + # attached previously. + if (encryption and connection_info['data'].get('device_path') is None): return + if encryption: encryptor = self._get_volume_encryptor(connection_info, encryption) diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index b738097377..66e06896c6 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -434,12 +434,27 @@ class Guest(object): LOG.debug('Successfully detached device %s from guest. ' 'Persistent? %s. Live? %s', device, persistent, live) - except libvirt.libvirtError as ex: with excutils.save_and_reraise_exception(reraise=False) as ctx: - if ex.get_error_code() == libvirt.VIR_ERR_DEVICE_MISSING: + code = ex.get_error_code() + msg = ex.get_error_message() + if code == libvirt.VIR_ERR_DEVICE_MISSING: raise exception.DeviceNotFound( device=alternative_device_name) + # NOTE(lyarwood): https://bugzilla.redhat.com/1878659 + # Ignore this known QEMU bug for the time being allowing + # our retry logic to fire again and hopefully see that + # the device has been removed asynchronously by QEMU + # in the meantime when the next call to detach raises + # VIR_ERR_DEVICE_MISSING. + if (code == libvirt.VIR_ERR_INTERNAL_ERROR and + msg and 'already in the process of unplug' in msg + ): + LOG.debug('Ignoring QEMU rejecting our request to ' + 'detach as it is caused by a previous ' + 'request still being in progress.') + return + # Re-raise the original exception if we're not raising # DeviceNotFound instead. This will avoid logging of a # "Original exception being dropped" traceback. |