From: Gorka Eguileor Date: Mon, 1 Jun 2015 16:30:01 +0000 (+0200) Subject: Fix cinder concurrency issues on rtstool X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=45c282afb442aec80cf24d226bef5ebb46963026;p=openstack-build%2Fcinder-build.git 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 be7c5dfda..79213cb31 100644 --- 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 5ba9c0dfe..05fdf97f5 100644 --- 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)