From: Sebastian Wagner Date: Tue, 24 Apr 2018 16:11:15 +0000 (+0200) Subject: mgr/dashboard: Improve exception handling X-Git-Tag: v14.0.0~185^2~8 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=7f16fb14796fda36f7effcbfacde79644a00e9b2;p=ceph.git mgr/dashboard: Improve exception handling * Added `dashboard_exception_handler()` to catch our exceptions. * Changed the behaviour of `ViewCache` to raise exceptions * Added `RadosReturnError` raised in `send_command()` * Added unit tests Signed-off-by: Sebastian Wagner --- diff --git a/src/pybind/mgr/dashboard/controllers/__init__.py b/src/pybind/mgr/dashboard/controllers/__init__.py index 0ba43099cc6..3c47ef96c48 100644 --- a/src/pybind/mgr/dashboard/controllers/__init__.py +++ b/src/pybind/mgr/dashboard/controllers/__init__.py @@ -5,7 +5,6 @@ from __future__ import absolute_import import collections from datetime import datetime, timedelta import fnmatch -from functools import wraps import importlib import inspect import json @@ -21,7 +20,9 @@ from six import add_metaclass from .. import logger from ..settings import Settings -from ..tools import Session, TaskManager +from ..tools import Session, wraps, getargspec, TaskManager +from ..exceptions import ViewCacheNoDataException, DashboardException +from ..services.exception import serialize_dashboard_exception def ApiController(path): @@ -31,7 +32,8 @@ def ApiController(path): config = { 'tools.sessions.on': True, 'tools.sessions.name': Session.NAME, - 'tools.session_expire_at_browser_close.on': True + 'tools.session_expire_at_browser_close.on': True, + 'tools.dashboard_exception_handler.on': True, } if not hasattr(cls, '_cp_config'): cls._cp_config = {} @@ -155,6 +157,7 @@ class ApiRoot(object): def browsable_api_view(meth): + @wraps(meth) def wrapper(self, *vpath, **kwargs): assert isinstance(self, BaseController) if not Settings.ENABLE_BROWSABLE_API: @@ -276,7 +279,7 @@ class Task(object): sig = inspect.signature(func) arg_list = [a for a in sig.parameters] else: - sig = inspect.getargspec(func) + sig = getargspec(func) arg_list = [a for a in sig.args] for idx, arg in enumerate(arg_list): @@ -336,10 +339,7 @@ class BaseControllerMeta(type): if isinstance(thing, (types.FunctionType, types.MethodType))\ and getattr(thing, 'exposed', False): - # @cherrypy.tools.json_out() is incompatible with our browsable_api_view decorator. - cp_config = getattr(thing, '_cp_config', {}) - if not cp_config.get('tools.json_out.on', False): - setattr(new_cls, a_name, browsable_api_view(thing)) + setattr(new_cls, a_name, browsable_api_view(thing)) return new_cls @@ -355,18 +355,9 @@ class BaseController(object): @classmethod def _parse_function_args(cls, func): - # pylint: disable=deprecated-method - if sys.version_info > (3, 0): # pylint: disable=no-else-return - sig = inspect.signature(func) - cargs = [k for k, v in sig.parameters.items() - if k != 'self' and v.default is inspect.Parameter.empty and - (v.kind == inspect.Parameter.POSITIONAL_ONLY or - v.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD)] - else: - func = getattr(func, '__wrapped__', func) - args = inspect.getargspec(func) - nd = len(args.args) if not args.defaults else -len(args.defaults) - cargs = args.args[1:nd] + args = getargspec(func) + nd = len(args.args) if not args.defaults else -len(args.defaults) + cargs = args.args[1:nd] # filter out controller path params for idx, step in enumerate(cls._cp_path_.split('/')): @@ -522,10 +513,7 @@ class RESTController(BaseController): @staticmethod def _function_args(func): - if sys.version_info > (3, 0): # pylint: disable=no-else-return - return list(inspect.signature(func).parameters.keys()) - else: - return inspect.getargspec(func).args[1:] # pylint: disable=deprecated-method + return getargspec(func).args[1:] @staticmethod def _takes_json(func): diff --git a/src/pybind/mgr/dashboard/exceptions.py b/src/pybind/mgr/dashboard/exceptions.py index ff356d303c7..c24eab497e8 100644 --- a/src/pybind/mgr/dashboard/exceptions.py +++ b/src/pybind/mgr/dashboard/exceptions.py @@ -2,4 +2,44 @@ from __future__ import absolute_import -# Add generic exceptions here. +class ViewCacheNoDataException(Exception): + def __init__(self): + self.status = 200 + super(ViewCacheNoDataException, self).__init__('ViewCache: unable to retrieve data') + + +class DashboardException(Exception): + """ + Used for exceptions that are already handled and should end up as a user error. + Or, as a replacement for cherrypy.HTTPError(...) + + Typically, you don't inherent from DashboardException + """ + + # pylint: disable=too-many-arguments + def __init__(self, e=None, code=None, component=None, http_status_code=None, msg=None): + super(DashboardException, self).__init__(msg) + self._code = code + self.component = component + if e: + self.e = e + if http_status_code: + self.status = http_status_code + else: + self.status = 400 + + def __str__(self): + try: + return str(self.e) + except AttributeError: + return super(DashboardException, self).__str__() + + @property + def errno(self): + return self.e.errno + + @property + def code(self): + if self._code: + return str(self._code) + return str(abs(self.errno)) diff --git a/src/pybind/mgr/dashboard/module.py b/src/pybind/mgr/dashboard/module.py index 8f4a542e062..55bf33d8029 100644 --- a/src/pybind/mgr/dashboard/module.py +++ b/src/pybind/mgr/dashboard/module.py @@ -23,6 +23,7 @@ except ImportError: try: import cherrypy + from cherrypy._cptools import HandlerWrapperTool except ImportError: # To be picked up and reported by .can_run() cherrypy = None @@ -60,6 +61,7 @@ from .controllers import generate_routes, json_error_page from .controllers.auth import Auth from .tools import SessionExpireAtBrowserCloseTool, NotificationQueue, \ RequestLoggingTool, TaskManager +from .services.exception import dashboard_exception_handler from .settings import options_command_list, options_schema_list, \ handle_option_command @@ -125,6 +127,8 @@ class SSLCherryPyConfig(object): cherrypy.tools.authenticate = cherrypy.Tool('before_handler', Auth.check_auth) cherrypy.tools.session_expire_at_browser_close = SessionExpireAtBrowserCloseTool() cherrypy.tools.request_logging = RequestLoggingTool() + cherrypy.tools.dashboard_exception_handler = HandlerWrapperTool(dashboard_exception_handler, + priority=31) # SSL initialization cert = self.get_store("crt") diff --git a/src/pybind/mgr/dashboard/services/ceph_service.py b/src/pybind/mgr/dashboard/services/ceph_service.py index 6e91165a3b9..7671f89cfa3 100644 --- a/src/pybind/mgr/dashboard/services/ceph_service.py +++ b/src/pybind/mgr/dashboard/services/ceph_service.py @@ -6,6 +6,8 @@ import collections from collections import defaultdict import json +import rados + from mgr_module import CommandResult try: @@ -20,6 +22,14 @@ except ImportError: from .. import logger, mgr +class SendCommandError(rados.Error): + def __init__(self, err, prefix, argdict, errno): + self.errno = errno + self.prefix = prefix + self.argdict = argdict + super(SendCommandError, self).__init__(err) + + class CephService(object): @classmethod def get_service_map(cls, service_name): @@ -153,7 +163,7 @@ class CephService(object): msg = "send_command '{}' failed. (r={}, outs=\"{}\", kwargs={})".format(prefix, r, outs, kwargs) logger.error(msg) - raise ValueError(msg) + raise SendCommandError(outs, prefix, argdict, r) else: try: return json.loads(outb) diff --git a/src/pybind/mgr/dashboard/services/exception.py b/src/pybind/mgr/dashboard/services/exception.py new file mode 100644 index 00000000000..93dff9a84cf --- /dev/null +++ b/src/pybind/mgr/dashboard/services/exception.py @@ -0,0 +1,114 @@ +# -*- coding: utf-8 -*- +from __future__ import absolute_import + +import json +import sys +from contextlib import contextmanager + +import cherrypy + +import rbd +import rados + +from .. import logger +from ..services.ceph_service import SendCommandError +from ..exceptions import ViewCacheNoDataException, DashboardException +from ..tools import wraps + +if sys.version_info < (3, 0): + # Monkey-patch a __call__ method into @contextmanager to make + # it compatible to Python 3 + + from contextlib import GeneratorContextManager # pylint: disable=no-name-in-module + + def init(self, *args): + if len(args) == 1: + self.gen = args[0] + elif len(args) == 3: + self.func, self.args, self.kwargs = args + else: + raise TypeError() + + def enter(self): + if hasattr(self, 'func'): + self.gen = self.func(*self.args, **self.kwargs) + try: + return self.gen.next() + except StopIteration: + raise RuntimeError("generator didn't yield") + + def call(self, f): + @wraps(f) + def wrapper(*args, **kwargs): + with self: + return f(*args, **kwargs) + + return wrapper + + GeneratorContextManager.__init__ = init + GeneratorContextManager.__enter__ = enter + GeneratorContextManager.__call__ = call + + # pylint: disable=function-redefined + def contextmanager(func): + + @wraps(func) + def helper(*args, **kwds): + return GeneratorContextManager(func, args, kwds) + + return helper + + +def serialize_dashboard_exception(e): + from ..tools import ViewCache + cherrypy.response.status = getattr(e, 'status', 400) + if isinstance(e, ViewCacheNoDataException): + return {'status': ViewCache.VALUE_NONE, 'value': None} + + out = dict(detail=str(e)) + try: + out['code'] = e.code + except AttributeError: + pass + component = getattr(e, 'component', None) + out['component'] = component if component else None + return out + + +def dashboard_exception_handler(handler, *args, **kwargs): + try: + with handle_rados_error(component=None): # make the None controller the fallback. + return handler(*args, **kwargs) + # Don't catch cherrypy.* Exceptions. + except (ViewCacheNoDataException, DashboardException) as e: + logger.exception('dashboard_exception_handler') + cherrypy.response.headers['Content-Type'] = 'application/json' + return json.dumps(serialize_dashboard_exception(e)).encode('utf-8') + + +@contextmanager +def handle_rbd_error(): + try: + yield + except rbd.OSError as e: + raise DashboardException(e, component='rbd') + except rbd.Error as e: + raise DashboardException(e, component='rbd', code=e.__class__.__name__) + + +@contextmanager +def handle_rados_error(component): + try: + yield + except rados.OSError as e: + raise DashboardException(e, component=component) + except rados.Error as e: + raise DashboardException(e, component=component, code=e.__class__.__name__) + + +@contextmanager +def handle_send_command_error(component): + try: + yield + except SendCommandError as e: + raise DashboardException(e, component=component) diff --git a/src/pybind/mgr/dashboard/tests/helper.py b/src/pybind/mgr/dashboard/tests/helper.py index b41b5e6be18..76702867682 100644 --- a/src/pybind/mgr/dashboard/tests/helper.py +++ b/src/pybind/mgr/dashboard/tests/helper.py @@ -7,11 +7,13 @@ import threading import time import cherrypy +from cherrypy._cptools import HandlerWrapperTool from cherrypy.test import helper from .. import logger from ..controllers.auth import Auth from ..controllers import json_error_page, generate_controller_routes +from ..services.exception import dashboard_exception_handler from ..tools import SessionExpireAtBrowserCloseTool @@ -31,6 +33,8 @@ class ControllerTestCase(helper.CPWebCase): def __init__(self, *args, **kwargs): cherrypy.tools.authenticate = cherrypy.Tool('before_handler', Auth.check_auth) cherrypy.tools.session_expire_at_browser_close = SessionExpireAtBrowserCloseTool() + cherrypy.tools.dashboard_exception_handler = HandlerWrapperTool(dashboard_exception_handler, + priority=31) cherrypy.config.update({'error_page.default': json_error_page}) super(ControllerTestCase, self).__init__(*args, **kwargs) diff --git a/src/pybind/mgr/dashboard/tests/test_exceptions.py b/src/pybind/mgr/dashboard/tests/test_exceptions.py new file mode 100644 index 00000000000..ae11192487b --- /dev/null +++ b/src/pybind/mgr/dashboard/tests/test_exceptions.py @@ -0,0 +1,156 @@ +# -*- coding: utf-8 -*- +from __future__ import absolute_import + +import cherrypy + +import rados +from ..services.ceph_service import SendCommandError +from ..controllers import RESTController, ApiController +from .helper import ControllerTestCase +from ..services.exception import handle_rados_error, handle_send_command_error, \ + serialize_dashboard_exception +from ..tools import ViewCache, TaskManager + + +# pylint: disable=W0613 +@ApiController('foo') +class FooResource(RESTController): + @cherrypy.expose + @cherrypy.tools.json_out() + @handle_rados_error('foo') + def no_exception(self, param1, param2): + return [param1, param2] + + @cherrypy.expose + @cherrypy.tools.json_out() + @handle_rados_error('foo') + def error_foo_controller(self): + raise rados.OSError('hi', errno=-42) + + @cherrypy.expose + @cherrypy.tools.json_out() + @handle_send_command_error('foo') + def error_send_command(self): + raise SendCommandError('hi', 'prefix', {}, -42) + + @cherrypy.expose + @cherrypy.tools.json_out() + def error_generic(self): + raise rados.Error('hi') + + @cherrypy.expose + @cherrypy.tools.json_out() + def vc_no_data(self): + @ViewCache(timeout=0) + def _no_data(): + import time + time.sleep(0.2) + + _no_data() + assert False + + @handle_rados_error('foo') + @cherrypy.expose + @cherrypy.tools.json_out() + def vc_exception(self): + @ViewCache(timeout=10) + def _raise(): + raise rados.OSError('hi', errno=-42) + + _raise() + assert False + + @cherrypy.expose + @cherrypy.tools.json_out() + def internal_server_error(self): + return 1/0 + + @handle_send_command_error('foo') + def list(self): + raise SendCommandError('list', 'prefix', {}, -42) + + @cherrypy.expose + @cherrypy.tools.json_out() + @Task('task_exceptions/task_exception', {}, 1.0, + exception_handler=serialize_dashboard_exception) + @handle_rados_error('foo') + def task_exception(self): + raise rados.OSError('hi', errno=-42) + + @cherrypy.expose + @cherrypy.tools.json_out() + def wait_task_exception(self): + ex, _ = TaskManager.list('task_exceptions/task_exception') + return bool(len(ex)) + + +# pylint: disable=C0102 +class Root(object): + foo = FooResource() + + +class RESTControllerTest(ControllerTestCase): + + @classmethod + def setup_server(cls): + cls.setup_controllers([FooResource]) + + def test_no_exception(self): + self._get('/foo/no_exception/a/b') + self.assertStatus(200) + self.assertJsonBody( + ['a', 'b'] + ) + + def test_error_foo_controller(self): + self._get('/foo/error_foo_controller') + self.assertStatus(400) + self.assertJsonBody( + {'detail': '[errno -42] hi', 'code': "42", 'component': 'foo'} + ) + + def test_error_send_command(self): + self._get('/foo/error_send_command') + self.assertStatus(400) + self.assertJsonBody( + {'detail': 'hi', 'code': "42", 'component': 'foo'} + ) + + def test_error_send_command_list(self): + self._get('/foo/') + self.assertStatus(400) + self.assertJsonBody( + {'detail': 'list', 'code': "42", 'component': 'foo'} + ) + + def test_error_send_command_bowsable_api(self): + self.getPage('/foo/error_send_command', headers=[('Accept', 'text/html')]) + for err in ["'detail': 'hi'", "'component': 'foo'"]: + self.assertIn(err.replace("'", "\'").encode('utf-8'), self.body) + + def test_error_foo_generic(self): + self._get('/foo/error_generic') + self.assertJsonBody({'detail': 'hi', 'code': 'Error', 'component': None}) + self.assertStatus(400) + + def test_viewcache_no_data(self): + self._get('/foo/vc_no_data') + self.assertStatus(200) + self.assertJsonBody({'status': ViewCache.VALUE_NONE, 'value': None}) + + def test_viewcache_exception(self): + self._get('/foo/vc_exception') + self.assertStatus(400) + self.assertJsonBody( + {'detail': '[errno -42] hi', 'code': "42", 'component': 'foo'} + ) + + def test_internal_server_error(self): + self._get('/foo/internal_server_error') + self.assertStatus(500) + self.assertIn('unexpected condition', self.jsonBody()['detail']) + + def test_404(self): + self._get('/foonot_found') + self.assertStatus(404) + self.assertIn('detail', self.jsonBody()) diff --git a/src/pybind/mgr/dashboard/tests/test_tools.py b/src/pybind/mgr/dashboard/tests/test_tools.py index e9a58d162cc..33ab223935f 100644 --- a/src/pybind/mgr/dashboard/tests/test_tools.py +++ b/src/pybind/mgr/dashboard/tests/test_tools.py @@ -3,9 +3,11 @@ from __future__ import absolute_import import unittest +import cherrypy from cherrypy.lib.sessions import RamSession from mock import patch +from ..services.exception import handle_rados_error from .helper import ControllerTestCase from ..controllers import RESTController, ApiController from ..tools import is_valid_ipv6_address, dict_contains_path @@ -47,6 +49,13 @@ class FooArgs(RESTController): def set(self, code, name=None, opt1=None, opt2=None): return {'code': code, 'name': name, 'opt1': opt1, 'opt2': opt2} + @handle_rados_error('foo') + def create(self, my_arg_name): + return my_arg_name + + def list(self): + raise cherrypy.NotFound() + # pylint: disable=blacklisted-name class Root(object): @@ -134,6 +143,10 @@ class RESTControllerTest(ControllerTestCase): method='put') self.assertStatus(404) + def test_create_form(self): + self.getPage('/fooargs', headers=[('Accept', 'text/html')]) + self.assertIn('my_arg_name', self.body.decode('utf-8')) + class TestFunctions(unittest.TestCase): diff --git a/src/pybind/mgr/dashboard/tools.py b/src/pybind/mgr/dashboard/tools.py index 2ff66bfe044..d1739e44eb0 100644 --- a/src/pybind/mgr/dashboard/tools.py +++ b/src/pybind/mgr/dashboard/tools.py @@ -1,6 +1,10 @@ # -*- coding: utf-8 -*- from __future__ import absolute_import +import sys +import inspect +import functools + import collections from datetime import datetime, timedelta import fnmatch @@ -11,6 +15,7 @@ from six.moves import urllib import cherrypy from . import logger +from .exceptions import ViewCacheNoDataException class RequestLoggingTool(cherrypy.Tool): @@ -197,7 +202,7 @@ class ViewCache(object): # We have some data, but it doesn't meet freshness requirements return ViewCache.VALUE_STALE, self.value # We have no data, not even stale data - return ViewCache.VALUE_NONE, None + raise ViewCacheNoDataException() def __init__(self, timeout=5): self.timeout = timeout @@ -708,3 +713,26 @@ def dict_contains_path(dct, keys): return dict_contains_path(dct, keys) return False return True + + +if sys.version_info > (3, 0): + wraps = functools.wraps + _getargspec = inspect.getfullargspec +else: + def wraps(func): + def decorator(wrapper): + new_wrapper = functools.wraps(func)(wrapper) + new_wrapper.__wrapped__ = func # set __wrapped__ even for Python 2 + return new_wrapper + return decorator + + _getargspec = inspect.getargspec + + +def getargspec(func): + try: + while True: + func = func.__wrapped__ + except AttributeError: + pass + return _getargspec(func)