From e3d7f8c7de9bad73bf1f9b5ee9b2cf46eb452351 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Mon, 29 Oct 2012 17:57:40 -0600 Subject: [PATCH] Detect and fix issues caused by vol ID migration The migration from volume ID to UUID neglected to update the provider_location field on the volume. As a result the iqn and volume name no long match and existing volumes are no longer able to be attached after an upgrade (essex -> folsom and then nova-vol->cinder). This patch adds a method to the volume driver that will check for the mismatch of volume name in the iqn during service start up. If detected it will update the provider_location field in the database to include the new ID. Also it will create a symlink to the device backing file that also has the correct naming convention. Note: We don't disturb an connections that are currently attached. For this case we add a check in manager.detach and do any provider_location cleanup that's needed at that time. This ensures that connections persist on restarts of tgtd and reboot. Change-Id: Ib41ebdc849ebc31a9225bdc4902209c5a23da104 Fixes: bug 1065702 --- etc/nova/rootwrap.d/volume.filters | 1 + nova/tests/api/ec2/test_cloud.py | 24 +++++++++-- nova/volume/driver.py | 66 ++++++++++++++++++++++++++++-- nova/volume/iscsi.py | 10 ++++- nova/volume/manager.py | 7 +++- 5 files changed, 98 insertions(+), 10 deletions(-) diff --git a/etc/nova/rootwrap.d/volume.filters b/etc/nova/rootwrap.d/volume.filters index 574fef7c251..98877ef0160 100644 --- a/etc/nova/rootwrap.d/volume.filters +++ b/etc/nova/rootwrap.d/volume.filters @@ -34,3 +34,4 @@ dmsetup_usr: CommandFilter, /usr/sbin/dmsetup, root #nova/volume/.py: utils.temporary_chown(path, 0), ... chown: CommandFilter, /bin/chown, root +ln: CommandFilter, /bin/ln, root \ No newline at end of file diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py index 9794090776a..459c17143dc 100644 --- a/nova/tests/api/ec2/test_cloud.py +++ b/nova/tests/api/ec2/test_cloud.py @@ -101,6 +101,8 @@ def setUp(self): self.stubs.Set(iscsi.TgtAdm, '_get_target', self.fake_get_target) self.stubs.Set(iscsi.TgtAdm, 'remove_iscsi_target', self.fake_remove_iscsi_target) + self.stubs.Set(iscsi.TgtAdm, 'create_iscsi_target', + self.fake_create_iscsi_target) def fake_show(meh, context, id): return {'id': id, @@ -168,6 +170,9 @@ def fake_get_target(obj, iqn): def fake_remove_iscsi_target(obj, tid, lun, vol_id, **kwargs): pass + def fake_create_iscsi_target(obj, name, tid, lun, path, **kwargs): + pass + def _stub_instance_get_with_fixed_ips(self, func_name): orig_func = getattr(self.cloud.compute_api, func_name) @@ -1203,8 +1208,8 @@ def fake_detail(meh, context, **kwargs): 'name': 'fake_name', 'container_format': 'ami', 'properties': { - 'kernel_id': 'cedef40a-ed67-4d10-800e-17455edce175', - 'ramdisk_id': 'cedef40a-ed67-4d10-800e-17455edce175', + 'kernel_id': 'cedef40a-ed67-4d10-800e-17455edce175', + 'ramdisk_id': 'cedef40a-ed67-4d10-800e-17455edce175', 'type': 'machine'}}] def fake_show_none(meh, context, id): @@ -2036,15 +2041,24 @@ def test_reboot_instances(self): def _volume_create(self, volume_id=None): location = '10.0.2.15:3260' - iqn = 'iqn.2010-10.org.openstack:%s' % volume_id + iqn = 'iqn.2010-10.org.openstack:volume-%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 - return db.volume_create(self.context, kwargs) + + volume_ref = db.volume_create(self.context, kwargs) + if volume_id is None: + model_update = {} + model_update['provider_location'] =\ + volume_ref['provider_location'].replace('volume-None', + volume_ref['name']) + db.volume_update(self.context, volume_ref['id'], model_update) + return volume_ref def _assert_volume_attached(self, vol, instance_uuid, mountpoint): self.assertEqual(vol['instance_uuid'], instance_uuid) @@ -2066,6 +2080,7 @@ def test_stop_start_with_volume(self): vol1 = self._volume_create() vol2 = self._volume_create() + kwargs = {'image_id': 'ami-1', 'instance_type': FLAGS.default_instance_type, 'max_count': 1, @@ -2132,6 +2147,7 @@ def test_stop_with_attached_volume(self): vol1 = self._volume_create() vol2 = self._volume_create() + kwargs = {'image_id': 'ami-1', 'instance_type': FLAGS.default_instance_type, 'max_count': 1, diff --git a/nova/volume/driver.py b/nova/volume/driver.py index 0781d65f99a..0149bea9332 100644 --- a/nova/volume/driver.py +++ b/nova/volume/driver.py @@ -21,6 +21,7 @@ """ import os +import re import tempfile import time import urllib @@ -327,14 +328,72 @@ def ensure_export(self, context, volume): 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']) + # Check for https://bugs.launchpad.net/nova/+bug/1065702 + old_name = None + volume_name = volume['name'] + if (volume['provider_location'] is not None and + volume['name'] not in volume['provider_location']): + + msg = _('Detected inconsistency in provider_location id') + LOG.debug(msg) + old_name = self._fix_id_migration(context, volume) + if 'in-use' in volume['status']: + volume_name = old_name + old_name = None + + 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) + check_exit_code=False, + old_name=old_name) + + def _fix_id_migration(self, context, volume): + """Fix provider_location and dev files to address bug 1065702. + + For volumes that the provider_location has NOT been updated + and are not currently in-use we'll create a new iscsi target + and remove the persist file. + + If the volume is in-use, we'll just stick with the old name + and when detach is called we'll feed back into ensure_export + again if necessary and fix things up then. + + Details at: https://bugs.launchpad.net/nova/+bug/1065702 + """ + + model_update = {} + pattern = re.compile(r":|\s") + fields = pattern.split(volume['provider_location']) + old_name = fields[3] + + volume['provider_location'] = \ + volume['provider_location'].replace(old_name, volume['name']) + model_update['provider_location'] = volume['provider_location'] + + self.db.volume_update(context, volume['id'], model_update) + + start = os.getcwd() + os.chdir('/dev/%s' % FLAGS.volume_group) + + try: + (out, err) = self._execute('readlink', old_name) + except exception.ProcessExecutionError: + link_path = '/dev/%s/%s' % (FLAGS.volume_group, old_name) + LOG.debug(_('Symbolic link %s not found') % link_path) + os.chdir(start) + return + + rel_path = out.rstrip() + self._execute('ln', + '-s', + rel_path, volume['name'], + run_as_root=True) + os.chdir(start) + return old_name def _ensure_iscsi_targets(self, context, host): """Ensure that target ids have been created in datastore.""" @@ -354,7 +413,6 @@ def _ensure_iscsi_targets(self, context, host): def create_export(self, context, volume): """Creates an export for a logical volume.""" - #BOOKMARK(jdg) iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name']) volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name']) diff --git a/nova/volume/iscsi.py b/nova/volume/iscsi.py index 275132ce6e8..c1bbf96f7d7 100644 --- a/nova/volume/iscsi.py +++ b/nova/volume/iscsi.py @@ -126,6 +126,11 @@ def create_iscsi_target(self, name, tid, lun, path, **kwargs): f.write(volume_conf) f.close() + old_persist_file = None + old_name = kwargs.get('old_name', None) + if old_name is not None: + old_persist_file = os.path.join(volumes_dir, old_name) + try: (out, err) = self._execute('tgt-admin', '--update', @@ -147,6 +152,9 @@ def create_iscsi_target(self, name, tid, lun, path, **kwargs): "contains 'include %(volumes_dir)s/*'") % locals()) raise exception.NotFound() + if old_persist_file is not None and os.path.exists(old_persist_file): + os.unlink(old_persist_file) + return tid def remove_iscsi_target(self, tid, lun, vol_id, **kwargs): @@ -164,7 +172,7 @@ def remove_iscsi_target(self, tid, lun, vol_id, **kwargs): iqn, run_as_root=True) except exception.ProcessExecutionError, e: - LOG.error(_("Failed to create iscsi target for volume " + LOG.error(_("Failed to remove iscsi target for volume " "id:%(volume_id)s.") % locals()) raise exception.ISCSITargetRemoveFailed(volume_id=vol_id) diff --git a/nova/volume/manager.py b/nova/volume/manager.py index fafdcd5be54..6d708a7e223 100644 --- a/nova/volume/manager.py +++ b/nova/volume/manager.py @@ -309,7 +309,12 @@ def detach_volume(self, context, volume_id): volume_id, {'status': 'error_detaching'}) - self.db.volume_detached(context, volume_id) + self.db.volume_detached(context.elevated(), volume_id) + + # Check for https://bugs.launchpad.net/nova/+bug/1065702 + volume_ref = self.db.volume_get(context, volume_id) + if volume_ref['name'] not in volume_ref['provider_location']: + self.driver.ensure_export(context, volume_ref) def _copy_image_to_volume(self, context, volume, image_id): """Downloads Glance image to the specified volume. """