Fix action on services raises TypeError

Freeze, thaw and failover_host actions on a service raises TypeError
when non-existent host or cluster name is passed to the service
update API. This TypeError exception raised is converted into
HTTPBadRequest at wsgi layer.

Fixed this issue by formatting error message when a service is not
found for a given host or cluster. Caught the ServiceNotFound at API
layer and raised InvalidInput error so a user will see 400 error
only instead of 404.

Closes-Bug: #1663205
Change-Id: I54b9575f293f64a5f31246d0345c753b8011d95f

cinder/api/contrib/services.py

@@ -112,13 +112,21 @@ class ServiceController(wsgi.Controller):
 
         return True
 
+    def _volume_api_proxy(self, fun, *args):
+        try:
+            return fun(*args)
+        except exception.ServiceNotFound as ex:
+            raise exception.InvalidInput(ex.msg)
+
     def _freeze(self, context, req, body):
         cluster_name, host = common.get_cluster_host(req, body, '3.26')
-        return self.volume_api.freeze_host(context, host, cluster_name)
+        return self._volume_api_proxy(self.volume_api.freeze_host, context,
+                                      host, cluster_name)
 
     def _thaw(self, context, req, body):
         cluster_name, host = common.get_cluster_host(req, body, '3.26')
-        return self.volume_api.thaw_host(context, host, cluster_name)
+        return self._volume_api_proxy(self.volume_api.thaw_host, context,
+                                      host, cluster_name)
 
     def _failover(self, context, req, body, clustered):
         # We set version to None to always get the cluster name from the body,
@@ -126,8 +134,8 @@ class ServiceController(wsgi.Controller):
         # it if the requested version is 3.26 or higher.
         version = '3.26' if clustered else False
         cluster_name, host = common.get_cluster_host(req, body, version)
-        self.volume_api.failover(context, host, cluster_name,
-                                 body.get('backend_id'))
+        self._volume_api_proxy(self.volume_api.failover, context, host,
+                               cluster_name, body.get('backend_id'))
         return webob.Response(status_int=http_client.ACCEPTED)
 
     def update(self, req, id, body):

cinder/tests/unit/api/contrib/test_services.py

@@ -795,6 +795,40 @@ class ServicesTest(test.TestCase):
                                               mock.sentinel.backend_id)
         self.assertEqual(202, res.status_code)
 
+    @ddt.data(('failover_host', {'host': mock.sentinel.host,
+                                 'backend_id': mock.sentinel.backend_id}),
+              ('freeze', {'host': mock.sentinel.host}),
+              ('thaw', {'host': mock.sentinel.host}))
+    @ddt.unpack
+    @mock.patch('cinder.objects.ServiceList.get_all')
+    def test_services_action_host_not_found(self, method, body,
+                                            mock_get_all_services):
+        url = '/v2/%s/os-services/%s' % (fake.PROJECT_ID, method)
+        req = fakes.HTTPRequest.blank(url)
+        mock_get_all_services.return_value = []
+        msg = 'No service found with host=%s' % mock.sentinel.host
+        result = self.assertRaises(exception.InvalidInput,
+                                   self.controller.update,
+                                   req, method, body)
+        self.assertEqual(msg, result.msg)
+
+    @ddt.data(('failover', {'cluster': mock.sentinel.cluster,
+                            'backend_id': mock.sentinel.backend_id}),
+              ('freeze', {'cluster': mock.sentinel.cluster}),
+              ('thaw', {'cluster': mock.sentinel.cluster}))
+    @ddt.unpack
+    @mock.patch('cinder.objects.ServiceList.get_all')
+    def test_services_action_cluster_not_found(self, method, body,
+                                               mock_get_all_services):
+        url = '/v3/%s/os-services/%s' % (fake.PROJECT_ID, method)
+        req = fakes.HTTPRequest.blank(url, version='3.26')
+        mock_get_all_services.return_value = []
+        msg = 'No service found with cluster=%s' % mock.sentinel.cluster
+        result = self.assertRaises(exception.InvalidInput,
+                                   self.controller.update, req,
+                                   method, body)
+        self.assertEqual(msg, result.msg)
+
     def test_services_freeze(self):
         url = '/v2/%s/os-services/freeze' % fake.PROJECT_ID
         req = fakes.HTTPRequest.blank(url)

cinder/volume/api.py

@@ -1718,10 +1718,12 @@ class API(base.Base):
                            'binary': constants.VOLUME_BINARY})
 
         if not services:
-            msg = _('No service found with ') + (
-                'host=%(host)s' if host else 'cluster=%(cluster_name)s')
-            raise exception.ServiceNotFound(msg, host=host,
-                                            cluster_name=cluster_name)
+            if host:
+                msg = _("No service found with host=%s") % host
+            else:
+                msg = _("No service found with cluster=%s") % cluster_name
+
+            raise exception.ServiceNotFound(msg)
 
         cluster = services[0].cluster
         # Check that the host or cluster we received only results in 1 host or