From 595866eb62db56fbd4f965619031ac902f99441a Mon Sep 17 00:00:00 2001
From: yenai <yenai@cmss.chinamobile.com>
Date: Wed, 23 Jan 2019 13:54:47 +0800
Subject: [PATCH] Ignore VolumeAttachmentNotFound exception in compute.manager

DriverVolumeBlockDevice will delete volume attachment when attach
fails, codes link:
https://github.com/openstack/nova/blob/907c7d2cf/nova/virt/block_device.py#L561-L568
However, nova.compute.manager will delete it again and it will raise
VolumeAttachmentNotFound exception. This outputs an incorrect error
log and this exception should be ignored.

Change-Id: I939c09e5b0efb3b17a9855af227e6d60c64d23e2
Closes-Bug: #1812969
---
 nova/compute/manager.py                 | 12 +++++-
 nova/tests/unit/compute/test_compute.py | 54 +++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index d3504b8fccf3..8688fdccaa2d 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -5502,8 +5502,16 @@ class ComputeManager(manager.Manager):
                                'mountpoint': bdm['mount_device']},
                               instance=instance)
                 if bdm['attachment_id']:
-                    self.volume_api.attachment_delete(context,
-                                                      bdm['attachment_id'])
+                    # Try to delete the attachment to make the volume
+                    # available again. Note that DriverVolumeBlockDevice
+                    # may have already deleted the attachment so ignore
+                    # VolumeAttachmentNotFound.
+                    try:
+                        self.volume_api.attachment_delete(
+                            context, bdm['attachment_id'])
+                    except exception.VolumeAttachmentNotFound as exc:
+                        LOG.debug('Ignoring VolumeAttachmentNotFound: %s',
+                                  exc, instance=instance)
                 else:
                     self.volume_api.unreserve_volume(context, bdm.volume_id)
                 tb = traceback.format_exc()
diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py
index 0d2df5a4143d..7620a7406bfe 100644
--- a/nova/tests/unit/compute/test_compute.py
+++ b/nova/tests/unit/compute/test_compute.py
@@ -487,6 +487,60 @@ class ComputeVolumeTestCase(BaseTestCase):
                           tb=mock.ANY),
             ])
 
+    @mock.patch.object(compute_manager.LOG, 'debug')
+    @mock.patch.object(compute_utils, 'EventReporter')
+    @mock.patch('nova.context.RequestContext.elevated')
+    @mock.patch('nova.compute.utils.notify_about_volume_attach_detach')
+    def test_attach_volume_ignore_VolumeAttachmentNotFound(
+            self, mock_notify, mock_elevate, mock_event, mock_debug_log):
+        """Tests the scenario that the DriverVolumeBlockDevice.attach flow
+        already deleted the volume attachment before the
+        ComputeManager.attach_volume flow tries to rollback the attachment
+        record and delete it, which raises VolumeAttachmentNotFound and is
+        ignored.
+        """
+        mock_elevate.return_value = self.context
+
+        attachment_id = uuids.attachment_id
+        fake_bdm = objects.BlockDeviceMapping(**self.fake_volume)
+        fake_bdm.attachment_id = attachment_id
+        instance = self._create_fake_instance_obj()
+        expected_exception = test.TestingException()
+
+        def fake_attach(*args, **kwargs):
+            raise expected_exception
+
+        with test.nested(
+            mock.patch.object(driver_block_device.DriverVolumeBlockDevice,
+                              'attach'),
+            mock.patch.object(cinder.API, 'attachment_delete'),
+            mock.patch.object(objects.BlockDeviceMapping,
+                              'destroy')
+        ) as (mock_attach, mock_attach_delete, mock_destroy):
+            mock_attach.side_effect = fake_attach
+            mock_attach_delete.side_effect = \
+                exception.VolumeAttachmentNotFound(
+                    attachment_id=attachment_id)
+            self.assertRaises(
+                    test.TestingException, self.compute.attach_volume,
+                    self.context, instance, fake_bdm)
+            mock_destroy.assert_called_once_with()
+            mock_notify.assert_has_calls([
+                mock.call(self.context, instance, 'fake-mini',
+                          action='volume_attach', phase='start',
+                          volume_id=uuids.volume_id),
+                mock.call(self.context, instance, 'fake-mini',
+                          action='volume_attach', phase='error',
+                          volume_id=uuids.volume_id,
+                          exception=expected_exception,
+                          tb=mock.ANY),
+            ])
+            mock_event.assert_called_once_with(
+                self.context, 'compute_attach_volume', CONF.host,
+                instance.uuid)
+            self.assertIsInstance(mock_debug_log.call_args[0][1],
+                                  exception.VolumeAttachmentNotFound)
+
     @mock.patch.object(compute_utils, 'EventReporter')
     def test_detach_volume_api_raises(self, mock_event):
         fake_bdm = objects.BlockDeviceMapping(**self.fake_volume)