Merge "Retry attachment delete API call for 504 Gateway Timeout" into…

… stable/yoga

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.