Fixes nova-volume support for multiple luns

 * stores lun in provider_location if specified
 * passes lun in iscsi_properties instead of hard coding
 * adds call to libvirt to list all used block devices
 * make sure to synchronize connect and disconnect commands
 * only disconnect from target if no luns are in use
 * allow double logins to targets
 * fixes typo in get_volume_connector in xenapi_connection
 * fixes bug 929790

Change-Id: I2466dc750a6fa5e0b07f94314d38873740aa6b29

6 files changed, 159 insertions, 35 deletions

diff --git a/nova/tests/test_libvirt.py b/nova/tests/test_libvirt.py
index 2eede7ed5..f0f1520ee 100644
--- a/nova/tests/test_libvirt.py
+++ b/nova/tests/test_libvirt.py
@@ -124,7 +124,11 @@ class LibvirtVolumeTestCase(test.TestCase):
 
             def get_hypervisor_type(self):
                 return self.hyperv
-        self.fake_conn = FakeLibvirtConnection("Xen")
+
+            def get_all_block_devices(self):
+                return []
+
+        self.fake_conn = FakeLibvirtConnection()
         self.connr = {
             'ip': '127.0.0.1',
             'initiator': 'fake_initiator'
@@ -167,6 +171,38 @@ class LibvirtVolumeTestCase(test.TestCase):
                               '-p', location, '--op', 'delete')]
         self.assertEqual(self.executes, expected_commands)
 
+    def test_libvirt_iscsi_driver_still_in_use(self):
+        # NOTE(vish) exists is to make driver assume connecting worked
+        self.stubs.Set(os.path, 'exists', lambda x: True)
+        vol_driver = volume_driver.ISCSIDriver()
+        libvirt_driver = volume.LibvirtISCSIVolumeDriver(self.fake_conn)
+        location = '10.0.2.15:3260'
+        name = 'volume-00000001'
+        iqn = 'iqn.2010-10.org.openstack:%s' % name
+        devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location, iqn)]
+        self.stubs.Set(self.fake_conn, 'get_all_block_devices', lambda: devs)
+        vol = {'id': 1,
+               'name': name,
+               'provider_auth': None,
+               'provider_location': '%s,fake %s' % (location, iqn)}
+        connection_info = vol_driver.initialize_connection(vol, self.connr)
+        mount_device = "vde"
+        xml = libvirt_driver.connect_volume(connection_info, mount_device)
+        tree = xml_to_tree(xml)
+        dev_str = '/dev/disk/by-path/ip-%s-iscsi-%s-lun-0' % (location, iqn)
+        self.assertEqual(tree.get('type'), 'block')
+        self.assertEqual(tree.find('./source').get('dev'), dev_str)
+        libvirt_driver.disconnect_volume(connection_info, mount_device)
+        connection_info = vol_driver.terminate_connection(vol, self.connr)
+        expected_commands = [('iscsiadm', '-m', 'node', '-T', iqn,
+                              '-p', location),
+                             ('iscsiadm', '-m', 'node', '-T', iqn,
+                              '-p', location, '--login'),
+                             ('iscsiadm', '-m', 'node', '-T', iqn,
+                              '-p', location, '--op', 'update',
+                              '-n', 'node.startup', '-v', 'automatic')]
+        self.assertEqual(self.executes, expected_commands)
+
     def test_libvirt_sheepdog_driver(self):
         vol_driver = volume_driver.SheepdogDriver()
         libvirt_driver = volume.LibvirtNetVolumeDriver(self.fake_conn)
@@ -469,6 +505,57 @@ class LibvirtConnTestCase(test.TestCase):
         # Only one should be listed, since domain with ID 0 must be skiped
         self.assertEquals(len(instances), 1)
 
+    def test_get_all_block_devices(self):
+        xml = [
+            # NOTE(vish): id 0 is skipped
+            None,
+            """
+                <domain type='kvm'>
+                    <devices>
+                        <disk type='file'>
+                            <source file='filename'/>
+                        </disk>
+                        <disk type='block'>
+                            <source dev='/path/to/dev/1'/>
+                        </disk>
+                    </devices>
+                </domain>
+            """,
+            """
+                <domain type='kvm'>
+                    <devices>
+                        <disk type='file'>
+                            <source file='filename'/>
+                        </disk>
+                    </devices>
+                </domain>
+            """,
+            """
+                <domain type='kvm'>
+                    <devices>
+                        <disk type='file'>
+                            <source file='filename'/>
+                        </disk>
+                        <disk type='block'>
+                            <source dev='/path/to/dev/3'/>
+                        </disk>
+                    </devices>
+                </domain>
+            """,
+        ]
+
+        def fake_lookup(id):
+            return FakeVirtDomain(xml[id])
+
+        self.mox.StubOutWithMock(connection.LibvirtConnection, '_conn')
+        connection.LibvirtConnection._conn.listDomainsID = lambda: range(4)
+        connection.LibvirtConnection._conn.lookupByID = fake_lookup
+
+        self.mox.ReplayAll()
+        conn = connection.LibvirtConnection(False)
+        devices = conn.get_all_block_devices()
+        self.assertEqual(devices, ['/path/to/dev/1', '/path/to/dev/3'])
+
     @test.skip_if(missing_libvirt(), "Test requires libvirt")
     def test_snapshot_in_ami_format(self):
         self.flags(image_service='nova.image.fake.FakeImageService')
