From 2522a4438b1bc6799845f733894bc1d64b2a755f Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Sun, 18 Sep 2011 12:04:46 +0100 Subject: Remove VolumeDriver.sync_exec method (lp:819997) We always use the same functions for sync_exec and execute. The execute method is always synchronous, so the distinction doesn't appear to make sense. Finally, it looks like it would make sense for execute to ever be async, so the distinction isn't even serving a useful documentation purpose. Change-Id: I86d491cfbf8be73672df7cfdf22e465627a86034 --- nova/tests/test_volume.py | 1 - nova/volume/driver.py | 91 ++++++++++++++++++++++------------------------- 2 files changed, 43 insertions(+), 49 deletions(-) diff --git a/nova/tests/test_volume.py b/nova/tests/test_volume.py index 88a73f550..d1b11aaf8 100644 --- a/nova/tests/test_volume.py +++ b/nova/tests/test_volume.py @@ -271,7 +271,6 @@ class DriverTestCase(test.TestCase): """Fake _execute.""" return self.output, None self.volume.driver._execute = _fake_execute - self.volume.driver._sync_execute = _fake_execute log = logging.getLogger() self.stream = cStringIO.StringIO() diff --git a/nova/volume/driver.py b/nova/volume/driver.py index 2692f5e9c..f1e30dc94 100644 --- a/nova/volume/driver.py +++ b/nova/volume/driver.py @@ -52,12 +52,10 @@ flags.DEFINE_string('rbd_pool', 'rbd', class VolumeDriver(object): """Executes commands relating to Volumes.""" - def __init__(self, execute=utils.execute, - sync_exec=utils.execute, *args, **kwargs): + def __init__(self, execute=utils.execute, *args, **kwargs): # NOTE(vish): db is set by Manager self.db = None self._execute = execute - self._sync_exec = sync_exec def _try_execute(self, *command, **kwargs): # NOTE(vish): Volume commands can partially fail due to timing, but @@ -238,19 +236,19 @@ class ISCSIDriver(VolumeDriver): iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name']) volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name']) - self._sync_exec('ietadm', '--op', 'new', - "--tid=%s" % iscsi_target, - '--params', - "Name=%s" % iscsi_name, - run_as_root=True, - check_exit_code=False) - self._sync_exec('ietadm', '--op', 'new', - "--tid=%s" % iscsi_target, - '--lun=0', - '--params', - "Path=%s,Type=fileio" % volume_path, - run_as_root=True, - check_exit_code=False) + self._execute('ietadm', '--op', 'new', + "--tid=%s" % iscsi_target, + '--params', + "Name=%s" % iscsi_name, + run_as_root=True, + check_exit_code=False) + self._execute('ietadm', '--op', 'new', + "--tid=%s" % iscsi_target, + '--lun=0', + '--params', + "Path=%s,Type=fileio" % volume_path, + run_as_root=True, + check_exit_code=False) def _ensure_iscsi_targets(self, context, host): """Ensure that target ids have been created in datastore.""" @@ -439,7 +437,6 @@ class FakeISCSIDriver(ISCSIDriver): """Logs calls instead of executing.""" def __init__(self, *args, **kwargs): super(FakeISCSIDriver, self).__init__(execute=self.fake_execute, - sync_exec=self.fake_execute, *args, **kwargs) def check_for_setup_error(self): @@ -718,14 +715,14 @@ class ZadaraBEDriver(ISCSIDriver): break try: - self._sync_exec('/var/lib/zadara/bin/zadara_sncfg', - 'create_qospart', - '--qos', qosstr, - '--pname', volume['name'], - '--psize', sizestr, - '--vsaid', vsa_id, - run_as_root=True, - check_exit_code=0) + self._execute('/var/lib/zadara/bin/zadara_sncfg', + 'create_qospart', + '--qos', qosstr, + '--pname', volume['name'], + '--psize', sizestr, + '--vsaid', vsa_id, + run_as_root=True, + check_exit_code=0) except exception.ProcessExecutionError: LOG.debug(_("VSA BE create_volume for %s failed"), volume['name']) raise @@ -743,11 +740,11 @@ class ZadaraBEDriver(ISCSIDriver): return try: - self._sync_exec('/var/lib/zadara/bin/zadara_sncfg', - 'delete_partition', - '--pname', volume['name'], - run_as_root=True, - check_exit_code=0) + self._execute('/var/lib/zadara/bin/zadara_sncfg', + 'delete_partition', + '--pname', volume['name'], + run_as_root=True, + check_exit_code=0) except exception.ProcessExecutionError: LOG.debug(_("VSA BE delete_volume for %s failed"), volume['name']) return @@ -883,12 +880,12 @@ class ZadaraBEDriver(ISCSIDriver): return try: - self._sync_exec('/var/lib/zadara/bin/zadara_sncfg', - 'remove_export', - '--pname', volume['name'], - '--tid', iscsi_target, - run_as_root=True, - check_exit_code=0) + self._execute('/var/lib/zadara/bin/zadara_sncfg', + 'remove_export', + '--pname', volume['name'], + '--tid', iscsi_target, + run_as_root=True, + check_exit_code=0) except exception.ProcessExecutionError: LOG.debug(_("VSA BE remove_export for %s failed"), volume['name']) return @@ -913,13 +910,12 @@ class ZadaraBEDriver(ISCSIDriver): Common logic that asks zadara_sncfg to setup iSCSI target/lun for this volume """ - (out, err) = self._sync_exec( - '/var/lib/zadara/bin/zadara_sncfg', - 'create_export', - '--pname', volume['name'], - '--tid', iscsi_target, - run_as_root=True, - check_exit_code=0) + (out, err) = self._execute('/var/lib/zadara/bin/zadara_sncfg', + 'create_export', + '--pname', volume['name'], + '--tid', iscsi_target, + run_as_root=True, + check_exit_code=0) result_xml = ElementTree.fromstring(out) response_node = result_xml.find("Sn") @@ -940,11 +936,10 @@ class ZadaraBEDriver(ISCSIDriver): def _get_qosgroup_summary(self): """gets the list of qosgroups from Zadara BE""" try: - (out, err) = self._sync_exec( - '/var/lib/zadara/bin/zadara_sncfg', - 'get_qosgroups_xml', - run_as_root=True, - check_exit_code=0) + (out, err) = self._execute('/var/lib/zadara/bin/zadara_sncfg', + 'get_qosgroups_xml', + run_as_root=True, + check_exit_code=0) except exception.ProcessExecutionError: LOG.debug(_("Failed to retrieve QoS info")) return {} -- cgit From 871141d4d3cc0ac739de72ca010aef3e5c13fe1f Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Thu, 11 Aug 2011 07:44:38 -0400 Subject: Allow the user to choose either ietadm or tgtadm (lp:819997) Also, refactor ietadm/tgtadm calls out into helper classes. Add a new TargetAdmin abstract base class and implement it using ietadm and tgtadm. This cleans up the code greatly and gets us some code reuse. (Based on a patch by Chuck Short ) Change-Id: I1c0064e5d35483a6c4059cfc61a484f5f576b2da --- nova/tests/test_iscsi.py | 116 ++++++++++++++++++++++++++++++++++ nova/tests/test_volume.py | 16 ++--- nova/volume/driver.py | 52 +++++++--------- nova/volume/iscsi.py | 156 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 301 insertions(+), 39 deletions(-) create mode 100644 nova/tests/test_iscsi.py create mode 100644 nova/volume/iscsi.py diff --git a/nova/tests/test_iscsi.py b/nova/tests/test_iscsi.py new file mode 100644 index 000000000..d7aed0fbd --- /dev/null +++ b/nova/tests/test_iscsi.py @@ -0,0 +1,116 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2011 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import string + +from nova import test +from nova.volume import iscsi + + +class TargetAdminTestCase(object): + + def setUp(self): + self.cmds = [] + + self.tid = 1 + self.target_name = 'iqn.2011-09.org.foo.bar:blaa' + self.lun = 10 + self.path = '/foo/bar/blaa' + + self.script_template = None + + def get_script_params(self): + return {'tid': self.tid, + 'target_name': self.target_name, + 'lun': self.lun, + 'path': self.path} + + def get_script(self): + return self.script_template % self.get_script_params() + + def fake_execute(self, *cmd, **kwargs): + self.cmds.append(string.join(cmd)) + return "", None + + def clear_cmds(self): + cmds = [] + + def verify_cmds(self, cmds): + self.assertEqual(len(cmds), len(self.cmds)) + for a, b in zip(cmds, self.cmds): + self.assertEqual(a, b) + + def verify(self): + script = self.get_script() + cmds = [] + for line in script.split('\n'): + if not line.strip(): + continue + cmds.append(line) + self.verify_cmds(cmds) + + def run_commands(self): + tgtadm = iscsi.get_target_admin() + tgtadm.set_execute(self.fake_execute) + tgtadm.new_target(self.target_name, self.tid) + tgtadm.show_target(self.tid) + tgtadm.new_logicalunit(self.tid, self.lun, self.path) + tgtadm.delete_logicalunit(self.tid, self.lun) + tgtadm.delete_target(self.tid) + + def test_target_admin(self): + self.clear_cmds() + self.run_commands() + self.verify() + + +class TgtAdmTestCase(test.TestCase, TargetAdminTestCase): + + def setUp(self): + super(TgtAdmTestCase, self).setUp() + TargetAdminTestCase.setUp(self) + self.flags(iscsi_helper='tgtadm') + self.script_template = """ +tgtadm --op new --lld=iscsi --mode=target --tid=%(tid)s \ +--targetname=%(target_name)s +tgtadm --op bind --lld=iscsi --mode=target --initiator-address=ALL \ +--tid=%(tid)s +tgtadm --op show --lld=iscsi --mode=target --tid=%(tid)s +tgtadm --op new --lld=iscsi --mode=logicalunit --tid=%(tid)s --lun=%(lun)d \ +--backing-store=%(path)s +tgtadm --op delete --lld=iscsi --mode=logicalunit --tid=%(tid)s --lun=%(lun)d +tgtadm --op delete --lld=iscsi --mode=target --tid=%(tid)s +""" + + def get_script_params(self): + params = super(TgtAdmTestCase, self).get_script_params() + params['lun'] += 1 + return params + + +class IetAdmTestCase(test.TestCase, TargetAdminTestCase): + + def setUp(self): + super(IetAdmTestCase, self).setUp() + TargetAdminTestCase.setUp(self) + self.flags(iscsi_helper='ietadm') + self.script_template = """ +ietadm --op new --tid=%(tid)s --params Name=%(target_name)s +ietadm --op show --tid=%(tid)s +ietadm --op new --tid=%(tid)s --lun=%(lun)d --params Path=%(path)s,Type=fileio +ietadm --op delete --tid=%(tid)s --lun=%(lun)d +ietadm --op delete --tid=%(tid)s +""" diff --git a/nova/tests/test_volume.py b/nova/tests/test_volume.py index d1b11aaf8..588b7a328 100644 --- a/nova/tests/test_volume.py +++ b/nova/tests/test_volume.py @@ -270,7 +270,7 @@ class DriverTestCase(test.TestCase): def _fake_execute(_command, *_args, **_kwargs): """Fake _execute.""" return self.output, None - self.volume.driver._execute = _fake_execute + self.volume.driver.set_execute(_fake_execute) log = logging.getLogger() self.stream = cStringIO.StringIO() @@ -333,12 +333,10 @@ class ISCSITestCase(DriverTestCase): """No log message when all the processes are running.""" volume_id_list = self._attach_volume() - self.mox.StubOutWithMock(self.volume.driver, '_execute') + self.mox.StubOutWithMock(self.volume.driver.tgtadm, 'show_target') for i in volume_id_list: tid = db.volume_get_iscsi_target_num(self.context, i) - self.volume.driver._execute("ietadm", "--op", "show", - "--tid=%(tid)d" % locals(), - run_as_root=True) + self.volume.driver.tgtadm.show_target(tid) self.stream.truncate(0) self.mox.ReplayAll() @@ -354,11 +352,9 @@ class ISCSITestCase(DriverTestCase): volume_id_list = self._attach_volume() tid = db.volume_get_iscsi_target_num(self.context, volume_id_list[0]) - self.mox.StubOutWithMock(self.volume.driver, '_execute') - self.volume.driver._execute("ietadm", "--op", "show", - "--tid=%(tid)d" % locals(), - run_as_root=True).AndRaise( - exception.ProcessExecutionError()) + self.mox.StubOutWithMock(self.volume.driver.tgtadm, 'show_target') + self.volume.driver.tgtadm.show_target(tid).AndRaise( + exception.ProcessExecutionError()) self.mox.ReplayAll() self.assertRaises(exception.ProcessExecutionError, diff --git a/nova/volume/driver.py b/nova/volume/driver.py index f1e30dc94..ea7386d86 100644 --- a/nova/volume/driver.py +++ b/nova/volume/driver.py @@ -28,6 +28,7 @@ from nova import exception from nova import flags from nova import log as logging from nova import utils +from nova.volume import iscsi from nova.volume import volume_types @@ -55,6 +56,9 @@ class VolumeDriver(object): def __init__(self, execute=utils.execute, *args, **kwargs): # NOTE(vish): db is set by Manager self.db = None + self.set_execute(execute) + + def set_execute(self, execute): self._execute = execute def _try_execute(self, *command, **kwargs): @@ -224,6 +228,14 @@ class ISCSIDriver(VolumeDriver): `CHAP` is the only auth_method in use at the moment. """ + def __init__(self, *args, **kwargs): + self.tgtadm = iscsi.get_target_admin() + super(ISCSIDriver, self).__init__(*args, **kwargs) + + def set_execute(self, execute): + super(ISCSIDriver, self).set_execute(execute) + self.tgtadm.set_execute(execute) + def ensure_export(self, context, volume): """Synchronously recreates an export for a logical volume.""" try: @@ -236,19 +248,10 @@ class ISCSIDriver(VolumeDriver): iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name']) volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name']) - self._execute('ietadm', '--op', 'new', - "--tid=%s" % iscsi_target, - '--params', - "Name=%s" % iscsi_name, - run_as_root=True, - check_exit_code=False) - self._execute('ietadm', '--op', 'new', - "--tid=%s" % iscsi_target, - '--lun=0', - '--params', - "Path=%s,Type=fileio" % volume_path, - run_as_root=True, - check_exit_code=False) + + self.tgtadm.new_target(iscsi_name, iscsi_target, check_exit_code=False) + self.tgtadm.new_logicalunit(iscsi_target, 0, volume_path, + check_exit_code=False) def _ensure_iscsi_targets(self, context, host): """Ensure that target ids have been created in datastore.""" @@ -268,13 +271,9 @@ class ISCSIDriver(VolumeDriver): volume['host']) iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name']) volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name']) - self._execute('ietadm', '--op', 'new', - '--tid=%s' % iscsi_target, - '--params', 'Name=%s' % iscsi_name, run_as_root=True) - self._execute('ietadm', '--op', 'new', - '--tid=%s' % iscsi_target, - '--lun=0', '--params', - 'Path=%s,Type=fileio' % volume_path, run_as_root=True) + + self.tgtadm.new_target(iscsi_name, iscsi_target) + self.tgtadm.new_logicalunit(iscsi_target, 0, volume_path) def remove_export(self, context, volume): """Removes an export for a logical volume.""" @@ -289,18 +288,14 @@ class ISCSIDriver(VolumeDriver): try: # ietadm show will exit with an error # this export has already been removed - self._execute('ietadm', '--op', 'show', - '--tid=%s' % iscsi_target, run_as_root=True) + self.tgtadm.show_target(iscsi_target) except Exception as e: LOG.info(_("Skipping remove_export. No iscsi_target " + "is presently exported for volume: %d"), volume['id']) return - self._execute('ietadm', '--op', 'delete', - '--tid=%s' % iscsi_target, - '--lun=0', run_as_root=True) - self._execute('ietadm', '--op', 'delete', - '--tid=%s' % iscsi_target, run_as_root=True) + self.tgtadm.delete_logicalunit(iscsi_target, 0) + self.tgtadm.delete_target(iscsi_target) def _do_iscsi_discovery(self, volume): #TODO(justinsb): Deprecate discovery and use stored info @@ -422,8 +417,7 @@ class ISCSIDriver(VolumeDriver): tid = self.db.volume_get_iscsi_target_num(context, volume_id) try: - self._execute('ietadm', '--op', 'show', - '--tid=%(tid)d' % locals(), run_as_root=True) + self.tgtadm.show_target(tid) except exception.ProcessExecutionError, e: # Instances remount read-only in this case. # /etc/init.d/iscsitarget restart and rebooting nova-volume diff --git a/nova/volume/iscsi.py b/nova/volume/iscsi.py new file mode 100644 index 000000000..d50dd5fd7 --- /dev/null +++ b/nova/volume/iscsi.py @@ -0,0 +1,156 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2010 United States Government as represented by the +# Administrator of the National Aeronautics and Space Administration. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +""" +Helper code for the iSCSI volume driver. + +""" + +from nova import flags +from nova import utils + + +FLAGS = flags.FLAGS +flags.DEFINE_string('iscsi_helper', 'ietadm', + 'iscsi target user-land tool to use') + + +class TargetAdmin(object): + """iSCSI target administration. + + Base class for iSCSI target admin helpers. + """ + + def __init__(self, cmd, execute): + self._cmd = cmd + self.set_execute(execute) + + def set_execute(self, execute): + """Set the function to be used to execute commands.""" + self._execute = execute + + def _run(self, *args, **kwargs): + self._execute(self._cmd, *args, run_as_root=True, **kwargs) + + def new_target(self, name, tid, **kwargs): + """Create a new iSCSI target.""" + raise NotImplementedError() + + def delete_target(self, tid, **kwargs): + """Delete a target.""" + raise NotImplementedError() + + def show_target(self, tid, **kwargs): + """Query the given target ID.""" + raise NotImplementedError() + + def new_logicalunit(self, tid, lun, path, **kwargs): + """Create a new LUN on a target using the supplied path.""" + raise NotImplementedError() + + def delete_logicalunit(self, tid, lun, **kwargs): + """Delete a logical unit from a target.""" + raise NotImplementedError() + + +class TgtAdm(TargetAdmin): + """iSCSI target administration using tgtadm.""" + + def __init__(self, execute=utils.execute): + super(TgtAdm, self).__init__('tgtadm', execute) + + def new_target(self, name, tid, **kwargs): + self._run('--op', 'new', + '--lld=iscsi', '--mode=target', + '--tid=%s' % tid, + '--targetname=%s' % name, + **kwargs) + self._run('--op', 'bind', + '--lld=iscsi', '--mode=target', + '--initiator-address=ALL', + '--tid=%s' % tid, + **kwargs) + + def delete_target(self, tid, **kwargs): + self._run('--op', 'delete', + '--lld=iscsi', '--mode=target', + '--tid=%s' % tid, + **kwargs) + + def show_target(self, tid, **kwargs): + self._run('--op', 'show', + '--lld=iscsi', '--mode=target', + '--tid=%s' % tid, + **kwargs) + + def new_logicalunit(self, tid, lun, path, **kwargs): + self._run('--op', 'new', + '--lld=iscsi', '--mode=logicalunit', + '--tid=%s' % tid, + '--lun=%d' % (lun + 1), # lun0 is reserved + '--backing-store=%s' % path, + **kwargs) + + def delete_logicalunit(self, tid, lun, **kwargs): + self._run('--op', 'delete', + '--lld=iscsi', '--mode=logicalunit', + '--tid=%s' % tid, + '--lun=%d' % (lun + 1), + **kwargs) + + +class IetAdm(TargetAdmin): + """iSCSI target administration using ietadm.""" + + def __init__(self, execute=utils.execute): + super(IetAdm, self).__init__('ietadm', execute) + + def new_target(self, name, tid, **kwargs): + self._run('--op', 'new', + '--tid=%s' % tid, + '--params', 'Name=%s' % name, + **kwargs) + + def delete_target(self, tid, **kwargs): + self._run('--op', 'delete', + '--tid=%s' % tid, + **kwargs) + + def show_target(self, tid, **kwargs): + self._run('--op', 'show', + '--tid=%s' % tid, + **kwargs) + + def new_logicalunit(self, tid, lun, path, **kwargs): + self._run('--op', 'new', + '--tid=%s' % tid, + '--lun=%d' % lun, + '--params', 'Path=%s,Type=fileio' % path, + **kwargs) + + def delete_logicalunit(self, tid, lun, **kwargs): + self._run('--op', 'delete', + '--tid=%s' % tid, + '--lun=%d' % lun, + **kwargs) + + +def get_target_admin(): + if FLAGS.iscsi_helper == 'tgtadm': + return TgtAdm() + else: + return IetAdm() -- cgit