Merge "Fix creation of iscsi targets"

6 files changed, 203 insertions, 140 deletions

diff --git a/nova/exception.py b/nova/exception.py
index 6a418af33..cd1eabc9d 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -425,6 +425,10 @@ class VirtDriverNotFound(NotFound):
     message = _("Could not find driver for connection_type %(name)s")
 
 
+class PersistentVolumeFileNotFound(NotFound):
+    message = _("Volume %(volume_id)s persistence file could not be found.")
+
+
 class VolumeNotFound(NotFound):
     message = _("Volume %(volume_id)s could not be found.")
 
@@ -470,6 +474,14 @@ class ISCSITargetNotFoundForVolume(NotFound):
     message = _("No target id found for volume %(volume_id)s.")
 
 
+class ISCSITargetCreateFailed(NovaException):
+    message = _("Failed to create iscsi target for volume %(volume_id)s.")
+
+
+class ISCSITargetRemoveFailed(NovaException):
+    message = _("Failed to remove iscsi target for volume %(volume_id)s.")
+
+
 class DiskNotFound(NotFound):
     message = _("No disk at %(location)s")
 
diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py
index a23ffaea0..ff84def77 100644
--- a/nova/tests/api/ec2/test_cloud.py
+++ b/nova/tests/api/ec2/test_cloud.py
@@ -46,6 +46,7 @@ from nova.tests import fake_network
 from nova.tests.image import fake
 from nova import utils
 from nova.virt import fake as fake_virt
+from nova.volume import iscsi
 
 
 LOG = logging.getLogger(__name__)
@@ -97,6 +98,7 @@ class CloudTestCase(test.TestCase):
         vol_tmpdir = tempfile.mkdtemp()
         self.flags(compute_driver='nova.virt.fake.FakeDriver',
                    volumes_dir=vol_tmpdir)
+        self.stubs.Set(iscsi.TgtAdm, '_get_target', self.fake_get_target)
 
         def fake_show(meh, context, id):
             return {'id': id,
@@ -158,6 +160,9 @@ class CloudTestCase(test.TestCase):
         super(CloudTestCase, self).tearDown()
         fake.FakeImageService_reset()
 
+    def fake_get_target(obj, iqn):
+        return 1
+
     def _stub_instance_get_with_fixed_ips(self, func_name):
         orig_func = getattr(self.cloud.compute_api, func_name)
 
@@ -1993,9 +1998,12 @@ class CloudTestCase(test.TestCase):
         self.assertTrue(result)
 
     def _volume_create(self, volume_id=None):
+        location = '10.0.2.15:3260'
+        iqn = 'iqn.2010-10.org.openstack:%s' % volume_id
         kwargs = {'status': 'available',
                   'host': self.volume.host,
                   'size': 1,
+                  'provider_location': '1 %s,fake %s' % (location, iqn),
                   'attach_status': 'detached', }
         if volume_id:
             kwargs['id'] = volume_id
diff --git a/nova/tests/test_iscsi.py b/nova/tests/test_iscsi.py
index b88bd3fce..d375313e2 100644
--- a/nova/tests/test_iscsi.py
+++ b/nova/tests/test_iscsi.py
@@ -35,6 +35,10 @@ class TargetAdminTestCase(object):
         self.script_template = None
         self.stubs.Set(os.path, 'isfile', lambda _: True)
         self.stubs.Set(os, 'unlink', lambda _: '')
+        self.stubs.Set(iscsi.TgtAdm, '_get_target', self.fake_get_target)
+
+    def fake_get_target(obj, iqn):
+        return 1
 
     def get_script_params(self):
         return {'tid': self.tid,
@@ -71,7 +75,7 @@ class TargetAdminTestCase(object):
         tgtadm.set_execute(self.fake_execute)
         tgtadm.create_iscsi_target(self.target_name, self.tid,
                 self.lun, self.path)
-        tgtadm.show_target(self.tid)
+        tgtadm.show_target(self.tid, iqn=self.target_name)
         tgtadm.remove_iscsi_target(self.tid, self.lun, self.vol_id)
 
     def test_target_admin(self):
@@ -88,9 +92,8 @@ class TgtAdmTestCase(test.TestCase, TargetAdminTestCase):
         self.flags(iscsi_helper='tgtadm')
         self.flags(volumes_dir="./")
         self.script_template = "\n".join([
-        "tgt-admin --execute --conf ./blaa --update blaa",
-        "tgtadm --op show --lld=iscsi --mode=target --tid=1",
-        "tgt-admin --delete iqn.2010-10.org.openstack:volume-blaa"])
+        'tgt-admin --update iqn.2011-09.org.foo.bar:blaa',
+        'tgt-admin --delete iqn.2010-10.org.openstack:volume-blaa'])
 
 
 class IetAdmTestCase(test.TestCase, TargetAdminTestCase):
