Split up libvirt volume's connect_volume method

The connect_volume method in libvirt/volume.py does a couple of
things. It does the host OS integration/setup tasks, and it returns
the libvirt xml. In some cases, only the libvirt xml is required,
and the setup tasks are not necessary.

The following patch separates the setup tasks and libvirt xml return
in connect_volume. A new method, called get_config, is added to just
return the xml config, separate from the connect_volume method.
The connect_volume method will continue to return the libvirt xml,
so that existing functionality is not broken. This way, get_config
can be used to get the libvirt xml without any side-effects, e.g.
re-running shell commands to setup the volume.

There will be a follow up patch, where libvirt's driver.py will
switch over to use get_config and connect_volume will no longer
need to return the libvirt xml.

Change-Id: I5d300f8fe99f714d63d51197dc89f556c3a5d43b
Related-Bug: #1359617

nova/tests/virt/libvirt/test_driver.py

@@ -377,7 +377,7 @@ class FakeVolumeDriver(object):
     def get_xml(self, *args):
         return ""
 
-    def connect_volume(self, *args):
+    def get_config(self, *args):
         """Connect the volume to a fake device."""
         conf = vconfig.LibvirtConfigGuestDisk()
         conf.source_type = "network"
@@ -387,6 +387,10 @@ class FakeVolumeDriver(object):
         conf.target_bus = "fake"
         return conf
 
+    def connect_volume(self, *args):
+        """Connect the volume to a fake device."""
+        return self.get_config()
+
 
 class FakeConfigGuestDisk(object):
     def __init__(self, *args, **kwargs):