diff --git a/nova/tests/test_xenapi.py b/nova/tests/test_xenapi.py
index 2673977f4..f3b48a56c 100644
--- a/nova/tests/test_xenapi.py
+++ b/nova/tests/test_xenapi.py
@@ -132,6 +132,7 @@ class XenAPIVolumeTestCase(test.TestCase):
                 'volume_id': 1,
                 'target_iqn': 'iqn.2010-10.org.openstack:volume-00000001',
                 'target_portal': '127.0.0.1:3260,fake',
+                'target_lun': None,
                 'auth_method': 'CHAP',
                 'auth_method': 'fake',
                 'auth_method': 'fake',
diff --git a/nova/virt/libvirt/connection.py b/nova/virt/libvirt/connection.py
index c3bfdedda..349faa157 100644
--- a/nova/virt/libvirt/connection.py
+++ b/nova/virt/libvirt/connection.py
@@ -1388,6 +1388,26 @@ class LibvirtConnection(driver.ComputeDriver):
 
         return domain
 
+    def get_all_block_devices(self):
+        """
+        Return all block devices in use on this node.
+        """
+        devices = []
+        for dom_id in self._conn.listDomainsID():
+            domain = self._conn.lookupByID(dom_id)
+            try:
+                doc = ElementTree.fromstring(domain.XMLDesc(0))
+            except Exception:
+                continue
+            ret = doc.findall('./devices/disk')
+            for node in ret:
+                if node.get('type') != 'block':
+                    continue
+                for child in node.getchildren():
+                    if child.tag == 'source':
+                        devices.append(child.get('dev'))
+        return devices
+
     def get_disks(self, instance_name):
         """
         Note that this function takes an instance name.
diff --git a/nova/virt/libvirt/volume.py b/nova/virt/libvirt/volume.py
index 75fa0e5a9..055e34abc 100644
--- a/nova/virt/libvirt/volume.py
+++ b/nova/virt/libvirt/volume.py
@@ -103,14 +103,16 @@ class LibvirtISCSIVolumeDriver(LibvirtVolumeDriver):
                          '-v', property_value)
         return self._run_iscsiadm(iscsi_properties, iscsi_command)
 
+    @utils.synchronized('connect_volume')
     def connect_volume(self, connection_info, mount_device):
         """Attach the volume to instance_name"""
         iscsi_properties = connection_info['data']
-        # NOTE(vish): if we are on the same host as nova volume, the
+        # NOTE(vish): If we are on the same host as nova volume, the
         #             discovery makes the target so we don't need to
         #             run --op new. Therefore, we check to see if the
         #             target exists, and if we get 255 (Not Found), then
-        #             we run --op new
+        #             we run --op new. This will also happen if another
+        #             volume is using the same target.
         try:
             self._run_iscsiadm(iscsi_properties, ())
         except exception.ProcessExecutionError as exc:
@@ -131,18 +133,17 @@ class LibvirtISCSIVolumeDriver(LibvirtVolumeDriver):
                                   "node.session.auth.password",
                                   iscsi_properties['auth_password'])
 
-        self._run_iscsiadm(iscsi_properties, ("--login",))
+        # NOTE(vish): If we have another lun on the same target, we may
+        #             have a duplicate login
+        self._run_iscsiadm(iscsi_properties, ("--login",),
+                           check_exit_code=[0, 255])
 
         self._iscsiadm_update(iscsi_properties, "node.startup", "automatic")
 
-        if FLAGS.iscsi_helper == 'tgtadm':
-            host_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-1" %
-                            (iscsi_properties['target_portal'],
-                             iscsi_properties['target_iqn']))
-        else:
-            host_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
-                            (iscsi_properties['target_portal'],
-                             iscsi_properties['target_iqn']))
+        host_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-%s" %
+                        (iscsi_properties['target_portal'],
+                         iscsi_properties['target_iqn'],
+                         iscsi_properties.get('target_lun', 0)))
 
         # The /dev/disk/by-path/... node is not always present immediately
         # TODO(justinsb): This retry-with-delay is a pattern, move to utils?
@@ -172,13 +173,22 @@ class LibvirtISCSIVolumeDriver(LibvirtVolumeDriver):
         sup = super(LibvirtISCSIVolumeDriver, self)
         return sup.connect_volume(connection_info, mount_device)
 
+    @utils.synchronized('connect_volume')
     def disconnect_volume(self, connection_info, mount_device):
         """Detach the volume from instance_name"""
         sup = super(LibvirtISCSIVolumeDriver, self)
         sup.disconnect_volume(connection_info, mount_device)
         iscsi_properties = connection_info['data']
-        self._iscsiadm_update(iscsi_properties, "node.startup", "manual")
-        self._run_iscsiadm(iscsi_properties, ("--logout",),
-                           check_exit_code=[0, 255])
-        self._run_iscsiadm(iscsi_properties, ('--op', 'delete'),
-                           check_exit_code=[0, 255])
+        # NOTE(vish): Only disconnect from the target if no luns from the
+        #             target are in use.
+        device_prefix = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-" %
+                         (iscsi_properties['target_portal'],
+                          iscsi_properties['target_iqn']))
+        devices = self.connection.get_all_block_devices()
+        devices = [dev for dev in devices if dev.startswith(device_prefix)]
+        if not devices:
+            self._iscsiadm_update(iscsi_properties, "node.startup", "manual")
+            self._run_iscsiadm(iscsi_properties, ("--logout",),
+                               check_exit_code=[0, 255])
+            self._run_iscsiadm(iscsi_properties, ('--op', 'delete'),
+                               check_exit_code=[0, 255])
diff --git a/nova/virt/xenapi_conn.py b/nova/virt/xenapi_conn.py
index 1e2d11bb0..08542268c 100644
--- a/nova/virt/xenapi_conn.py
+++ b/nova/virt/xenapi_conn.py
@@ -346,7 +346,7 @@ class XenAPIConnection(driver.ComputeDriver):
     def get_volume_connector(self, _instance):
         """Return volume connector information"""
         if not self._initiator:
-            stats = self.get_host_stats(update=True)
+            stats = self.get_host_stats(refresh=True)
             try:
                 self._initiator = stats['host_other-config']['iscsi_iqn']
             except (TypeError, KeyError):
diff --git a/nova/volume/driver.py b/nova/volume/driver.py
index a92c51c6b..924991860 100644
--- a/nova/volume/driver.py
+++ b/nova/volume/driver.py
@@ -295,8 +295,12 @@ class ISCSIDriver(VolumeDriver):
         self.tgtadm.new_logicalunit(iscsi_target, 0, volume_path)
 
         model_update = {}
+        if FLAGS.iscsi_helper == 'tgtadm':
+            lun = 1
+        else:
+            lun = 0
         model_update['provider_location'] = _iscsi_location(
-            FLAGS.iscsi_ip_address, iscsi_target, iscsi_name)
+            FLAGS.iscsi_ip_address, iscsi_target, iscsi_name, lun)
         return model_update
 
     def remove_export(self, context, volume):
@@ -349,6 +353,8 @@ class ISCSIDriver(VolumeDriver):
 
         :target_portal:    the portal of the iSCSI target
 
+        :target_lun:    the lun of the iSCSI target
+
         :volume_id:    the id of the volume (currently used by xen)
 
         :auth_method:, :auth_username:, :auth_password:
@@ -376,16 +382,20 @@ class ISCSIDriver(VolumeDriver):
             LOG.debug(_("ISCSI Discovery: Found %s") % (location))
             properties['target_discovered'] = True
 
-        (iscsi_target, _sep, iscsi_name) = location.partition(" ")
-
-        iscsi_portal = iscsi_target.split(",")[0]
+        results = location.split(" ")
+        properties['target_portal'] = results[0].split(",")[0]
+        properties['target_iqn'] = results[1]
+        try:
+            properties['target_lun'] = int(results[2])
+        except (IndexError, ValueError):
+            if FLAGS.iscsi_helper == 'tgtadm':
+                properties['target_lun'] = 1
+            else:
+                properties['target_lun'] = 0
 
         properties['volume_id'] = volume['id']
-        properties['target_iqn'] = iscsi_name
-        properties['target_portal'] = iscsi_portal
 
         auth = volume['provider_auth']
-
         if auth:
             (auth_method, auth_username, auth_secret) = auth.split()
 
@@ -794,14 +804,10 @@ class ZadaraBEDriver(ISCSIDriver):
 
         self._iscsiadm_update(iscsi_properties, "node.startup", "automatic")
 
-        if FLAGS.iscsi_helper == 'tgtadm':
-            mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-1" %
-                            (iscsi_properties['target_portal'],
-                             iscsi_properties['target_iqn']))
-        else:
-            mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
-                            (iscsi_properties['target_portal'],
-                             iscsi_properties['target_iqn']))
+        mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-%s" %
+                        (iscsi_properties['target_portal'],
+                         iscsi_properties['target_iqn'],
+                         iscsi_properties['target_lun']))
 
         # The /dev/disk/by-path/... node is not always present immediately
         # TODO(justinsb): This retry-with-delay is a pattern, move to utils?
@@ -1007,5 +1013,5 @@ class ZadaraBEDriver(ISCSIDriver):
         return {'drive_qos_info': drive_info}
 
 
-def _iscsi_location(ip, target, iqn):
-    return "%s:%s,%s %s" % (ip, FLAGS.iscsi_port, target, iqn)
+def _iscsi_location(ip, target, iqn, lun=None):
+    return "%s:%s,%s %s %s" % (ip, FLAGS.iscsi_port, target, iqn, lun)