Remove iscsi_helper calls from base iscsi driver

There are a number of drivers that inherit from the base
iscsi driver, using the base implementation of initialize_connection
and terminate_connection.  These drivers also don't have iscsi_helper
defined.

We've kept all iscsi_helper specific code in the tgt helpers
themselves, or in the various versions of the LVM driver
which is where they belong.

Over time however there have been some iscsi_helper specific
calls that have crept in to the base volume.driver:ISCSIDriver
class.  These were LIO specific calls and as a result when a
configuration is set with multi-backend to use lioadm as the
target helper, or even if it's not used but the default iscsi_helper
is set to lioadm the initialize_connection and terminate_connection
calls will try and access the LIO specific calls and raise because
the drivers inheriting from this class don't have target_helper
members.

This went unnoticed mostly because there's no significant LIO testing
in place and there were no distros setting LIO as the default iscsi_helper,
it appears that this has changed however.

This patch moves the LIO specific calls back where they belong
and keeps the base ISCSIDriver generic.  I've also added a test
case for this in the SolidFire driver because it's impacted by
this.

Change-Id: Ibe010b62bb2518685753dd515326e56ffcc99cea
Closes-Bug: #1400804

diff --git a/cinder/tests/test_solidfire.py b/cinder/tests/test_solidfire.py
index 1a37be387e9fa111d7d4c86f36e11ead4c627f02..a3da1435fa01ad5d28bfd9e379cc5606cadbf5e2 100644 (file)
--- a/cinder/tests/test_solidfire.py
+++ b/cinder/tests/test_solidfire.py
@@ -14,6 +14,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import mock
 import mox
 from oslo.utils import timeutils
 from oslo.utils import units
@@ -280,6 +281,66 @@ class SolidFireVolumeTestCase(test.TestCase):
         sfv.create_cloned_volume(testvol_b, testvol)
 
     def test_initialize_connector_with_blocksizes(self):
+        expected_iqn = 'iqn.2010-01.com.solidfire:'\
+                       '87hg.uuid-2cc06226-cc74-4cb7-bd55-14aed659a0cc.4060'
+        expected_properties = \
+            {'driver_volume_type': 'iscsi',
+             'data': {'target_discovered': False,
+                      'encrypted': False,
+                      'logical_block_size': '4096',
+                      'physical_block_size': '4096',
+                      'target_iqn': expected_iqn,
+                      'target_portal': '10.10.7.1:3260',
+                      'volume_id': 'a720b3c0-d1f0-11e1-9b23-0800200c9a66',
+                      'target_lun': 0,
+                      'auth_password': '2FE0CQ8J196R',
+                      'auth_username':
+                          'stack-1-a60e2611875f40199931f2c76370d66b',
+                      'auth_method': 'CHAP'}}
+
+        connector = {'initiator': 'iqn.2012-07.org.fake:01'}
+        testvol = {'project_id': 'testprjid',
+                   'name': 'testvol',
+                   'size': 1,
+                   'id': 'a720b3c0-d1f0-11e1-9b23-0800200c9a66',
+                   'volume_type_id': None,
+                   'provider_location': '10.10.7.1:3260 iqn.2010-01.com.'
+                                        'solidfire:87hg.uuid-2cc06226-cc'
+                                        '74-4cb7-bd55-14aed659a0cc.4060 0',
+                   'provider_auth': 'CHAP stack-1-a60e2611875f40199931f2'
+                                    'c76370d66b 2FE0CQ8J196R',
+                   'provider_geometry': '4096 4096',
+                   'created_at': timeutils.utcnow(),
+                   }
+
+        sfv = SolidFireDriver(configuration=self.configuration)
+        self.assertEqual(sfv.initialize_connection(testvol, connector),
+                         expected_properties)
+
+    @mock.patch('cinder.volume.driver.CONF')
+    def test_iscsi_helpers_not_in_base_iscsi_driver(self, mock_conf):
+        # This test is added to check for bug: 1400804
+        # The base iscsi driver should be clean from specifics
+        # regarding tgtadm or LVM driver, this check is here
+        # to make sure nothing regarding specific iscsi_helpers
+        # sneak back in
+        expected_iqn = 'iqn.2010-01.com.solidfire:'\
+                       '87hg.uuid-2cc06226-cc74-4cb7-bd55-14aed659a0cc.4060'
+        expected_properties = \
+            {'driver_volume_type': 'iscsi',
+             'data': {'target_discovered': False,
+                      'encrypted': False,
+                      'logical_block_size': '4096',
+                      'physical_block_size': '4096',
+                      'target_iqn': expected_iqn,
+                      'target_portal': '10.10.7.1:3260',
+                      'volume_id': 'a720b3c0-d1f0-11e1-9b23-0800200c9a66',
+                      'target_lun': 0,
+                      'auth_password': '2FE0CQ8J196R',
+                      'auth_username':
+                          'stack-1-a60e2611875f40199931f2c76370d66b',
+                      'auth_method': 'CHAP'}}
+
         connector = {'initiator': 'iqn.2012-07.org.fake:01'}
         testvol = {'project_id': 'testprjid',
                    'name': 'testvol',
@@ -295,10 +356,20 @@ class SolidFireVolumeTestCase(test.TestCase):
                    'created_at': timeutils.utcnow(),
                    }
 
