Merge lp:~markmc/nova/iscsi-tgtadm-choice into lp:~hudson-openstack/nova/trunk

[Mark McLoughlin]

Add --iscsi_helper flag and support tgtadm (lp:819997)

The iSCSI enterprise target project (aka iet) never went into the upstream kernel and never got into Fedora.

On the other hand, the tgt project did get its kernel module upstream and its userland packaged in Fedora as scsi-target-utils. It seems that tgt is the preferred option for Ubuntu too.

Nova upstream uses ietadm from the former, but since this isn't available in Fedora we need the option of using tgtadm instead.

This merge adds an --iscsi_helper flag which can be set to ietadm or tgtadm. The former is used by default to preserve the current behaviour.

[Chuck Short (zulcss)]

lgtm

[Christopher MacGown (0x44)]

lgtm, also kudos for clean and understandable tests.

review: Approve

[Mark McLoughlin (markmc)]

Okay, closing this because it's in gerrit now:

https://review.openstack.org/#q,status:open+project:openstack/nova+branch:master+topic:iscsi-tgtadm-choice,n,z

Unmerged revisions

1422. By Mark McLoughlin on 2011-09-18

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.

1421. By Mark McLoughlin on 2011-09-18

Remove VolumeDriver.sync_exec method

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.

1420. By Mark McLoughlin on 2011-09-17

Fix pep8 issues

1419. By Mark McLoughlin on 2011-09-17

Merge trunk

1418. By Chuck Short on 2011-08-11

Fix pep8 errors

1417. By Chuck Short on 2011-08-11

Fixup thinkos

1416. By Chuck Short on 2011-08-11

Allow the user to choose either ietadm or tgtadm

=== added file 'nova/tests/test_iscsi.py'
--- nova/tests/test_iscsi.py	1970-01-01 00:00:00 +0000
+++ nova/tests/test_iscsi.py	2011-09-18 15:48:23 +0000
@@ -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
+"""

=== modified file 'nova/tests/test_volume.py'
--- nova/tests/test_volume.py	2011-08-05 14:23:48 +0000
+++ nova/tests/test_volume.py	2011-09-18 15:48:23 +0000
@@ -270,8 +270,7 @@
         def _fake_execute(_command, *_args, **_kwargs):
             """Fake _execute."""
             return self.output, None
-        self.volume.driver._execute = _fake_execute
-        self.volume.driver._sync_execute = _fake_execute
+        self.volume.driver.set_execute(_fake_execute)
 
         log = logging.getLogger()
         self.stream = cStringIO.StringIO()
@@ -411,12 +410,10 @@
         """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("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()
@@ -433,11 +430,9 @@
 
         # 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("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,

=== modified file 'nova/volume/driver.py'
--- nova/volume/driver.py	2011-09-13 21:32:24 +0000
+++ nova/volume/driver.py	2011-09-18 15:48:23 +0000
@@ -28,6 +28,7 @@
 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
 
 
@@ -60,12 +61,13 @@
 
 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.set_execute(execute)
+
+    def set_execute(self, execute):
         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
@@ -328,7 +330,6 @@
 
     def __init__(self, *args, **kwargs):
         super(FakeAOEDriver, self).__init__(execute=self.fake_execute,
-                                            sync_exec=self.fake_execute,
                                             *args, **kwargs)
 
     def check_for_setup_error(self):
@@ -356,6 +357,14 @@
                        `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:
@@ -368,19 +377,10 @@
 
         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.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."""
@@ -400,13 +400,9 @@
                                                       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."""
@@ -421,18 +417,14 @@
         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
@@ -583,8 +575,7 @@
 
         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
@@ -598,7 +589,6 @@
     """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):
@@ -870,14 +860,14 @@
                 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
@@ -895,11 +885,11 @@
             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
@@ -980,12 +970,12 @@
             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
@@ -1010,13 +1000,12 @@
         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")
@@ -1037,11 +1026,10 @@
     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 {}

=== added file 'nova/volume/iscsi.py'
--- nova/volume/iscsi.py	1970-01-01 00:00:00 +0000
+++ nova/volume/iscsi.py	2011-09-18 15:48:23 +0000
@@ -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()