Fix the wrong exception used to retry detach API calls

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
(cherry picked from commit f6206269b71d9bcaf65aa0313c21cc6b75a54fb3)
(cherry picked from commit 513241a7e4f3f3246c75e66a910db6c864c3981e)
(cherry picked from commit 7168314dd125fca8d1cecac035477ea0314ab828)

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 4ca8e4ee3e..7809533981 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.v2 import limits as cinder_limits
 from keystoneauth1 import loading as ks_loading
@@ -547,11 +546,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)
 
@@ -562,16 +562,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,
@@ -595,7 +596,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,
@@ -668,27 +669,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))
@@ -819,11 +823,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')
 
@@ -834,7 +840,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 fdbcb50c9b..1eaa5bcbda 100644
--- a/nova/volume/cinder.py
+++ b/nova/volume/cinder.py
@@ -24,7 +24,6 @@ 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
@@ -568,7 +567,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)
@@ -633,7 +633,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)
@@ -882,7 +883,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(