openstack/nova
Code Issues Proposed changes

Fix the wrong exception used to retry detach API calls

Browse Source 
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
This commit is contained in:
Takashi Kajinami 2021-09-19 00:06:56 +09:00 committed by Balazs Gibizer
parent 1c502ebaec
commit f6206269b7
2 changed files with 30 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
nova/tests/unit/volume/test_cinder.py
View File

@ -14,7 +14,6 @@
# under the License. # under the License.
from cinderclient import api_versions as cinder_api_versions 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 import exceptions as cinder_exception
from cinderclient.v3 import limits as cinder_limits from cinderclient.v3 import limits as cinder_limits
from keystoneauth1 import loading as ks_loading 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', mock_cinderclient.assert_called_once_with(self.ctx, '3.44',
skip_version_check=True) skip_version_check=True)
@mock.patch('nova.volume.cinder.cinderclient', @mock.patch('nova.volume.cinder.cinderclient')
side_effect=cinder_apiclient.exceptions.InternalServerError)
def test_attachment_delete_internal_server_error(self, mock_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.api.attachment_delete,
self.ctx, uuids.attachment_id) self.ctx, uuids.attachment_id)
@ -561,16 +561,17 @@ class CinderApiTestCase(test.NoDBTestCase):
self, mock_cinderclient): self, mock_cinderclient):
# generate exception, and then have a normal return on the next retry # generate exception, and then have a normal return on the next retry
mock_cinderclient.return_value.attachments.delete.side_effect = [ mock_cinderclient.return_value.attachments.delete.side_effect = [
cinder_apiclient.exceptions.InternalServerError, None] cinder_exception.ClientException(500), None]
attachment_id = uuids.attachment attachment_id = uuids.attachment
self.api.attachment_delete(self.ctx, attachment_id) self.api.attachment_delete(self.ctx, attachment_id)
self.assertEqual(2, mock_cinderclient.call_count) self.assertEqual(2, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient', @mock.patch('nova.volume.cinder.cinderclient')
side_effect=cinder_exception.BadRequest(code=400))
def test_attachment_delete_bad_request_exception(self, mock_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.assertRaises(exception.InvalidInput,
self.api.attachment_delete, self.api.attachment_delete,
@ -594,7 +595,7 @@ class CinderApiTestCase(test.NoDBTestCase):
@mock.patch('nova.volume.cinder.cinderclient') @mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_complete_failed(self, mock_cinderclient): def test_attachment_complete_failed(self, mock_cinderclient):
mock_cinderclient.return_value.attachments.complete.side_effect = ( mock_cinderclient.return_value.attachments.complete.side_effect = (
cinder_exception.NotFound(404, '404')) cinder_exception.NotFound(404))
attachment_id = uuids.attachment attachment_id = uuids.attachment
ex = self.assertRaises(exception.VolumeAttachmentNotFound, ex = self.assertRaises(exception.VolumeAttachmentNotFound,
@ -667,27 +668,30 @@ class CinderApiTestCase(test.NoDBTestCase):
mock_cinderclient.assert_called_with(self.ctx, microversion=None) mock_cinderclient.assert_called_with(self.ctx, microversion=None)
mock_volumes.detach.assert_called_once_with('id1', 'fakeid') mock_volumes.detach.assert_called_once_with('id1', 'fakeid')
@mock.patch('nova.volume.cinder.cinderclient', @mock.patch('nova.volume.cinder.cinderclient')
side_effect=cinder_apiclient.exceptions.InternalServerError)
def test_detach_internal_server_error(self, mock_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.api.detach,
self.ctx, 'id1', instance_uuid='fake_uuid') 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') @mock.patch('nova.volume.cinder.cinderclient')
def test_detach_internal_server_error_do_not_raise( def test_detach_internal_server_error_do_not_raise(
self, mock_cinderclient): self, mock_cinderclient):
# generate exception, and then have a normal return on the next retry # generate exception, and then have a normal return on the next retry
mock_cinderclient.return_value.volumes.detach.side_effect = [ 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', self.api.detach(self.ctx, 'id1', instance_uuid='fake_uuid',
attachment_id='fakeid') 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', @mock.patch('nova.volume.cinder.cinderclient',
side_effect=cinder_exception.BadRequest(code=400)) side_effect=cinder_exception.BadRequest(code=400))
@ -818,11 +822,13 @@ class CinderApiTestCase(test.NoDBTestCase):
mock_volumes.terminate_connection.assert_called_once_with('id1', mock_volumes.terminate_connection.assert_called_once_with('id1',
'connector') 'connector')
@mock.patch('nova.volume.cinder.cinderclient', @mock.patch('nova.volume.cinder.cinderclient')
side_effect=cinder_apiclient.exceptions.InternalServerError)
def test_terminate_connection_internal_server_error( def test_terminate_connection_internal_server_error(
self, mock_cinderclient): 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.api.terminate_connection,
self.ctx, 'id1', 'connector') self.ctx, 'id1', 'connector')
@ -833,7 +839,7 @@ class CinderApiTestCase(test.NoDBTestCase):
self, mock_cinderclient): self, mock_cinderclient):
# generate exception, and then have a normal return on the next retry # generate exception, and then have a normal return on the next retry
mock_cinderclient.return_value.volumes.terminate_connection.\ mock_cinderclient.return_value.volumes.terminate_connection.\
side_effect = [cinder_apiclient.exceptions.InternalServerError, side_effect = [cinder_exception.ClientException(500),
None] None]
self.api.terminate_connection(self.ctx, 'id1', 'connector') self.api.terminate_connection(self.ctx, 'id1', 'connector')

10
nova/volume/cinder.py
View File

@ -25,7 +25,6 @@ import sys
import urllib import urllib
from cinderclient import api_versions as cinder_api_versions 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 client as cinder_client
from cinderclient import exceptions as cinder_exception from cinderclient import exceptions as cinder_exception
from keystoneauth1 import exceptions as keystone_exception from keystoneauth1 import exceptions as keystone_exception
@ -567,7 +566,8 @@ class API(object):
@translate_volume_exception @translate_volume_exception
@retrying.retry(stop_max_attempt_number=5, @retrying.retry(stop_max_attempt_number=5,
retry_on_exception=lambda e: 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, def detach(self, context, volume_id, instance_uuid=None,
attachment_id=None): attachment_id=None):
client = cinderclient(context) client = cinderclient(context)
@ -632,7 +632,8 @@ class API(object):
@translate_volume_exception @translate_volume_exception
@retrying.retry(stop_max_attempt_number=5, @retrying.retry(stop_max_attempt_number=5,
retry_on_exception=lambda e: 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): def terminate_connection(self, context, volume_id, connector):
return cinderclient(context).volumes.terminate_connection(volume_id, return cinderclient(context).volumes.terminate_connection(volume_id,
connector) connector)
@ -881,7 +882,8 @@ class API(object):
@translate_attachment_exception @translate_attachment_exception
@retrying.retry(stop_max_attempt_number=5, @retrying.retry(stop_max_attempt_number=5,
retry_on_exception=lambda e: 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): def attachment_delete(self, context, attachment_id):
try: try:
cinderclient( cinderclient(