From 3cb1e35b5e3a3f8949bb0fd31fb8a246c5346703 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Mon, 13 Jun 2022 14:48:24 +0900 Subject: [PATCH] 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. Conflicts: nova/tests/unit/volume/test_cinder.py nova/volume/cinder.py NOTE(melwitt): The conflicts are due to the following changes not in Victoria: * I23bb9e539d08f5c6202909054c2dd49b6c7a7a0e (Remove six.text_type (1/2)) * I779bd1446dc1f070fa5100ccccda7881fa508d79 (Remove six.text_type (2/2)) Closes-Bug: #1978444 Change-Id: I593011d9f4c43cdae7a3d53b556c6e2a2b939989 (cherry picked from commit 8f4b740ca5292556f8e953a30f2a11ed4fbc2945) (cherry picked from commit b94ffb1123b1a6cf0a8675e0d6f1072e9625f570) (cherry picked from commit 14f9b7627e8a48f546e2f1c79d4336e1e4923501) (cherry picked from commit 9b1c078112f11eafbd8e174efbd0e0f9d2c951ee) --- nova/tests/unit/volume/test_cinder.py | 40 +++++++++++++++++-- nova/volume/cinder.py | 18 +++++---- .../notes/bug-1978444-db46df5f3d5ea19e.yaml | 7 ++++ 3 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/bug-1978444-db46df5f3d5ea19e.yaml diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index 780953398101..c6bb2c10cad5 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -521,16 +521,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, six.text_type(ex)) + self.assertEqual(400, ex.code) @mock.patch('nova.volume.cinder.cinderclient', side_effect=exception.CinderAPIVersionNotAvailable( @@ -546,6 +545,16 @@ class CinderApiTestCase(test.NoDBTestCase): 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 = ( @@ -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 = ( diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 1eaa5bcbda94..08f8fa0b0186 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -884,19 +884,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': six.text_type(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': six.text_type(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 000000000000..6c1980407452 --- /dev/null +++ b/releasenotes/notes/bug-1978444-db46df5f3d5ea19e.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + `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.