From 66f6a9edce3ccd624aba5d2a6bf3362901ed57f7 Mon Sep 17 00:00:00 2001 From: Chuck Short Date: Tue, 28 Aug 2012 19:00:49 -0500 Subject: 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 wasnt 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 also set the tid in the configuration file now as well. Fixes LP: #1038062 Change-Id: I23719390fbfaea5b55389a5c8ebaa8966cc283a8 Signed-off-by: Chuck Short --- nova/exception.py | 12 ++++ nova/tests/api/ec2/test_cloud.py | 8 +++ nova/tests/test_iscsi.py | 23 ++++---- nova/tests/test_volume.py | 65 ++++----------------- nova/volume/driver.py | 122 ++++++++++++++++++++++++++++----------- nova/volume/iscsi.py | 113 +++++++++++++++++++++++------------- 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 b085b9ab1..18071b193 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 = """ + + backing-store %s + + """ % (name, path) - 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: + (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) -- cgit