libvirt: add discard support for attached volumes

If Cinder reports discard=true capability then add
discard=unmap to disk config setting on attached volume.

To have this actually provide unmap/trim support the instance must have
been configured with a controller+bus type that can support it. As of
now the only one known not to work is virtio-blk. In the case where we
detect a configuration that will not work a warning will be logged, but
the attachment will continue with the default behavior.

An example of how to get a controller and bus to support this is to set
the following Glance properties on an image:

hw_scsi_model=virtio-scsi
hw_disk_bus=scsi

This feature also has some requirements for the libvirt and qemu
versions being used. There is a check to ensure that libvirt is at least
1.0.6 and qemu is at least 1.6.0. If the minimum versions are not met
a warning will be logged, but the attachment will continue with the
default behavior.

Change-Id: Icd2d20bb6906d9106df34a970b3d81b17bc3ec07
Implements: bp cinder-backend-report-discard

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,

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]"))

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)

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):

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.
+