]> review.fuel-infra Code Review - openstack-build/cinder-build.git/commitdiff
Fix FC connection handling in the storwize driver
authorZoltan Arnold Nagy <nag@zurich.ibm.com>
Wed, 12 Feb 2014 22:01:10 +0000 (23:01 +0100)
committerLi Min Liu <liminliu@cn.ibm.com>
Mon, 24 Feb 2014 02:26:45 +0000 (10:26 +0800)
Creating FC connections was not working due to several issues:

1. The lsfabric command in the Storwize/SVC omitted the delimeter and so
the output was not correctly parsed by the driver.
2. In some cases comparisons of WWPNs were not case insensitive - ensure
we compare lower-case WWPNs.
3. If the host supplied extra information in the connector (e.g. iSCSI
initiator name in case of an FC setup, or FC WWNNs/WWPNs in an iSCSI
setup), the driver would try to make those host mappings on the storage
too.  This could lead into unwanted behaviour and confusing error
messages (for example, in an FC setup reaching the maximum number of
iSCSI mappings).

In addition, unit tests weren't properly testing FC - instead they were
always testing iSCSI due to an error in parsing extra_specs. This patch
fixes that error and the unit tests as well.

These changes must all be made together for unit tests to pass.

Closes-bug: 1279758
Change-Id: I64e21609ba089cf5bfd52ce2644f6f229bcc69dc

cinder/tests/test_storwize_svc.py
cinder/volume/drivers/ibm/storwize_svc/__init__.py
cinder/volume/drivers/ibm/storwize_svc/helpers.py
cinder/volume/drivers/ibm/storwize_svc/ssh.py

index af3a14ed603834bd7d6f431abcc963cbcf3ad7d4..55cf201158c2da48a04aad0e24b3e9aab4dcf53e 100644 (file)
@@ -491,12 +491,12 @@ port_speed!N/A
         return self._print_info_cmd(rows=rows, **kwargs)
 
     def _cmd_lsfabric(self, **kwargs):
-        host_name = kwargs['host'] if 'host' in kwargs else None
+        host_name = kwargs['host'].strip('\'\"') if 'host' in kwargs else None
         target_wwpn = kwargs['wwpn'] if 'wwpn' in kwargs else None
         host_infos = []
 
         for hv in self._hosts_list.itervalues():
-            if not host_name or hv['host_name'].startswith(host_name):
+            if (not host_name) or (hv['host_name'] == host_name):
                 for mv in self._mappings_list.itervalues():
                     if mv['host'] == hv['host_name']:
                         if not target_wwpn or target_wwpn in hv['wwpns']:
@@ -1988,6 +1988,18 @@ class StorwizeSVCDriverTestCase(test.TestCase):
             opts = {'storage_protocol': '<in> ' + protocol}
             types[protocol] = volume_types.create(ctxt, protocol, opts)
 
+        expected = {'FC': {'driver_volume_type': 'fibre_channel',
+                           'data': {'target_lun': '0',
+                                    'target_wwn': 'AABBCCDDEEFF0011',
+                                    'target_discovered': False}},
+                    'iSCSI': {'driver_volume_type': 'iscsi',
+                              'data': {'target_discovered': False,
+                                       'target_iqn':
+                                       'iqn.1982-01.com.ibm:1234.sim.node1',
+                                       'target_portal': '1.234.56.78:3260',
+                                       'target_lun': '0',
+                                       'auth_method': 'CHAP'}}}
+
         for protocol in ['FC', 'iSCSI']:
             volume1['volume_type_id'] = types[protocol]['id']
             volume2['volume_type_id'] = types[protocol]['id']
@@ -2003,10 +2015,18 @@ class StorwizeSVCDriverTestCase(test.TestCase):
             self._assert_vol_exists(volume2['name'], True)
 
             # Initialize connection from the first volume to a host
-            self.driver.initialize_connection(volume1, self._connector)
+            ret = self.driver.initialize_connection(volume1, self._connector)
+            self.assertEqual(ret['driver_volume_type'],
+                             expected[protocol]['driver_volume_type'])
+            for k, v in expected[protocol]['data'].iteritems():
+                self.assertEqual(ret['data'][k], v)
 
             # Initialize again, should notice it and do nothing