@@ -100,9 +103,9 @@ class IetAdmTestCase(test.TestCase, TargetAdminTestCase):
         TargetAdminTestCase.setUp(self)
         self.flags(iscsi_helper='ietadm')
         self.script_template = "\n".join([
-        "ietadm --op new --tid=%(tid)s --params Name=%(target_name)s",
-        "ietadm --op new --tid=%(tid)s --lun=%(lun)s "
-                "--params Path=%(path)s,Type=fileio",
-        "ietadm --op show --tid=%(tid)s",
-        "ietadm --op delete --tid=%(tid)s",
-        "ietadm --op delete --tid=%(tid)s --lun=%(lun)s"])
+        'ietadm --op new --tid=%(tid)s --params Name=%(target_name)s',
+        'ietadm --op new --tid=%(tid)s --lun=%(lun)s '
+                '--params Path=%(path)s,Type=fileio',
+        'ietadm --op show --tid=%(tid)s',
+        'ietadm --op delete --tid=%(tid)s',
+        'ietadm --op delete --tid=%(tid)s --lun=%(lun)s'])
diff --git a/nova/tests/test_volume.py b/nova/tests/test_volume.py
index 56ee53ee6..44735bf7c 100644
--- a/nova/tests/test_volume.py
+++ b/nova/tests/test_volume.py
@@ -37,7 +37,7 @@ from nova.openstack.common import rpc
 import nova.policy
 from nova import quota
 from nova import test
-import nova.volume.api
+from nova.volume import iscsi
 
 QUOTAS = quota.QUOTAS
 FLAGS = flags.FLAGS
@@ -54,6 +54,7 @@ class VolumeTestCase(test.TestCase):
                    volumes_dir=vol_tmpdir)
         self.stubs.Set(nova.flags.FLAGS, 'notification_driver',
                 ['nova.openstack.common.notifier.test_notifier'])
+        self.stubs.Set(iscsi.TgtAdm, '_get_target', self.fake_get_target)
         self.volume = importutils.import_object(FLAGS.volume_manager)
         self.context = context.get_admin_context()
         instance = db.instance_create(self.context, {})
@@ -70,6 +71,9 @@ class VolumeTestCase(test.TestCase):
         notifier_api._reset_drivers()
         super(VolumeTestCase, self).tearDown()
 
+    def fake_get_target(obj, iqn):
+        return 1
+
     @staticmethod
     def _create_volume(size=0, snapshot_id=None):
         """Create a volume object."""
@@ -214,23 +218,6 @@ class VolumeTestCase(test.TestCase):
         except TypeError:
             pass
 
-    def test_too_many_volumes(self):
-        """Ensure that NoMoreTargets is raised when we run out of volumes."""
-        vols = []
-        total_slots = FLAGS.iscsi_num_targets
-        for _index in xrange(total_slots):
-            volume = self._create_volume()
-            self.volume.create_volume(self.context, volume['id'])
-            vols.append(volume['id'])
-        volume = self._create_volume()
-        self.assertRaises(db.NoMoreTargets,
-                          self.volume.create_volume,
-                          self.context,
-                          volume['id'])
-        db.volume_destroy(context.get_admin_context(), volume['id'])
-        for volume_id in vols:
-            self.volume.delete_volume(self.context, volume_id)
-
     def test_run_attach_detach_volume(self):
         """Make sure volume can be attached and detached from instance."""
         inst = {}
@@ -295,11 +282,10 @@ class VolumeTestCase(test.TestCase):
                                                           volume_id)
             self.assert_(iscsi_target not in targets)
             targets.append(iscsi_target)
+
         total_slots = FLAGS.iscsi_num_targets
         for _index in xrange(total_slots):
-            volume = self._create_volume()
-            d = self.volume.create_volume(self.context, volume['id'])
-            _check(d)
+            self._create_volume()
         for volume_id in volume_ids:
             self.volume.delete_volume(self.context, volume_id)
 
@@ -518,6 +504,7 @@ class DriverTestCase(test.TestCase):
         self.volume = importutils.import_object(FLAGS.volume_manager)
         self.context = context.get_admin_context()
         self.output = ""
+        self.stubs.Set(iscsi.TgtAdm, '_get_target', self.fake_get_target)
 
         def _fake_execute(_command, *_args, **_kwargs):
             """Fake _execute."""
@@ -535,6 +522,9 @@ class DriverTestCase(test.TestCase):
             pass
         super(DriverTestCase, self).tearDown()
 
