openstack/cinder
Code Issues Proposed changes

Fix creation of iscsi targets

Browse Source 
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>
This commit is contained in:
Chuck Short
2012-08-28 15:06:25 -05:00
committed by John Griffith
parent a55430ce28
commit 9785963c84
5 changed files with 197 additions and 149 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
cinder/exception.py
View File

@@ -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")

23
cinder/tests/test_iscsi.py
View File

@@ -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'])

67
cinder/tests/test_volume.py
View File

@@ -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):

123
cinder/volume/driver.py
View File

@@ -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

118
cinder/volume/iscsi.py
View File

@@ -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 = """
<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:
if not os.path.exists(FLAGS.volumes_dir):
os.makedirs(FLAGS.volumes_dir)
(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())
# grab the volume id
vol_id = name.split(':')[1]
#Dont forget to remove the persistent file we created
os.unlink(volume_path)
raise exception.ISCSITargetCreateFailed(volume_id=vol_id)
volume_conf = """
<target %s>
backing-store %s
</target>
""" % (name, path)
iqn = '%s%s' % (FLAGS.iscsi_target_prefix, vol_id)
tid = self._get_target(iqn)
if tid is None:
raise exception.NotFound()
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)
except Exception as ex:
LOG.exception(ex)
raise exception.Error(_('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.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)