@@ -4472,6 +4476,26 @@ class LibvirtConnTestCase(test.TestCase,
         ret = conn._create_snapshot_metadata(base, instance, img_fmt, snp_name)
         self.assertEqual(ret, expected)
 
+    @mock.patch('nova.virt.libvirt.volume.LibvirtFakeVolumeDriver.'
+                'connect_volume')
+    @mock.patch('nova.virt.libvirt.volume.LibvirtFakeVolumeDriver.get_config')
+    def test_get_volume_config(self, get_config, connect_volume):
+        conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        connection_info = {'driver_volume_type': 'fake',
+                           'data': {'device_path': '/fake',
+                                    'access_mode': 'rw'}}
+        bdm = {'device_name': 'vdb',
+               'disk_bus': 'fake-bus',
+               'device_type': 'fake-type'}
+        disk_info = {'bus': bdm['disk_bus'], 'type': bdm['device_type'],
+                     'dev': 'vdb'}
+        mock_config = mock.MagicMock()
+
+        get_config.return_value = mock_config
+        config = conn._get_volume_config(connection_info, disk_info)
+        get_config.assert_called_once_with(connection_info, disk_info)
+        self.assertEqual(mock_config, config)
+
     def test_attach_invalid_volume_type(self):
         self.create_fake_libvirt_mock()
         libvirt_driver.LibvirtDriver._conn.lookupByName = self.fake_lookup

nova/tests/virt/libvirt/test_volume.py

@@ -227,6 +227,8 @@ class LibvirtVolumeTestCase(test.NoDBTestCase):
         self.assertIsNotNone(readonly)
 
     def iscsi_connection(self, volume, location, iqn):
+        dev_name = 'ip-%s-iscsi-%s-lun-1' % (location, iqn)
+        dev_path = '/dev/disk/by-path/%s' % (dev_name)
         return {
                 'driver_volume_type': 'iscsi',
                 'data': {
@@ -234,6 +236,7 @@ class LibvirtVolumeTestCase(test.NoDBTestCase):
                     'target_portal': location,
                     'target_iqn': iqn,
                     'target_lun': 1,
+                    'host_device': dev_path,
                     'qos_specs': {
                         'total_bytes_sec': '102400',
                         'read_iops_sec': '200',
@@ -372,6 +375,24 @@ Setting up iSCSI targets: unused
                                             ['-f', 'fake-multipath-devname'],
                                             check_exit_code=[0, 1])
 
+    def test_libvirt_iscsi_driver_get_config(self):
+        libvirt_driver = volume.LibvirtISCSIVolumeDriver(self.fake_conn)
+        dev_name = 'ip-%s-iscsi-%s-lun-1' % (self.location, self.iqn)
+        dev_path = '/dev/disk/by-path/%s' % (dev_name)
+        vol = {'id': 1, 'name': self.name}
+        connection_info = self.iscsi_connection(vol, self.location,
+                                                self.iqn)
+        conf = libvirt_driver.get_config(connection_info, self.disk_info)
+        tree = conf.format_dom()
+        self.assertEqual('block', tree.get('type'))
+        self.assertEqual(dev_path, tree.find('./source').get('dev'))
+
+        libvirt_driver.use_multipath = True
+        conf = libvirt_driver.get_config(connection_info, self.disk_info)
+        tree = conf.format_dom()
+        self.assertEqual('block', tree.get('type'))
+        self.assertEqual(dev_path, tree.find('./source').get('dev'))
+
     def test_sanitize_log_run_iscsiadm(self):
         # Tests that the parameters to the _run_iscsiadm function are sanitized
         # for passwords when logged.
@@ -763,6 +784,22 @@ Setting up iSCSI targets: unused
         libvirt_driver.disconnect_volume(connection_info, "vde")
         self.assertTrue(mock_LOG_exception.called)
 
+    def test_libvirt_nfs_driver_get_config(self):
+        libvirt_driver = volume.LibvirtNFSVolumeDriver(self.fake_conn)
+        mnt_base = '/mnt'
+        self.flags(nfs_mount_point_base=mnt_base, group='libvirt')
+        export_string = '192.168.1.1:/nfs/share1'
+        export_mnt_base = os.path.join(mnt_base,
+                                       utils.get_hash_str(export_string))
+        file_path = os.path.join(export_mnt_base, self.name)
+
+        connection_info = {'data': {'export': export_string,
+                                    'name': self.name}}
+        conf = libvirt_driver.get_config(connection_info, self.disk_info)
+        tree = conf.format_dom()
+        self._assertFileTypeEquals(tree, file_path)
+        self.assertEqual('raw', tree.find('./driver').get('type'))
+
     def test_libvirt_nfs_driver_already_mounted(self):
         # NOTE(vish) exists is to make driver assume connecting worked
         mnt_base = '/mnt'
@@ -839,6 +876,18 @@ Setting up iSCSI targets: unused
         self.assertEqual(tree.find('./source').get('dev'), aoedevpath)
         libvirt_driver.disconnect_volume(connection_info, "vde")
 
+    def test_libvirt_aoe_driver_get_config(self):
+        libvirt_driver = volume.LibvirtAOEVolumeDriver(self.fake_conn)
+        shelf = '100'
+        lun = '1'
+        connection_info = self.aoe_connection(shelf, lun)
+        conf = libvirt_driver.get_config(connection_info, self.disk_info)
+        tree = conf.format_dom()
+        aoedevpath = '/dev/etherd/e%s.%s' % (shelf, lun)
+        self.assertEqual('block', tree.get('type'))
+        self.assertEqual(aoedevpath, tree.find('./source').get('dev'))
+        libvirt_driver.disconnect_volume(connection_info, "vde")
+
     def test_libvirt_glusterfs_driver(self):
         mnt_base = '/mnt'
         self.flags(glusterfs_mount_point_base=mnt_base, group='libvirt')
@@ -863,6 +912,23 @@ Setting up iSCSI targets: unused
             ('umount', export_mnt_base)]
         self.assertEqual(expected_commands, self.executes)
 
+    def test_libvirt_glusterfs_driver_get_config(self):
+        mnt_base = '/mnt'
+        self.flags(glusterfs_mount_point_base=mnt_base, group='libvirt')
+
+        libvirt_driver = volume.LibvirtGlusterfsVolumeDriver(self.fake_conn)
+        export_string = '192.168.1.1:/volume-00001'
+        export_mnt_base = os.path.join(mnt_base,
+                                       utils.get_hash_str(export_string))
+        file_path = os.path.join(export_mnt_base, self.name)
+
+        connection_info = {'data': {'export': export_string,
+                                    'name': self.name}}
+        conf = libvirt_driver.get_config(connection_info, self.disk_info)
+        tree = conf.format_dom()
+        self._assertFileTypeEquals(tree, file_path)
+        self.assertEqual('raw', tree.find('./driver').get('type'))
+
     def test_libvirt_glusterfs_driver_already_mounted(self):
         mnt_base = '/mnt'
         self.flags(glusterfs_mount_point_base=mnt_base, group='libvirt')
@@ -1032,6 +1098,18 @@ Setting up iSCSI targets: unused
                           libvirt_driver.connect_volume,
                           connection_info, self.disk_info)
 
+    def test_libvirt_fibrechan_driver_get_config(self):
+        libvirt_driver = volume.LibvirtFibreChannelVolumeDriver(self.fake_conn)
+        connection_info = self.fibrechan_connection(self.vol,
+                                                    self.location, 123)
+        connection_info['data']['device_path'] = ("/sys/devices/pci0000:00"
+            "/0000:00:03.0/0000:05:00.3/host2/fc_host/host2")
+        conf = libvirt_driver.get_config(connection_info, self.disk_info)
+        tree = conf.format_dom()
+        self.assertEqual('block', tree.get('type'))
+        self.assertEqual(connection_info['data']['device_path'],
+                         tree.find('./source').get('dev'))
+
     def test_libvirt_fibrechan_getpci_num(self):
         libvirt_driver = volume.LibvirtFibreChannelVolumeDriver(self.fake_conn)
         hba = {'device_path': "/sys/devices/pci0000:00/0000:00:03.0"

nova/virt/libvirt/driver.py

@@ -1325,6 +1325,13 @@ class LibvirtDriver(driver.ComputeDriver):
         driver = self.volume_drivers[driver_type]
         return driver.disconnect_volume(connection_info, disk_dev)
 
+    def _get_volume_config(self, connection_info, disk_info):
+        driver_type = connection_info.get('driver_volume_type')
+        if driver_type not in self.volume_drivers:
+            raise exception.VolumeDriverNotFound(driver_type=driver_type)
+        driver = self.volume_drivers[driver_type]
+        return driver.get_config(connection_info, disk_info)
+
     def _get_volume_encryptor(self, connection_info, encryption):
         encryptor = encryptors.get_volume_encryptor(connection_info,
                                                     **encryption)

nova/virt/libvirt/volume.py

@@ -93,14 +93,14 @@ class LibvirtBaseVolumeDriver(object):
         self.connection = connection
         self.is_block_dev = is_block_dev
 
-    def connect_volume(self, connection_info, disk_info):
-        """Connect the volume. Returns xml for libvirt."""
-
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = vconfig.LibvirtConfigGuestDisk()
         conf.driver_name = libvirt_utils.pick_disk_driver_name(
             self.connection._get_hypervisor_version(),
             self.is_block_dev
         )
+
         conf.source_device = disk_info['type']
         conf.driver_format = "raw"
         conf.driver_cache = "none"
@@ -142,10 +142,14 @@ class LibvirtBaseVolumeDriver(object):
                               'connection_info/access_mode: %s'),
                           access_mode)
                 raise exception.InvalidVolumeAccessMode(
-                                                    access_mode=access_mode)
+                    access_mode=access_mode)
 
         return conf
 
