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 f620626)
(cherry picked from commit 513241a)

Author: kajinamit

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
@@ -546,11 +545,12 @@ def test_attachment_delete_unsupported_api_version(self,
         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 @@ 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]
+            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 @@ def test_attachment_complete(self, mock_cinderclient):
     @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 @@ def test_detach_no_attachment_id_multiattach(self, mock_cinderclient):
         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 @@ def test_terminate_connection(self, mock_cinderclient):
         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 @@ 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,
+            side_effect = [cinder_exception.ClientException(500),
                            None]
 
         self.api.terminate_connection(self.ctx, 'id1', 'connector')

nova/volume/cinder.py

@@ -25,7 +25,6 @@
 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 @@ def attach(self, context, volume_id, instance_uuid, mountpoint, mode='rw'):
     @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 @@ def initialize_connection(self, context, volume_id, connector):
     @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 @@ def attachment_update(self, context, attachment_id, connector,
     @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(