+    def fake_get_target(obj, iqn):
+        return 1
+
     def _attach_volume(self):
         """Attach volumes to an instance."""
         return []
@@ -593,39 +583,6 @@ class ISCSITestCase(DriverTestCase):
     def test_check_for_export_with_no_volume(self):
         self.volume.check_for_export(self.context, self.instance_id)
 
-    def test_check_for_export_with_all_volume_exported(self):
-        volume_id_list = self._attach_volume()
-
-        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.tgtadm.show_target(tid)
-
-        self.mox.ReplayAll()
-        self.volume.check_for_export(self.context, self.instance_id)
-        self.mox.UnsetStubs()
-
-        self._detach_volume(volume_id_list)
-
-    def test_check_for_export_with_some_volume_missing(self):
-        """Output a warning message when some volumes are not recognied
-           by ietd."""
-        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.tgtadm, 'show_target')
-        self.volume.driver.tgtadm.show_target(tid).AndRaise(
-            exception.ProcessExecutionError())
-
-        self.mox.ReplayAll()
-        self.assertRaises(exception.ProcessExecutionError,
-                          self.volume.check_for_export,
-                          self.context,
-                          self.instance_id)
-        self.mox.UnsetStubs()
-
-        self._detach_volume(volume_id_list)
-
 
 class VolumePolicyTestCase(test.TestCase):
 
