From 1b757c328d43d8154282794fddb8822ea265b1cf Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Thu, 10 Dec 2015 17:34:09 -0500 Subject: [PATCH] Fix wrap_exception to get all arguments for payload The wrap_exception decorator that attempts to send a notification when exceptions occur was not sending all the arguments it was intending to. It relies on getcallargs to get the arguments and argument names for the called method but if the method has another decorator on it getcallargs pulls information for the decorator rather than the decorated method. This pulls the decorated method with get_wrapped_function and then calls getcallargs. get_wrapped_function was moved to safeutils because utils.py can't be imported by exception.py without a circular import. A few tests were updated to include the id on the instance object used. This was done because serialization of the object includes the instance.name field which assumes that id is set to populate the CONF.instance_name_template. When id is not set it triggers a lazy load which fails in the test environment. Change-Id: I87d8691a2aae6f3555177364f3c40a490a6f7591 Closes-bug: 1525282 --- nova/compute/manager.py | 6 +-- nova/exception.py | 6 ++- nova/safe_utils.py | 21 +++++++++ nova/tests/unit/compute/test_compute.py | 6 ++- nova/tests/unit/test_safeutils.py | 54 ++++++++++++++++++++++ nova/tests/unit/test_utils.py | 53 --------------------- nova/tests/unit/virt/hyperv/test_driver.py | 4 +- nova/utils.py | 24 +--------- 8 files changed, 90 insertions(+), 84 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index d7f8e2a5d953..4f165be7e796 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -287,7 +287,7 @@ def errors_out_migration(function): return function(self, context, *args, **kwargs) except Exception as ex: with excutils.save_and_reraise_exception(): - wrapped_func = utils.get_wrapped_function(function) + wrapped_func = safe_utils.get_wrapped_function(function) keyed_args = safe_utils.getcallargs(wrapped_func, self, context, *args, **kwargs) migration = keyed_args['migration'] @@ -329,7 +329,7 @@ def reverts_task_state(function): e.format_message()) except Exception: with excutils.save_and_reraise_exception(): - wrapped_func = utils.get_wrapped_function(function) + wrapped_func = safe_utils.get_wrapped_function(function) keyed_args = safe_utils.getcallargs(wrapped_func, self, context, *args, **kwargs) # NOTE(mriedem): 'instance' must be in keyed_args because we @@ -389,7 +389,7 @@ def wrap_instance_event(function): @functools.wraps(function) def decorated_function(self, context, *args, **kwargs): - wrapped_func = utils.get_wrapped_function(function) + wrapped_func = safe_utils.get_wrapped_function(function) keyed_args = safe_utils.getcallargs(wrapped_func, self, context, *args, **kwargs) instance_uuid = keyed_args['instance']['uuid'] diff --git a/nova/exception.py b/nova/exception.py index b9ca300459bc..c01e68e37650 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -90,8 +90,10 @@ def wrap_exception(notifier=None, get_notifier=None): with excutils.save_and_reraise_exception(): if notifier or get_notifier: payload = dict(exception=e) - call_dict = safe_utils.getcallargs(f, self, context, - *args, **kw) + wrapped_func = safe_utils.get_wrapped_function(f) + call_dict = safe_utils.getcallargs(wrapped_func, self, + context, *args, + **kw) # self can't be serialized and shouldn't be in the # payload call_dict.pop('self', None) diff --git a/nova/safe_utils.py b/nova/safe_utils.py index 2fad77b2a663..8fafdc0f70d5 100644 --- a/nova/safe_utils.py +++ b/nova/safe_utils.py @@ -49,3 +49,24 @@ def getcallargs(function, *args, **kwargs): keyed_args[argname] = value return keyed_args + + +def get_wrapped_function(function): + """Get the method at the bottom of a stack of decorators.""" + if not hasattr(function, '__closure__') or not function.__closure__: + return function + + def _get_wrapped_function(function): + if not hasattr(function, '__closure__') or not function.__closure__: + return None + + for closure in function.__closure__: + func = closure.cell_contents + + deeper_func = _get_wrapped_function(func) + if deeper_func: + return deeper_func + elif hasattr(closure.cell_contents, '__call__'): + return closure.cell_contents + + return _get_wrapped_function(function) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 724e7e243e9b..ef465ad9f5eb 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -9458,6 +9458,7 @@ class ComputeAPITestCase(BaseTestCase): def test_attach_interface_failed(self): new_type = flavors.get_flavor_by_flavor_id('4') instance = objects.Instance( + id=42, uuid='f0000000-0000-0000-0000-000000000000', image_ref='foo', system_metadata={}, @@ -9500,7 +9501,7 @@ class ComputeAPITestCase(BaseTestCase): def test_detach_interface_failed(self): nwinfo, port_id = self.test_attach_interface() - instance = objects.Instance() + instance = objects.Instance(id=42) instance['uuid'] = 'f6000000-0000-0000-0000-000000000000' instance.info_cache = objects.InstanceInfoCache.new( self.context, 'f6000000-0000-0000-0000-000000000000') @@ -9524,7 +9525,7 @@ class ComputeAPITestCase(BaseTestCase): # Tests that when deallocate_port_for_instance fails we log the failure # before exiting compute.detach_interface. nwinfo, port_id = self.test_attach_interface() - instance = objects.Instance(uuid=uuidutils.generate_uuid()) + instance = objects.Instance(id=42, uuid=uuidutils.generate_uuid()) instance.info_cache = objects.InstanceInfoCache.new( self.context, 'f6000000-0000-0000-0000-000000000000') instance.info_cache.network_info = network_model.NetworkInfo.hydrate( @@ -9558,6 +9559,7 @@ class ComputeAPITestCase(BaseTestCase): block_device_obj.BlockDeviceMapping(), fake_bdm) instance = self._create_fake_instance_obj() + instance.id = 42 fake_volume = {'id': 'fake-volume-id'} with test.nested( diff --git a/nova/tests/unit/test_safeutils.py b/nova/tests/unit/test_safeutils.py index dc088e56981b..cf0d496bd984 100644 --- a/nova/tests/unit/test_safeutils.py +++ b/nova/tests/unit/test_safeutils.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import functools + from nova import safe_utils from nova import test @@ -108,3 +110,55 @@ class GetCallArgsTestCase(test.NoDBTestCase): self.assertEqual(1, len(callargs)) self.assertIn('instance', callargs) self.assertEqual({'uuid': 1}, callargs['instance']) + + +class WrappedCodeTestCase(test.NoDBTestCase): + """Test the get_wrapped_function utility method.""" + + def _wrapper(self, function): + @functools.wraps(function) + def decorated_function(self, *args, **kwargs): + function(self, *args, **kwargs) + return decorated_function + + def test_single_wrapped(self): + @self._wrapper + def wrapped(self, instance, red=None, blue=None): + pass + + func = safe_utils.get_wrapped_function(wrapped) + func_code = func.__code__ + self.assertEqual(4, len(func_code.co_varnames)) + self.assertIn('self', func_code.co_varnames) + self.assertIn('instance', func_code.co_varnames) + self.assertIn('red', func_code.co_varnames) + self.assertIn('blue', func_code.co_varnames) + + def test_double_wrapped(self): + @self._wrapper + @self._wrapper + def wrapped(self, instance, red=None, blue=None): + pass + + func = safe_utils.get_wrapped_function(wrapped) + func_code = func.__code__ + self.assertEqual(4, len(func_code.co_varnames)) + self.assertIn('self', func_code.co_varnames) + self.assertIn('instance', func_code.co_varnames) + self.assertIn('red', func_code.co_varnames) + self.assertIn('blue', func_code.co_varnames) + + def test_triple_wrapped(self): + @self._wrapper + @self._wrapper + @self._wrapper + def wrapped(self, instance, red=None, blue=None): + pass + + func = safe_utils.get_wrapped_function(wrapped) + func_code = func.__code__ + self.assertEqual(4, len(func_code.co_varnames)) + self.assertIn('self', func_code.co_varnames) + self.assertIn('instance', func_code.co_varnames) + self.assertIn('red', func_code.co_varnames) + self.assertIn('blue', func_code.co_varnames) diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index 72aeb4d9f8e0..d89cbe240a90 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -13,7 +13,6 @@ # under the License. import datetime -import functools import hashlib import importlib import logging @@ -847,58 +846,6 @@ class MetadataToDictTestCase(test.NoDBTestCase): self.assertEqual(utils.dict_to_metadata({}), []) -class WrappedCodeTestCase(test.NoDBTestCase): - """Test the get_wrapped_function utility method.""" - - def _wrapper(self, function): - @functools.wraps(function) - def decorated_function(self, *args, **kwargs): - function(self, *args, **kwargs) - return decorated_function - - def test_single_wrapped(self): - @self._wrapper - def wrapped(self, instance, red=None, blue=None): - pass - - func = utils.get_wrapped_function(wrapped) - func_code = func.__code__ - self.assertEqual(4, len(func_code.co_varnames)) - self.assertIn('self', func_code.co_varnames) - self.assertIn('instance', func_code.co_varnames) - self.assertIn('red', func_code.co_varnames) - self.assertIn('blue', func_code.co_varnames) - - def test_double_wrapped(self): - @self._wrapper - @self._wrapper - def wrapped(self, instance, red=None, blue=None): - pass - - func = utils.get_wrapped_function(wrapped) - func_code = func.__code__ - self.assertEqual(4, len(func_code.co_varnames)) - self.assertIn('self', func_code.co_varnames) - self.assertIn('instance', func_code.co_varnames) - self.assertIn('red', func_code.co_varnames) - self.assertIn('blue', func_code.co_varnames) - - def test_triple_wrapped(self): - @self._wrapper - @self._wrapper - @self._wrapper - def wrapped(self, instance, red=None, blue=None): - pass - - func = utils.get_wrapped_function(wrapped) - func_code = func.__code__ - self.assertEqual(4, len(func_code.co_varnames)) - self.assertIn('self', func_code.co_varnames) - self.assertIn('instance', func_code.co_varnames) - self.assertIn('red', func_code.co_varnames) - self.assertIn('blue', func_code.co_varnames) - - class ExpectedArgsTestCase(test.NoDBTestCase): def test_passes(self): @utils.expects_func_args('foo', 'baz') diff --git a/nova/tests/unit/virt/hyperv/test_driver.py b/nova/tests/unit/virt/hyperv/test_driver.py index cd726abbf1c1..39b72f0a7467 100644 --- a/nova/tests/unit/virt/hyperv/test_driver.py +++ b/nova/tests/unit/virt/hyperv/test_driver.py @@ -23,9 +23,9 @@ import mock from os_win import exceptions as os_win_exc from nova import exception +from nova import safe_utils from nova.tests.unit import fake_instance from nova.tests.unit.virt.hyperv import test_base -from nova import utils from nova.virt import driver as base_driver from nova.virt.hyperv import driver @@ -66,7 +66,7 @@ class HyperVDriverTestCase(test_base.HyperVBaseTestCase): if callable(class_member): mocked_method = mock.patch.object( driver.HyperVDriver, attr, - utils.get_wrapped_function(class_member)) + safe_utils.get_wrapped_function(class_member)) mocked_method.start() self.addCleanup(mocked_method.stop) diff --git a/nova/utils.py b/nova/utils.py index 4654fcd5cd36..5f1007fe73aa 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -57,6 +57,7 @@ from six.moves import range from nova import exception from nova.i18n import _, _LE, _LI, _LW +from nova import safe_utils notify_decorator = 'nova.notifications.notify_decorator' @@ -1041,32 +1042,11 @@ def instance_sys_meta(instance): include_deleted=True) -def get_wrapped_function(function): - """Get the method at the bottom of a stack of decorators.""" - if not hasattr(function, '__closure__') or not function.__closure__: - return function - - def _get_wrapped_function(function): - if not hasattr(function, '__closure__') or not function.__closure__: - return None - - for closure in function.__closure__: - func = closure.cell_contents - - deeper_func = _get_wrapped_function(func) - if deeper_func: - return deeper_func - elif hasattr(closure.cell_contents, '__call__'): - return closure.cell_contents - - return _get_wrapped_function(function) - - def expects_func_args(*args): def _decorator_checker(dec): @functools.wraps(dec) def _decorator(f): - base_f = get_wrapped_function(f) + base_f = safe_utils.get_wrapped_function(f) arg_names, a, kw, _default = inspect.getargspec(base_f) if a or kw or set(args) <= set(arg_names): # NOTE (ndipanov): We can't really tell if correct stuff will