From: Mitsuhiro Tanino Date: Sat, 17 Jan 2015 03:04:17 +0000 (-0500) Subject: Refactoring for export functions in Target object X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=fb8bfea97b54f3d0971fdc1b0473e8a60b1badb3;p=openstack-build%2Fcinder-build.git Refactoring for export functions in Target object Currently, export functions such as create_export() are implemented in individual Target code, but most of them are same and these are common features in each target. This patch moves these methods to parent ISCSITarget class to commonalize and each Target simply inherit these methods from parent class. As a result of this change, LioAdm can inherit ISCSITarget class directly instead of inheriting TgtAdm class. This simplifies dependency of targets and improves maintainability. By refactoring these methods, this patch also fixes following issues. (a) Fix bug #1410566 After transitioning to the new driver and target model, iscsi_targets is not added to the table during create_export() phase. However, remove_export() in LIO Target is still reffering empty iscsi_targets table. This causes NotFound exception and remove_export() skips to do remove_iscsi_target(). As a result, iscsi target is not removed and the target continues to grab the volume(logical volume) as an in-use status. This patch fix the problem. (b) Re-export a volume with CHAP Current Tgt Target recreate iscsi target without CHAP during ensure_export() even if the volume is exported with CHAP previously. This patch changes this bahaviour to recreate iscsi target using previous CHAP which is stored in volume file on state_path dir. Closes-Bug: 1410566 Change-Id: Iea3d94e35a4ced4dafc1b61e2df6b075cf200577 --- diff --git a/cinder/tests/targets/test_base_iscsi_driver.py b/cinder/tests/targets/test_base_iscsi_driver.py index 8b2f5a980..a6ee62598 100644 --- a/cinder/tests/targets/test_base_iscsi_driver.py +++ b/cinder/tests/targets/test_base_iscsi_driver.py @@ -10,33 +10,19 @@ # License for the specific language governing permissions and limitations # under the License. +import mock from oslo_config import cfg from oslo_utils import timeutils +from cinder import context from cinder import exception from cinder import test from cinder import utils from cinder.volume import configuration as conf +from cinder.volume.targets import fake from cinder.volume.targets import iscsi -class FakeDriver(iscsi.ISCSITarget): - def __init__(self, *args, **kwargs): - super(FakeDriver, self).__init__(*args, **kwargs) - - def create_export(self, context, vref): - pass - - def ensure_export(self, context, vref, vol_path): - pass - - def remove_export(self, context, vref): - pass - - def terminate_connection(self, vref, **kwargs): - pass - - class FakeIncompleteDriver(iscsi.ISCSITarget): def null_method(): pass @@ -47,19 +33,19 @@ class TestBaseISCSITargetDriver(test.TestCase): def setUp(self): super(TestBaseISCSITargetDriver, self).setUp() self.configuration = conf.Configuration(None) - self.fake_id_1 = 'ed2c1fd4-5fc0-11e4-aa15-123b93f75cba' - self.fake_id_2 = 'ed2c2222-5fc0-11e4-aa15-123b93f75cba' - self.target = FakeDriver(root_helper=utils.get_root_helper(), - configuration=self.configuration) - self.testvol_1 =\ - {'project_id': self.fake_id_1, + self.fake_project_id = 'ed2c1fd4-5fc0-11e4-aa15-123b93f75cba' + self.fake_volume_id = 'ed2c2222-5fc0-11e4-aa15-123b93f75cba' + self.target = fake.FakeTarget(root_helper=utils.get_root_helper(), + configuration=self.configuration) + self.testvol =\ + {'project_id': self.fake_project_id, 'name': 'testvol', 'size': 1, - 'id': self.fake_id_2, + 'id': self.fake_volume_id, 'volume_type_id': None, 'provider_location': '10.10.7.1:3260 ' 'iqn.2010-10.org.openstack:' - 'volume-%s 0' % self.fake_id_2, + 'volume-%s 0' % self.fake_volume_id, 'provider_auth': 'CHAP stack-1-a60e2611875f40199931f2' 'c76370d66b 2FE0CQ8J196R', 'provider_geometry': '512 512', @@ -75,10 +61,10 @@ class TestBaseISCSITargetDriver(test.TestCase): 'physical_block_size': '512', 'target_discovered': False, 'target_iqn': 'iqn.2010-10.org.openstack:volume-%s' % - self.fake_id_2, + self.fake_volume_id, 'target_lun': 0, 'target_portal': '10.10.7.1:3260', - 'volume_id': self.fake_id_2} + 'volume_id': self.fake_volume_id} def test_abc_methods_not_present_fails(self): configuration = conf.Configuration(cfg.StrOpt('iscsi_target_prefix', @@ -90,7 +76,7 @@ class TestBaseISCSITargetDriver(test.TestCase): def test_get_iscsi_properties(self): self.assertEqual(self.expected_iscsi_properties, - self.target._get_iscsi_properties(self.testvol_1)) + self.target._get_iscsi_properties(self.testvol)) def test_build_iscsi_auth_string(self): auth_string = 'chap chap-user chap-password' @@ -102,7 +88,7 @@ class TestBaseISCSITargetDriver(test.TestCase): def test_do_iscsi_discovery(self): target_string = '127.0.0.1:3260,1 '\ 'iqn.2010-10.org.openstack:'\ - 'volume-%s' % self.testvol_1['id'] + 'volume-%s' % self.testvol['id'] def _fake_execute(*args, **kwargs): return target_string, None @@ -119,13 +105,57 @@ class TestBaseISCSITargetDriver(test.TestCase): _fake_execute) self.assertEqual(target_string, - self.target._do_iscsi_discovery(self.testvol_1)) + self.target._do_iscsi_discovery(self.testvol)) + + def test_remove_export(self): + + with mock.patch.object(self.target, '_get_target_and_lun') as \ + mock_get_target,\ + mock.patch.object(self.target, 'show_target'),\ + mock.patch.object(self.target, 'remove_iscsi_target') as \ + mock_remove_target: + + mock_get_target.return_value = (0, 1) + iscsi_target, lun = mock_get_target.return_value + ctxt = context.get_admin_context() + self.target.remove_export(ctxt, self.testvol) + mock_remove_target.assert_called_once_with( + iscsi_target, + lun, + 'ed2c2222-5fc0-11e4-aa15-123b93f75cba', + 'testvol') + + def test_remove_export_notfound(self): + + with mock.patch.object(self.target, '_get_target_and_lun') as \ + mock_get_target,\ + mock.patch.object(self.target, 'show_target'),\ + mock.patch.object(self.target, 'remove_iscsi_target'): + + mock_get_target.side_effect = exception.NotFound + ctxt = context.get_admin_context() + self.assertEqual(None, self.target.remove_export(ctxt, + self.testvol)) + + def test_remove_export_show_error(self): + + with mock.patch.object(self.target, '_get_target_and_lun') as \ + mock_get_target,\ + mock.patch.object(self.target, 'show_target') as mshow,\ + mock.patch.object(self.target, 'remove_iscsi_target'): + + mock_get_target.return_value = (0, 1) + iscsi_target, lun = mock_get_target.return_value + mshow.side_effect = Exception + ctxt = context.get_admin_context() + self.assertEqual(None, self.target.remove_export(ctxt, + self.testvol)) def test_initialize_connection(self): expected = {'driver_volume_type': 'iscsi', 'data': self.expected_iscsi_properties} self.assertEqual(expected, - self.target.initialize_connection(self.testvol_1, {})) + self.target.initialize_connection(self.testvol, {})) def test_validate_connector(self): bad_connector = {'no_initiator': 'nada'} @@ -136,3 +166,22 @@ class TestBaseISCSITargetDriver(test.TestCase): connector = {'initiator': 'fake_init'} self.assertTrue(self.target.validate_connector, connector) + + def test_show_target_error(self): + self.assertRaises(exception.InvalidParameterValue, + self.target.show_target, + 0, None) + + with mock.patch.object(self.target, '_get_target') as mock_get_target: + mock_get_target.side_effect = exception.NotFound() + self.assertRaises(exception.NotFound, + self.target.show_target, 0, + self.expected_iscsi_properties['target_iqn']) + + def test_iscsi_location(self): + location = self.target._iscsi_location('portal', 1, 'target', 2) + self.assertEqual('portal:3260,1 target 2', location) + + location = self.target._iscsi_location('portal', 1, 'target', 2, + ['portal2']) + self.assertEqual('portal:3260;portal2:3260,1 target 2', location) diff --git a/cinder/tests/targets/test_iser_driver.py b/cinder/tests/targets/test_iser_driver.py index c806789f2..bfdb5a92b 100644 --- a/cinder/tests/targets/test_iser_driver.py +++ b/cinder/tests/targets/test_iser_driver.py @@ -41,7 +41,7 @@ class TestIserAdmDriver(test_tgt.TestTgtAdmDriver): expected_return = {'driver_volume_type': 'iser', 'data': {}} self.assertEqual(expected_return, - self.target.initialize_connection(self.testvol_1, + self.target.initialize_connection(self.testvol, connector)) def test_iscsi_protocol(self): @@ -70,7 +70,7 @@ class TestIserTgtDriver(test_tgt.TestTgtAdmDriver): expected_return = {'driver_volume_type': 'iser', 'data': {}} self.assertEqual(expected_return, - self.target.initialize_connection(self.testvol_1, + self.target.initialize_connection(self.testvol, connector)) @@ -96,6 +96,6 @@ class TestIserLioAdmDriver(test_lio.TestLioAdmDriver): connector = {'initiator': 'fake_init'} mock_get_iscsi.return_value = {} - ret = self.target.initialize_connection(self.testvol_1, connector) + ret = self.target.initialize_connection(self.testvol, connector) driver_volume_type = ret['driver_volume_type'] self.assertEqual(driver_volume_type, 'iser') diff --git a/cinder/tests/targets/test_lio_driver.py b/cinder/tests/targets/test_lio_driver.py index f2cb81596..ee9a992b5 100644 --- a/cinder/tests/targets/test_lio_driver.py +++ b/cinder/tests/targets/test_lio_driver.py @@ -12,18 +12,28 @@ import mock from oslo_concurrency import processutils as putils +from oslo_utils import timeutils from cinder import context from cinder import exception -from cinder.tests.targets import test_tgt_driver as test_tgt +from cinder import test from cinder import utils +from cinder.volume import configuration as conf from cinder.volume.targets import lio -class TestLioAdmDriver(test_tgt.TestTgtAdmDriver): +class TestLioAdmDriver(test.TestCase): def setUp(self): super(TestLioAdmDriver, self).setUp() + self.configuration = conf.Configuration(None) + self.configuration.append_config_values = mock.Mock(return_value=0) + self.configuration.safe_get = mock.Mock(side_effect=self.fake_safe_get) + self.configuration.iscsi_ip_address = '10.9.8.7' + self.fake_volumes_dir = '/tmp/tmpfile' + self.iscsi_target_prefix = 'iqn.2010-10.org.openstack:' + self.fake_project_id = 'ed2c1fd4-5fc0-11e4-aa15-123b93f75cba' + self.fake_volume_id = '83c2e877-feed-46be-8435-77884fe55b45' with mock.patch.object(lio.LioAdm, '_verify_rtstool'): self.target = lio.LioAdm(root_helper=utils.get_root_helper(), configuration=self.configuration) @@ -32,6 +42,28 @@ class TestLioAdmDriver(test_tgt.TestTgtAdmDriver): self.target.db = mock.MagicMock( volume_get=lambda x, y: {'provider_auth': 'IncomingUser foo bar'}) + self.testvol =\ + {'project_id': self.fake_project_id, + 'name': 'volume-%s' % self.fake_volume_id, + 'size': 1, + 'id': self.fake_volume_id, + 'volume_type_id': None, + 'provider_location': '10.9.8.7:3260 ' + 'iqn.2010-10.org.openstack:' + 'volume-%s 0' % self.fake_volume_id, + 'provider_auth': 'CHAP c76370d66b 2FE0CQ8J196R', + 'provider_geometry': '512 512', + 'created_at': timeutils.utcnow(), + 'host': 'fake_host@lvm#lvm'} + + def fake_safe_get(self, value): + if value == 'volumes_dir': + return self.fake_volumes_dir + elif value == 'iscsi_protocol': + return self.configuration.iscsi_protocol + elif value == 'iscsi_target_prefix': + return self.iscsi_target_prefix + def test_get_target(self): def _fake_execute(*args, **kwargs): @@ -47,31 +79,52 @@ class TestLioAdmDriver(test_tgt.TestTgtAdmDriver): 'volume-83c2e877-feed-46be-' '8435-77884fe55b45')) - def test_verify_backing_lun(self): - pass + def test_get_iscsi_target(self): + ctxt = context.get_admin_context() + expected = 0 + self.assertEqual(expected, + self.target._get_iscsi_target(ctxt, + self.testvol['id'])) + + def test_get_target_and_lun(self): + lun = 0 + iscsi_target = 0 + ctxt = context.get_admin_context() + expected = (iscsi_target, lun) + self.assertEqual(expected, + self.target._get_target_and_lun(ctxt, self.testvol)) def test_get_target_chap_auth(self): - pass + ctxt = context.get_admin_context() + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' - def test_create_iscsi_target_already_exists(self): - def _fake_execute(*args, **kwargs): - raise putils.ProcessExecutionError(exit_code=1) + self.assertEqual(('foo', 'bar'), + self.target._get_target_chap_auth(ctxt, test_vol)) - self.stubs.Set(utils, - 'execute', - _fake_execute) + @mock.patch.object(utils, 'execute') + @mock.patch.object(lio.LioAdm, '_get_target') + def test_create_iscsi_target(self, mget_target, mexecute): - self.stubs.Set(self.target, - '_get_target', - lambda x: 1) + mget_target.return_value = 1 + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + self.assertEqual( + 1, + self.target.create_iscsi_target( + test_vol, + 1, + 0, + self.fake_volumes_dir)) - self.stubs.Set(self.target, - '_verify_backing_lun', - lambda x, y: True) + @mock.patch.object(utils, 'execute') + @mock.patch.object(lio.LioAdm, '_get_target') + def test_create_iscsi_target_already_exists(self, mget_target, mexecute): + mexecute.side_effect = putils.ProcessExecutionError test_vol = 'iqn.2010-10.org.openstack:'\ 'volume-83c2e877-feed-46be-8435-77884fe55b45' - chap_auth = 'chap foo bar' + chap_auth = ('foo', 'bar') self.assertRaises(exception.ISCSITargetCreateFailed, self.target.create_iscsi_target, test_vol, @@ -80,48 +133,98 @@ class TestLioAdmDriver(test_tgt.TestTgtAdmDriver): self.fake_volumes_dir, chap_auth) - def test_delete_target_not_found(self): - # NOTE(jdg): This test inherits from the - # tgt driver tests, this particular test - # is tgt driver specific and does not apply here. - # We implement it and pass because if we don't it - # calls the parent and fails due to missing mocks. - pass + @mock.patch.object(utils, 'execute') + def test_remove_iscsi_target(self, mexecute): + + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + + # 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', + test_vol, + run_as_root=True) + + # Test the failure case: putils.ProcessExecutionError + mexecute.side_effect = putils.ProcessExecutionError + self.assertRaises(exception.ISCSITargetRemoveFailed, + self.target.remove_iscsi_target, + 0, + 0, + self.testvol['id'], + self.testvol['name']) + @mock.patch.object(lio.LioAdm, '_get_target_chap_auth') @mock.patch.object(lio.LioAdm, 'create_iscsi_target') - def test_ensure_export(self, _mock_create): + def test_ensure_export(self, _mock_create, mock_get_chap): ctxt = context.get_admin_context() + mock_get_chap.return_value = ('foo', 'bar') self.target.ensure_export(ctxt, - self.testvol_1, + self.testvol, self.fake_volumes_dir) - self.target.create_iscsi_target.assert_called_once_with( - 'iqn.2010-10.org.openstack:testvol', - 1, 0, self.fake_volumes_dir, 'IncomingUser foo bar', - check_exit_code=False) + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + _mock_create.assert_called_once_with( + test_vol, + 0, 0, self.fake_volumes_dir, ('foo', 'bar'), + check_exit_code=False, + old_name=None) + + @mock.patch.object(utils, 'execute') + @mock.patch.object(lio.LioAdm, '_get_iscsi_properties') + def test_initialize_connection(self, mock_get_iscsi, mock_execute): + + connector = {'initiator': 'fake_init'} + + # Test the normal case + mock_get_iscsi.return_value = 'foo bar' + expected_return = {'driver_volume_type': 'iscsi', + 'data': 'foo bar'} + self.assertEqual(expected_return, + self.target.initialize_connection(self.testvol, + connector)) + + mock_execute.assert_called_once_with( + 'cinder-rtstool', 'add-initiator', + 'iqn.2010-10.org.openstack:' + 'volume-83c2e877-feed-46be-8435-77884fe55b45', + 'c76370d66b', '2FE0CQ8J196R', + connector['initiator'], + run_as_root=True) + + # Test the failure case: putils.ProcessExecutionError + mock_execute.side_effect = putils.ProcessExecutionError + self.assertRaises(exception.ISCSITargetAttachFailed, + self.target.initialize_connection, + self.testvol, + connector) @mock.patch.object(utils, 'execute') def test_terminate_connection(self, _mock_execute): connector = {'initiator': 'fake_init'} - self.target.terminate_connection(self.testvol_1, + self.target.terminate_connection(self.testvol, connector) _mock_execute.assert_called_once_with( 'cinder-rtstool', 'delete-initiator', 'iqn.2010-10.org.openstack:' - 'volume-ed2c2222-5fc0-11e4-aa15-123b93f75cba', + 'volume-83c2e877-feed-46be-8435-77884fe55b45', connector['initiator'], run_as_root=True) @mock.patch.object(utils, 'execute') def test_terminate_connection_fail(self, _mock_execute): - _mock_execute.side_effect = \ - exception.ISCSITargetDetachFailed(self.testvol_1['id']) + _mock_execute.side_effect = putils.ProcessExecutionError connector = {'initiator': 'fake_init'} self.assertRaises(exception.ISCSITargetDetachFailed, self.target.terminate_connection, - self.testvol_1, + self.testvol, connector) def test_iscsi_protocol(self): diff --git a/cinder/tests/targets/test_tgt_driver.py b/cinder/tests/targets/test_tgt_driver.py index 306c8b165..a59aae4cb 100644 --- a/cinder/tests/targets/test_tgt_driver.py +++ b/cinder/tests/targets/test_tgt_driver.py @@ -12,6 +12,7 @@ import os import tempfile +import time import mock from oslo_concurrency import processutils as putils @@ -34,26 +35,31 @@ class TestTgtAdmDriver(test.TestCase): self.configuration.append_config_values = mock.Mock(return_value=0) self.configuration.iscsi_ip_address = '10.9.8.7' self.fake_volumes_dir = tempfile.mkdtemp() - self.fake_id_1 = 'ed2c1fd4-5fc0-11e4-aa15-123b93f75cba' - self.fake_id_2 = 'ed2c2222-5fc0-11e4-aa15-123b93f75cba' + self.iscsi_target_prefix = 'iqn.2010-10.org.openstack:' + self.fake_project_id = 'ed2c1fd4-5fc0-11e4-aa15-123b93f75cba' + self.fake_volume_id = '83c2e877-feed-46be-8435-77884fe55b45' self.stubs.Set(self.configuration, 'safe_get', self.fake_safe_get) self.target = tgt.TgtAdm(root_helper=utils.get_root_helper(), configuration=self.configuration) - self.testvol_1 =\ - {'project_id': self.fake_id_1, - 'name': 'testvol', + self.testvol =\ + {'project_id': self.fake_project_id, + 'name': 'volume-%s' % self.fake_volume_id, 'size': 1, - 'id': self.fake_id_2, + 'id': self.fake_volume_id, 'volume_type_id': None, 'provider_location': '10.9.8.7:3260 ' 'iqn.2010-10.org.openstack:' - 'volume-%s 0' % self.fake_id_2, + 'volume-%s 0' % self.fake_volume_id, 'provider_auth': 'CHAP stack-1-a60e2611875f40199931f2' 'c76370d66b 2FE0CQ8J196R', 'provider_geometry': '512 512', 'created_at': timeutils.utcnow(), 'host': 'fake_host@lvm#lvm'} + self.testvol_path = \ + '/dev/stack-volumes-lvmdriver-1/'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + self.expected_iscsi_properties = \ {'auth_method': 'CHAP', 'auth_password': '2FE0CQ8J196R', @@ -63,10 +69,10 @@ class TestTgtAdmDriver(test.TestCase): 'physical_block_size': '512', 'target_discovered': False, 'target_iqn': 'iqn.2010-10.org.openstack:volume-%s' % - self.fake_id_2, + self.fake_volume_id, 'target_lun': 0, 'target_portal': '10.9.8.7:3260', - 'volume_id': self.fake_id_2} + 'volume_id': self.fake_volume_id} self.fake_iscsi_scan =\ ('Target 1: iqn.2010-10.org.openstack:volume-83c2e877-feed-46be-8435-77884fe55b45\n' # noqa @@ -113,6 +119,8 @@ class TestTgtAdmDriver(test.TestCase): return self.fake_volumes_dir elif value == 'iscsi_protocol': return self.configuration.iscsi_protocol + elif value == 'iscsi_target_prefix': + return self.iscsi_target_prefix def test_iscsi_protocol(self): self.assertEqual(self.target.iscsi_protocol, 'iscsi') @@ -160,6 +168,49 @@ class TestTgtAdmDriver(test.TestCase): 'volume-83c2e877-feed-46be-' '8435-77884fe55b45', '1')) + @mock.patch.object(time, 'sleep') + @mock.patch.object(utils, 'execute') + def test_recreate_backing_lun(self, mock_execute, mock_sleep): + + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + mock_execute.return_value = ('out', 'err') + self.target._recreate_backing_lun(test_vol, '1', + self.testvol['name'], + self.testvol_path) + + expected_command = ('tgtadm', '--lld', 'iscsi', '--op', 'new', + '--mode', 'logicalunit', '--tid', '1', + '--lun', '1', '-b', + '/dev/stack-volumes-lvmdriver-1/' + 'volume-83c2e877-feed-46be-8435-77884fe55b45') + + mock_execute.assert_called_once_with(*expected_command, + run_as_root=True) + + # Test the failure case + mock_execute.side_effect = putils.ProcessExecutionError + self.assertFalse(self.target._recreate_backing_lun( + test_vol, + '1', + self.testvol['name'], + self.testvol_path)) + + def test_get_iscsi_target(self): + ctxt = context.get_admin_context() + expected = 0 + self.assertEqual(expected, + self.target._get_iscsi_target(ctxt, + self.testvol['id'])) + + def test_get_target_and_lun(self): + lun = 1 + iscsi_target = 0 + ctxt = context.get_admin_context() + expected = (iscsi_target, lun) + self.assertEqual(expected, + self.target._get_target_and_lun(ctxt, self.testvol)) + def test_get_target_chap_auth(self): persist_file =\ '\n'\ @@ -175,8 +226,10 @@ class TestTgtAdmDriver(test.TestCase): test_vol.split(':')[1]), 'wb') as tmp_file: tmp_file.write(persist_file) + ctxt = context.get_admin_context() expected = ('otzLy2UYbYfnP4zXLG5z', '234Zweo38VGBBvrpK9nt') - self.assertEqual(expected, self.target._get_target_chap_auth(test_vol)) + self.assertEqual(expected, + self.target._get_target_chap_auth(ctxt, test_vol)) def test_create_iscsi_target(self): @@ -283,6 +336,57 @@ class TestTgtAdmDriver(test.TestCase): test_vol_id, test_vol_name) + @mock.patch.object(tgt.TgtAdm, '_get_iscsi_properties') + def test_initialize_connection(self, mock_get_iscsi): + + connector = {'initiator': 'fake_init'} + + # Test the normal case + mock_get_iscsi.return_value = 'foo bar' + expected_return = {'driver_volume_type': 'iscsi', + 'data': 'foo bar'} + self.assertEqual(expected_return, + self.target.initialize_connection(self.testvol, + connector)) + + @mock.patch.object(utils, 'execute') + @mock.patch.object(tgt.TgtAdm, '_get_target') + @mock.patch.object(os.path, 'exists') + @mock.patch.object(os.path, 'isfile') + @mock.patch.object(os, 'unlink') + def test_remove_iscsi_target(self, + mock_unlink, + mock_isfile, + mock_path_exists, + mock_get_target, + mock_execute): + + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + + # Test the failure case: path does not exist + mock_path_exists.return_value = None + self.assertEqual(None, + self.target.remove_iscsi_target( + 0, + 1, + self.testvol['id'], + self.testvol['name'])) + + # Test the normal case + mock_path_exists.return_value = True + mock_isfile.return_value = True + self.target.remove_iscsi_target(0, + 1, + self.testvol['id'], + self.testvol['name']) + calls = [mock.call('tgt-admin', '--force', '--delete', test_vol, + run_as_root=True), + mock.call('tgt-admin', '--delete', test_vol, + run_as_root=True)] + + mock_execute.assert_has_calls(calls) + def test_create_export(self): def _fake_execute(*args, **kwargs): @@ -302,7 +406,7 @@ class TestTgtAdmDriver(test.TestCase): self.stubs.Set(self.target, '_get_target_chap_auth', - lambda x: None) + lambda x, y: None) self.stubs.Set(vutils, 'generate_username', lambda: 'QZJbisGmn9AL954FNF4D') @@ -311,46 +415,42 @@ class TestTgtAdmDriver(test.TestCase): lambda: 'P68eE7u9eFqDGexd28DQ') expected_result = {'location': '10.9.8.7:3260,1 ' - 'iqn.2010-10.org.openstack:testvol 1', + 'iqn.2010-10.org.openstack:' + 'volume-83c2e877-feed-46be-8435-77884fe55b45 1', 'auth': 'CHAP ' 'QZJbisGmn9AL954FNF4D P68eE7u9eFqDGexd28DQ'} ctxt = context.get_admin_context() self.assertEqual(expected_result, self.target.create_export(ctxt, - self.testvol_1, + self.testvol, self.fake_volumes_dir)) self.stubs.Set(self.target, '_get_target_chap_auth', - lambda x: ('otzLy2UYbYfnP4zXLG5z', - '234Zweo38VGBBvrpK9nt')) + lambda x, y: ('otzLy2UYbYfnP4zXLG5z', + '234Zweo38VGBBvrpK9nt')) expected_result['auth'] = ('CHAP ' 'otzLy2UYbYfnP4zXLG5z 234Zweo38VGBBvrpK9nt') self.assertEqual(expected_result, self.target.create_export(ctxt, - self.testvol_1, + self.testvol, self.fake_volumes_dir)) - def test_ensure_export(self): + @mock.patch.object(tgt.TgtAdm, '_get_target_chap_auth') + @mock.patch.object(tgt.TgtAdm, 'create_iscsi_target') + def test_ensure_export(self, _mock_create, mock_get_chap): ctxt = context.get_admin_context() - with mock.patch.object(self.target, 'create_iscsi_target'): - self.target.ensure_export(ctxt, - self.testvol_1, - self.fake_volumes_dir) - self.target.create_iscsi_target.assert_called_once_with( - 'iqn.2010-10.org.openstack:testvol', - 1, 0, self.fake_volumes_dir, None, - iscsi_write_cache='on', - check_exit_code=False, - old_name=None) - - def test_iscsi_location(self): - location = self.target._iscsi_location('portal', 1, 'target', 2) - self.assertEqual('portal:3260,1 target 2', location) - - location = self.target._iscsi_location('portal', 1, 'target', 2, - ['portal2']) - self.assertEqual('portal:3260;portal2:3260,1 target 2', location) + mock_get_chap.return_value = ('foo', 'bar') + self.target.ensure_export(ctxt, + self.testvol, + self.fake_volumes_dir) + test_vol = 'iqn.2010-10.org.openstack:'\ + 'volume-83c2e877-feed-46be-8435-77884fe55b45' + _mock_create.assert_called_once_with( + test_vol, + 0, 1, self.fake_volumes_dir, ('foo', 'bar'), + check_exit_code=False, + old_name=None) diff --git a/cinder/volume/targets/driver.py b/cinder/volume/targets/driver.py index e120768d7..fbee7f613 100644 --- a/cinder/volume/targets/driver.py +++ b/cinder/volume/targets/driver.py @@ -63,6 +63,7 @@ class Target(object): """Allow connection to connector and return connection info.""" pass + @abc.abstractmethod def terminate_connection(self, volume, connector, **kwargs): """Disallow connection from connector.""" pass diff --git a/cinder/volume/targets/fake.py b/cinder/volume/targets/fake.py index 08257e1e1..1d89afb08 100644 --- a/cinder/volume/targets/fake.py +++ b/cinder/volume/targets/fake.py @@ -19,20 +19,21 @@ class FakeTarget(iscsi.ISCSITarget): def __init__(self, *args, **kwargs): super(FakeTarget, self).__init__(*args, **kwargs) - def ensure_export(self, context, volume, volume_path): + def _get_target_and_lun(self, context, volume): + return(0, 0) + + def _get_target_chap_auth(self, context, iscsi_name): pass - def create_export(self, context, volume, volume_path): - return { - 'location': "fake_location", - 'auth': "fake_auth" - } + def create_iscsi_target(self, name, tid, lun, path, + chap_auth, **kwargs): + pass - def remove_export(self, context, volume): + def remove_iscsi_target(self, tid, lun, vol_id, vol_name, **kwargs): pass - def initialize_connection(self, volume, connector): + def _get_iscsi_target(self, context, vol_id): pass - def terminate_connection(self, volume, connector, **kwargs): + def _get_target(self, iqn): pass diff --git a/cinder/volume/targets/iscsi.py b/cinder/volume/targets/iscsi.py index 669a32f61..a71dda35e 100644 --- a/cinder/volume/targets/iscsi.py +++ b/cinder/volume/targets/iscsi.py @@ -10,13 +10,16 @@ # License for the specific language governing permissions and limitations # under the License. +import abc + from oslo_concurrency import processutils from cinder import exception -from cinder.i18n import _, _LW, _LE +from cinder.i18n import _, _LI, _LW, _LE from cinder.openstack.common import log as logging from cinder import utils from cinder.volume.targets import driver +from cinder.volume import utils as vutils LOG = logging.getLogger(__name__) @@ -37,6 +40,7 @@ class ISCSITarget(driver.Target): self.iscsi_protocol = \ self.configuration.safe_get('iscsi_protocol') self.protocol = 'iSCSI' + self.volumes_dir = self.configuration.safe_get('volumes_dir') def _get_iscsi_properties(self, volume, multipath=False): """Gets iscsi configuration @@ -169,9 +173,80 @@ class ISCSITarget(driver.Target): return target return None - def _get_target_chap_auth(self, volume_id): - """Get the current chap auth username and password.""" - return None + def create_export(self, context, volume, volume_path): + """Creates an export for a logical volume.""" + # 'iscsi_name': 'iqn.2010-10.org.openstack:volume-00000001' + iscsi_name = "%s%s" % (self.configuration.iscsi_target_prefix, + volume['name']) + iscsi_target, lun = self._get_target_and_lun(context, volume) + + # Verify we haven't setup a CHAP creds file already + # if DNE no big deal, we'll just create it + chap_auth = self._get_target_chap_auth(context, iscsi_name) + if not chap_auth: + chap_auth = (vutils.generate_username(), + vutils.generate_password()) + + # NOTE(jdg): For TgtAdm case iscsi_name is the ONLY param we need + # should clean this all up at some point in the future + tid = self.create_iscsi_target(iscsi_name, + iscsi_target, + lun, + volume_path, + chap_auth) + data = {} + data['location'] = self._iscsi_location( + self.configuration.iscsi_ip_address, tid, iscsi_name, lun, + self.configuration.iscsi_secondary_ip_addresses) + LOG.debug('Set provider_location to: %s', data['location']) + data['auth'] = self._iscsi_authentication( + 'CHAP', *chap_auth) + return data + + def remove_export(self, context, volume): + try: + iscsi_target, lun = self._get_target_and_lun(context, volume) + except exception.NotFound: + LOG.info(_LI("Skipping remove_export. No iscsi_target " + "provisioned for volume: %s"), volume['id']) + return + try: + + # NOTE: provider_location may be unset if the volume hasn't + # been exported + location = volume['provider_location'].split(' ') + iqn = location[1] + + # ietadm show will exit with an error + # this export has already been removed + self.show_target(iscsi_target, iqn=iqn) + + except Exception: + LOG.info(_LI("Skipping remove_export. No iscsi_target " + "is presently exported for volume: %s"), volume['id']) + return + + # NOTE: For TgtAdm case volume['id'] is the ONLY param we need + self.remove_iscsi_target(iscsi_target, lun, volume['id'], + volume['name']) + + def ensure_export(self, context, volume, volume_path): + """Recreates an export for a logical volume.""" + iscsi_name = "%s%s" % (self.configuration.iscsi_target_prefix, + volume['name']) + + # Verify we haven't setup a CHAP creds file already + # if DNE no big deal, we'll just create it + chap_auth = self._get_target_chap_auth(context, iscsi_name) + if not chap_auth: + LOG.info(_LI("Skipping ensure_export. No iscsi_target " + "provision for volume: %s"), volume['id']) + + iscsi_target, lun = self._get_target_and_lun(context, volume) + self.create_iscsi_target( + iscsi_name, iscsi_target, lun, volume_path, + chap_auth, check_exit_code=False, + old_name=None) def initialize_connection(self, volume, connector): """Initializes the connection and returns connection info. @@ -198,6 +273,9 @@ class ISCSITarget(driver.Target): 'data': iscsi_properties } + def terminate_connection(self, volume, connector, **kwargs): + pass + def validate_connector(self, connector): # NOTE(jdg): api passes in connector which is initiator info if 'initiator' not in connector: @@ -206,3 +284,46 @@ class ISCSITarget(driver.Target): LOG.error(err_msg) raise exception.InvalidConnectorException(missing='initiator') return True + + def _iscsi_location(self, ip, target, iqn, lun=None, ip_secondary=None): + ip_secondary = ip_secondary or [] + port = self.configuration.iscsi_port + portals = map(lambda x: "%s:%s" % (x, port), [ip] + ip_secondary) + return ("%(portals)s,%(target)s %(iqn)s %(lun)s" + % ({'portals': ";".join(portals), + 'target': target, 'iqn': iqn, 'lun': lun})) + + def show_target(self, iscsi_target, iqn, **kwargs): + if iqn is None: + raise exception.InvalidParameterValue( + err=_('valid iqn needed for show_target')) + + tid = self._get_target(iqn) + if tid is None: + raise exception.NotFound() + + @abc.abstractmethod + def _get_target_and_lun(self, context, volume): + """Get iscsi target and lun.""" + pass + + @abc.abstractmethod + def _get_target_chap_auth(self, context, iscsi_name): + pass + + @abc.abstractmethod + def create_iscsi_target(self, name, tid, lun, path, + chap_auth, **kwargs): + pass + + @abc.abstractmethod + def remove_iscsi_target(self, tid, lun, vol_id, vol_name, **kwargs): + pass + + @abc.abstractmethod + def _get_iscsi_target(self, context, vol_id): + pass + + @abc.abstractmethod + def _get_target(self, iqn): + pass diff --git a/cinder/volume/targets/lio.py b/cinder/volume/targets/lio.py index 12ccc7791..66388b446 100644 --- a/cinder/volume/targets/lio.py +++ b/cinder/volume/targets/lio.py @@ -13,15 +13,16 @@ from oslo_concurrency import processutils as putils from cinder import exception -from cinder.i18n import _, _LE, _LI, _LW +from cinder.i18n import _LE, _LI, _LW from cinder.openstack.common import log as logging from cinder import utils -from cinder.volume.targets.tgt import TgtAdm +from cinder.volume.targets import iscsi + LOG = logging.getLogger(__name__) -class LioAdm(TgtAdm): +class LioAdm(iscsi.ISCSITarget): """iSCSI target administration for LIO using python-rtslib.""" def __init__(self, *args, **kwargs): super(LioAdm, self).__init__(*args, **kwargs) @@ -38,39 +39,17 @@ class LioAdm(TgtAdm): self._verify_rtstool() - def _get_target_chap_auth(self, name): - pass - - def remove_export(self, context, volume): - try: - iscsi_target = self.db.volume_get_iscsi_target_num(context, - volume['id']) - except exception.NotFound: - LOG.info(_LI("Skipping remove_export. No iscsi_target " - "provisioned for volume: %s"), volume['id']) - return - - self.remove_iscsi_target(iscsi_target, 0, volume['id'], volume['name']) - - def ensure_export(self, context, volume, volume_path): + def _get_target_chap_auth(self, context, iscsi_name): + """Get the current chap auth username and password.""" try: - volume_info = self.db.volume_get(context, volume['id']) - (auth_method, - auth_user, - auth_pass) = volume_info['provider_auth'].split(' ', 3) - chap_auth = self._iscsi_authentication(auth_method, - auth_user, - auth_pass) + # 'iscsi_name': 'iqn.2010-10.org.openstack:volume-00000001' + vol_id = iscsi_name.split(':volume-')[1] + volume_info = self.db.volume_get(context, vol_id) + # 'provider_auth': 'CHAP user_id password' + if volume_info['provider_auth']: + return tuple(volume_info['provider_auth'].split(' ', 3)[1:]) except exception.NotFound: - LOG.debug(("volume_info:%s"), volume_info) - LOG.info(_LI("Skipping ensure_export. No iscsi_target " - "provision for volume: %s"), volume['id']) - - iscsi_target = 1 - iscsi_name = "%s%s" % (self.configuration.iscsi_target_prefix, - volume['name']) - self.create_iscsi_target(iscsi_name, iscsi_target, 0, volume_path, - chap_auth, check_exit_code=False) + LOG.debug('Failed to get CHAP auth from DB for %s', vol_id) def _verify_rtstool(self): try: @@ -90,6 +69,14 @@ class LioAdm(TgtAdm): return None + def _get_iscsi_target(self, context, vol_id): + return 0 + + def _get_target_and_lun(self, context, volume): + lun = 0 # For lio, the lun starts at lun 0. + iscsi_target = 0 # NOTE: Not used by lio. + return iscsi_target, lun + def create_iscsi_target(self, name, tid, lun, path, chap_auth=None, **kwargs): # tid and lun are not used @@ -101,7 +88,7 @@ class LioAdm(TgtAdm): chap_auth_userid = "" chap_auth_password = "" if chap_auth is not None: - (chap_auth_userid, chap_auth_password) = chap_auth.split(' ')[1:] + (chap_auth_userid, chap_auth_password) = chap_auth try: command_args = ['cinder-rtstool', @@ -144,15 +131,6 @@ class LioAdm(TgtAdm): LOG.error(_LE("%s") % e) raise exception.ISCSITargetRemoveFailed(volume_id=vol_id) - def show_target(self, tid, iqn=None, **kwargs): - if iqn is None: - raise exception.InvalidParameterValue( - err=_('valid iqn needed for show_target')) - - tid = self._get_target(iqn) - if tid is None: - raise exception.NotFound() - def initialize_connection(self, volume, connector): volume_iqn = volume['provider_location'].split(' ')[1] @@ -177,10 +155,6 @@ class LioAdm(TgtAdm): connector.get( 'multipath')) - # FIXME(jdg): For LIO the target_lun is 0, other than that all data - # is the same as it is for tgtadm, just modify it here - iscsi_properties['target_lun'] = 0 - return { 'driver_volume_type': self.iscsi_protocol, 'data': iscsi_properties diff --git a/cinder/volume/targets/scst.py b/cinder/volume/targets/scst.py index 78b9490af..c8f5aeb4e 100644 --- a/cinder/volume/targets/scst.py +++ b/cinder/volume/targets/scst.py @@ -218,6 +218,14 @@ class SCSTAdm(iscsi.ISCSITarget): return "%s:%s,%s %s %s" % (ip, self.configuration.iscsi_port, target, iqn, lun) + def _get_iscsi_target(self, context, vol_id): + # FIXME(jdg): Need to implement abc method + pass + + def _get_target_chap_auth(self, context, iscsi_name): + # FIXME(jdg): Need to implement abc method + pass + def ensure_export(self, context, volume, volume_path): iscsi_name = "%s%s" % (self.configuration.iscsi_target_prefix, volume['name']) diff --git a/cinder/volume/targets/tgt.py b/cinder/volume/targets/tgt.py index 3b0c00903..892d7a34f 100644 --- a/cinder/volume/targets/tgt.py +++ b/cinder/volume/targets/tgt.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. - import os import re import time @@ -20,11 +19,10 @@ import six from cinder import exception from cinder.openstack.common import fileutils -from cinder.i18n import _, _LI, _LW, _LE +from cinder.i18n import _LI, _LW, _LE from cinder.openstack.common import log as logging from cinder import utils from cinder.volume.targets import iscsi -from cinder.volume import utils as vutils LOG = logging.getLogger(__name__) @@ -56,7 +54,6 @@ class TgtAdm(iscsi.ISCSITarget): def __init__(self, *args, **kwargs): super(TgtAdm, self).__init__(*args, **kwargs) - self.volumes_dir = self.configuration.safe_get('volumes_dir') def _get_target(self, iqn): (out, err) = utils.execute('tgt-admin', '--show', run_as_root=True) @@ -116,14 +113,6 @@ class TgtAdm(iscsi.ISCSITarget): LOG.debug('StdOut from recreate backing lun: %s' % out) LOG.debug('StdErr from recreate backing lun: %s' % err) - def _iscsi_location(self, ip, target, iqn, lun=None, ip_secondary=None): - ip_secondary = ip_secondary or [] - port = self.configuration.iscsi_port - portals = map(lambda x: "%s:%s" % (x, port), [ip] + ip_secondary) - return ("%(portals)s,%(target)s %(iqn)s %(lun)s" - % ({'portals': ";".join(portals), - 'target': target, 'iqn': iqn, 'lun': lun})) - def _get_iscsi_target(self, context, vol_id): return 0 @@ -132,9 +121,10 @@ class TgtAdm(iscsi.ISCSITarget): iscsi_target = 0 # NOTE(jdg): Not used by tgtadm return iscsi_target, lun - def _get_target_chap_auth(self, name): + def _get_target_chap_auth(self, context, iscsi_name): + """Get the current chap auth username and password.""" volumes_dir = self.volumes_dir - vol_id = name.split(':')[1] + vol_id = iscsi_name.split(':')[1] volume_path = os.path.join(volumes_dir, vol_id) try: @@ -158,25 +148,6 @@ class TgtAdm(iscsi.ISCSITarget): LOG.debug("StdOut from tgt-admin --update: %s", out) LOG.debug("StdErr from tgt-admin --update: %s", err) - def ensure_export(self, context, volume, volume_path): - chap_auth = None - old_name = None - - # FIXME (jdg): This appears to be broken in existing code - # we recreate the iscsi target but we pass in None - # for CHAP, so we just recreated without CHAP even if - # we had it set on initial create - - iscsi_name = "%s%s" % (self.configuration.iscsi_target_prefix, - volume['name']) - iscsi_write_cache = self.configuration.get('iscsi_write_cache', 'on') - self.create_iscsi_target( - iscsi_name, - 1, 0, volume_path, - chap_auth, check_exit_code=False, - old_name=old_name, - iscsi_write_cache=iscsi_write_cache) - def create_iscsi_target(self, name, tid, lun, path, chap_auth=None, **kwargs): @@ -198,13 +169,13 @@ class TgtAdm(iscsi.ISCSITarget): fileutils.ensure_tree(self.volumes_dir) vol_id = name.split(':')[1] - write_cache = kwargs.get('iscsi_write_cache', 'on') + write_cache = self.configuration.get('iscsi_write_cache', 'on') driver = self.iscsi_protocol if chap_auth is None: volume_conf = self.VOLUME_CONF % (name, path, driver, write_cache) else: - chap_str = re.sub('^IncomingUser ', 'incominguser ', chap_auth) + chap_str = 'incominguser %s %s' % chap_auth volume_conf = self.VOLUME_CONF_WITH_CHAP_AUTH % (name, path, driver, chap_str, write_cache) @@ -298,65 +269,6 @@ class TgtAdm(iscsi.ISCSITarget): return tid - def create_export(self, context, volume, volume_path): - """Creates an export for a logical volume.""" - iscsi_name = "%s%s" % (self.configuration.iscsi_target_prefix, - volume['name']) - iscsi_target, lun = self._get_target_and_lun(context, volume) - - # Verify we haven't setup a CHAP creds file already - # if DNE no big deal, we'll just create it - current_chap_auth = self._get_target_chap_auth(iscsi_name) - if current_chap_auth: - (chap_username, chap_password) = current_chap_auth - else: - chap_username = vutils.generate_username() - chap_password = vutils.generate_password() - chap_auth = self._iscsi_authentication('IncomingUser', chap_username, - chap_password) - # NOTE(jdg): For TgtAdm case iscsi_name is the ONLY param we need - # should clean this all up at some point in the future - iscsi_write_cache = self.configuration.get('iscsi_write_cache', 'on') - tid = self.create_iscsi_target(iscsi_name, - iscsi_target, - 0, - volume_path, - chap_auth, - iscsi_write_cache=iscsi_write_cache) - data = {} - data['location'] = self._iscsi_location( - self.configuration.iscsi_ip_address, tid, iscsi_name, lun, - self.configuration.iscsi_secondary_ip_addresses) - LOG.debug('Set provider_location to: %s', data['location']) - data['auth'] = self._iscsi_authentication( - 'CHAP', chap_username, chap_password) - return data - - def remove_export(self, context, volume): - try: - iscsi_target = self._get_iscsi_target(context, volume['id']) - except exception.NotFound: - LOG.info(_LI("Skipping remove_export. No iscsi_target " - "provisioned for volume: %s"), volume['id']) - return - try: - - # NOTE: provider_location may be unset if the volume hasn't - # been exported - location = volume['provider_location'].split(' ') - iqn = location[1] - - # ietadm show will exit with an error - # this export has already been removed - self.show_target(iscsi_target, iqn=iqn) - - except Exception: - LOG.info(_LI("Skipping remove_export. No iscsi_target " - "is presently exported for volume: %s"), volume['id']) - return - - self.remove_iscsi_target(iscsi_target, 0, volume['id'], volume['name']) - def initialize_connection(self, volume, connector): iscsi_properties = self._get_iscsi_properties(volume, connector.get( @@ -430,12 +342,3 @@ class TgtAdm(iscsi.ISCSITarget): else: LOG.debug('Volume path %s not found at end, ' 'of remove_iscsi_target.' % volume_path) - - def show_target(self, tid, iqn=None, **kwargs): - if iqn is None: - raise exception.InvalidParameterValue( - err=_('valid iqn needed for show_target')) - - tid = self._get_target(iqn) - if tid is None: - raise exception.NotFound()