Fix creation of iscsi targets

Previously when creating iscsi volumes, we were using

tgt-admin -e -c <config file> --update volume-id

Unfortunately the side affect of this is that tgt-admin
removed other volumes that weren't connected to an iscsi
connector. Which is obvlously not what we want.

In order to fix this we create the targets.conf for the
volume but we call tgt-admin --update icssi qualified name.

We're dropping the use of iscsi_targets table when using TgtAdm.
Compatability for other target admin types is maintained.

Fixes LP: #1038062

Change-Id: I9060a43208df5b79e9b17dadcab8bc0a8eeef55e
Signed-off-by: Chuck Short <chuck.short@canonical.com>

cinder/exception.py

@@ -24,9 +24,6 @@
 
 """
 
-import functools
-import sys
-
 import webob.exc
 
 from cinder.openstack.common import log as logging
@@ -222,6 +219,10 @@ class NotFound(CinderException):
     safe = True
 
 
+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.")
 
@@ -271,6 +272,14 @@ class ISCSITargetNotFoundForVolume(NotFound):
     message = _("No target id found for volume %(volume_id)s.")
 
 
+class ISCSITargetCreateFailed(CinderException):
+    message = _("Failed to create iscsi target for volume %(volume_id)s.")
+
+
+class ISCSITargetRemoveFailed(CinderException):
+    message = _("Failed to remove iscsi target for volume %(volume_id)s.")
+
+
 class DiskNotFound(NotFound):
     message = _("No disk at %(location)s")
 

cinder/tests/test_iscsi.py

@@ -35,6 +35,10 @@ def setUp(self):
         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 @@ def run_commands(self):
         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 @@ def setUp(self):
         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 @@ def setUp(self):
         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'])

cinder/tests/test_volume.py

@@ -38,7 +38,7 @@
 import cinder.policy
 from cinder import quota
 from cinder import test
-import cinder.volume.api
+from cinder.volume import iscsi
 
 FLAGS = flags.FLAGS
 
@@ -55,6 +55,7 @@ def setUp(self):
         self.context = context.get_admin_context()
         self.stubs.Set(cinder.flags.FLAGS, 'notification_driver',
             'cinder.openstack.common.notifier.test_notifier')
+        self.stubs.Set(iscsi.TgtAdm, '_get_target', self.fake_get_target)
         fake_image.stub_out_image_service(self.stubs)
         test_notifier.NOTIFICATIONS = []
 
@@ -65,6 +66,9 @@ def tearDown(self):
             pass
         super(VolumeTestCase, self).tearDown()
 
+    def fake_get_target(obj, iqn):
+        return 1
+
     @staticmethod
     def _create_volume(size='0', snapshot_id=None, image_id=None,
                        metadata=None):
@@ -168,23 +172,6 @@ def test_too_big_volume(self):
         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."""
         instance_uuid = '12345678-1234-5678-1234-567812345678'
@@ -239,11 +226,10 @@ def _check(volume_id):
                                                           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)
 
@@ -638,6 +624,7 @@ def setUp(self):
         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."""
@@ -651,6 +638,9 @@ def tearDown(self):
             pass
         super(DriverTestCase, self).tearDown()
 
+    def fake_get_target(obj, iqn):
+        return 1
+
     def _attach_volume(self):
         """Attach volumes to an instance. """
         return []
@@ -711,41 +701,6 @@ def test_check_for_export_with_no_volume(self):
         instance_uuid = '12345678-1234-5678-1234-567812345678'
         self.volume.check_for_export(self.context, instance_uuid)
 
-    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()
-        instance_uuid = '12345678-1234-5678-1234-567812345678'
-        self.volume.check_for_export(self.context, instance_uuid)
-        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()
-        instance_uuid = '12345678-1234-5678-1234-567812345678'
-
-        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,
-                          instance_uuid)
-        self.mox.UnsetStubs()
-
-        self._detach_volume(volume_id_list)
-
 
 class VolumePolicyTestCase(test.TestCase):
 

cinder/volume/driver.py

@@ -20,6 +20,7 @@
 
 """
 
+import os
 import tempfile
 import time
 import urllib
@@ -297,65 +298,102 @@ def set_execute(self, execute):
 
     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'])
@@ -487,10 +525,23 @@ def terminate_connection(self, volume, connector):
 
     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 cinder-volume