compute: Only destroy BDMs after successful detach call

Previously if detach was called with destroy_bdm=True the BDM would
always be destroyed first before any detach call was made to cinder.
This could result in the destruction of a BDM while the volume remained
marked as attached in Cinder.

This has now changed so the BDM is only destroyed after a successful
detach call has been made to cinder. In the event of a failure during
this detach call to Cinder an operator can now make another attempt to
detach the volume via Nova as the BDM is no longer destroyed.

The associated volume.detach notification is also now only sent after a
successful detach call to Cinder and before the BDM is destroyed.

Change-Id: I2581ff9f9c0e7cfc14a25acf45eb1860df69eacf

nova/compute/manager.py

@@ -4929,15 +4929,14 @@ class ComputeManager(manager.Manager):
                 connector = stashed_connector
 
         self.volume_api.terminate_connection(context, volume_id, connector)
-
-        if destroy_bdm:
-            bdm.destroy()
-
+        self.volume_api.detach(context.elevated(), volume_id, instance.uuid,
+                               attachment_id)
         info = dict(volume_id=volume_id)
         self._notify_about_instance_usage(
             context, instance, "volume.detach", extra_usage_info=info)
-        self.volume_api.detach(context.elevated(), volume_id, instance.uuid,
-                               attachment_id)
+
+        if destroy_bdm:
+            bdm.destroy()
 
     @wrap_exception()
     @wrap_instance_fault

nova/tests/unit/compute/test_compute.py

@@ -430,9 +430,27 @@ class ComputeVolumeTestCase(BaseTestCase):
             self.assertRaises(
                     test.TestingException, self.compute.detach_volume,
                     self.context, 'fake', instance, 'fake_id')
-            mock_internal_detach.assert_called_once_with(self.context,
-                                                         instance,
-                                                         fake_bdm, {})
+            self.assertFalse(mock_destroy.called)
+
+    def test_detach_volume_bdm_destroyed(self):
+        # Assert that the BDM is destroyed given a successful call to detach
+        # the volume from the instance in Cinder.
+        fake_bdm = objects.BlockDeviceMapping(**self.fake_volume)
+        instance = self._create_fake_instance_obj()
+
+        with test.nested(
+            mock.patch.object(self.compute, '_driver_detach_volume'),
+            mock.patch.object(self.compute.volume_api, 'detach'),
+            mock.patch.object(objects.BlockDeviceMapping,
+                              'get_by_volume_and_instance'),
+            mock.patch.object(fake_bdm, 'destroy')
+        ) as (mock_internal_detach, mock_detach, mock_get, mock_destroy):
+            mock_get.return_value = fake_bdm
+            self.compute.detach_volume(self.context, uuids.volume_id, instance,
+                                       uuids.attachment_id)
+            mock_detach.assert_called_once_with(mock.ANY, uuids.volume_id,
+                                                instance.uuid,
+                                                uuids.attachment_id)
             self.assertTrue(mock_destroy.called)
 
     def test_await_block_device_created_too_slow(self):