From 0f46dbb6e413249a5bae39dc2010c928d3fd2f40 Mon Sep 17 00:00:00 2001 From: Francois Palin Date: Mon, 8 Jul 2019 10:12:25 -0400 Subject: [PATCH] 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 01c334cbdd859f4e486ac2c369a4bdb3ec7709cc) (cherry picked from commit 118ee682571a4bd41c8009dbe2e47fdd1f85a630) (cherry picked from commit 1d66884af7b945dca2831c2b2ade534d87c934c9) (cherry picked from commit 10bd369589ab18094b1b96afe743686473294ef5) --- nova/tests/unit/volume/test_cinder.py | 97 +++++++++++++++++++++++++++ nova/volume/cinder.py | 11 +++ 2 files changed, 108 insertions(+) diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index 728fdd78e27e..16d0273f6e04 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -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 @@ -540,6 +541,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() @@ -629,6 +662,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() @@ -748,6 +813,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() diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 2f11dae35951..5650113370ba 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -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 @@ -543,6 +545,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) @@ -606,6 +611,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) @@ -844,6 +852,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(