From 224d99a25d40e11c6ea6209603224e5b9e871d6a Mon Sep 17 00:00:00 2001
From: Mark McLoughlin <markmc@redhat.com>
Date: Sun, 18 Sep 2011 16:02:43 +0100
Subject: [PATCH] Refactor ietadm/tgtadm calls out into helper classes
(lp:819997)
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.
Change-Id: I1c0064e5d35483a6c4059cfc61a484f5f576b2da
---
nova/flags.py | 3 -
nova/tests/test_iscsi.py | 116 +++++++++++++++++++++++++++++++++
nova/tests/test_volume.py | 17 ++---
nova/volume/driver.py | 131 ++++++++------------------------------
nova/volume/iscsi.py | 156 +++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 304 insertions(+), 119 deletions(-)
create mode 100644 nova/tests/test_iscsi.py
create mode 100644 nova/volume/iscsi.py
diff --git a/nova/flags.py b/nova/flags.py
index 2280222..792407b 100644
--- a/nova/flags.py
+++ b/nova/flags.py
@@ -416,9 +416,6 @@ DEFINE_string('root_helper', 'sudo',
DEFINE_string('network_driver', 'nova.network.linux_net',
'Driver to use for network creation')
-DEFINE_string('iscsi_helper', 'ietadm',
- 'iscsi target user-land tool to use')
-
DEFINE_bool('use_ipv6', False, 'use ipv6')
DEFINE_bool('monkey_patch', False,
diff --git a/nova/tests/test_iscsi.py b/nova/tests/test_iscsi.py
new file mode 100644
index 0000000..d7aed0f
--- /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 8a80aeb..7c9a978 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()
@@ -438,13 +438,10 @@ class ISCSITestCase(DriverTestCase):
"""No log message when all the vblade 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("%s" % FLAGS.iscsi_helper,
- "--op", "show",
- "--tid=%(tid)d" % locals(),
- run_as_root=True)
+ self.volume.driver.tgtadm.show_target(tid)
self.stream.truncate(0)
self.mox.ReplayAll()
@@ -461,11 +458,9 @@ class ISCSITestCase(DriverTestCase):
# the first vblade process isn't running
tid = db.volume_get_iscsi_target_num(self.context, volume_id_list[0])
- self.mox.StubOutWithMock(self.volume.driver, '_execute')
- self.volume.driver._execute("%s" % FLAGS.iscsi_helper, "--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 67ea423..82f0106 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
@@ -63,6 +64,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):
@@ -353,6 +357,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:
@@ -365,40 +377,10 @@ class ISCSIDriver(VolumeDriver):
iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name'])
volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name'])
- if FLAGS.iscsi_helper == 'tgtadm':
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'new',
- '--lld=iscsi', '--mode=target',
- "--tid=%s" % iscsi_target,
- "--targetname=%s" % iscsi_name,
- run_as_root=True,
- check_exit_code=False)
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'bind'
- '--lld=iscsi', '--mode=target',
- '--initiator-address=ALL',
- "--tid=%s" % iscsi_target,
- run_as_root=True,
- check_exit_code=False)
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'new',
- '--lld=iscsi', '--mode=logicalunit',
- "--tid=%s" % iscsi_target,
- '--lun=1',
- "--backing-store=%s,Type=fileio" % volume_path,
- run_as_root=True,
- check_exit_code=False)
- else:
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'new',
- "--tid=%s" % iscsi_target,
- '--params',
- "Name=%s" % iscsi_name,
- run_as_root=True,
- check_exit_code=False)
- self._execute('%s' % FLAGS.iscsi_helper, '--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."""
@@ -418,33 +400,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'])
- if FLAGS.iscsi_helper == 'tgtadm':
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'new',
- '--lld=iscsi', '--mode=target',
- '--tid=%s' % iscsi_target,
- '--params', 'Name=%s' % iscsi_name,
- run_as_root=True)
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'bind',
- '--lld=iscsi', '--mode=target',
- '--initiator-address=ALL',
- "--tid=%s" % iscsi_target,
- run_as_root=True)
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'new',
- '--ld=iscsi', '--mode=logicalunit',
- '--tid=%s' % iscsi_target,
- '--lun=0', '--params',
- 'Path=%s,Type=fileio' % volume_path,
- run_as_root=True)
- else:
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'new',
- '--tid=%s' % iscsi_target,
- '--params', 'Name=%s' % iscsi_name,
- run_as_root=True)
- self._execute('%s' % FLAGS.iscsi_helper, '--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."""
@@ -459,38 +417,14 @@ class ISCSIDriver(VolumeDriver):
try:
# ietadm show will exit with an error
# this export has already been removed
- if FLAGS.iscsi_helper == 'tgtadm':
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'show',
- '--lld=iscsi', '--mode=target',
- '--tid=%s' % iscsi_target,
- run_as_root=True)
- else:
- self._execute('%s' % FLAGS.iscsi_helper, '--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
- if FLAGS.iscsi_helper == 'tgtadm':
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'delete',
- '--lld=iscsi', '--mode=logicalunit',
- '--tid=%s' % iscsi_target,
- '--lun=1',
- run_as_root=True)
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'delete',
- '--lld=iscsi', '--mode=target',
- '--tid=%s' % iscsi_target,
- run_as_root=True)
- else:
- self._execute('%s' % FLAGS.iscsi_helper, '--op' 'delete',
- '--tid=%s' % iscsi_target,
- '--lun=0',
- run_as_root=True)
- self._execute('%s' % FLAGS.iscsi_helper, '--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
@@ -599,14 +533,9 @@ class ISCSIDriver(VolumeDriver):
self._iscsiadm_update(iscsi_properties, "node.startup", "automatic")
- if FLAGS.iscsi_helper == 'tgtadm':
- mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
- (iscsi_properties['target_portal'],
- iscsi_properties['target_iqn']))
- else:
- mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
- (iscsi_properties['target_portal'],
- iscsi_properties['target_iqn']))
+ mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
+ (iscsi_properties['target_portal'],
+ iscsi_properties['target_iqn']))
# The /dev/disk/by-path/... node is not always present immediately
# TODO(justinsb): This retry-with-delay is a pattern, move to utils?
@@ -646,15 +575,7 @@ class ISCSIDriver(VolumeDriver):
tid = self.db.volume_get_iscsi_target_num(context, volume_id)
try:
- if FLAGS.iscsi_helper == 'tgtadm':
- self._execute('%s' % FLAGS.iscsi_helper, '--op', 'show'
- '--lld=iscsi', '--mode=target',
- '--tid=%(tid)d' % locals(),
- run_as_root=True)
- else:
- self._execute('%s' % FLAGS.iscsi_helper, '--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 0000000..d50dd5f
--- /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()
