Fix cinder concurrency issues on rtstool

Since there's no synchronized access to configfs in rtslib it can happen
that rtstool or rtslib access an element that no longer exists because
it has been removed just in the middle of a loop by another Cinder
request.

This results in quite a different number of exceptions:
- .dump()
- KeyError
- IOError
- RTSLibError on storage_object

This patch synchronizes access to all rtstool calls that access or
modify configfs using utils.synchronized decorator.

Change-Id: I341a10da54ab01be68a0cae843f35e5c841c6d81
Closes-Bug: #1460692

diff --git a/cinder/tests/unit/targets/test_lio_driver.py b/cinder/tests/unit/targets/test_lio_driver.py
index be7c5dfdae50380ff3ca63cefee8600642210c41..79213cb31a8f602dc44468255d7b7a3fc9c71a4e 100644 (file)
--- a/cinder/tests/unit/targets/test_lio_driver.py
+++ b/cinder/tests/unit/targets/test_lio_driver.py
@@ -31,11 +31,16 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
         self.target.db = mock.MagicMock(
             volume_get=lambda x, y: {'provider_auth': 'IncomingUser foo bar'})
 
-    def test_get_target(self):
-        with mock.patch('cinder.utils.execute',
-                        return_value=(self.test_vol, None)):
-            self.assertEqual(self.test_vol,
-                             self.target._get_target(self.test_vol))
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
+    @mock.patch.object(lio.LioAdm, '_persist_configuration')
+    @mock.patch('cinder.utils.execute')
+    def test_get_target(self, mexecute, mpersist_cfg, mlock_exec):
+        mexecute.return_value = (self.test_vol, None)
+        self.assertEqual(self.test_vol, self.target._get_target(self.test_vol))
+        self.assertFalse(mpersist_cfg.called)
+        expected_args = ('cinder-rtstool', 'get-targets')
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
+        mexecute.assert_called_once_with(*expected_args, run_as_root=True)
 
     def test_get_iscsi_target(self):
         ctxt = context.get_admin_context()
@@ -58,10 +63,12 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
                          self.target._get_target_chap_auth(ctxt,
                                                            self.test_vol))
 
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
     @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch('cinder.utils.execute')
     @mock.patch.object(lio.LioAdm, '_get_target')
-    def test_create_iscsi_target(self, mget_target, mexecute, mpersist_cfg):
+    def test_create_iscsi_target(self, mget_target, mexecute, mpersist_cfg,
+                                 mlock_exec):
 
         mget_target.return_value = 1
         # create_iscsi_target sends volume_name instead of volume_id on error
@@ -83,9 +90,12 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
             self.target.iscsi_protocol == 'iser',
             run_as_root=True)
 
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
+    @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch.object(utils, 'execute')
     @mock.patch.object(lio.LioAdm, '_get_target', return_value=1)
-    def test_create_iscsi_target_port_ip(self, mget_target, mexecute):
+    def test_create_iscsi_target_port_ip(self, mget_target, mexecute,
+                                         mpersist_cfg, mlock_exec):
         test_vol = 'iqn.2010-10.org.openstack:'\
                    'volume-83c2e877-feed-46be-8435-77884fe55b45'
         ip = '10.0.0.15'
@@ -100,7 +110,7 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
                 path=self.fake_volumes_dir,
                 **{'portals_port': port, 'portals_ips': [ip]}))
 
-        mexecute.assert_any_call(
+        expected_args = (
             'cinder-rtstool',
             'create',
             self.fake_volumes_dir,
@@ -109,12 +119,18 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
             '',
             self.target.iscsi_protocol == 'iser',
             '-p%s' % port,
-            '-a' + ip,
-            run_as_root=True)
+            '-a' + ip)
 
+        mlock_exec.assert_any_call(*expected_args, run_as_root=True)
+        mexecute.assert_any_call(*expected_args, run_as_root=True)
+        mpersist_cfg.assert_called_once_with(self.volume_name)
+
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
+    @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch.object(utils, 'execute')
     @mock.patch.object(lio.LioAdm, '_get_target', return_value=1)
-    def test_create_iscsi_target_port_ips(self, mget_target, mexecute):
+    def test_create_iscsi_target_port_ips(self, mget_target, mexecute,
+                                          mpersist_cfg, mlock_exec):
         test_vol = 'iqn.2010-10.org.openstack:'\
                    'volume-83c2e877-feed-46be-8435-77884fe55b45'
         ips = ['10.0.0.15', '127.0.0.1']
@@ -129,7 +145,7 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
                 path=self.fake_volumes_dir,
                 **{'portals_port': port, 'portals_ips': ips}))
 
-        mexecute.assert_any_call(
+        expected_args = (
             'cinder-rtstool',
             'create',
             self.fake_volumes_dir,
@@ -138,15 +154,19 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
             '',
             self.target.iscsi_protocol == 'iser',
             '-p%s' % port,
-            '-a' + ','.join(ips),
-            run_as_root=True)
+            '-a' + ','.join(ips))
 
