openstack
/
nova
Code Issues Proposed changes

Add retry to cinder API calls related to volume detach 

When shutting down an instance for which volume needs to be
deleted, if cinder RPC timeout expires before cinder volume
driver terminates connection, then an unknown cinder exception
is received and the volume is not removed.

This fix adds a retry mechanism directly in cinder API calls
attachment_delete, terminate_connection, and detach.

Change-Id: I3c9ae47d0ceb64fa3082a01cb7df27faa4f5a00d
Closes-Bug: #1834659
(cherry picked from commit 01c334cbdd)
(cherry picked from commit 118ee68257)
(cherry picked from commit 1d66884af7)
This commit is contained in:
Francois Palin 2019-07-08 10:12:25 -04:00
parent 019e3da75b
commit 10bd369589
2 changed files with 108 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -14,6 +14,7 @@
# 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.v2 import limits as cinder_limits
from keystoneauth1 import loading as ks_loading
@ -539,6 +540,38 @@ 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)
def test_attachment_delete_internal_server_error(self, mock_cinderclient):
self.assertRaises(cinder_apiclient.exceptions.InternalServerError,
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_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.attachments.delete.side_effect = [
cinder_apiclient.exceptions.InternalServerError, 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))
def test_attachment_delete_bad_request_exception(self, mock_cinderclient):
self.assertRaises(exception.InvalidInput,
self.api.attachment_delete,
self.ctx, uuids.attachment_id)
self.assertEqual(1, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_complete(self, mock_cinderclient):
mock_attachments = mock.MagicMock()
@ -628,6 +661,38 @@ 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)
def test_detach_internal_server_error(self, mock_cinderclient):
self.assertRaises(cinder_apiclient.exceptions.InternalServerError,
self.api.detach,
self.ctx, 'id1', instance_uuid='fake_uuid')
self.assertEqual(5, mock_cinderclient.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]
self.api.detach(self.ctx, 'id1', instance_uuid='fake_uuid',
attachment_id='fakeid')
self.assertEqual(2, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient',
side_effect=cinder_exception.BadRequest(code=400))
def test_detach_bad_request_exception(self, mock_cinderclient):
self.assertRaises(exception.InvalidInput,
self.api.detach,
self.ctx, 'id1', instance_uuid='fake_uuid')
self.assertEqual(1, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_get(self, mock_cinderclient):
mock_attachment = mock.MagicMock()
@ -747,6 +812,38 @@ 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)
def test_terminate_connection_internal_server_error(
self, mock_cinderclient):
self.assertRaises(cinder_apiclient.exceptions.InternalServerError,
self.api.terminate_connection,
self.ctx, 'id1', 'connector')
self.assertEqual(5, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient')
def test_terminate_connection_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.terminate_connection.\
side_effect = [cinder_apiclient.exceptions.InternalServerError,
None]
self.api.terminate_connection(self.ctx, 'id1', 'connector')
self.assertEqual(2, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient',
side_effect=cinder_exception.BadRequest(code=400))
def test_terminate_connection_bad_request_exception(
self, mock_cinderclient):
self.assertRaises(exception.InvalidInput,
self.api.terminate_connection,
self.ctx, 'id1', 'connector')
self.assertEqual(1, mock_cinderclient.call_count)
@mock.patch('nova.volume.cinder.cinderclient')
def test_delete(self, mock_cinderclient):
mock_volumes = mock.MagicMock()

11
nova/volume/cinder.py
View File

@ -24,6 +24,7 @@ import functools
import sys
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
@ -33,6 +34,7 @@ from oslo_serialization import jsonutils
from oslo_utils import encodeutils
from oslo_utils import excutils
from oslo_utils import strutils
import retrying
import six
from six.moves import urllib
@ -547,6 +549,9 @@ class API(object):
mountpoint, mode=mode)
@translate_volume_exception
@retrying.retry(stop_max_attempt_number=5,
retry_on_exception=lambda e:
type(e) == cinder_apiclient.exceptions.InternalServerError)
def detach(self, context, volume_id, instance_uuid=None,
attachment_id=None):
client = cinderclient(context)
@ -610,6 +615,9 @@ class API(object):
exc.code if hasattr(exc, 'code') else None)})
@translate_volume_exception
@retrying.retry(stop_max_attempt_number=5,
retry_on_exception=lambda e:
type(e) == cinder_apiclient.exceptions.InternalServerError)
def terminate_connection(self, context, volume_id, connector):
return cinderclient(context).volumes.terminate_connection(volume_id,
connector)
@ -846,6 +854,9 @@ class API(object):
'code': getattr(ex, 'code', None)})
@translate_attachment_exception
@retrying.retry(stop_max_attempt_number=5,
retry_on_exception=lambda e:
type(e) == cinder_apiclient.exceptions.InternalServerError)
def attachment_delete(self, context, attachment_id):
try:
cinderclient(