Merge "Retry attachment delete API call for 504 Gateway Timeout" into stable/xena

3 files changed, 54 insertions, 11 deletions

diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py
index 6aa89cafd5..127e6c6544 100644
--- a/nova/tests/unit/volume/test_cinder.py
+++ b/nova/tests/unit/volume/test_cinder.py
@@ -520,16 +520,15 @@ class CinderApiTestCase(test.NoDBTestCase):
     @mock.patch('nova.volume.cinder.cinderclient')
     def test_attachment_delete_failed(self, mock_cinderclient, mock_log):
         mock_cinderclient.return_value.attachments.delete.side_effect = (
-                cinder_exception.NotFound(404, '404'))
+                cinder_exception.BadRequest(400, '400'))
 
         attachment_id = uuids.attachment
-        ex = self.assertRaises(exception.VolumeAttachmentNotFound,
+        ex = self.assertRaises(exception.InvalidInput,
                                self.api.attachment_delete,
                                self.ctx,
                                attachment_id)
 
-        self.assertEqual(404, ex.code)
-        self.assertIn(attachment_id, str(ex))
+        self.assertEqual(400, ex.code)
 
     @mock.patch('nova.volume.cinder.cinderclient',
                 side_effect=exception.CinderAPIVersionNotAvailable(
@@ -546,6 +545,16 @@ class CinderApiTestCase(test.NoDBTestCase):
                                                   skip_version_check=True)
 
     @mock.patch('nova.volume.cinder.cinderclient')
+    def test_attachment_delete_not_found(self, mock_cinderclient):
+        mock_cinderclient.return_value.attachments.delete.side_effect = (
+            cinder_exception.ClientException(404))
+
+        attachment_id = uuids.attachment
+        self.api.attachment_delete(self.ctx, attachment_id)
+
+        self.assertEqual(1, mock_cinderclient.call_count)
+
+    @mock.patch('nova.volume.cinder.cinderclient')
     def test_attachment_delete_internal_server_error(self, mock_cinderclient):
         mock_cinderclient.return_value.attachments.delete.side_effect = (
             cinder_exception.ClientException(500))
@@ -569,6 +578,29 @@ class CinderApiTestCase(test.NoDBTestCase):
         self.assertEqual(2, mock_cinderclient.call_count)
 
     @mock.patch('nova.volume.cinder.cinderclient')
+    def test_attachment_delete_gateway_timeout(self, mock_cinderclient):
+        mock_cinderclient.return_value.attachments.delete.side_effect = (
+            cinder_exception.ClientException(504))
+
+        self.assertRaises(cinder_exception.ClientException,
+                          self.api.attachment_delete,
+                          self.ctx, uuids.attachment_id)
+
+        self.assertEqual(5, mock_cinderclient.call_count)
+
+    @mock.patch('nova.volume.cinder.cinderclient')
+    def test_attachment_delete_gateway_timeout_do_not_raise(
+                                                      self, mock_cinderclient):
+        # generate exception, and then have a normal return on the next retry
+        mock_cinderclient.return_value.attachments.delete.side_effect = [
+            cinder_exception.ClientException(504), None]
+
+        attachment_id = uuids.attachment
+        self.api.attachment_delete(self.ctx, attachment_id)
+
+        self.assertEqual(2, mock_cinderclient.call_count)
+
+    @mock.patch('nova.volume.cinder.cinderclient')
     def test_attachment_delete_bad_request_exception(self, mock_cinderclient):
         mock_cinderclient.return_value.attachments.delete.side_effect = (
             cinder_exception.BadRequest(400))
diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py
index 4727598d4f..5d97b8b6b8 100644
--- a/nova/volume/cinder.py
+++ b/nova/volume/cinder.py
@@ -883,19 +883,23 @@ class API(object):
     @retrying.retry(stop_max_attempt_number=5,
                     retry_on_exception=lambda e:
                     (isinstance(e, cinder_exception.ClientException) and
-                     e.code == 500))
+                     e.code in (500, 504)))
     def attachment_delete(self, context, attachment_id):
         try:
             cinderclient(
                 context, '3.44', skip_version_check=True).attachments.delete(
                     attachment_id)
         except cinder_exception.ClientException as ex:
-            with excutils.save_and_reraise_exception():
-                LOG.error('Delete attachment failed for attachment '
-                          '%(id)s. Error: %(msg)s Code: %(code)s',
-                          {'id': attachment_id,
-                           'msg': str(ex),
-                           'code': getattr(ex, 'code', None)})
+            if ex.code == 404:
+                LOG.warning('Attachment %(id)s does not exist. Ignoring.',
+                            {'id': attachment_id})
+            else:
+                with excutils.save_and_reraise_exception():
+                    LOG.error('Delete attachment failed for attachment '
+                              '%(id)s. Error: %(msg)s Code: %(code)s',
+                              {'id': attachment_id,
+                               'msg': str(ex),
+                               'code': getattr(ex, 'code', None)})
 
     @translate_attachment_exception
     def attachment_complete(self, context, attachment_id):
diff --git a/releasenotes/notes/bug-1978444-db46df5f3d5ea19e.yaml b/releasenotes/notes/bug-1978444-db46df5f3d5ea19e.yaml
new file mode 100644
index 0000000000..6c19804074
--- /dev/null
+++ b/releasenotes/notes/bug-1978444-db46df5f3d5ea19e.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    `Bug #1978444 <https://bugs.launchpad.net/nova/+bug/1978444>`_: Now nova
+    retries deleting a volume attachment in case Cinder API returns
+    ``504 Gateway Timeout``. Also, ``404 Not Found`` is now ignored and
+    leaves only a warning message.