Merge "Add retry to cinder API calls related to volume detach"

2 files changed, 108 insertions, 0 deletions

diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py
index bcf1637b9b..4ca8e4ee3e 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
@@ -546,6 +547,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()
@@ -635,6 +668,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()
@@ -754,6 +819,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 d139b9f480..2c6c936f0d 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
 
@@ -566,6 +568,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)
@@ -629,6 +634,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)
@@ -875,6 +883,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(