diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 486bff1f2fa2..d251a62b56f7 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -5186,6 +5186,37 @@ class LibvirtConnTestCase(test.NoDBTestCase): instance, "/dev/sda") + def _test_check_discard(self, mock_log, driver_discard=None, + bus=None, should_log=False): + mock_config = mock.Mock() + mock_config.driver_discard = driver_discard + mock_config.target_bus = bus + mock_instance = mock.Mock() + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + drvr._check_discard_for_attach_volume(mock_config, mock_instance) + self.assertEqual(should_log, mock_log.called) + + @mock.patch('nova.virt.libvirt.driver.LOG.debug') + def test_check_discard_for_attach_volume_no_unmap(self, mock_log): + self._test_check_discard(mock_log, driver_discard=None, + bus='scsi', should_log=False) + + @mock.patch('nova.virt.libvirt.driver.LOG.debug') + def test_check_discard_for_attach_volume_blk_controller(self, mock_log): + self._test_check_discard(mock_log, driver_discard='unmap', + bus='virtio', should_log=True) + + @mock.patch('nova.virt.libvirt.driver.LOG.debug') + def test_check_discard_for_attach_volume_valid_controller(self, mock_log): + self._test_check_discard(mock_log, driver_discard='unmap', + bus='scsi', should_log=False) + + @mock.patch('nova.virt.libvirt.driver.LOG.debug') + def test_check_discard_for_attach_volume_blk_controller_no_unmap(self, + mock_log): + self._test_check_discard(mock_log, driver_discard=None, + bus='virtio', should_log=False) + @mock.patch('nova.utils.get_image_from_system_metadata') @mock.patch('nova.virt.libvirt.blockinfo.get_info_from_bdm') @mock.patch('nova.virt.libvirt.host.Host.get_domain') @@ -5215,9 +5246,10 @@ class LibvirtConnTestCase(test.NoDBTestCase): mock.patch.object(drvr, '_connect_volume'), mock.patch.object(drvr, '_get_volume_config', return_value=mock_conf), - mock.patch.object(drvr, '_set_cache_mode') + mock.patch.object(drvr, '_set_cache_mode'), + mock.patch.object(drvr, '_check_discard_for_attach_volume') ) as (mock_connect_volume, mock_get_volume_config, - mock_set_cache_mode): + mock_set_cache_mode, mock_check_discard): for state in (power_state.RUNNING, power_state.PAUSED): mock_dom.info.return_value = [state, 512, 512, 2, 1234, 5678] @@ -5238,6 +5270,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): mock_set_cache_mode.assert_called_with(mock_conf) mock_dom.attachDeviceFlags.assert_called_with( mock_conf.to_xml(), flags=flags) + mock_check_discard.assert_called_with(mock_conf, instance) @mock.patch('nova.virt.libvirt.host.Host.get_domain') def test_detach_volume_with_vir_domain_affect_live_flag(self, diff --git a/nova/tests/unit/virt/libvirt/volume/test_volume.py b/nova/tests/unit/virt/libvirt/volume/test_volume.py index d522435eea59..9ed2f9ba0048 100644 --- a/nova/tests/unit/virt/libvirt/volume/test_volume.py +++ b/nova/tests/unit/virt/libvirt/volume/test_volume.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import mock + from nova import exception from nova import test from nova.tests.unit.virt.libvirt import fakelibvirt @@ -167,6 +169,7 @@ class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase): self.assertEqual('block', tree.get('type')) self.assertEqual('fake_serial', tree.find('./serial').text) self.assertIsNone(tree.find('./blockio')) + self.assertIsNone(tree.find("driver[@discard]")) def test_libvirt_volume_driver_blockio(self): libvirt_driver = volume.LibvirtVolumeDriver(self.fake_conn) @@ -258,3 +261,58 @@ class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase): tree = conf.format_dom() readonly = tree.find('./readonly') self.assertIsNotNone(readonly) + + @mock.patch('nova.virt.libvirt.host.Host.has_min_version') + def test_libvirt_volume_driver_discard_true(self, mock_has_min_version): + # Check the discard attrib is present in driver section + mock_has_min_version.return_value = True + libvirt_driver = volume.LibvirtVolumeDriver(self.fake_conn) + connection_info = { + 'driver_volume_type': 'fake', + 'data': { + 'device_path': '/foo', + 'discard': True, + }, + 'serial': 'fake_serial', + } + + conf = libvirt_driver.get_config(connection_info, self.disk_info) + tree = conf.format_dom() + driver_node = tree.find("driver[@discard]") + self.assertIsNotNone(driver_node) + self.assertEqual('unmap', driver_node.attrib['discard']) + + def test_libvirt_volume_driver_discard_false(self): + # Check the discard attrib is not present in driver section + libvirt_driver = volume.LibvirtVolumeDriver(self.fake_conn) + connection_info = { + 'driver_volume_type': 'fake', + 'data': { + 'device_path': '/foo', + 'discard': False, + }, + 'serial': 'fake_serial', + } + + conf = libvirt_driver.get_config(connection_info, self.disk_info) + tree = conf.format_dom() + self.assertIsNone(tree.find("driver[@discard]")) + + @mock.patch('nova.virt.libvirt.host.Host.has_min_version') + def test_libvirt_volume_driver_discard_true_bad_version( + self, mock_has_min_version): + # Check the discard attrib is not present in driver section + mock_has_min_version.return_value = False + libvirt_driver = volume.LibvirtVolumeDriver(self.fake_conn) + connection_info = { + 'driver_volume_type': 'fake', + 'data': { + 'device_path': '/foo', + 'discard': True, + }, + 'serial': 'fake_serial', + } + + conf = libvirt_driver.get_config(connection_info, self.disk_info) + tree = conf.format_dom() + self.assertIsNone(tree.find("driver[@discard]")) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index b9eecd296e74..587bce020b20 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1095,6 +1095,23 @@ class LibvirtDriver(driver.ComputeDriver): **encryption) return encryptor + def _check_discard_for_attach_volume(self, conf, instance): + """Perform some checks for volumes configured for discard support. + + If discard is configured for the volume, and the guest is using a + configuration known to not work, we will log a message explaining + the reason why. + """ + if conf.driver_discard == 'unmap' and conf.target_bus == 'virtio': + LOG.debug('Attempting to attach volume %(id)s with discard ' + 'support enabled to an instance using an ' + 'unsupported configuration. target_bus = ' + '%(bus)s. Trim commands will not be issued to ' + 'the storage device.', + {'bus': conf.target_bus, + 'id': conf.serial}, + instance=instance) + def attach_volume(self, context, connection_info, instance, mountpoint, disk_bus=None, device_type=None, encryption=None): image_meta = objects.ImageMeta.from_instance(instance) @@ -1128,6 +1145,8 @@ class LibvirtDriver(driver.ComputeDriver): conf = self._get_volume_config(connection_info, disk_info) self._set_cache_mode(conf) + self._check_discard_for_attach_volume(conf, instance) + try: state = guest.get_power_state(self._host) live = state in (power_state.RUNNING, power_state.PAUSED) diff --git a/nova/virt/libvirt/volume/volume.py b/nova/virt/libvirt/volume/volume.py index cad72f531327..6ce01fbd346c 100644 --- a/nova/virt/libvirt/volume/volume.py +++ b/nova/virt/libvirt/volume/volume.py @@ -24,6 +24,8 @@ from nova import exception from nova.i18n import _LE from nova.i18n import _LW from nova.virt.libvirt import config as vconfig +import nova.virt.libvirt.driver +from nova.virt.libvirt import host from nova.virt.libvirt import utils as libvirt_utils LOG = logging.getLogger(__name__) @@ -38,6 +40,8 @@ volume_opts = [ CONF = cfg.CONF CONF.register_opts(volume_opts, 'libvirt') +SHOULD_LOG_DISCARD_WARNING = True + class LibvirtBaseVolumeDriver(object): """Base class for volume drivers.""" @@ -96,6 +100,29 @@ class LibvirtBaseVolumeDriver(object): raise exception.InvalidVolumeAccessMode( access_mode=access_mode) + # Configure usage of discard + if data.get('discard', False) is True: + min_qemu = nova.virt.libvirt.driver.MIN_QEMU_DISCARD_VERSION + min_libvirt = nova.virt.libvirt.driver.MIN_LIBVIRT_DISCARD_VERSION + if self.connection._host.has_min_version(min_libvirt, + min_qemu, + host.HV_DRIVER_QEMU): + conf.driver_discard = 'unmap' + else: + global SHOULD_LOG_DISCARD_WARNING + if SHOULD_LOG_DISCARD_WARNING: + SHOULD_LOG_DISCARD_WARNING = False + LOG.warning(_LW('Unable to attach %(type)s volume ' + '%(serial)s with discard enabled: qemu ' + '%(qemu)s and libvirt %(libvirt)s or ' + 'later are required.'), + { + 'qemu': min_qemu, + 'libvirt': min_libvirt, + 'serial': conf.serial, + 'type': connection_info['driver_volume_type'] + }) + return conf def connect_volume(self, connection_info, disk_info): diff --git a/releasenotes/notes/cinder-backend-report-discard-1def1c28140def9b.yaml b/releasenotes/notes/cinder-backend-report-discard-1def1c28140def9b.yaml new file mode 100644 index 000000000000..654c95e6c117 --- /dev/null +++ b/releasenotes/notes/cinder-backend-report-discard-1def1c28140def9b.yaml @@ -0,0 +1,9 @@ +--- +features: + - Add support for enabling discard support for block devices with libvirt. + This will be enabled for Cinder volume attachments that specify support + for the feature in their connection properties. This requires support to + be present in the version of libvirt (v1.0.6+) and qemu (v1.6.0+) used + along with the configured virtual drivers for the instance. The + virtio-blk driver does not support this functionality. +