+    def connect_volume(self, connection_info, disk_info):
+        """Connect the volume. Returns xml for libvirt."""
+        return self.get_config(connection_info, disk_info)
+
     def disconnect_volume(self, connection_info, disk_dev):
         """Disconnect the volume."""
         pass
@@ -157,11 +161,10 @@ class LibvirtVolumeDriver(LibvirtBaseVolumeDriver):
         super(LibvirtVolumeDriver,
               self).__init__(connection, is_block_dev=True)
 
-    def connect_volume(self, connection_info, disk_info):
-        """Connect the volume to a local device."""
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = super(LibvirtVolumeDriver,
-                     self).connect_volume(connection_info,
-                                          disk_info)
+                     self).get_config(connection_info, disk_info)
         conf.source_type = "block"
         conf.source_path = connection_info['data']['device_path']
         return conf
@@ -173,11 +176,10 @@ class LibvirtFakeVolumeDriver(LibvirtBaseVolumeDriver):
         super(LibvirtFakeVolumeDriver,
               self).__init__(connection, is_block_dev=True)
 
-    def connect_volume(self, connection_info, disk_info):
-        """Connect the volume to a fake device."""
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = super(LibvirtFakeVolumeDriver,
-                     self).connect_volume(connection_info,
-                                          disk_info)
+                     self).get_config(connection_info, disk_info)
         conf.source_type = "network"
         conf.source_protocol = "fake"
         conf.source_name = "fake"
@@ -190,10 +192,11 @@ class LibvirtNetVolumeDriver(LibvirtBaseVolumeDriver):
         super(LibvirtNetVolumeDriver,
               self).__init__(connection, is_block_dev=False)
 
