cinder os-detach api returns 500

If invalid 'attachment_id' is passed to the os-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.

Fixed typo in comment of volume manager.

Closes-Bug: 1449870
Change-Id: I92b03e0af3b7ab517e34edaeffd88f0a00516000

diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py
index 423cf17a288de4ba43c0926011ddb5b9de3addf7..254aff53ec29a500ab3e52dea18707a0a8187011 100644 (file)
--- a/cinder/api/contrib/volume_actions.py
+++ b/cinder/api/contrib/volume_actions.py
@@ -132,7 +132,19 @@ class VolumeActionsController(wsgi.Controller):
         if body['os-detach']:
             attachment_id = body['os-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 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 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-reserve')

diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py
index b7a5225188c89548c205e5759a75772f78ff9bf1..da0ec998753a686cf92ad5193ce666e14adebab5 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_volume_actions.py
+++ b/cinder/tests/unit/api/contrib/test_volume_actions.py
@@ -44,6 +44,7 @@ class VolumeActionsTest(test.TestCase):
     def setUp(self):
         super(VolumeActionsTest, self).setUp()
         self.UUID = uuid.uuid4()
+        self.controller = volume_actions.VolumeActionsController()
         self.api_patchers = {}
         for _meth in self._methods:
             self.api_patchers[_meth] = mock.patch('cinder.volume.API.' + _meth)
@@ -189,6 +190,37 @@ class VolumeActionsTest(test.TestCase):
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(202, res.status_int)
 
+    def test_volume_detach_raises_remote_error(self):
+        volume_remote_error = \
+            messaging.RemoteError(exc_type='VolumeAttachmentNotFound')
+        with mock.patch.object(volume_api.API, 'detach',
+                               side_effect=volume_remote_error):
+            id = 1
+            vol = {"attachment_id": self.UUID}
+            body = {"os-detach": vol}
+            req = fakes.HTTPRequest.blank('/v2/tenant1/volumes/%s/action' % id)
+            self.assertRaises(webob.exc.HTTPBadRequest,
+                              self.controller._detach,
+                              req,
+                              id,
+                              body)
+
+    def test_volume_detach_raises_db_error(self):
+        # In case of DB error 500 error code is returned to user
+        volume_remote_error = \
+            messaging.RemoteError(exc_type='DBError')
+        with mock.patch.object(volume_api.API, 'detach',
+                               side_effect=volume_remote_error):
+            id = 1
+            vol = {"attachment_id": self.UUID}
+            body = {"os-detach": vol}
+            req = fakes.HTTPRequest.blank('/v2/tenant1/volumes/%s/action' % id)
+            self.assertRaises(messaging.RemoteError,
+                              self.controller._detach,
+                              req,
+                              id,
+                              body)
+
     def test_attach_with_invalid_arguments(self):
         # Invalid request to attach volume an invalid target
         body = {'os-attach': {'mountpoint': '/dev/vdc'}}

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index ee3be401eb01c0d81da2a1f5861ebe3d7cb85f92..54e23c1c91134f2300c14b98d9bf48f39c1ed092 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -855,7 +855,7 @@ class VolumeManager(manager.SchedulerDependentManager):
                           resource=volume)
                 raise
         else:
-            # We can try and degrade gracefuly here by trying to detach
+            # We can try and degrade gracefully here by trying to detach
             # a volume without the attachment_id here if the volume only has
             # one attachment.  This is for backwards compatibility.
             attachments = self.db.volume_attachment_get_used_by_volume_id(