+        mlock_exec.assert_any_call(*expected_args, run_as_root=True)
+        mexecute.assert_any_call(*expected_args, run_as_root=True)
+        mpersist_cfg.assert_called_once_with(self.volume_name)
+
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
     @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch('cinder.utils.execute',
                 side_effect=putils.ProcessExecutionError)
     @mock.patch.object(lio.LioAdm, '_get_target')
     def test_create_iscsi_target_already_exists(self, mget_target, mexecute,
-                                                mpersist_cfg):
+                                                mpersist_cfg, mlock_exec):
         chap_auth = ('foo', 'bar')
         self.assertRaises(exception.ISCSITargetCreateFailed,
                           self.target.create_iscsi_target,
@@ -155,25 +175,31 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
                           0,
                           self.fake_volumes_dir,
                           chap_auth)
-        self.assertEqual(0, mpersist_cfg.call_count)
+        self.assertFalse(mpersist_cfg.called)
+        expected_args = ('cinder-rtstool', 'create', self.fake_volumes_dir,
+                         self.test_vol, chap_auth[0], chap_auth[1], False)
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
+        mexecute.assert_called_once_with(*expected_args, run_as_root=True)
 
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
     @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch('cinder.utils.execute')
-    def test_remove_iscsi_target(self, mexecute, mpersist_cfg):
+    def test_remove_iscsi_target(self, mexecute, mpersist_cfg, mlock_exec):
         # Test the normal case
         self.target.remove_iscsi_target(0,
                                         0,
                                         self.testvol['id'],
                                         self.testvol['name'])
-        mexecute.assert_called_once_with('cinder-rtstool',
-                                         'delete',
-                                         self.iscsi_target_prefix +
-                                         self.testvol['name'],
-                                         run_as_root=True)
+        expected_args = ('cinder-rtstool', 'delete',
+                         self.iscsi_target_prefix + self.testvol['name'])
 
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
+        mexecute.assert_called_once_with(*expected_args, run_as_root=True)
         mpersist_cfg.assert_called_once_with(self.fake_volume_id)
 
         # Test the failure case: putils.ProcessExecutionError
+        mlock_exec.reset_mock()
+        mpersist_cfg.reset_mock()
         mexecute.side_effect = putils.ProcessExecutionError
         self.assertRaises(exception.ISCSITargetRemoveFailed,
                           self.target.remove_iscsi_target,
@@ -181,9 +207,10 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
                           0,
                           self.testvol['id'],
                           self.testvol['name'])
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
 
-        # Ensure there have been no more calls to persist configuration
-        self.assertEqual(1, mpersist_cfg.call_count)
+        # Ensure there have been no calls to persist configuration
+        self.assertFalse(mpersist_cfg.called)
 
     @mock.patch.object(lio.LioAdm, '_get_target_chap_auth')
     @mock.patch.object(lio.LioAdm, 'create_iscsi_target')
@@ -203,11 +230,12 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
             portals_ips=[self.configuration.iscsi_ip_address],
             portals_port=self.configuration.iscsi_port)
 
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
     @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch('cinder.utils.execute')
     @mock.patch.object(lio.LioAdm, '_get_iscsi_properties')
     def test_initialize_connection(self, mock_get_iscsi, mock_execute,
-                                   mpersist_cfg):
+                                   mpersist_cfg, mlock_exec):
         target_id = self.iscsi_target_prefix + 'volume-' + self.fake_volume_id
         connector = {'initiator': 'fake_init'}
 
@@ -219,48 +247,64 @@ class TestLioAdmDriver(tf.TargetDriverFixture):
                          self.target.initialize_connection(self.testvol,
                                                            connector))
 
-        mock_execute.assert_called_once_with(
-            'cinder-rtstool', 'add-initiator', target_id,
-            self.expected_iscsi_properties['auth_username'],
-            '2FE0CQ8J196R', connector['initiator'],
-            run_as_root=True)
+        expected_args = ('cinder-rtstool', 'add-initiator', target_id,
+                         self.expected_iscsi_properties['auth_username'],
+                         '2FE0CQ8J196R', connector['initiator'])
 
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
+        mock_execute.assert_called_once_with(*expected_args, run_as_root=True)
         mpersist_cfg.assert_called_once_with(self.fake_volume_id)
 
         # Test the failure case: putils.ProcessExecutionError
+        mlock_exec.reset_mock()
+        mpersist_cfg.reset_mock()
         mock_execute.side_effect = putils.ProcessExecutionError
         self.assertRaises(exception.ISCSITargetAttachFailed,
                           self.target.initialize_connection,
                           self.testvol,
                           connector)
 
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
+
+        # Ensure there have been no calls to persist configuration
+        self.assertFalse(mpersist_cfg.called)
+
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
     @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch('cinder.utils.execute')
-    def test_terminate_connection(self, _mock_execute, mpersist_cfg):
+    def test_terminate_connection(self, mock_execute, mpersist_cfg,
+                                  mlock_exec):
 
         target_id = self.iscsi_target_prefix + 'volume-' + self.fake_volume_id
 
         connector = {'initiator': 'fake_init'}
         self.target.terminate_connection(self.testvol,
                                          connector)