-    def connect_volume(self, connection_info, disk_info):
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = super(LibvirtNetVolumeDriver,
-                     self).connect_volume(connection_info,
-                                          disk_info)
+                     self).get_config(connection_info, disk_info)
+
         netdisk_properties = connection_info['data']
         conf.source_type = "network"
         conf.source_protocol = connection_info['driver_volume_type']
@@ -256,13 +259,17 @@ class LibvirtISCSIVolumeDriver(LibvirtBaseVolumeDriver):
                 targets.append(data)
         return targets
 
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
+        conf = super(LibvirtISCSIVolumeDriver,
+                     self).get_config(connection_info, disk_info)
+        conf.source_type = "block"
+        conf.source_path = connection_info['data']['host_device']
+        return conf
+
     @utils.synchronized('connect_volume')
     def connect_volume(self, connection_info, disk_info):
         """Attach the volume to instance_name."""
-        conf = super(LibvirtISCSIVolumeDriver,
-                     self).connect_volume(connection_info,
-                                          disk_info)
-
         iscsi_properties = connection_info['data']
 
         if self.use_multipath:
@@ -328,9 +335,8 @@ class LibvirtISCSIVolumeDriver(LibvirtBaseVolumeDriver):
             if multipath_device is not None:
                 host_device = multipath_device
 
-        conf.source_type = "block"
-        conf.source_path = host_device
-        return conf
+        connection_info['data']['host_device'] = host_device
+        return self.get_config(connection_info, disk_info)
 
     @utils.synchronized('connect_volume')
     def disconnect_volume(self, connection_info, disk_dev):
@@ -648,19 +654,26 @@ class LibvirtNFSVolumeDriver(LibvirtBaseVolumeDriver):
         super(LibvirtNFSVolumeDriver,
               self).__init__(connection, is_block_dev=False)
 
-    def connect_volume(self, connection_info, disk_info):
-        """Connect the volume. Returns xml for libvirt."""
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = super(LibvirtNFSVolumeDriver,
-                     self).connect_volume(connection_info,
-                                          disk_info)
-        options = connection_info['data'].get('options')
-        path = self._ensure_mounted(connection_info['data']['export'], options)
+                     self).get_config(connection_info, disk_info)
+
+        path = os.path.join(CONF.libvirt.nfs_mount_point_base,
+            utils.get_hash_str(connection_info['data']['export']))
         path = os.path.join(path, connection_info['data']['name'])
         conf.source_type = 'file'
         conf.source_path = path
         conf.driver_format = connection_info['data'].get('format', 'raw')
         return conf
 
+    def connect_volume(self, connection_info, disk_info):
+        """Connect the volume. Returns xml for libvirt."""
+        options = connection_info['data'].get('options')
+        self._ensure_mounted(connection_info['data']['export'], options)
+
+        return self.get_config(connection_info, disk_info)
+
     def disconnect_volume(self, connection_info, disk_dev):
         """Disconnect the volume."""
 
@@ -726,6 +739,20 @@ class LibvirtAOEVolumeDriver(LibvirtBaseVolumeDriver):
                                    run_as_root=True, check_exit_code=0)
         return (out, err)
 
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
+        conf = super(LibvirtAOEVolumeDriver,
+                     self).get_config(connection_info, disk_info)
+
+        shelf = connection_info['data']['target_shelf']
+        lun = connection_info['data']['target_lun']
+        aoedev = 'e%s.%s' % (shelf, lun)
+        aoedevpath = '/dev/etherd/%s' % (aoedev)
+
+        conf.source_type = "block"
+        conf.source_path = aoedevpath
+        return conf
+
     def connect_volume(self, connection_info, mount_device):
         shelf = connection_info['data']['target_shelf']
         lun = connection_info['data']['target_lun']
@@ -767,11 +794,7 @@ class LibvirtAOEVolumeDriver(LibvirtBaseVolumeDriver):
                       {'aoedevpath': aoedevpath,
                        'tries': tries})
 
-        conf = super(LibvirtAOEVolumeDriver,
-                     self).connect_volume(connection_info, mount_device)
-        conf.source_type = "block"
-        conf.source_path = aoedevpath
-        return conf
+        return self.get_config(connection_info, mount_device)
 
 
 class LibvirtGlusterfsVolumeDriver(LibvirtBaseVolumeDriver):
@@ -782,10 +805,10 @@ class LibvirtGlusterfsVolumeDriver(LibvirtBaseVolumeDriver):
         super(LibvirtGlusterfsVolumeDriver,
               self).__init__(connection, is_block_dev=False)
 
