From f6206269b71d9bcaf65aa0313c21cc6b75a54fb3 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Sun, 19 Sep 2021 00:06:56 +0900 Subject: [PATCH] Fix the wrong exception used to retry detach API calls When cinderclient receives an error response from API, it raises one of the exceptions from the cinderclient.exceptions module instead of the cinderclient.apiclient.exceptions module. This change fixes the wrong exception used to detect 500 error from cinder API, so that API calls to detach volumes are properly retried. Closes-Bug: #1944043 Change-Id: I741cb6b29a67da8c60708c6251c441d778ca74d0 --- nova/tests/unit/volume/test_cinder.py | 42 +++++++++++++++------------ nova/volume/cinder.py | 10 ++++--- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index 8c4eb2c8f1f5..6aa89cafd58e 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -14,7 +14,6 @@ # under the License. from cinderclient import api_versions as cinder_api_versions -from cinderclient import apiclient as cinder_apiclient from cinderclient import exceptions as cinder_exception from cinderclient.v3 import limits as cinder_limits from keystoneauth1 import loading as ks_loading @@ -546,11 +545,12 @@ class CinderApiTestCase(test.NoDBTestCase): mock_cinderclient.assert_called_once_with(self.ctx, '3.44', skip_version_check=True) - @mock.patch('nova.volume.cinder.cinderclient', - side_effect=cinder_apiclient.exceptions.InternalServerError) + @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)) - self.assertRaises(cinder_apiclient.exceptions.InternalServerError, + self.assertRaises(cinder_exception.ClientException, self.api.attachment_delete, self.ctx, uuids.attachment_id) @@ -561,16 +561,17 @@ class CinderApiTestCase(test.NoDBTestCase): self, mock_cinderclient): # generate exception, and then have a normal return on the next retry mock_cinderclient.return_value.attachments.delete.side_effect = [ - cinder_apiclient.exceptions.InternalServerError, None] + cinder_exception.ClientException(500), 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', - side_effect=cinder_exception.BadRequest(code=400)) + @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)) self.assertRaises(exception.InvalidInput, self.api.attachment_delete, @@ -594,7 +595,7 @@ class CinderApiTestCase(test.NoDBTestCase): @mock.patch('nova.volume.cinder.cinderclient') def test_attachment_complete_failed(self, mock_cinderclient): mock_cinderclient.return_value.attachments.complete.side_effect = ( - cinder_exception.NotFound(404, '404')) + cinder_exception.NotFound(404)) attachment_id = uuids.attachment ex = self.assertRaises(exception.VolumeAttachmentNotFound, @@ -667,27 +668,30 @@ class CinderApiTestCase(test.NoDBTestCase): mock_cinderclient.assert_called_with(self.ctx, microversion=None) mock_volumes.detach.assert_called_once_with('id1', 'fakeid') - @mock.patch('nova.volume.cinder.cinderclient', - side_effect=cinder_apiclient.exceptions.InternalServerError) + @mock.patch('nova.volume.cinder.cinderclient') def test_detach_internal_server_error(self, mock_cinderclient): + mock_cinderclient.return_value.volumes.detach.side_effect = ( + cinder_exception.ClientException(500)) - self.assertRaises(cinder_apiclient.exceptions.InternalServerError, + self.assertRaises(cinder_exception.ClientException, self.api.detach, self.ctx, 'id1', instance_uuid='fake_uuid') - self.assertEqual(5, mock_cinderclient.call_count) + self.assertEqual( + 5, mock_cinderclient.return_value.volumes.detach.call_count) @mock.patch('nova.volume.cinder.cinderclient') def test_detach_internal_server_error_do_not_raise( self, mock_cinderclient): # generate exception, and then have a normal return on the next retry mock_cinderclient.return_value.volumes.detach.side_effect = [ - cinder_apiclient.exceptions.InternalServerError, None] + cinder_exception.ClientException(500), None] self.api.detach(self.ctx, 'id1', instance_uuid='fake_uuid', attachment_id='fakeid') - self.assertEqual(2, mock_cinderclient.call_count) + self.assertEqual( + 2, mock_cinderclient.return_value.volumes.detach.call_count) @mock.patch('nova.volume.cinder.cinderclient', side_effect=cinder_exception.BadRequest(code=400)) @@ -818,11 +822,13 @@ class CinderApiTestCase(test.NoDBTestCase): mock_volumes.terminate_connection.assert_called_once_with('id1', 'connector') - @mock.patch('nova.volume.cinder.cinderclient', - side_effect=cinder_apiclient.exceptions.InternalServerError) + @mock.patch('nova.volume.cinder.cinderclient') def test_terminate_connection_internal_server_error( self, mock_cinderclient): - self.assertRaises(cinder_apiclient.exceptions.InternalServerError, + mock_cinderclient.return_value.volumes.terminate_connection.\ + side_effect = cinder_exception.ClientException(500) + + self.assertRaises(cinder_exception.ClientException, self.api.terminate_connection, self.ctx, 'id1', 'connector') @@ -833,7 +839,7 @@ class CinderApiTestCase(test.NoDBTestCase): self, mock_cinderclient): # generate exception, and then have a normal return on the next retry mock_cinderclient.return_value.volumes.terminate_connection.\ - side_effect = [cinder_apiclient.exceptions.InternalServerError, + side_effect = [cinder_exception.ClientException(500), None] self.api.terminate_connection(self.ctx, 'id1', 'connector') diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index f295dc6c7305..4727598d4f7b 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -25,7 +25,6 @@ import sys import urllib from cinderclient import api_versions as cinder_api_versions -from cinderclient import apiclient as cinder_apiclient from cinderclient import client as cinder_client from cinderclient import exceptions as cinder_exception from keystoneauth1 import exceptions as keystone_exception @@ -567,7 +566,8 @@ class API(object): @translate_volume_exception @retrying.retry(stop_max_attempt_number=5, retry_on_exception=lambda e: - type(e) == cinder_apiclient.exceptions.InternalServerError) + (isinstance(e, cinder_exception.ClientException) and + e.code == 500)) def detach(self, context, volume_id, instance_uuid=None, attachment_id=None): client = cinderclient(context) @@ -632,7 +632,8 @@ class API(object): @translate_volume_exception @retrying.retry(stop_max_attempt_number=5, retry_on_exception=lambda e: - type(e) == cinder_apiclient.exceptions.InternalServerError) + (isinstance(e, cinder_exception.ClientException) and + e.code == 500)) def terminate_connection(self, context, volume_id, connector): return cinderclient(context).volumes.terminate_connection(volume_id, connector) @@ -881,7 +882,8 @@ class API(object): @translate_attachment_exception @retrying.retry(stop_max_attempt_number=5, retry_on_exception=lambda e: - type(e) == cinder_apiclient.exceptions.InternalServerError) + (isinstance(e, cinder_exception.ClientException) and + e.code == 500)) def attachment_delete(self, context, attachment_id): try: cinderclient(