Retry attachment delete API call for 504 Gateway Timeout

When cinder-api runs behind a load balancer(eg haproxy), the load
balancer can return 504 Gateway Timeout when cinder-api does not
respond within timeout. This change ensures nova retries deleting
a volume attachment in that case.

Also this change makes nova ignore 404 in the API call. This is
required because cinder might continue deleting the attachment even if
the load balancer returns 504. This also helps us in the situation
where the volume attachment was accidentally removed by users.

Closes-Bug: #1978444
Change-Id: I593011d9f4c43cdae7a3d53b556c6e2a2b939989

Author: kajinamit

nova/tests/unit/volume/test_cinder.py

@@ -520,16 +520,15 @@ def test_attachment_delete(self, mock_cinderclient):
     @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(
@@ -545,6 +544,16 @@ def test_attachment_delete_unsupported_api_version(self,
         mock_cinderclient.assert_called_once_with(self.ctx, '3.44',
                                                   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 = (
@@ -568,6 +577,29 @@ def test_attachment_delete_internal_server_error_do_not_raise(
 
         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 = (

nova/volume/cinder.py

@@ -888,19 +888,23 @@ def attachment_update(self, context, attachment_id, connector,
     @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):

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.