-    def connect_volume(self, connection_info, mount_device):
-        """Connect the volume. Returns xml for libvirt."""
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = super(LibvirtGlusterfsVolumeDriver,
-                     self).connect_volume(connection_info, mount_device)
+                     self).get_config(connection_info, disk_info)
 
         data = connection_info['data']
 
@@ -799,7 +822,8 @@ class LibvirtGlusterfsVolumeDriver(LibvirtBaseVolumeDriver):
             conf.source_hosts = [source_host]
             conf.source_name = '%s/%s' % (vol_name, data['name'])
         else:
-            path = self._ensure_mounted(data['export'], data.get('options'))
+            path = os.path.join(CONF.libvirt.glusterfs_mount_point_base,
+                utils.get_hash_str(data['export']))
             path = os.path.join(path, data['name'])
             conf.source_type = 'file'
             conf.source_path = path
@@ -808,6 +832,14 @@ class LibvirtGlusterfsVolumeDriver(LibvirtBaseVolumeDriver):
 
         return conf
 
+    def connect_volume(self, connection_info, mount_device):
+        data = connection_info['data']
+
+        if 'gluster' not in CONF.libvirt.qemu_allowed_storage_drivers:
+            self._ensure_mounted(data['export'], data.get('options'))
+
+        return self.get_config(connection_info, mount_device)
+
     def disconnect_volume(self, connection_info, disk_dev):
         """Disconnect the volume."""
 
@@ -885,6 +917,15 @@ class LibvirtFibreChannelVolumeDriver(LibvirtBaseVolumeDriver):
 
         return pci_num
 
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
+        conf = super(LibvirtFibreChannelVolumeDriver,
+                     self).get_config(connection_info, disk_info)
+
+        conf.source_type = "block"
+        conf.source_path = connection_info['data']['device_path']
+        return conf
+
     @utils.synchronized('connect_volume')
     def connect_volume(self, connection_info, disk_info):
         """Attach the volume to instance_name."""
@@ -968,6 +1009,7 @@ class LibvirtFibreChannelVolumeDriver(LibvirtBaseVolumeDriver):
             LOG.debug("Multipath device discovered %(device)s",
                       {'device': mdev_info['device']})
             device_path = mdev_info['device']
+            connection_info['data']['device_path'] = device_path
             connection_info['data']['devices'] = mdev_info['devices']
             connection_info['data']['multipath_id'] = mdev_info['id']
         else:
@@ -975,14 +1017,10 @@ class LibvirtFibreChannelVolumeDriver(LibvirtBaseVolumeDriver):
             # so we assume the kernel only sees 1 device
             device_path = self.host_device
             device_info = linuxscsi.get_device_info(self.device_name)
+            connection_info['data']['device_path'] = device_path
             connection_info['data']['devices'] = [device_info]
 
-        conf = super(LibvirtFibreChannelVolumeDriver,
-                     self).connect_volume(connection_info, disk_info)
-
-        conf.source_type = "block"
-        conf.source_path = device_path
-        return conf
+        return self.get_config(connection_info, disk_info)
 
     @utils.synchronized('connect_volume')
     def disconnect_volume(self, connection_info, mount_device):
@@ -1021,12 +1059,10 @@ class LibvirtScalityVolumeDriver(LibvirtBaseVolumeDriver):
         super(LibvirtScalityVolumeDriver,
               self).__init__(connection, is_block_dev=False)
 
-    def connect_volume(self, connection_info, disk_info):
-        """Connect the volume. Returns xml for libvirt."""
-        self._check_prerequisites()
-        self._mount_sofs()
+    def get_config(self, connection_info, disk_info):
+        """Returns xml for libvirt."""
         conf = super(LibvirtScalityVolumeDriver,
-                     self).connect_volume(connection_info, disk_info)
+                     self).get_config(connection_info, disk_info)
         path = os.path.join(CONF.libvirt.scality_sofs_mount_point,
                             connection_info['data']['sofs_path'])
         conf.source_type = 'file'
@@ -1040,6 +1076,13 @@ class LibvirtScalityVolumeDriver(LibvirtBaseVolumeDriver):
 
         return conf
 
+    def connect_volume(self, connection_info, disk_info):
+        """Connect the volume. Returns xml for libvirt."""
+        self._check_prerequisites()
+        self._mount_sofs()
+
+        return self.get_config(connection_info, disk_info)
+
     def _check_prerequisites(self):
         """Sanity checks before attempting to mount SOFS."""