Cinder os-force_detach api returns 500

If invalid 'attachment_id' is passed to the os-force_detach
api, then it returns 500 error because even though
VolumeAttachmentNotFound exception is raised from manager,
it gets converted to RemoteError due to rpcapi call.

Caught RemoteError exception in the api and returned 400
error to the user if exception type is VolumeAttachmentNotFound
or InvalidVolume. In other cases 500 error is raised.

Closes-Bug: 1450431
Change-Id: I5e308d9f4b3772284caaedb3d2d0b3f7d25e1e5d

diff --git a/cinder/api/contrib/admin_actions.py b/cinder/api/contrib/admin_actions.py
index cc5852474b7774c7821b6eb3188e68b69da96265..5c51646d7f63fbf4514f4d5b5ee1b2236c87c802 100644 (file)
--- a/cinder/api/contrib/admin_actions.py
+++ b/cinder/api/contrib/admin_actions.py
@@ -13,6 +13,7 @@
 #   under the License.
 
 from oslo_log import log as logging
+import oslo_messaging as messaging
 from oslo_utils import strutils
 import webob
 from webob import exc
@@ -188,7 +189,21 @@ class VolumeAdminController(AdminController):
 
         attachment_id = body['os-force_detach'].get('attachment_id', None)
 
-        self.volume_api.detach(context, volume, attachment_id)
+        try:
+            self.volume_api.detach(context, volume, attachment_id)
+        except messaging.RemoteError as error:
+            if error.exc_type in ['VolumeAttachmentNotFound',
+                                  'InvalidVolume']:
+                msg = "Error force detaching volume - %(err_type)s: " \
+                      "%(err_msg)s" % {'err_type': error.exc_type,
+                                       'err_msg': error.value}
+                raise webob.exc.HTTPBadRequest(explanation=msg)
+            else:
+                # There are also few cases where force-detach call could fail
+                # due to db or volume driver errors. These errors shouldn't
+                # be exposed to the user and in such cases it should raise
+                # 500 error.
+                raise
         return webob.Response(status_int=202)
 
     @wsgi.action('os-migrate_volume')

diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py
index a1dff4da4fddc7fb101a40f054612da29e409c05..71aece524838d826644fd3c75b50e53b0615359c 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_admin_actions.py
+++ b/cinder/tests/unit/api/contrib/test_admin_actions.py
@@ -17,6 +17,7 @@ import mock
 from oslo_concurrency import lockutils
 from oslo_config import cfg
 from oslo_config import fixture as config_fixture
+import oslo_messaging as messaging
 from oslo_serialization import jsonutils
 from oslo_utils import timeutils
 import webob
@@ -492,6 +493,104 @@ class AdminActionsTest(test.TestCase):
         # cleanup
         svc.stop()
 