-            self.driver.initialize_connection(volume1, self._connector)
+            ret = self.driver.initialize_connection(volume1, self._connector)
+            self.assertEqual(ret['driver_volume_type'],
+                             expected[protocol]['driver_volume_type'])
+            for k, v in expected[protocol]['data'].iteritems():
+                self.assertEqual(ret['data'][k], v)
 
             # Try to delete the 1st volume (should fail because it is mapped)
             self.assertRaises(exception.VolumeBackendAPIException,
index a6df1eced330dd55e294162718b5d2a21c601d6c..ea150df42e6329098bb0d326d6a0967dc1fcdc44 100644 (file)
@@ -111,9 +111,12 @@ class StorwizeSVCDriver(san.SanDriver):
     1.2.0 - Added retype
     1.2.1 - Code refactor, improved exception handling
     1.2.2 - Fix bug #1274123 (races in host-related functions)
+    1.2.3 - Fix Fibre Channel connectivity: bug #1279758 (add delim to
+            lsfabric, clear unused data from connections, ensure matching
+            WWPNs by comparing lower case
     """
 
-    VERSION = "1.2.2"
+    VERSION = "1.2.3"
 
     def __init__(self, *args, **kwargs):
         super(StorwizeSVCDriver, self).__init__(*args, **kwargs)
@@ -285,6 +288,19 @@ class StorwizeSVCDriver(san.SanDriver):
         host_name = connector['host']
         volume_name = volume['name']
 
+        # Delete irrelevant connection information that later could result
+        # in unwanted behaviour. For example, if FC is used yet the hosts
+        # return iSCSI data, the driver will try to create the iSCSI connection
+        # which can result in a nice error about reaching the per-host maximum
+        # iSCSI initiator limit.
+        # First make a copy so we don't mess with a caller's connector.
+        connector = connector.copy()
+        if vol_opts['protocol'] == 'FC':
+            connector.pop('initiator', None)
+        elif vol_opts['protocol'] == 'iSCSI':
+            connector.pop('wwnns', None)
+            connector.pop('wwpns', None)
+
         # Check if a host object is defined for this host name
         host_name = self._helpers.get_host_from_connector(connector)
         if host_name is None:
index c6071a27cb1ed61db9fafa482ef77cb597a24ea8..b71cb4372e168a70e87244a242a7609053a70609 100644 (file)
@@ -170,7 +170,8 @@ class StorwizeHelpers(object):
         wwpns = []
         resp = self.ssh.lsfabric(host=host)
         for wwpn in resp.select('local_wwpn'):
-            wwpns.append(wwpn)
+            if wwpn is not None:
+                wwpns.append(wwpn)
         return wwpns
 
     def get_host_from_connector(self, connector):
@@ -184,13 +185,16 @@ class StorwizeHelpers(object):
                 resp = self.ssh.lsfabric(wwpn=wwpn)
                 for wwpn_info in resp:
                     try:
-                        if wwpn_info['remote_wwpn'] == wwpn:
+                        if (wwpn_info['remote_wwpn'] and
+                                wwpn_info['name'] and
+                                wwpn_info['remote_wwpn'].lower() ==
+                                wwpn.lower()):
                             host_name = wwpn_info['name']
                     except KeyError:
                         self.handle_keyerror('lsfabric', str(wwpn_info))
 
         # That didn't work, so try exhaustive search
-        if not host_name:
+        if host_name is None:
             hosts_info = self.ssh.lshost()
             for name in hosts_info.select('name'):
                 resp = self.ssh.lshost(host=name)
@@ -202,7 +206,7 @@ class StorwizeHelpers(object):
                           len(connector['wwpns']) and
                           wwpn and
                           wwpn.lower() in
-                          [str(x).lower for x in connector['wwpns']]):
+                          [str(x).lower() for x in connector['wwpns']]):
                         host_name = name
 
         LOG.debug(_('leave: get_host_from_connector: host %s') % host_name)
@@ -445,7 +449,8 @@ class StorwizeHelpers(object):
                 # protocol is a special case where the user asks for a given
                 # protocol and we want both the scheduler and the driver to act
                 # on the value.
-                if scope == 'capabilities' and key == 'storage_protocol':
+                if ((not scope or scope == 'capabilities') and
+                        key == 'storage_protocol'):
                     scope = None
                     key = 'protocol'
                     words = value.split()
index a0309b9c490bcbde1cc8bbb0f32e8c60e4964692..a6f6f7d3da39c0159b2b07b53eb3e33ae7f2f7e0 100644 (file)
@@ -135,10 +135,11 @@ class StorwizeSSH(object):
         return self.run_ssh_info(ssh_cmd, with_header=True)
 
     def lsfabric(self, wwpn=None, host=None):
+        ssh_cmd = ['svcinfo', 'lsfabric', '-delim', '!']
         if wwpn:
-            ssh_cmd = ['svcinfo', 'lsfabric', '-wwpn', wwpn, '-delim', '!']
+            ssh_cmd.extend(['-wwpn', wwpn])
         elif host:
-            ssh_cmd = ['svcinfo', 'lsfabric', '-host', '"%s"' % host]
+            ssh_cmd.extend(['-host', '"%s"' % host])
         else:
             msg = (_('Must pass wwpn or host to lsfabric.'))
             LOG.error(msg)