From 18ded430d0e256ce5f9eec3ef61b5337596f914a Mon Sep 17 00:00:00 2001 From: huangtianhua Date: Thu, 9 Jul 2015 09:15:10 +0800 Subject: [PATCH] Return error message if validate fail when clearing hook There are some situations: 1. User can run the command "heat hook-clear" on any resource in the stack, even if there is no hook defined on it, and no error or warning message is returned. 2. User run the command "heat hook-clear" on resource to clear the hook which resource unset, and no error or warning message is returned. 3. User run the command "heat hook-clear" on resource even if the resource's action is not support to signal, and no error or warning message is returned. 4. User run the command "heat hook-clear" to clear invalid hooks, and no error or warnning message is returned. This patch will check the situations above before call resource.signal, and will return error messages to user. Change-Id: Ifb9befad864ebe1bb5f8b419b95d1b3a95530573 Closes-Bug: #1472515 --- heat/api/aws/exception.py | 2 + heat/api/middleware/fault.py | 1 + heat/engine/resource.py | 69 +++++++----- heat/engine/service.py | 11 +- heat/tests/test_engine_service.py | 100 ++++++++++-------- heat/tests/test_resource.py | 13 ++- heat/tests/test_signal.py | 23 ++-- .../functional/test_autoscaling.py | 11 +- 8 files changed, 139 insertions(+), 91 deletions(-) diff --git a/heat/api/aws/exception.py b/heat/api/aws/exception.py index 656a9d9bdf..ae8cb7280e 100644 --- a/heat/api/aws/exception.py +++ b/heat/api/aws/exception.py @@ -302,6 +302,8 @@ def map_remote_error(ex): 'MissingCredentialError', 'ResourcePropertyConflict', 'PropertyUnspecifiedError', + 'NotSupported', + 'InvalidBreakPointHook', ) denied_errors = ('Forbidden', 'NotAuthorized') already_exists_errors = ('StackExists') diff --git a/heat/api/middleware/fault.py b/heat/api/middleware/fault.py index 889effb552..ac6725d334 100644 --- a/heat/api/middleware/fault.py +++ b/heat/api/middleware/fault.py @@ -92,6 +92,7 @@ class FaultWrapper(wsgi.Middleware): 'OrphanedObjectError': webob.exc.HTTPBadRequest, 'UnsupportedObjectError': webob.exc.HTTPBadRequest, 'ResourceTypeUnavailable': webob.exc.HTTPBadRequest, + 'InvalidBreakPointHook': webob.exc.HTTPBadRequest, } def _map_exception_to_error(self, class_exception): diff --git a/heat/engine/resource.py b/heat/engine/resource.py index d8328566e6..3f10dc0959 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -1478,17 +1478,40 @@ class Resource(object): ''' return base64.b64encode(data) - def signal(self, details=None): - ''' - signal the resource. Subclasses should provide a handle_signal() method - to implement the signal, the base-class raise an exception if no - handler is implemented. - ''' + def _signal_check_action(self): if self.action in self.no_signal_actions: self._add_event(self.action, self.status, 'Cannot signal resource during %s' % self.action) - ex = Exception(_('Cannot signal resource during %s') % self.action) - raise exception.ResourceFailure(ex, self) + msg = _('Signal resource during %s') % self.action + raise exception.NotSupported(feature=msg) + + def _signal_check_hook(self, details): + if details and 'unset_hook' in details: + hook = details['unset_hook'] + if not environment.valid_hook_type(hook): + msg = (_('Invalid hook type "%(hook)s" for %(resource)s') % + {'hook': hook, 'resource': six.text_type(self)}) + raise exception.InvalidBreakPointHook(message=msg) + + if not self.has_hook(hook): + msg = (_('The "%(hook)s" hook is not defined ' + 'on %(resource)s') % + {'hook': hook, 'resource': six.text_type(self)}) + raise exception.InvalidBreakPointHook(message=msg) + + def _unset_hook(self, details): + # Clear the hook without interfering with resources' + # `handle_signal` callbacks: + hook = details['unset_hook'] + self.clear_hook(hook) + LOG.info(_LI('Clearing %(hook)s hook on %(resource)s'), + {'hook': hook, 'resource': six.text_type(self)}) + self._add_event(self.action, self.status, + "Hook %s is cleared" % hook) + + def _handle_signal(self, details): + if not callable(getattr(self, 'handle_signal', None)): + raise exception.ResourceActionNotSupported(action='signal') def get_string_details(): if details is None: @@ -1507,22 +1530,6 @@ class Resource(object): return 'Unknown' - # Clear the hook without interfering with resources' - # `handle_signal` callbacks: - if (details and 'unset_hook' in details and - environment.valid_hook_type(details.get('unset_hook'))): - hook = details['unset_hook'] - if self.has_hook(hook): - self.clear_hook(hook) - LOG.info(_LI('Clearing %(hook)s hook on %(resource)s'), - {'hook': hook, 'resource': six.text_type(self)}) - self._add_event(self.action, self.status, - "Hook %s is cleared" % hook) - return - - if not callable(getattr(self, 'handle_signal', None)): - raise exception.ResourceActionNotSupported(action='signal') - try: signal_result = self.handle_signal(details) if signal_result: @@ -1539,6 +1546,20 @@ class Resource(object): failure = exception.ResourceFailure(ex, self) raise failure + def signal(self, details=None, need_check=True): + ''' + signal the resource. Subclasses should provide a handle_signal() method + to implement the signal, the base-class raise an exception if no + handler is implemented. + ''' + if need_check: + self._signal_check_action() + self._signal_check_hook(details) + if details and 'unset_hook' in details: + self._unset_hook(details) + return + self._handle_signal(details) + def handle_update(self, json_snippet=None, tmpl_diff=None, prop_diff=None): if prop_diff: raise exception.UpdateReplace(self.name) diff --git a/heat/engine/service.py b/heat/engine/service.py index e38ca4198c..0ee0ec30fd 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -1329,9 +1329,9 @@ class EngineService(service.Service): implementation. ''' - def _resource_signal(stack, rsrc, details): + def _resource_signal(stack, rsrc, details, need_check): LOG.debug("signaling resource %s:%s" % (stack.name, rsrc.name)) - rsrc.signal(details) + rsrc.signal(details, need_check) if not rsrc.signal_needs_metadata_updates: return @@ -1355,13 +1355,16 @@ class EngineService(service.Service): self._verify_stack_resource(stack, resource_name) rsrc = stack[resource_name] + if callable(rsrc.signal): + rsrc._signal_check_action() + rsrc._signal_check_hook(details) if sync_call: - _resource_signal(stack, rsrc, details) + _resource_signal(stack, rsrc, details, False) return rsrc.metadata_get() else: self.thread_group_mgr.start(stack.id, _resource_signal, - stack, rsrc, details) + stack, rsrc, details, False) @context.request_context def find_physical_resource(self, cnxt, physical_resource_id): diff --git a/heat/tests/test_engine_service.py b/heat/tests/test_engine_service.py index 93d2d5f694..24e67edc15 100644 --- a/heat/tests/test_engine_service.py +++ b/heat/tests/test_engine_service.py @@ -1240,21 +1240,24 @@ class StackServiceTest(common.HeatTestCase): self.m.VerifyAll() - def test_signal_reception_async(self): - self.eng.thread_group_mgr = tools.DummyThreadGroupMgrLogStart() - stack_name = 'signal_reception_async' + def _stack_create(self, stack_name): stack = tools.get_stack(stack_name, self.ctx, policy_template) - self.stack = stack tools.setup_keystone_mocks(self.m, stack) self.m.ReplayAll() stack.store() stack.create() - test_data = {'food': 'yum'} - self.m.StubOutWithMock(service.EngineService, '_get_stack') - s = stack_object.Stack.get_by_id(self.ctx, self.stack.id) + s = stack_object.Stack.get_by_id(self.ctx, stack.id) service.EngineService._get_stack(self.ctx, - self.stack.identifier()).AndReturn(s) + stack.identifier()).AndReturn(s) + self.m.ReplayAll() + return stack + + def test_signal_reception_async(self): + self.eng.thread_group_mgr = tools.DummyThreadGroupMgrLogStart() + stack_name = 'signal_reception_async' + self.stack = self._stack_create(stack_name) + test_data = {'food': 'yum'} self.m.ReplayAll() @@ -1269,21 +1272,11 @@ class StackServiceTest(common.HeatTestCase): def test_signal_reception_sync(self): stack_name = 'signal_reception_sync' - stack = tools.get_stack(stack_name, self.ctx, policy_template) - self.stack = stack - tools.setup_keystone_mocks(self.m, stack) - self.m.ReplayAll() - stack.store() - stack.create() + self.stack = self._stack_create(stack_name) test_data = {'food': 'yum'} - self.m.StubOutWithMock(service.EngineService, '_get_stack') - s = stack_object.Stack.get_by_id(self.ctx, self.stack.id) - service.EngineService._get_stack(self.ctx, - self.stack.identifier()).AndReturn(s) - self.m.StubOutWithMock(res.Resource, 'signal') - res.Resource.signal(mox.IgnoreArg()).AndReturn(None) + res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None) self.m.ReplayAll() self.eng.resource_signal(self.ctx, @@ -1295,20 +1288,9 @@ class StackServiceTest(common.HeatTestCase): def test_signal_reception_no_resource(self): stack_name = 'signal_reception_no_resource' - stack = tools.get_stack(stack_name, self.ctx, policy_template) - tools.setup_keystone_mocks(self.m, stack) - self.stack = stack - self.m.ReplayAll() - stack.store() - stack.create() + self.stack = self._stack_create(stack_name) test_data = {'food': 'yum'} - self.m.StubOutWithMock(service.EngineService, '_get_stack') - s = stack_object.Stack.get_by_id(self.ctx, self.stack.id) - service.EngineService._get_stack(self.ctx, - self.stack.identifier()).AndReturn(s) - self.m.ReplayAll() - ex = self.assertRaises(dispatcher.ExpectedException, self.eng.resource_signal, self.ctx, dict(self.stack.identifier()), @@ -1345,23 +1327,13 @@ class StackServiceTest(common.HeatTestCase): self.m.VerifyAll() def test_signal_returns_metadata(self): - stack = tools.get_stack('signal_reception', self.ctx, policy_template) - self.stack = stack - tools.setup_keystone_mocks(self.m, stack) - self.m.ReplayAll() - stack.store() - stack.create() + self.stack = self._stack_create('signal_reception') + rsrc = self.stack['WebServerScaleDownPolicy'] test_metadata = {'food': 'yum'} - rsrc = stack['WebServerScaleDownPolicy'] rsrc.metadata_set(test_metadata) - self.m.StubOutWithMock(service.EngineService, '_get_stack') - s = stack_object.Stack.get_by_id(self.ctx, self.stack.id) - service.EngineService._get_stack(self.ctx, - self.stack.identifier()).AndReturn(s) - self.m.StubOutWithMock(res.Resource, 'signal') - res.Resource.signal(mox.IgnoreArg()).AndReturn(None) + res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None) self.m.ReplayAll() md = self.eng.resource_signal(self.ctx, @@ -1369,6 +1341,40 @@ class StackServiceTest(common.HeatTestCase): 'WebServerScaleDownPolicy', None, sync_call=True) self.assertEqual(test_metadata, md) + + self.m.VerifyAll() + + def test_signal_unset_invalid_hook(self): + self.stack = self._stack_create('signal_unset_invalid_hook') + details = {'unset_hook': 'invalid_hook'} + + ex = self.assertRaises(dispatcher.ExpectedException, + self.eng.resource_signal, + self.ctx, + dict(self.stack.identifier()), + 'WebServerScaleDownPolicy', + details) + msg = 'Invalid hook type "invalid_hook"' + self.assertIn(msg, six.text_type(ex.exc_info[1])) + self.assertEqual(exception.InvalidBreakPointHook, + ex.exc_info[0]) + self.m.VerifyAll() + + def test_signal_unset_not_defined_hook(self): + self.stack = self._stack_create('signal_unset_not_defined_hook') + details = {'unset_hook': 'pre-update'} + ex = self.assertRaises(dispatcher.ExpectedException, + self.eng.resource_signal, + self.ctx, + dict(self.stack.identifier()), + 'WebServerScaleDownPolicy', + details) + msg = ('The "pre-update" hook is not defined on ' + 'AWSScalingPolicy "WebServerScaleDownPolicy"') + self.assertIn(msg, six.text_type(ex.exc_info[1])) + self.assertEqual(exception.InvalidBreakPointHook, + ex.exc_info[0]) + self.m.VerifyAll() def test_signal_calls_metadata_update(self): @@ -1385,7 +1391,7 @@ class StackServiceTest(common.HeatTestCase): self.stack.identifier()).AndReturn(s) self.m.StubOutWithMock(res.Resource, 'signal') - res.Resource.signal(mox.IgnoreArg()).AndReturn(None) + res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None) self.m.StubOutWithMock(res.Resource, 'metadata_update') # this will be called once for the Random resource res.Resource.metadata_update().AndReturn(None) @@ -1413,7 +1419,7 @@ class StackServiceTest(common.HeatTestCase): self.stack.identifier()).AndReturn(s) self.m.StubOutWithMock(res.Resource, 'signal') - res.Resource.signal(mox.IgnoreArg()).AndReturn(None) + res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None) # this will never be called self.m.StubOutWithMock(res.Resource, 'metadata_update') self.m.ReplayAll() diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 4bc89593c7..ba27113c80 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -199,11 +199,10 @@ class ResourceTest(common.HeatTestCase): for status in res.STATUSES: res.state_set(action, status) ev = self.patchobject(res, '_add_event') - ex = self.assertRaises(exception.ResourceFailure, + ex = self.assertRaises(exception.NotSupported, res.signal) - self.assertEqual('Exception: resources.res: ' - 'Cannot signal resource during ' - '%s' % action, six.text_type(ex)) + self.assertEqual('Signal resource during %s is not ' + 'supported.' % action, six.text_type(ex)) ev.assert_called_with( action, status, 'Cannot signal resource during %s' % action) @@ -2643,10 +2642,10 @@ class ResourceHookTest(common.HeatTestCase): self.assertFalse(res.clear_hook.called) self.assertRaises(exception.ResourceActionNotSupported, - res.signal, {}) + res.signal, {'other_hook': 'alarm'}) self.assertFalse(res.clear_hook.called) - self.assertRaises(exception.ResourceActionNotSupported, + self.assertRaises(exception.InvalidBreakPointHook, res.signal, {'unset_hook': 'unknown_hook'}) self.assertFalse(res.clear_hook.called) @@ -2657,7 +2656,7 @@ class ResourceHookTest(common.HeatTestCase): res.clear_hook.assert_called_with('pre-update') res.has_hook = mock.Mock(return_value=False) - self.assertRaises(exception.ResourceActionNotSupported, + self.assertRaises(exception.InvalidBreakPointHook, res.signal, {'unset_hook': 'pre-create'}) diff --git a/heat/tests/test_signal.py b/heat/tests/test_signal.py index c955001ded..9c6a5d8a6f 100644 --- a/heat/tests/test_signal.py +++ b/heat/tests/test_signal.py @@ -618,9 +618,7 @@ class SignalTest(common.HeatTestCase): self.m.VerifyAll() - def test_signal_reception_wrong_state(self): - # assert that we get the correct exception when calling a - # resource.signal() that is in having a destructive action. + def _test_signal_not_supported_action(self, action='DELETE'): self.stack = self.create_stack() self.m.ReplayAll() @@ -629,14 +627,25 @@ class SignalTest(common.HeatTestCase): rsrc = self.stack['signal_handler'] self.assertEqual((rsrc.CREATE, rsrc.COMPLETE), rsrc.state) # manually override the action to DELETE - rsrc.action = rsrc.DELETE + rsrc.action = action err_metadata = {'Data': 'foo', 'Status': 'SUCCESS', 'UniqueId': '123'} - self.assertRaises(exception.ResourceFailure, rsrc.signal, - details=err_metadata) - + msg = 'Signal resource during %s is not supported.' % action + exc = self.assertRaises(exception.NotSupported, rsrc.signal, + details=err_metadata) + self.assertEqual(msg, six.text_type(exc)) self.m.VerifyAll() + def test_signal_in_delete_state(self): + # assert that we get the correct exception when calling a + # resource.signal() that is in delete action. + self._test_signal_not_supported_action() + + def test_signal_in_suspend_state(self): + # assert that we get the correct exception when calling a + # resource.signal() that is in suspend action. + self._test_signal_not_supported_action(action='SUSPEND') + def test_signal_reception_failed_call(self): # assert that we get the correct exception from resource.signal() # when resource.handle_signal() raises an exception. diff --git a/heat_integrationtests/functional/test_autoscaling.py b/heat_integrationtests/functional/test_autoscaling.py index 1b9fe991ba..90414051d1 100644 --- a/heat_integrationtests/functional/test_autoscaling.py +++ b/heat_integrationtests/functional/test_autoscaling.py @@ -13,7 +13,9 @@ import copy import json +from heatclient import exc from oslo_log import log as logging +import six from testtools import matchers from heat_integrationtests.common import test @@ -728,8 +730,13 @@ outputs: self._wait_for_resource_status( stack_identifier, 'JobServerGroup', 'SUSPEND_COMPLETE') - # Send a signal and confirm nothing happened. - self.client.resources.signal(stack_identifier, 'ScaleUpPolicy') + # Send a signal and a exception will raise + ex = self.assertRaises(exc.BadRequest, + self.client.resources.signal, + stack_identifier, 'ScaleUpPolicy') + + error_msg = 'Signal resource during SUSPEND is not supported' + self.assertIn(error_msg, six.text_type(ex)) ev = self.wait_for_event_with_reason( stack_identifier, reason='Cannot signal resource during SUSPEND',