+    def test_volume_force_detach_raises_remote_error(self):
+        # admin context
+        ctx = context.RequestContext('admin', 'fake', True)
+        # current status is available
+        volume = db.volume_create(ctx, {'status': 'available', 'host': 'test',
+                                        'provider_location': '', 'size': 1})
+        connector = {'initiator': 'iqn.2012-07.org.fake:01'}
+        # start service to handle rpc messages for attach requests
+        svc = self.start_service('volume', host='test')
+        self.volume_api.reserve_volume(ctx, volume)
+        mountpoint = '/dev/vbd'
+        attachment = self.volume_api.attach(ctx, volume, stubs.FAKE_UUID,
+                                            None, mountpoint, 'rw')
+        # volume is attached
+        volume = db.volume_get(ctx, volume['id'])
+        self.assertEqual(volume['status'], 'in-use')
+        self.assertEqual(attachment['instance_uuid'], stubs.FAKE_UUID)
+        self.assertEqual(attachment['mountpoint'], mountpoint)
+        self.assertEqual(attachment['attach_status'], 'attached')
+        admin_metadata = volume['volume_admin_metadata']
+        self.assertEqual(len(admin_metadata), 2)
+        self.assertEqual(admin_metadata[0]['key'], 'readonly')
+        self.assertEqual(admin_metadata[0]['value'], 'False')
+        self.assertEqual(admin_metadata[1]['key'], 'attached_mode')
+        self.assertEqual(admin_metadata[1]['value'], 'rw')
+        conn_info = self.volume_api.initialize_connection(ctx,
+                                                          volume,
+                                                          connector)
+        self.assertEqual(conn_info['data']['access_mode'], 'rw')
+        # build request to force detach
+        volume_remote_error = \
+            messaging.RemoteError(exc_type='VolumeAttachmentNotFound')
+        with mock.patch.object(volume_api.API, 'detach',
+                               side_effect=volume_remote_error):
+            req = webob.Request.blank('/v2/fake/volumes/%s/action' %
+                                      volume['id'])
+            req.method = 'POST'
+            req.headers['content-type'] = 'application/json'
+            req.body = jsonutils.dumps({'os-force_detach':
+                                        {'attachment_id': 'fake'}})
+            # attach admin context to request
+            req.environ['cinder.context'] = ctx
+            # make request
+            resp = req.get_response(app())
+            self.assertEqual(resp.status_int, 400)
+        # cleanup
+        svc.stop()
+
+    def test_volume_force_detach_raises_db_error(self):
+        # In case of DB error 500 error code is returned to user
+        # admin context
+        ctx = context.RequestContext('admin', 'fake', True)
+        # current status is available
+        volume = db.volume_create(ctx, {'status': 'available', 'host': 'test',
+                                        'provider_location': '', 'size': 1})
+        connector = {'initiator': 'iqn.2012-07.org.fake:01'}
+        # start service to handle rpc messages for attach requests
+        svc = self.start_service('volume', host='test')
+        self.volume_api.reserve_volume(ctx, volume)
+        mountpoint = '/dev/vbd'
+        attachment = self.volume_api.attach(ctx, volume, stubs.FAKE_UUID,
+                                            None, mountpoint, 'rw')
+        # volume is attached
+        volume = db.volume_get(ctx, volume['id'])
+        self.assertEqual(volume['status'], 'in-use')
+        self.assertEqual(attachment['instance_uuid'], stubs.FAKE_UUID)
+        self.assertEqual(attachment['mountpoint'], mountpoint)
+        self.assertEqual(attachment['attach_status'], 'attached')
+        admin_metadata = volume['volume_admin_metadata']
+        self.assertEqual(len(admin_metadata), 2)
+        self.assertEqual(admin_metadata[0]['key'], 'readonly')
+        self.assertEqual(admin_metadata[0]['value'], 'False')
+        self.assertEqual(admin_metadata[1]['key'], 'attached_mode')
+        self.assertEqual(admin_metadata[1]['value'], 'rw')
+        conn_info = self.volume_api.initialize_connection(ctx,
+                                                          volume,
+                                                          connector)
+        self.assertEqual(conn_info['data']['access_mode'], 'rw')
+        # build request to force detach
+        volume_remote_error = \
+            messaging.RemoteError(exc_type='DBError')
+        with mock.patch.object(volume_api.API, 'detach',
+                               side_effect=volume_remote_error):
+            req = webob.Request.blank('/v2/fake/volumes/%s/action' %
+                                      volume['id'])
+            req.method = 'POST'
+            req.headers['content-type'] = 'application/json'
+            req.body = jsonutils.dumps({'os-force_detach':
+                                        {'attachment_id': 'fake'}})
+            # attach admin context to request
+            req.environ['cinder.context'] = ctx
+            # make request
+            self.assertRaises(messaging.RemoteError,
+                              req.get_response,
+                              app())
+        # cleanup
+        svc.stop()
+
     def test_attach_in_used_volume_by_instance(self):
         """Test that attaching to an in-use volume fails."""
         # admin context