Refactors handling of detach volume

 * removes unnecessary flags in detach_volume call
 * stops double detach reported in bug 887402
 * moves volume.API() into init

Change-Id: I65332cabedf2edb88acb48b3293cba291d440238

1 files changed, 72 insertions, 70 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index d75aad603..d6912bb56 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -33,7 +33,6 @@ terminating it.
 
 """
 
-import datetime
 import functools
 import os
 import socket
@@ -146,6 +145,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             sys.exit(1)
 
         self.network_api = network.API()
+        self.volume_api = volume.API()
         self.network_manager = utils.import_object(FLAGS.network_manager)
         self._last_host_check = 0
         self._last_bw_usage_poll = 0
@@ -245,7 +245,6 @@ class ComputeManager(manager.SchedulerDependentManager):
 
     def _setup_block_device_mapping(self, context, instance):
         """setup volumes for block device mapping"""
-        volume_api = volume.API()
         block_device_mapping = []
         swap = None
         ephemerals = []
@@ -273,18 +272,18 @@ class ComputeManager(manager.SchedulerDependentManager):
             if ((bdm['snapshot_id'] is not None) and
                 (bdm['volume_id'] is None)):
                 # TODO(yamahata): default name and description
-                vol = volume_api.create(context, bdm['volume_size'],
-                                        bdm['snapshot_id'], '', '')
+                vol = self.volume_api.create(context, bdm['volume_size'],
+                                             bdm['snapshot_id'], '', '')
                 # TODO(yamahata): creating volume simultaneously
                 #                 reduces creation time?
-                volume_api.wait_creation(context, vol['id'])
+                self.volume_api.wait_creation(context, vol['id'])
                 self.db.block_device_mapping_update(
                     context, bdm['id'], {'volume_id': vol['id']})
                 bdm['volume_id'] = vol['id']
 
             if bdm['volume_id'] is not None:
-                volume_api.check_attach(context,
-                                        volume_id=bdm['volume_id'])
+                self.volume_api.check_attach(context,
+                                             volume_id=bdm['volume_id'])
                 cinfo = self._attach_volume_boot(context, instance,
                                                     bdm['volume_id'],
                                                     bdm['device_name'])
@@ -507,6 +506,15 @@ class ComputeManager(manager.SchedulerDependentManager):
                                                                 instance_id)
         return [bdm for bdm in bdms if bdm['volume_id']]
 
+    def _get_instance_volume_bdm(self, context, instance_id, volume_id):
+        bdms = self._get_instance_volume_bdms(context, instance_id)
+        for bdm in bdms:
+            # NOTE(vish): Comparing as strings because the os_api doesn't
+            #             convert to integer and we may wish to support uuids
+            #             in the future.
+            if str(bdm['volume_id']) == str(volume_id):
+                return bdm
+
     def _get_instance_volume_block_device_info(self, context, instance_id):
         bdms = self._get_instance_volume_bdms(context, instance_id)
         block_device_mapping = []
@@ -515,8 +523,8 @@ class ComputeManager(manager.SchedulerDependentManager):
             block_device_mapping.append({'connection_info': cinfo,
                                          'mount_device':
                                          bdm['device_name']})
-        ## NOTE(vish): The mapping is passed in so the driver can disconnect
-        ##             from remote volumes if necessary
+        # NOTE(vish): The mapping is passed in so the driver can disconnect
+        #             from remote volumes if necessary
         return {'block_device_mapping': block_device_mapping}
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
@@ -546,29 +554,33 @@ class ComputeManager(manager.SchedulerDependentManager):
         if not FLAGS.stub_network:
             self.network_api.deallocate_for_instance(context, instance)
 
-        for bdm in self._get_instance_volume_bdms(context, instance_id):
-            volume_id = bdm['volume_id']
-            try:
-                self._detach_volume(context, instance_uuid, volume_id)
-            except exception.DiskNotFound as exc:
-                LOG.warn(_("Ignoring DiskNotFound: %s") % exc)
-
         if instance['power_state'] == power_state.SHUTOFF:
             self.db.instance_destroy(context, instance_id)
             raise exception.Error(_('trying to destroy already destroyed'
                                     ' instance: %s') % instance_uuid)
+        # NOTE(vish) get bdms before destroying the instance
+        bdms = self._get_instance_volume_bdms(context, instance_id)
         block_device_info = self._get_instance_volume_block_device_info(
             context, instance_id)
         self.driver.destroy(instance, network_info, block_device_info, cleanup)
+        for bdm in bdms:
+            try:
+                # NOTE(vish): actual driver detach done in driver.destroy, so
+                #             just tell nova-volume that we are done with it.
+                self.volume_api.terminate_connection(context,
+                                                     bdm['volume_id'],
+                                                     FLAGS.my_ip)
+                self.volume_api.detach(context, bdm['volume_id'])
+            except exception.DiskNotFound as exc:
+                LOG.warn(_("Ignoring DiskNotFound: %s") % exc)
 
     def _cleanup_volumes(self, context, instance_id):
-        volume_api = volume.API()
         bdms = self.db.block_device_mapping_get_all_by_instance(context,
                                                                 instance_id)
         for bdm in bdms:
             LOG.debug(_("terminating bdm %s") % bdm)
             if bdm['volume_id'] and bdm['delete_on_termination']:
-                volume_api.delete(context, bdm['volume_id'])
+                self.volume_api.delete(context, bdm['volume_id'])
             # NOTE(vish): bdms will be deleted on instance destroy
 
     def _delete_instance(self, context, instance):
@@ -1400,13 +1412,13 @@ class ComputeManager(manager.SchedulerDependentManager):
                     "volume %(volume_id)s at %(mountpoint)s") %
                   locals(), context=context)
         address = FLAGS.my_ip
-        volume_api = volume.API()
-        connection_info = volume_api.initialize_connection(context,
-                                                           volume_id,
-                                                           address)
-        volume_api.attach(context, volume_id, instance_id, mountpoint)
+        connection_info = self.volume_api.initialize_connection(context,
+                                                                volume_id,
+                                                                address)
+        self.volume_api.attach(context, volume_id, instance_id, mountpoint)
         return connection_info
 
+    @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
     @checks_instance_lock
     def attach_volume(self, context, instance_uuid, volume_id, mountpoint):
         """Attach a volume to an instance."""
@@ -1416,11 +1428,10 @@ class ComputeManager(manager.SchedulerDependentManager):
         LOG.audit(
             _("instance %(instance_uuid)s: attaching volume %(volume_id)s"
               " to %(mountpoint)s") % locals(), context=context)
-        volume_api = volume.API()
         address = FLAGS.my_ip
-        connection_info = volume_api.initialize_connection(context,
-                                                           volume_id,
-                                                           address)
+        connection_info = self.volume_api.initialize_connection(context,
+                                                                volume_id,
+                                                                address)
         try:
             self.driver.attach_volume(connection_info,
                                       instance_ref['name'],
@@ -1432,10 +1443,10 @@ class ComputeManager(manager.SchedulerDependentManager):
             #             ecxception below.
             LOG.exception(_("instance %(instance_uuid)s: attach failed"
                     " %(mountpoint)s, removing") % locals(), context=context)
-            volume_api.terminate_connection(context, volume_id, address)
+            self.volume_api.terminate_connection(context, volume_id, address)
             raise exc
 
-        volume_api.attach(context, volume_id, instance_id, mountpoint)
+        self.volume_api.attach(context, volume_id, instance_id, mountpoint)
         values = {
             'instance_id': instance_id,
             'connection_info': utils.dumps(connection_info),
@@ -1449,60 +1460,51 @@ class ComputeManager(manager.SchedulerDependentManager):
         self.db.block_device_mapping_create(context, values)
         return True
 
-    @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
-    @checks_instance_lock
-    def _detach_volume(self, context, instance_uuid, volume_id,
-                       destroy_bdm=False, mark_detached=True,
-                       force_detach=False):
-        """Detach a volume from an instance."""
-        context = context.elevated()
-        instance_ref = self.db.instance_get_by_uuid(context, instance_uuid)
-        instance_id = instance_ref['id']
-        bdms = self.db.block_device_mapping_get_all_by_instance(
-                context, instance_id)
-        for item in bdms:
-            # NOTE(vish): Comparing as strings because the os_api doesn't
-            #             convert to integer and we may wish to support uuids
-            #             in the future.
-            if str(item['volume_id']) == str(volume_id):
-                bdm = item
-                break
+    def _detach_volume(self, context, instance_name, bdm):
+        """Do the actual driver detach using block device mapping."""
         mp = bdm['device_name']
+        volume_id = bdm['volume_id']
 
         LOG.audit(_("Detach volume %(volume_id)s from mountpoint %(mp)s"
-                " on instance %(instance_id)s") % locals(), context=context)
-        volume_api = volume.API()
-        if (instance_ref['name'] not in self.driver.list_instances() and
-            not force_detach):
+                " on instance %(instance_name)s") % locals(), context=context)
+
+        if instance_name not in self.driver.list_instances():
             LOG.warn(_("Detaching volume from unknown instance %s"),
-                     instance_id, context=context)
-        else:
-            self.driver.detach_volume(utils.loads(bdm['connection_info']),
-                                      instance_ref['name'],
-                                      bdm['device_name'])
-        address = FLAGS.my_ip
-        volume_api.terminate_connection(context, volume_id, address)
-        if mark_detached:
-            volume_api.detach(context, volume_id)
-        if destroy_bdm:
-            self.db.block_device_mapping_destroy_by_instance_and_volume(
-                context, instance_id, volume_id)
-        return True
+                     instance_name, context=context)
+        self.driver.detach_volume(utils.loads(bdm['connection_info']),
+                                  instance_name,
+                                  mp)
 
+    @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @checks_instance_lock
     def detach_volume(self, context, instance_uuid, volume_id):
         """Detach a volume from an instance."""
-        return self._detach_volume(context, instance_uuid, volume_id, True)
+        instance_ref = self.db.instance_get_by_uuid(context, instance_uuid)
+        instance_id = instance_ref['id']
+        bdm = self._get_instance_volume_bdm(context, instance_id, volume_id)
+        self._detach_volume(context, instance_ref['name'], bdm)
+        self.volume_api.terminate_connection(context, volume_id, FLAGS.my_ip)
+        self.volume_api.detach(context, volume_id)
+        self.db.block_device_mapping_destroy_by_instance_and_volume(
+            context, instance_id, volume_id)
+        return True
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
     def remove_volume_connection(self, context, instance_id, volume_id):
-        """Detach a volume from an instance.,"""
+        """Remove a volume connection using the volume api"""
         # NOTE(vish): We don't want to actually mark the volume
         #             detached, or delete the bdm, just remove the
         #             connection from this host.
         try:
             instance_ref = self.db.instance_get(context, instance_id)
-            self._detach_volume(context, instance_ref['uuid'], volume_id,
-                                False, False, True)
+            bdm = self._get_instance_volume_bdm(context,
+                                                instance_id,
+                                                volume_id)
+            self._detach_volume(context, instance_ref['name'],
+                                bdm['volume_id'], bdm['device_name'])
+            self.volume_api.terminate_connection(context,
+                                                 volume_id,
+                                                 FLAGS.my_ip)
         except exception.NotFound:
             pass
 
@@ -1821,8 +1823,8 @@ class ComputeManager(manager.SchedulerDependentManager):
         for bdm in self._get_instance_volume_bdms(context, instance_ref['id']):
             volume_id = bdm['volume_id']
             self.db.volume_update(context, volume_id, {'status': 'in-use'})
-            volume.API().remove_from_compute(context, instance_ref['id'],
-                                             volume_id, dest)
+            self.volume_api.remove_from_compute(context, instance_ref['id'],
+                                                volume_id, dest)
 
         # Block migration needs empty image at destination host
         # before migration starts, so if any failure occurs,