Merge "Fix the wrong exception used to retry detach API calls"

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 8c4eb2c8f1..6aa89cafd5 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 f295dc6c73..4727598d4f 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(