diff --git a/nova/volume/driver.py b/nova/volume/driver.py
index acbf65a3e..dee0df953 100644
--- a/nova/volume/driver.py
+++ b/nova/volume/driver.py
@@ -20,6 +20,7 @@ Drivers for volumes.
 
 """
 
+import os
 import time
 
 from nova import exception
@@ -263,65 +264,103 @@ class ISCSIDriver(VolumeDriver):
 
     def ensure_export(self, context, volume):
         """Synchronously recreates an export for a logical volume."""
-        try:
-            iscsi_target = self.db.volume_get_iscsi_target_num(context,
-                                                           volume['id'])
-        except exception.NotFound:
-            LOG.info(_("Skipping ensure_export. No iscsi_target "
-                       "provisioned for volume: %s"), volume['id'])
-            return
+        # NOTE(jdg): tgtadm doesn't use the iscsi_targets table
+        # TODO(jdg): In the future move all of the dependent stuff into the
+        # cooresponding target admin class
+        if not isinstance(self.tgtadm, iscsi.TgtAdm):
+            try:
+                iscsi_target = self.db.volume_get_iscsi_target_num(context,
+                                                               volume['id'])
+            except exception.NotFound:
+                LOG.info(_("Skipping ensure_export. No iscsi_target "
+                           "provisioned for volume: %s"), volume['id'])
+                return
+        else:
+            iscsi_target = 1  # dummy value when using TgtAdm
 
         iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name'])
         volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name'])
 
+        # NOTE(jdg): For TgtAdm case iscsi_name is the ONLY param we need
+        # should clean this all up at some point in the future
         self.tgtadm.create_iscsi_target(iscsi_name, iscsi_target,
-                0, volume_path, check_exit_code=False)
+                                        0, volume_path,
+                                        check_exit_code=False)
 
     def _ensure_iscsi_targets(self, context, host):
         """Ensure that target ids have been created in datastore."""
-        host_iscsi_targets = self.db.iscsi_target_count_by_host(context, host)
-        if host_iscsi_targets >= FLAGS.iscsi_num_targets:
-            return
-        # NOTE(vish): Target ids start at 1, not 0.
-        for target_num in xrange(1, FLAGS.iscsi_num_targets + 1):
-            target = {'host': host, 'target_num': target_num}
-            self.db.iscsi_target_create_safe(context, target)
+        # NOTE(jdg): tgtadm doesn't use the iscsi_targets table
+        # TODO(jdg): In the future move all of the dependent stuff into the
+        # cooresponding target admin class
+        if not isinstance(self.tgtadm, iscsi.TgtAdm):
+            host_iscsi_targets = self.db.iscsi_target_count_by_host(context,
+                                                                    host)
+            if host_iscsi_targets >= FLAGS.iscsi_num_targets:
+                return
+
+            # NOTE(vish): Target ids start at 1, not 0.
+            for target_num in xrange(1, FLAGS.iscsi_num_targets + 1):
+                target = {'host': host, 'target_num': target_num}
+                self.db.iscsi_target_create_safe(context, target)
 
     def create_export(self, context, volume):
         """Creates an export for a logical volume."""
-        self._ensure_iscsi_targets(context, volume['host'])
-        iscsi_target = self.db.volume_allocate_iscsi_target(context,
-                                                      volume['id'],
-                                                      volume['host'])
+        #BOOKMARK(jdg)
+
         iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name'])
         volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name'])
 
-        self.tgtadm.create_iscsi_target(iscsi_name, iscsi_target,
-                0, volume_path)
-
         model_update = {}
-        if FLAGS.iscsi_helper == 'tgtadm':
-            lun = 1
-        else:
+
+        # TODO(jdg): In the future move all of the dependent stuff into the
+        # cooresponding target admin class
+        if not isinstance(self.tgtadm, iscsi.TgtAdm):
             lun = 0
+            self._ensure_iscsi_targets(context, volume['host'])
+            iscsi_target = self.db.volume_allocate_iscsi_target(context,
+                                                                volume['id'],
+                                                                volume['host'])
+        else:
+            lun = 1  # For tgtadm the controller is lun 0, dev starts at lun 1
+            iscsi_target = 0  # NOTE(jdg): Not used by tgtadm
+
+        # 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.tgtadm.create_iscsi_target(iscsi_name,
+                                              iscsi_target,
+                                              0,
+                                              volume_path)
         model_update['provider_location'] = _iscsi_location(
-            FLAGS.iscsi_ip_address, iscsi_target, iscsi_name, lun)
+            FLAGS.iscsi_ip_address, tid, iscsi_name, lun)
         return model_update
 
     def remove_export(self, context, volume):
         """Removes an export for a logical volume."""
-        try:
-            iscsi_target = self.db.volume_get_iscsi_target_num(context,
-                                                           volume['id'])
-        except exception.NotFound:
-            LOG.info(_("Skipping remove_export. No iscsi_target "
-                       "provisioned for volume: %s"), volume['id'])
+        #BOOKMARK jdg
+        location = volume['provider_location'].split(' ')
+        iqn = location[1]
+        if 'iqn' not in iqn:
+            LOG.warning(_("Jacked... didn't get an iqn"))
+            return
+
+        # NOTE(jdg): tgtadm doesn't use the iscsi_targets table
+        # TODO(jdg): In the future move all of the dependent stuff into the
+        # cooresponding target admin class
+        if not isinstance(self.tgtadm, iscsi.TgtAdm):
+            try:
+                iscsi_target = self.db.volume_get_iscsi_target_num(context,
+                                                               volume['id'])
+            except exception.NotFound:
+                LOG.info(_("Skipping remove_export. No iscsi_target "
+                           "provisioned for volume: %s"), volume['id'])
             return
+        else:
+            iscsi_target = 0
 
         try:
             # ietadm show will exit with an error
             # this export has already been removed
-            self.tgtadm.show_target(iscsi_target)
+            self.tgtadm.show_target(iscsi_target, iqn=iqn)
         except Exception as e:
             LOG.info(_("Skipping remove_export. No iscsi_target "
                        "is presently exported for volume: %s"), volume['id'])
@@ -453,10 +492,23 @@ class ISCSIDriver(VolumeDriver):
 
     def check_for_export(self, context, volume_id):
         """Make sure volume is exported."""
+        vol_uuid_file = 'volume-%s' % volume_id
+        volume_path = os.path.join(FLAGS.volumes_dir, vol_uuid_file)
+        if os.path.isfile(volume_path):
+            iqn = '%s%s' % (FLAGS.iscsi_target_prefix,
+                            vol_uuid_file)
+        else:
+            raise exception.PersistentVolumeFileNotFound(volume_id=volume_id)
+
+        # TODO(jdg): In the future move all of the dependent stuff into the
+        # cooresponding target admin class
+        if not isinstance(self.tgtadm, iscsi.TgtAdm):
+            tid = self.db.volume_get_iscsi_target_num(context, volume_id)
+        else:
+            tid = 0
 
-        tid = self.db.volume_get_iscsi_target_num(context, volume_id)
         try:
-            self.tgtadm.show_target(tid)
+            self.tgtadm.show_target(tid, iqn=iqn)
         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
index b08a8032b..ccc768827 100644
--- a/nova/volume/iscsi.py
+++ b/nova/volume/iscsi.py
@@ -94,58 +94,88 @@ class TgtAdm(TargetAdmin):
     def __init__(self, execute=utils.execute):
         super(TgtAdm, self).__init__('tgtadm', execute)
 
+    def _get_target(self, iqn):
+        (out, err) = self._execute('tgt-admin', '--show', run_as_root=True)
+        lines = out.split('\n')
+        for line in lines:
+            if iqn in line:
+                parsed = line.split()
+                tid = parsed[1]
+                return tid[:-1]
+
+        return None
+
     def create_iscsi_target(self, name, tid, lun, path, **kwargs):
-        try:
-            utils.ensure_tree(FLAGS.volumes_dir)
+        # Note(jdg) tid and lun aren't used by TgtAdm but remain for
+        # compatability
+
+        utils.ensure_tree(FLAGS.volumes_dir)
 
-            # grab the volume id
-            vol_id = name.split(':')[1]
+        vol_id = name.split(':')[1]
+        volume_conf = """
+            <target %s>
+                backing-store %s
+            </target>
+        """ % (name, path)
 
-            volume_conf = """
-                <target %s>
-                    backing-store %s
-                </target>
-            """ % (name, path)
+        LOG.info(_('Creating volume: %s') % vol_id)
+        volume_path = os.path.join(FLAGS.volumes_dir, vol_id)
+
+        f = open(volume_path, 'w+')
+        f.write(volume_conf)
+        f.close()
+
+        try:
+            (out, err) = self._execute('tgt-admin',
+                                       '--update',
+                                       name,
+                                       run_as_root=True)
+        except exception.ProcessExecutionError, e:
+            LOG.error(_("Failed to create iscsi target for volume "
+                        "id:%(volume_id)s.") % locals())
 
-            LOG.info(_('Creating volume: %s') % vol_id)
-            volume_path = os.path.join(FLAGS.volumes_dir, vol_id)
-            if not os.path.isfile(volume_path):
-                f = open(volume_path, 'w+')
-                f.write(volume_conf)
-                f.close()
+            #Dont forget to remove the persistent file we created
+            os.unlink(volume_path)
+            raise exception.ISCSITargetCreateFailed(volume_id=vol_id)
 
-            self._execute('tgt-admin', '--execute',
-                          '--conf', volume_path,
-                          '--update', vol_id, run_as_root=True)
+        iqn = '%s%s' % (FLAGS.iscsi_target_prefix, vol_id)
+        tid = self._get_target(iqn)
+        if tid is None:
+            raise exception.NotFound()
 
-        except Exception as ex:
-            LOG.exception(ex)
-            raise exception.NovaException(_('Failed to create volume: %s')
-                % vol_id)
+        return tid
 
     def remove_iscsi_target(self, tid, lun, vol_id, **kwargs):
+        LOG.info(_('Removing volume: %s') % vol_id)
+        vol_uuid_file = 'volume-%s' % vol_id
+        volume_path = os.path.join(FLAGS.volumes_dir, vol_uuid_file)
+        if os.path.isfile(volume_path):
+            iqn = '%s%s' % (FLAGS.iscsi_target_prefix,
+                            vol_uuid_file)
+        else:
+            raise exception.ISCSITargetRemoveFailed(volume_id=vol_id)
         try:
-            LOG.info(_('Removing volume: %s') % vol_id)
-            vol_uuid_file = 'volume-%s' % vol_id
-            volume_path = os.path.join(FLAGS.volumes_dir, vol_uuid_file)
-            if os.path.isfile(volume_path):
-                delete_file = '%s%s' % (FLAGS.iscsi_target_prefix,
-                                        vol_uuid_file)
-                self._execute('tgt-admin',
-                              '--delete',
-                              delete_file,
-                              run_as_root=True)
-                os.unlink(volume_path)
-        except Exception as ex:
-            LOG.exception(ex)
-            raise exception.NovaException(_('Failed to remove volume: %s')
-                    % vol_id)
+            self._execute('tgt-admin',
+                          '--delete',
+                          iqn,
+                          run_as_root=True)
+        except exception.ProcessExecutionError, e:
+            LOG.error(_("Failed to create iscsi target for volume "
+                        "id:%(volume_id)s.") % locals())
+            raise exception.ISCSITargetRemoveFailed(volume_id=vol_id)
+
+        os.unlink(volume_path)
 
     def show_target(self, tid, **kwargs):
-        self._run('--op', 'show',
-                  '--lld=iscsi', '--mode=target',
-                  '--tid=%s' % tid,
-                  **kwargs)
+        iqn = kwargs.get('iqn', None)
+
+        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()
 
 
 class IetAdm(TargetAdmin):
@@ -157,6 +187,7 @@ class IetAdm(TargetAdmin):
     def create_iscsi_target(self, name, tid, lun, path, **kwargs):
         self._new_target(name, tid, **kwargs)
         self._new_logicalunit(tid, lun, path, **kwargs)
+        return tid
 
     def remove_iscsi_target(self, tid, lun, vol_id, **kwargs):
         LOG.info(_('Removing volume: %s') % vol_id)