From 706878deaaa5acbb8a577942a5a774c58fe16332 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 2 Mar 2015 15:51:07 +0100 Subject: [PATCH] Make lio iSCSI changes persistent to avoid lost To avoid losing iSCSI configuration when target.service is started/restarted we make persistent any change Cinder makes to the configuration. This will allow us to coordinate access from multiple rtslib users (target daemon, targetcli, cinder...) This patch changes rtstool and adds an additional command "save" with optional parameter the filename where we want to save current configuration. If no parameter is provided it saves to default location defined by rtslib. Closes-Bug: #1427301 Change-Id: I74bd09ed16a2e0e584d3e04762aec8cbdf101f6d --- cinder/cmd/rtstool.py | 20 +++++++++ cinder/tests/targets/test_lio_driver.py | 55 +++++++++++++++++-------- cinder/tests/test_cmd.py | 25 +++++++++++ cinder/volume/targets/lio.py | 23 +++++++++++ 4 files changed, 106 insertions(+), 17 deletions(-) diff --git a/cinder/cmd/rtstool.py b/cinder/cmd/rtstool.py index 4ec404a9ec6..58794ebf243 100644 --- a/cinder/cmd/rtstool.py +++ b/cinder/cmd/rtstool.py @@ -172,9 +172,21 @@ def usage(): print(sys.argv[0] + " get-targets") print(sys.argv[0] + " delete [iqn]") print(sys.argv[0] + " verify") + print(sys.argv[0] + " save [path_to_file]") sys.exit(1) +def save_to_file(destination_file): + rtsroot = rtslib.root.RTSRoot() + try: + rtsroot.save_to_file(destination_file) + except OSError: + if not destination_file: + destination_file = 'default location' + raise RtstoolError(_('Could not save configuration to %(file_path)s'), + {'file_path': destination_file}) + + def main(argv=None): if argv is None: argv = sys.argv @@ -238,6 +250,14 @@ def main(argv=None): verify_rtslib() return 0 + elif argv[1] == 'save': + if len(argv) > 3: + usage() + + destination_file = argv[2] if len(argv) > 2 else None + save_to_file(destination_file) + return 0 + else: usage() diff --git a/cinder/tests/targets/test_lio_driver.py b/cinder/tests/targets/test_lio_driver.py index ee9a992b58b..478d276f70b 100644 --- a/cinder/tests/targets/test_lio_driver.py +++ b/cinder/tests/targets/test_lio_driver.py @@ -102,13 +102,15 @@ class TestLioAdmDriver(test.TestCase): self.assertEqual(('foo', 'bar'), self.target._get_target_chap_auth(ctxt, test_vol)) + @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch.object(utils, 'execute') @mock.patch.object(lio.LioAdm, '_get_target') - def test_create_iscsi_target(self, mget_target, mexecute): + def test_create_iscsi_target(self, mget_target, mexecute, mpersist_cfg): mget_target.return_value = 1 - test_vol = 'iqn.2010-10.org.openstack:'\ - 'volume-83c2e877-feed-46be-8435-77884fe55b45' + # create_iscsi_target sends volume_name instead of volume_id on error + volume_name = 'volume-83c2e877-feed-46be-8435-77884fe55b45' + test_vol = 'iqn.2010-10.org.openstack:' + volume_name self.assertEqual( 1, self.target.create_iscsi_target( @@ -116,10 +118,13 @@ class TestLioAdmDriver(test.TestCase): 1, 0, self.fake_volumes_dir)) + mpersist_cfg.assert_called_once_with(volume_name) + @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch.object(utils, 'execute') @mock.patch.object(lio.LioAdm, '_get_target') - def test_create_iscsi_target_already_exists(self, mget_target, mexecute): + def test_create_iscsi_target_already_exists(self, mget_target, mexecute, + mpersist_cfg): mexecute.side_effect = putils.ProcessExecutionError test_vol = 'iqn.2010-10.org.openstack:'\ @@ -132,12 +137,14 @@ class TestLioAdmDriver(test.TestCase): 0, self.fake_volumes_dir, chap_auth) + self.assertEqual(0, mpersist_cfg.call_count) + @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch.object(utils, 'execute') - def test_remove_iscsi_target(self, mexecute): + def test_remove_iscsi_target(self, mexecute, mpersist_cfg): - test_vol = 'iqn.2010-10.org.openstack:'\ - 'volume-83c2e877-feed-46be-8435-77884fe55b45' + volume_id = '83c2e877-feed-46be-8435-77884fe55b45' + test_vol = 'iqn.2010-10.org.openstack:volume-' + volume_id # Test the normal case self.target.remove_iscsi_target(0, @@ -149,6 +156,8 @@ class TestLioAdmDriver(test.TestCase): test_vol, run_as_root=True) + mpersist_cfg.assert_called_once_with(volume_id) + # Test the failure case: putils.ProcessExecutionError mexecute.side_effect = putils.ProcessExecutionError self.assertRaises(exception.ISCSITargetRemoveFailed, @@ -158,6 +167,9 @@ class TestLioAdmDriver(test.TestCase): self.testvol['id'], self.testvol['name']) + # Ensure there have been no more calls to persist configuration + self.assertEqual(1, mpersist_cfg.call_count) + @mock.patch.object(lio.LioAdm, '_get_target_chap_auth') @mock.patch.object(lio.LioAdm, 'create_iscsi_target') def test_ensure_export(self, _mock_create, mock_get_chap): @@ -175,10 +187,13 @@ class TestLioAdmDriver(test.TestCase): check_exit_code=False, old_name=None) + @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch.object(utils, 'execute') @mock.patch.object(lio.LioAdm, '_get_iscsi_properties') - def test_initialize_connection(self, mock_get_iscsi, mock_execute): - + def test_initialize_connection(self, mock_get_iscsi, mock_execute, + mpersist_cfg): + volume_id = '83c2e877-feed-46be-8435-77884fe55b45' + target_id = 'iqn.2010-10.org.openstack:volume-' + volume_id connector = {'initiator': 'fake_init'} # Test the normal case @@ -190,13 +205,13 @@ class TestLioAdmDriver(test.TestCase): connector)) mock_execute.assert_called_once_with( - 'cinder-rtstool', 'add-initiator', - 'iqn.2010-10.org.openstack:' - 'volume-83c2e877-feed-46be-8435-77884fe55b45', + 'cinder-rtstool', 'add-initiator', target_id, 'c76370d66b', '2FE0CQ8J196R', connector['initiator'], run_as_root=True) + mpersist_cfg.assert_called_once_with(volume_id) + # Test the failure case: putils.ProcessExecutionError mock_execute.side_effect = putils.ProcessExecutionError self.assertRaises(exception.ISCSITargetAttachFailed, @@ -204,21 +219,26 @@ class TestLioAdmDriver(test.TestCase): self.testvol, connector) + @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch.object(utils, 'execute') - def test_terminate_connection(self, _mock_execute): + def test_terminate_connection(self, _mock_execute, mpersist_cfg): + + volume_id = '83c2e877-feed-46be-8435-77884fe55b45' + target_id = 'iqn.2010-10.org.openstack:volume-' + volume_id connector = {'initiator': 'fake_init'} self.target.terminate_connection(self.testvol, connector) _mock_execute.assert_called_once_with( - 'cinder-rtstool', 'delete-initiator', - 'iqn.2010-10.org.openstack:' - 'volume-83c2e877-feed-46be-8435-77884fe55b45', + 'cinder-rtstool', 'delete-initiator', target_id, connector['initiator'], run_as_root=True) + mpersist_cfg.assert_called_once_with(volume_id) + + @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch.object(utils, 'execute') - def test_terminate_connection_fail(self, _mock_execute): + def test_terminate_connection_fail(self, _mock_execute, mpersist_cfg): _mock_execute.side_effect = putils.ProcessExecutionError connector = {'initiator': 'fake_init'} @@ -226,6 +246,7 @@ class TestLioAdmDriver(test.TestCase): self.target.terminate_connection, self.testvol, connector) + self.assertEqual(0, mpersist_cfg.call_count) def test_iscsi_protocol(self): self.assertEqual(self.target.iscsi_protocol, 'iscsi') diff --git a/cinder/tests/test_cmd.py b/cinder/tests/test_cmd.py index 5c18be97328..c3306ba8d38 100644 --- a/cinder/tests/test_cmd.py +++ b/cinder/tests/test_cmd.py @@ -908,6 +908,14 @@ class TestCinderRtstoolCmd(test.TestCase): target.delete.assert_called_once_with() storage_object.delete.assert_called_once_with() + @mock.patch.object(cinder_rtstool, 'rtslib', autospec=True) + def test_save(self, mock_rtslib): + filename = mock.sentinel.filename + cinder_rtstool.save_to_file(filename) + rtsroot = mock_rtslib.root.RTSRoot + rtsroot.assert_called_once_with() + rtsroot.return_value.save_to_file.assert_called_once_with(filename) + def test_usage(self): exit = self.assertRaises(SystemExit, cinder_rtstool.usage) @@ -954,6 +962,23 @@ class TestCinderRtstoolCmd(test.TestCase): self.assertTrue(usage.called) self.assertEqual(exit.code, 1) + @mock.patch('cinder.cmd.rtstool.save_to_file') + def test_main_save(self, mock_save): + sys.argv = ['cinder-rtstool', + 'save'] + rc = cinder_rtstool.main() + mock_save.assert_called_once_with(None) + self.assertEqual(0, rc) + + @mock.patch('cinder.cmd.rtstool.save_to_file') + def test_main_save_with_file(self, mock_save): + sys.argv = ['cinder-rtstool', + 'save', + mock.sentinel.filename] + rc = cinder_rtstool.main() + mock_save.assert_called_once_with(mock.sentinel.filename) + self.assertEqual(0, rc) + def test_main_create(self): with mock.patch('cinder.cmd.rtstool.create') as create: sys.argv = ['cinder-rtstool', diff --git a/cinder/volume/targets/lio.py b/cinder/volume/targets/lio.py index a8f7276a2ad..21f9b635e98 100644 --- a/cinder/volume/targets/lio.py +++ b/cinder/volume/targets/lio.py @@ -77,6 +77,17 @@ class LioAdm(iscsi.ISCSITarget): iscsi_target = 0 # NOTE: Not used by lio. return iscsi_target, lun + def _persist_configuration(self, vol_id): + try: + utils.execute('cinder-rtstool', 'save', run_as_root=True) + + # On persistence failure we don't raise an exception, as target has + # been successfully created. + except putils.ProcessExecutionError: + LOG.warning(_LW("Failed to save iscsi LIO configuration when " + "modifying volume id: %(vol_id)s."), + {'vol_id': vol_id}) + def create_iscsi_target(self, name, tid, lun, path, chap_auth=None, **kwargs): # tid and lun are not used @@ -113,6 +124,9 @@ class LioAdm(iscsi.ISCSITarget): "id:%s.") % vol_id) raise exception.NotFound() + # We make changes persistent + self._persist_configuration(vol_id) + return tid def remove_iscsi_target(self, tid, lun, vol_id, vol_name, **kwargs): @@ -131,6 +145,9 @@ class LioAdm(iscsi.ISCSITarget): LOG.error(_LE("%s") % e) raise exception.ISCSITargetRemoveFailed(volume_id=vol_id) + # We make changes persistent + self._persist_configuration(vol_id) + def initialize_connection(self, volume, connector): volume_iqn = volume['provider_location'].split(' ')[1] @@ -151,6 +168,9 @@ class LioAdm(iscsi.ISCSITarget): raise exception.ISCSITargetAttachFailed( volume_id=volume['id']) + # We make changes persistent + self._persist_configuration(volume['id']) + iscsi_properties = self._get_iscsi_properties(volume, connector.get( 'multipath')) @@ -173,3 +193,6 @@ class LioAdm(iscsi.ISCSITarget): LOG.error(_LE("Failed to delete initiator iqn %s to target.") % connector['initiator']) raise exception.ISCSITargetDetachFailed(volume_id=volume['id']) + + # We make changes persistent + self._persist_configuration(volume['id'])