From d7e9c9a0cd6edbd3788b517fc64162221e3b1087 Mon Sep 17 00:00:00 2001 From: Radomir Dopieralski Date: Thu, 27 Feb 2014 11:33:59 +0100 Subject: [PATCH] Error handling in the API Currently error handling in Tuskar-UI involves a lot of repeated code, as the same pattern is used everywhere and the same API calls tend to be handled in the same way no matter where they appear. This patch introduces a decorator that can be used to add default error handling to all API calls that take a request as a parameter. That default error handling can be disabled or modified using the special parameters passed to the call, all of which start with "_error" to avoid conflicts. Change-Id: Ie441789c471deeb6d64267bf4424f5661ef073af --- tuskar_ui/api.py | 17 ++++-- tuskar_ui/handle_errors.py | 56 +++++++++++++++++++ .../infrastructure/node_profiles/tables.py | 11 +--- .../infrastructure/node_profiles/views.py | 10 +--- tuskar_ui/infrastructure/nodes/tabs.py | 53 ++++-------------- tuskar_ui/infrastructure/nodes/tests.py | 15 +++-- tuskar_ui/infrastructure/nodes/views.py | 30 ++-------- tuskar_ui/infrastructure/overcloud/tabs.py | 8 +-- tuskar_ui/infrastructure/overcloud/tests.py | 10 ++-- tuskar_ui/infrastructure/overcloud/views.py | 16 ++---- 10 files changed, 109 insertions(+), 117 deletions(-) create mode 100644 tuskar_ui/handle_errors.py diff --git a/tuskar_ui/api.py b/tuskar_ui/api.py index ef91447f5..d712af6b4 100644 --- a/tuskar_ui/api.py +++ b/tuskar_ui/api.py @@ -15,18 +15,18 @@ import django.conf import heatclient import logging +from django.utils.translation import ugettext_lazy as _ from horizon.utils import memoized - from novaclient.v1_1.contrib import baremetal -from tuskarclient.v1 import client as tuskar_client - from openstack_dashboard.api import base from openstack_dashboard.api import glance from openstack_dashboard.api import heat from openstack_dashboard.api import nova from openstack_dashboard.test.test_data import utils +from tuskarclient.v1 import client as tuskar_client from tuskar_ui.cached_property import cached_property # noqa +from tuskar_ui.handle_errors import handle_errors # noqa from tuskar_ui.test.test_data import tuskar_data LOG = logging.getLogger(__name__) @@ -133,12 +133,14 @@ class NodeProfile(object): return cls(nova.flavor_get(request, node_profile_id)) @classmethod + @handle_errors(_("Unable to retrieve node profile list."), []) def list(cls, request): return [cls(item) for item in nova.flavor_list(request)] - @staticmethod + @classmethod @memoized.memoized - def list_deployed_ids(request): + @handle_errors(_("Unable to retrieve existing servers list."), []) + def list_deployed_ids(cls, request): """Get and memoize ID's of deployed node profiles.""" servers = nova.server_list(request)[0] return set(server.flavor['id'] for server in servers) @@ -199,6 +201,7 @@ class Overcloud(base.APIResourceWrapper): return [cls(oc, request=request) for oc in ocs] @classmethod + @handle_errors(_("Unable to retrieve deployment")) def get(cls, request, overcloud_id): """Return the Tuskar Overcloud that matches the ID @@ -489,6 +492,7 @@ class Node(base.APIResourceWrapper): return cls(node) @classmethod + @handle_errors(_("Unable to retrieve node")) def get(cls, request, uuid): """Return the Node in Ironic that matches the ID @@ -542,6 +546,7 @@ class Node(base.APIResourceWrapper): return cls(node, instance=server, request=request) @classmethod + @handle_errors(_("Unable to retrieve nodes"), []) def list(cls, request, associated=None): """Return a list of Nodes in Ironic @@ -799,6 +804,7 @@ class OvercloudRole(base.APIResourceWrapper): _attrs = ('id', 'name', 'description', 'image_name', 'flavor_id') @classmethod + @handle_errors(_("Unable to retrieve overcloud roles"), []) def list(cls, request): """Return a list of Overcloud Roles in Tuskar @@ -813,6 +819,7 @@ class OvercloudRole(base.APIResourceWrapper): return [cls(role) for role in roles] @classmethod + @handle_errors(_("Unable to retrieve overcloud role")) def get(cls, request, role_id): """Return the Tuskar OvercloudRole that matches the ID diff --git a/tuskar_ui/handle_errors.py b/tuskar_ui/handle_errors.py new file mode 100644 index 000000000..b1f14a3b5 --- /dev/null +++ b/tuskar_ui/handle_errors.py @@ -0,0 +1,56 @@ +# -*- coding: utf8 -*- +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import functools + +import horizon.exceptions + + +def handle_errors(error_message, error_default=None): + """A decorator for adding default error handling to API calls. + + It wraps the original method in a try-except block, with horizon's + error handling added. + + Note: it should only be used on methods that take request as the first + (second after self or cls) argument. + + The decorated method accepts a number of additional parameters: + + :param _error_handle: whether to handle the errors in this call + :param _error_message: override the error message + :param _error_default: override the default value returned on error + :param _error_redirect: specify a redirect url for errors + :param _error_ignore: ignore known errors + """ + def decorator(func): + @functools.wraps(func) + def wrapper(self, request, *args, **kwargs): + _error_handle = kwargs.pop('_error_handle', True) + _error_message = kwargs.pop('_error_message', error_message) + _error_default = kwargs.pop('_error_default', error_default) + _error_redirect = kwargs.pop('_error_redirect', None) + _error_ignore = kwargs.pop('_error_ignore', False) + if not _error_handle: + return func(self, request, *args, **kwargs) + try: + return func(self, request, *args, **kwargs) + except Exception: + horizon.exceptions.handle(request, _error_message, + ignore=_error_ignore, + redirect=_error_redirect) + return _error_default + wrapper.wrapped = func + return wrapper + return decorator diff --git a/tuskar_ui/infrastructure/node_profiles/tables.py b/tuskar_ui/infrastructure/node_profiles/tables.py index 1b7b7d1a6..9c2b9ad2a 100644 --- a/tuskar_ui/infrastructure/node_profiles/tables.py +++ b/tuskar_ui/infrastructure/node_profiles/tables.py @@ -14,7 +14,6 @@ from django.utils.translation import ugettext_lazy as _ -from horizon import exceptions from horizon import tables from openstack_dashboard.dashboards.admin.flavors \ @@ -46,13 +45,9 @@ class DeleteNodeProfile(flavor_tables.DeleteFlavor): :type datum: tuskar_ui.api.NodeProfile """ if datum is not None: - try: - deployed_profiles = api.NodeProfile.list_deployed_ids(request) - except Exception: - msg = _('Unable to retrieve existing servers list.') - exceptions.handle(request, msg) - return False - if datum.id in deployed_profiles: + deployed_profiles = api.NodeProfile.list_deployed_ids( + request, _error_default=None) + if deployed_profiles is None or datum.id in deployed_profiles: return False return super(DeleteNodeProfile, self).allowed(request, datum) diff --git a/tuskar_ui/infrastructure/node_profiles/views.py b/tuskar_ui/infrastructure/node_profiles/views.py index 0f24331d6..d40cb1a2c 100644 --- a/tuskar_ui/infrastructure/node_profiles/views.py +++ b/tuskar_ui/infrastructure/node_profiles/views.py @@ -12,9 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -from django.utils.translation import ugettext_lazy as _ - -from horizon import exceptions from horizon import tables from horizon import workflows @@ -30,12 +27,7 @@ class IndexView(tables.DataTableView): template_name = 'infrastructure/node_profiles/index.html' def get_data(self): - try: - node_profiles = api.NodeProfile.list(self.request) - except Exception: - exceptions.handle(self.request, - _('Unable to retrieve node profile list.')) - return [] + node_profiles = api.NodeProfile.list(self.request) node_profiles.sort(key=lambda np: (np.vcpus, np.ram, np.disk)) return node_profiles diff --git a/tuskar_ui/infrastructure/nodes/tabs.py b/tuskar_ui/infrastructure/nodes/tabs.py index 0b01133d4..0a4bc23f0 100644 --- a/tuskar_ui/infrastructure/nodes/tabs.py +++ b/tuskar_ui/infrastructure/nodes/tabs.py @@ -17,7 +17,6 @@ from django.core import urlresolvers from django.utils.translation import ugettext_lazy as _ -from horizon import exceptions from horizon import tabs from tuskar_ui import api @@ -30,14 +29,8 @@ class OverviewTab(tabs.Tab): template_name = ("infrastructure/nodes/_overview.html") def get_context_data(self, request): - try: - free_nodes = api.Node.list(request, associated=False) - deployed_nodes = api.Node.list(request, associated=True) - except Exception: - free_nodes = [] - deployed_nodes = [] - exceptions.handle(request, - _('Unable to retrieve nodes.')) + free_nodes = api.Node.list(request, associated=False) + deployed_nodes = api.Node.list(request, associated=True) free_nodes_down = [node for node in free_nodes if node.power_state != 'on'] @@ -67,25 +60,14 @@ class DeployedTab(tabs.TableTab): template_name = ("horizon/common/_detail_table.html") def get_items_count(self): - try: - deployed_nodes_count = len(api.Node.list(self.request, - associated=True)) - except Exception: - deployed_nodes_count = 0 - exceptions.handle(self.request, - _('Unable to retrieve deployed nodes count.')) + deployed_nodes_count = len(api.Node.list(self.request, + associated=True)) return deployed_nodes_count def get_deployed_nodes_data(self): - try: - deployed_nodes = api.Node.list(self.request, associated=True) - except Exception: - deployed_nodes = [] - redirect = urlresolvers.reverse( - 'horizon:infrastructure:nodes:index') - exceptions.handle(self.request, - _('Unable to retrieve deployed nodes.'), - redirect=redirect) + redirect = urlresolvers.reverse('horizon:infrastructure:nodes:index') + deployed_nodes = api.Node.list(self.request, associated=True, + _error_redirect=redirect) return deployed_nodes @@ -97,25 +79,14 @@ class FreeTab(tabs.TableTab): template_name = ("horizon/common/_detail_table.html") def get_items_count(self): - try: - free_nodes_count = len(api.Node.list(self.request, - associated=False)) - except Exception: - free_nodes_count = "0" - exceptions.handle(self.request, - _('Unable to retrieve free nodes count.')) + free_nodes_count = len(api.Node.list(self.request, + associated=False)) return free_nodes_count def get_free_nodes_data(self): - try: - free_nodes = api.Node.list(self.request, associated=False) - except Exception: - free_nodes = [] - redirect = urlresolvers.reverse( - 'horizon:infrastructure:nodes:index') - exceptions.handle(self.request, - _('Unable to retrieve free nodes.'), - redirect=redirect) + redirect = urlresolvers.reverse('horizon:infrastructure:nodes:index') + free_nodes = api.Node.list(self.request, associated=False, + _error_redirect=redirect) return free_nodes diff --git a/tuskar_ui/infrastructure/nodes/tests.py b/tuskar_ui/infrastructure/nodes/tests.py index e8e50b152..2d24068ea 100644 --- a/tuskar_ui/infrastructure/nodes/tests.py +++ b/tuskar_ui/infrastructure/nodes/tests.py @@ -18,6 +18,7 @@ from mock import patch, call # noqa from openstack_dashboard.test.test_data import utils from tuskar_ui import api as api +from tuskar_ui.handle_errors import handle_errors # noqa from tuskar_ui.test import helpers as test from tuskar_ui.test.test_data import tuskar_data @@ -30,6 +31,10 @@ tuskar_data.data(TEST_DATA) class NodesTests(test.BaseAdminViewTests): + @handle_errors("Error!", []) + def _raise_tuskar_exception(self, request, *args, **kwargs): + raise self.exceptions.tuskar + def test_index_get(self): with patch('tuskar_ui.api.Node', **{ @@ -65,10 +70,10 @@ class NodesTests(test.BaseAdminViewTests): def test_free_nodes_list_exception(self): with patch('tuskar_ui.api.Node', **{ 'spec_set': ['list'], - 'list.side_effect': self.exceptions.tuskar, + 'list.side_effect': self._raise_tuskar_exception, }) as mock: res = self.client.get(INDEX_URL + '?tab=nodes__free') - self.assertEqual(mock.list.call_count, 2) + self.assertEqual(mock.list.call_count, 3) self.assertRedirectsNoFollow(res, INDEX_URL) @@ -101,10 +106,10 @@ class NodesTests(test.BaseAdminViewTests): with patch('tuskar_ui.api.Node', **{ 'spec_set': ['list', 'instance'], 'instance': instance, - 'list.side_effect': self.exceptions.tuskar, + 'list.side_effect': self._raise_tuskar_exception, }) as mock: res = self.client.get(INDEX_URL + '?tab=nodes__deployed') - self.assertEqual(mock.list.call_count, 2) + self.assertEqual(mock.list.call_count, 3) self.assertRedirectsNoFollow(res, INDEX_URL) @@ -197,7 +202,7 @@ class NodesTests(test.BaseAdminViewTests): def test_node_detail_exception(self): with patch('tuskar_ui.api.Node', **{ 'spec_set': ['get'], - 'get.side_effect': self.exceptions.tuskar, + 'get.side_effect': self._raise_tuskar_exception, }) as mock: res = self.client.get( urlresolvers.reverse(DETAIL_VIEW, args=('no-such-node',)) diff --git a/tuskar_ui/infrastructure/nodes/views.py b/tuskar_ui/infrastructure/nodes/views.py index 792b2c0bc..cb99f647f 100644 --- a/tuskar_ui/infrastructure/nodes/views.py +++ b/tuskar_ui/infrastructure/nodes/views.py @@ -13,9 +13,7 @@ # under the License. from django.core.urlresolvers import reverse_lazy -from django.utils.translation import ugettext_lazy as _ -from horizon import exceptions from horizon import forms as horizon_forms from horizon import tabs as horizon_tabs from horizon import views as horizon_views @@ -30,23 +28,12 @@ class IndexView(horizon_tabs.TabbedTableView): template_name = 'infrastructure/nodes/index.html' def get_free_nodes_count(self): - try: - free_nodes_count = len(api.Node.list(self.request, - associated=False)) - except Exception: - free_nodes_count = 0 - exceptions.handle(self.request, - _('Unable to retrieve free nodes.')) + free_nodes_count = len(api.Node.list(self.request, associated=False)) return free_nodes_count def get_deployed_nodes_count(self): - try: - deployed_nodes_count = len(api.Node.list(self.request, - associated=True)) - except Exception: - deployed_nodes_count = 0 - exceptions.handle(self.request, - _('Unable to retrieve deployed nodes.')) + deployed_nodes_count = len(api.Node.list(self.request, + associated=True)) return deployed_nodes_count def get_context_data(self, **kwargs): @@ -81,14 +68,7 @@ class DetailView(horizon_views.APIView): def get_data(self, request, context, *args, **kwargs): node_uuid = kwargs.get('node_uuid') - - try: - node = api.Node.get(request, node_uuid) - except Exception: - node = None - redirect = reverse_lazy('horizon:infrastructure:nodes:index') - msg = _('Unable to retrieve node with UUID "%s"') % node_uuid - exceptions.handle(request, msg, redirect=redirect) - + redirect = reverse_lazy('horizon:infrastructure:nodes:index') + node = api.Node.get(request, node_uuid, _error_redirect=redirect) context['node'] = node return context diff --git a/tuskar_ui/infrastructure/overcloud/tabs.py b/tuskar_ui/infrastructure/overcloud/tabs.py index 207cb2245..2f4308a8b 100644 --- a/tuskar_ui/infrastructure/overcloud/tabs.py +++ b/tuskar_ui/infrastructure/overcloud/tabs.py @@ -16,7 +16,6 @@ from django.utils.translation import ugettext_lazy as _ from django.utils.translation import ungettext_lazy -from horizon import exceptions from horizon import tabs from tuskar_ui import api @@ -64,12 +63,7 @@ class OverviewTab(tabs.Tab): def get_context_data(self, request, **kwargs): overcloud = self.tab_group.kwargs['overcloud'] - try: - roles = api.OvercloudRole.list(request) - except Exception: - roles = [] - exceptions.handle(request, - _("Unable to retrieve overcloud roles.")) + roles = api.OvercloudRole.list(request) role_data = [_get_role_data(overcloud, role) for role in roles] total = sum(d['node_count'] for d in role_data) progress = 100 * sum(d.get('running_node_count', 0) diff --git a/tuskar_ui/infrastructure/overcloud/tests.py b/tuskar_ui/infrastructure/overcloud/tests.py index 97a872595..56d64ec32 100644 --- a/tuskar_ui/infrastructure/overcloud/tests.py +++ b/tuskar_ui/infrastructure/overcloud/tests.py @@ -60,11 +60,11 @@ def _mock_overcloud(**kwargs): 'update', ], 'counts': [], - 'create.side_effect': lambda *args: oc, + 'create.side_effect': lambda *args, **kwargs: oc, 'dashboard_url': '', 'delete.return_value': None, - 'get.side_effect': lambda *args: oc, - 'get_the_overcloud.side_effect': lambda *args: oc, + 'get.side_effect': lambda *args, **kwargs: oc, + 'get_the_overcloud.side_effect': lambda *args, **kwargs: oc, 'id': 1, 'is_deployed': True, 'is_deploying': False, @@ -72,7 +72,7 @@ def _mock_overcloud(**kwargs): 'resources.return_value': [], 'stack_events': [], 'stack': stack, - 'update.side_effect': lambda *args: oc, + 'update.side_effect': lambda *args, **kwargs: oc, } params.update(kwargs) with patch('tuskar_ui.api.Overcloud', **params) as Overcloud: @@ -379,7 +379,7 @@ class OvercloudTests(test.BaseAdminViewTests): 'image_name', 'flavor_id', ], - 'get.side_effect': lambda *args: role, + 'get.side_effect': lambda *args, **kwargs: role, 'name': 'Compute', 'description': '...', 'image_name': '', diff --git a/tuskar_ui/infrastructure/overcloud/views.py b/tuskar_ui/infrastructure/overcloud/views.py index fe23b3b0e..b86655e97 100644 --- a/tuskar_ui/infrastructure/overcloud/views.py +++ b/tuskar_ui/infrastructure/overcloud/views.py @@ -16,7 +16,6 @@ from django.core.urlresolvers import reverse from django.utils.translation import ugettext_lazy as _ from django.views.generic import base as base_views -from horizon import exceptions import horizon.forms from horizon import tables as horizon_tables from horizon import tabs as horizon_tabs @@ -41,12 +40,8 @@ class OvercloudMixin(object): if redirect is None: redirect = reverse(INDEX_URL) overcloud_id = self.kwargs['overcloud_id'] - try: - overcloud = api.Overcloud.get(self.request, overcloud_id) - except Exception: - msg = _("Unable to retrieve deployment.") - exceptions.handle(self.request, msg, redirect=redirect) - + overcloud = api.Overcloud.get(self.request, overcloud_id, + _error_redirect=redirect) return overcloud @@ -54,11 +49,8 @@ class OvercloudRoleMixin(object): @memoized.memoized def get_role(self, redirect=None): role_id = self.kwargs['role_id'] - try: - role = api.OvercloudRole.get(self.request, role_id) - except Exception: - msg = _("Unable to retrieve overcloud role.") - exceptions.handle(self.request, msg, redirect=redirect) + role = api.OvercloudRole.get(self.request, role_id, + _error_redirect=redirect) return role