From 716e64a3e9dfc0d1ea29da7d6302eab0ae244bfc Mon Sep 17 00:00:00 2001
From: PranaliDeore <pranali11.deore@nttdata.com>
Date: Tue, 28 Apr 2015 05:00:28 -0700
Subject: [PATCH] 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
---
 cinder/api/contrib/volume_actions.py          | 14 +++++++-
 .../unit/api/contrib/test_volume_actions.py   | 32 +++++++++++++++++++
 cinder/volume/manager.py                      |  2 +-
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py
index 423cf17a2..254aff53e 100644
--- 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 b7a522518..da0ec9987 100644
--- 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 ee3be401e..54e23c1c9 100644
--- 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(
-- 
2.45.2