From 92bd7ea118fec4791b903871cdc95d0cd71583e2 Mon Sep 17 00:00:00 2001 From: Sahid Orentino Ferdjaoui Date: Wed, 25 Oct 2017 05:57:11 -0400 Subject: [PATCH] libvirt: disconnect volume from host during detach Under certain failure scenarios it may be that although the libvirt definition for the volume has been removed for the instance that the associated storage lun on the compute server may not have been fully cleaned up yet. In case users try an other attempt to detach volume we should not stop the process whether the device is not found in domain definition but try to disconnect the logical device from host. This commit makes the process to attempt a disconnect volume even if the device is not attached to the guest. Closes-Bug: #1727260 Signed-off-by: Sahid Orentino Ferdjaoui (cherry picked from commit ce531dd1b763704b9043ddde8e80ac99cd193660) (cherry picked from commit d6a072b5c5a3ff9de7f4b42cda517ead17efe561) Conflicts: nova/tests/unit/virt/libvirt/test_driver.py NOTE: The conflicts were due to the newer testcase mocking 'nova.virt.libvirt.host.Host._get_domain' where the older code calls 'nova.virt.libvirt.host.Host.get_domain', and also dealing with the fact that the older code doesn't pass 'encryption' to self._disconnect_volume(). This latter issue means that we need to move the call to encryptor.detach_volume() to ensure it gets called if we hit exception.DeviceNotFound when detaching the device from the guest. This is similar to the original code proposed in https://review.openstack.org/#/c/515008/9/nova/virt/libvirt/driver.py but it requires special handling for the scenario where cryptsetup tries to destroy a dm-crypt device that has already been destroyed. Signed-off-by: Chris Friesen Change-Id: I4182642aab3fd2ffb1c97d2de9bdca58982289d8 --- nova/tests/unit/virt/libvirt/test_driver.py | 113 +++++++++++++++++++- nova/virt/libvirt/driver.py | 31 ++++-- 2 files changed, 135 insertions(+), 9 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index e4ae0a0c2161..2549d099be75 100755 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -6819,8 +6819,10 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_disconnect_volume.assert_called_with( connection_info, 'vdc', instance) + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') @mock.patch('nova.virt.libvirt.host.Host.get_domain') - def test_detach_volume_disk_not_found(self, mock_get_domain): + def test_detach_volume_disk_not_found(self, mock_get_domain, + mock_disconnect_volume): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) instance = objects.Instance(**self.test_instance) mock_xml_without_disk = """ @@ -6836,10 +6838,115 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_dom.info.return_value = [power_state.RUNNING, 512, 512, 2, 1234, 5678] mock_get_domain.return_value = mock_dom - self.assertRaises(exception.DiskNotFound, drvr.detach_volume, - connection_info, instance, '/dev/vdc') + + drvr.detach_volume(connection_info, instance, '/dev/vdc') mock_get_domain.assert_called_once_with(instance) + mock_disconnect_volume.assert_called_once_with( + connection_info, 'vdc', instance) + + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') + @mock.patch('nova.virt.libvirt.host.Host.get_domain') + def test_detach_volume_disk_not_found_encryption(self, mock_get_domain, + mock_disconnect_volume, + mock_get_encryptor): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + instance = objects.Instance(**self.test_instance) + mock_xml_without_disk = """ + + +""" + mock_dom = mock.MagicMock(return_value=mock_xml_without_disk) + encryption = {"provider": "NoOpEncryptor"} + mock_encryptor = mock.MagicMock(spec=encryptors.nop.NoOpEncryptor) + mock_get_encryptor.return_value = mock_encryptor + + connection_info = {"driver_volume_type": "fake", + "data": {"device_path": "/fake", + "access_mode": "rw"}} + + mock_dom.info.return_value = [power_state.RUNNING, 512, 512, 2, 1234, + 5678] + mock_get_domain.return_value = mock_dom + + drvr.detach_volume(connection_info, instance, '/dev/vdc', + encryption) + mock_get_encryptor.assert_called_once_with(connection_info, encryption) + mock_encryptor.detach_volume.assert_called_once_with(**encryption) + mock_disconnect_volume.assert_called_once_with( + connection_info, 'vdc', instance) + + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') + @mock.patch('nova.virt.libvirt.host.Host.get_domain') + def test_detach_volume_disk_not_found_encryption_err(self, mock_get_domain, + mock_disconnect_volume, + mock_get_encryptor): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + instance = objects.Instance(**self.test_instance) + mock_xml_without_disk = """ + + +""" + mock_dom = mock.MagicMock(return_value=mock_xml_without_disk) + encryption = {"provider": "NoOpEncryptor"} + mock_encryptor = mock.MagicMock(spec=encryptors.nop.NoOpEncryptor) + mock_encryptor.detach_volume = mock.MagicMock( + side_effect=processutils.ProcessExecutionError(exit_code=4) + ) + mock_get_encryptor.return_value = mock_encryptor + + connection_info = {"driver_volume_type": "fake", + "data": {"device_path": "/fake", + "access_mode": "rw"}} + + mock_dom.info.return_value = [power_state.RUNNING, 512, 512, 2, 1234, + 5678] + mock_get_domain.return_value = mock_dom + + drvr.detach_volume(connection_info, instance, '/dev/vdc', + encryption) + mock_get_encryptor.assert_called_once_with(connection_info, encryption) + mock_encryptor.detach_volume.assert_called_once_with(**encryption) + mock_disconnect_volume.assert_called_once_with( + connection_info, 'vdc', instance) + + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') + @mock.patch('nova.virt.libvirt.host.Host.get_domain') + def test_detach_volume_disk_not_found_encryption_err_reraise( + self, mock_get_domain, mock_disconnect_volume, + mock_get_encryptor): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + instance = objects.Instance(**self.test_instance) + mock_xml_without_disk = """ + + +""" + mock_dom = mock.MagicMock(return_value=mock_xml_without_disk) + encryption = {"provider": "NoOpEncryptor"} + mock_encryptor = mock.MagicMock(spec=encryptors.nop.NoOpEncryptor) + # Any nonzero exit code other than 4 would work here. + mock_encryptor.detach_volume = mock.MagicMock( + side_effect=processutils.ProcessExecutionError(exit_code=1) + ) + mock_get_encryptor.return_value = mock_encryptor + + connection_info = {"driver_volume_type": "fake", + "data": {"device_path": "/fake", + "access_mode": "rw"}} + + mock_dom.info.return_value = [power_state.RUNNING, 512, 512, 2, 1234, + 5678] + mock_get_domain.return_value = mock_dom + + self.assertRaises(processutils.ProcessExecutionError, + drvr.detach_volume, connection_info, instance, + '/dev/vdc', encryption) + mock_get_encryptor.assert_called_once_with(connection_info, encryption) + mock_encryptor.detach_volume.assert_called_once_with(**encryption) + mock_disconnect_volume.assert_not_called() @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index e9c956ee0a41..aaee780c2411 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1422,11 +1422,6 @@ class LibvirtDriver(driver.ComputeDriver): live=live) wait_for_detach() - if encryption: - encryptor = self._get_volume_encryptor(connection_info, - encryption) - encryptor.detach_volume(**encryption) - except exception.InstanceNotFound: # NOTE(zhaoqin): If the instance does not exist, _lookup_by_name() # will throw InstanceNotFound exception. Need to @@ -1434,7 +1429,11 @@ class LibvirtDriver(driver.ComputeDriver): LOG.warning("During detach_volume, instance disappeared.", instance=instance) except exception.DeviceNotFound: - raise exception.DiskNotFound(location=disk_dev) + # We should still try to disconnect logical device from + # host, an error might have happened during a previous + # call. + LOG.info("Device %s not found in instance.", + disk_dev, instance=instance) except libvirt.libvirtError as ex: # NOTE(vish): This is called to cleanup volumes after live # migration, so we should still disconnect even if @@ -1447,6 +1446,26 @@ class LibvirtDriver(driver.ComputeDriver): else: raise + try: + if encryption: + encryptor = self._get_volume_encryptor(connection_info, + encryption) + encryptor.detach_volume(**encryption) + except processutils.ProcessExecutionError as e: + # cryptsetup returns 4 when attempting to destroy a non-existent + # dm-crypt device. We assume here that the caller hasn't specified + # the wrong device, and that it doesn't exist because it has + # already been destroyed. + if e.exit_code == 4: + LOG.debug("Ignoring exit code 4, volume already destroyed") + else: + with excutils.save_and_reraise_exception(): + LOG.warning("Could not disconnect encrypted volume " + "%(volume)s. If dm-crypt device is still " + "active it will have to be destroyed manually " + "for cleanup to succeed.", + {'volume': disk_dev}) + self._disconnect_volume(connection_info, disk_dev, instance) def extend_volume(self, connection_info, instance):