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

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

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,

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

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)
 

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. """