-        _mock_execute.assert_called_once_with(
-            'cinder-rtstool', 'delete-initiator', target_id,
-            connector['initiator'],
-            run_as_root=True)
+        expected_args = ('cinder-rtstool', 'delete-initiator', target_id,
+                         connector['initiator'])
 
+        mlock_exec.assert_called_once_with(*expected_args, run_as_root=True)
+        mock_execute.assert_called_once_with(*expected_args, run_as_root=True)
         mpersist_cfg.assert_called_once_with(self.fake_volume_id)
 
+    @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute)
     @mock.patch.object(lio.LioAdm, '_persist_configuration')
     @mock.patch('cinder.utils.execute')
-    def test_terminate_connection_fail(self, _mock_execute, mpersist_cfg):
+    def test_terminate_connection_fail(self, mock_execute, mpersist_cfg,
+                                       mlock_exec):
 
-        _mock_execute.side_effect = putils.ProcessExecutionError
+        target_id = self.iscsi_target_prefix + 'volume-' + self.fake_volume_id
+        mock_execute.side_effect = putils.ProcessExecutionError
         connector = {'initiator': 'fake_init'}
         self.assertRaises(exception.ISCSITargetDetachFailed,
                           self.target.terminate_connection,
                           self.testvol,
                           connector)
-        self.assertEqual(0, mpersist_cfg.call_count)
+        mlock_exec.assert_called_once_with('cinder-rtstool',
+                                           'delete-initiator', target_id,
+                                           connector['initiator'],
+                                           run_as_root=True)
+        self.assertFalse(mpersist_cfg.called)
 
     def test_iscsi_protocol(self):
         self.assertEqual(self.target.iscsi_protocol, 'iscsi')

diff --git a/cinder/volume/targets/lio.py b/cinder/volume/targets/lio.py
index 5ba9c0dfe4240c352b3c328e7c18001f2e020efd..05fdf97f5eab381b78936d9ccc4095568ec14be8 100644 (file)
--- a/cinder/volume/targets/lio.py
+++ b/cinder/volume/targets/lio.py
@@ -53,13 +53,24 @@ class LioAdm(iscsi.ISCSITarget):
 
     def _verify_rtstool(self):
         try:
+            # This call doesn't need locking
             utils.execute('cinder-rtstool', 'verify')
         except (OSError, putils.ProcessExecutionError):
             LOG.error(_LE('cinder-rtstool is not installed correctly'))
             raise
 
+    @staticmethod
+    @utils.synchronized('lioadm', external=True)
+    def _execute(*args, **kwargs):
+        """Locked execution to prevent racing issues.
+
+        Racing issues are derived from a bug in RTSLib:
+            https://github.com/agrover/rtslib-fb/issues/36
+        """
+        return utils.execute(*args, **kwargs)
+
     def _get_target(self, iqn):
-        (out, err) = utils.execute('cinder-rtstool',
+        (out, err) = self._execute('cinder-rtstool',
                                    'get-targets',
                                    run_as_root=True)
         lines = out.split('\n')
@@ -79,7 +90,7 @@ class LioAdm(iscsi.ISCSITarget):
 
     def _persist_configuration(self, vol_id):
         try:
-            utils.execute('cinder-rtstool', 'save', run_as_root=True)
+            self._execute('cinder-rtstool', 'save', run_as_root=True)
 
         # On persistence failure we don't raise an exception, as target has
         # been successfully created.
@@ -116,7 +127,7 @@ class LioAdm(iscsi.ISCSITarget):
                             chap_auth_userid,
                             chap_auth_password,
                             self.iscsi_protocol == 'iser'] + optional_args
-            utils.execute(*command_args, run_as_root=True)
+            self._execute(*command_args, run_as_root=True)
         except putils.ProcessExecutionError:
             LOG.exception(_LE("Failed to create iscsi target for volume "
                               "id:%s."), vol_id)
@@ -141,7 +152,7 @@ class LioAdm(iscsi.ISCSITarget):
         iqn = '%s%s' % (self.iscsi_target_prefix, vol_uuid_name)
 
         try:
-            utils.execute('cinder-rtstool',
+            self._execute('cinder-rtstool',
                           'delete',
                           iqn,
                           run_as_root=True)
@@ -161,7 +172,7 @@ class LioAdm(iscsi.ISCSITarget):
 
         # Add initiator iqns to target ACL
         try:
-            utils.execute('cinder-rtstool', 'add-initiator',
+            self._execute('cinder-rtstool', 'add-initiator',
                           volume_iqn,
                           auth_user,
                           auth_pass,
@@ -190,7 +201,7 @@ class LioAdm(iscsi.ISCSITarget):
 
         # Delete initiator iqns from target ACL
         try:
-            utils.execute('cinder-rtstool', 'delete-initiator',
+            self._execute('cinder-rtstool', 'delete-initiator',
                           volume_iqn,
                           connector['initiator'],
                           run_as_root=True)