+        mock_conf.iscsi_helper = 'lioadm'
+        sfv = SolidFireDriver(configuration=self.configuration)
+        self.assertEqual(sfv.initialize_connection(testvol, connector),
+                         expected_properties)
+
+        mock_conf.iscsi_helper = 'iseradm'
+        sfv = SolidFireDriver(configuration=self.configuration)
+        self.assertEqual(sfv.initialize_connection(testvol, connector),
+                         expected_properties)
+
+        mock_conf.iscsi_helper = 'tgtadm'
         sfv = SolidFireDriver(configuration=self.configuration)
-        properties = sfv.initialize_connection(testvol, connector)
-        self.assertEqual(properties['data']['physical_block_size'], '4096')
-        self.assertEqual(properties['data']['logical_block_size'], '4096')
+        self.assertEqual(sfv.initialize_connection(testvol, connector),
+                         expected_properties)
 
     def test_create_volume_with_qos(self):
         preset_qos = {}

diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py
index 48611512f42c07ae2672b93b85b02bc992b92705..4c07865ad89e3bcd5370543408ea9a4bdc26ee36 100755 (executable)
--- a/cinder/volume/driver.py
+++ b/cinder/volume/driver.py
@@ -1041,10 +1041,6 @@ class ISCSIDriver(VolumeDriver):
             }
 
         """
-
-        if CONF.iscsi_helper == 'lioadm':
-            self.target_helper.initialize_connection(volume, connector)
-
         iscsi_properties = self._get_iscsi_properties(volume)
         return {
             'driver_volume_type': 'iscsi',
@@ -1060,8 +1056,7 @@ class ISCSIDriver(VolumeDriver):
             raise exception.VolumeBackendAPIException(data=err_msg)
 
     def terminate_connection(self, volume, connector, **kwargs):
-        if CONF.iscsi_helper == 'lioadm':
-            self.target_helper.terminate_connection(volume, connector)
+        pass
 
     def get_volume_stats(self, refresh=False):
         """Get volume stats.
@@ -1111,6 +1106,8 @@ class ISCSIDriver(VolumeDriver):
 
     def get_target_helper(self, db):
         root_helper = utils.get_root_helper()
+        # FIXME(jdg): These work because the driver will overide
+        # but we need to move these to use self.configuraiton
         if CONF.iscsi_helper == 'iseradm':
             return iscsi.ISERTgtAdm(root_helper, CONF.volumes_dir,
                                     CONF.iscsi_target_prefix, db=db)
@@ -1241,7 +1238,6 @@ class ISERDriver(ISCSIDriver):
             }
 
         """
-
         iser_properties = self._get_iscsi_properties(volume)
         return {
             'driver_volume_type': 'iser',

diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py
index 95c885902c1b29dcdb908b0578f9ac065e0ed386..1ae4fc037e75bc862c85aee67007345a61da7e80 100644 (file)
--- a/cinder/volume/drivers/lvm.py
+++ b/cinder/volume/drivers/lvm.py
@@ -548,6 +548,23 @@ class LVMISCSIDriver(LVMVolumeDriver, driver.ISCSIDriver):
     def create_export(self, context, volume):
         return self._create_export(context, volume)
 
+    def initialize_connection(self, volume, connector):
+        """Initializes the connection and returns connection info. """
+
+        # We have a special case for lioadm here, that's fine, we can
+        # keep the call in the parent class (driver:ISCSIDriver) generic
+        # and still use it throughout, just override and call super here
+        # no duplication, same effect but doesn't break things
+        # see bug: #1400804
+        if self.configuration.iscsi_helper == 'lioadm':
+            self.target_helper.initialize_connection(volume, connector)
+        return super(LVMISCSIDriver, self).initialize_connection(volume,
+                                                                 connector)
+
+    def terminate_connection(self, volume, connector, **kwargs):
+        if self.configuration.iscsi_helper == 'lioadm':
+            self.target_helper.terminate_connection(volume, connector)
+
     def _create_export(self, context, volume, vg=None):
         """Creates an export for a logical volume."""
         if vg is None: