From: Chuck Short Date: Tue, 28 Aug 2012 20:06:25 +0000 (-0500) Subject: Fix creation of iscsi targets X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=9785963c841aff5ea1c159da81f93c2bf62c70c6;p=openstack-build%2Fcinder-build.git Fix creation of iscsi targets Previously when creating iscsi volumes, we were using tgt-admin -e -c --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 --- diff --git a/cinder/exception.py b/cinder/exception.py index f624173d5..cf2490a45 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -24,9 +24,6 @@ SHOULD include dedicated exception logging. """ -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") diff --git a/cinder/tests/test_iscsi.py b/cinder/tests/test_iscsi.py index 84122e0ad..1b358d928 100644 --- a/cinder/tests/test_iscsi.py +++ b/cinder/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/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 45ffa73b9..0b12c0bdc 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -38,7 +38,7 @@ from cinder.openstack.common import rpc 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 @@ class VolumeTestCase(test.TestCase): 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 @@ class VolumeTestCase(test.TestCase): 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 @@ 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.""" instance_uuid = '12345678-1234-5678-1234-567812345678' @@ -239,11 +226,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) @@ -638,6 +624,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.""" @@ -651,6 +638,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 [] @@ -711,41 +701,6 @@ class ISCSITestCase(DriverTestCase): 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): diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index bcc1b3b2f..8c667aeb5 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -20,6 +20,7 @@ Drivers for volumes. """ +import os import tempfile import time import urllib @@ -297,65 +298,102 @@ 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']) @@ -487,10 +525,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 cinder-volume diff --git a/cinder/volume/iscsi.py b/cinder/volume/iscsi.py index 260eb6a58..691314393 100644 --- a/cinder/volume/iscsi.py +++ b/cinder/volume/iscsi.py @@ -94,60 +94,89 @@ 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): + # Note(jdg) tid and lun aren't used by TgtAdm but remain for + # compatability + + if not os.path.exists(FLAGS.volumes_dir): + os.makedirs(FLAGS.volumes_dir) + + vol_id = name.split(':')[1] + volume_conf = """ + + backing-store %s + + """ % (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: - if not os.path.exists(FLAGS.volumes_dir): - os.makedirs(FLAGS.volumes_dir) - - # grab the volume id - vol_id = name.split(':')[1] - - volume_conf = """ - - backing-store %s - - """ % (name, path) - - 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() - - self._execute('tgt-admin', '--execute', - '--conf', volume_path, - '--update', vol_id, - run_as_root=True) + (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()) + + #Dont forget to remove the persistent file we created + os.unlink(volume_path) + raise exception.ISCSITargetCreateFailed(volume_id=vol_id) - except Exception as ex: - LOG.exception(ex) - raise exception.Error(_('Failed to create volume: %s') - % vol_id) + iqn = '%s%s' % (FLAGS.iscsi_target_prefix, vol_id) + tid = self._get_target(iqn) + if tid is None: + raise exception.NotFound() + + 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.Error(_('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): @@ -159,6 +188,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)