From b862908d4cdade7a15863d81900d3be6748e73d4 Mon Sep 17 00:00:00 2001 From: Thomas Herve Date: Fri, 12 Feb 2016 12:14:12 +0100 Subject: [PATCH] Optimize nested stack status check Currently, StackResource loads the whole stack when checking for status (in check_*_complete method), but only care about the state of the stack. This is a fairly expensive operation, as it retrieves the template and reparses everything. This simplifies it with a new API that simply query the stack status from the database. Closes-Bug: #1549213 Change-Id: I18df89a2b9959241ddbec2593a53c5e2aa6a4717 --- heat/db/api.py | 4 + heat/db/sqlalchemy/api.py | 12 +++ heat/engine/resources/stack_resource.py | 68 +++++++------- heat/objects/stack.py | 5 + heat/tests/db/test_sqlalchemy_api.py | 16 ++++ heat/tests/test_stack_resource.py | 117 ++++++++---------------- 6 files changed, 109 insertions(+), 113 deletions(-) diff --git a/heat/db/api.py b/heat/db/api.py index f5240511a..677b93077 100644 --- a/heat/db/api.py +++ b/heat/db/api.py @@ -134,6 +134,10 @@ def stack_get(context, stack_id, show_deleted=False, tenant_safe=True, eager_load=eager_load) +def stack_get_status(context, stack_id): + return IMPL.stack_get_status(context, stack_id) + + def stack_get_by_name_and_owner_id(context, stack_name, owner_id): return IMPL.stack_get_by_name_and_owner_id(context, stack_name, owner_id=owner_id) diff --git a/heat/db/sqlalchemy/api.py b/heat/db/sqlalchemy/api.py index b16cbb424..5c2c9ec1f 100644 --- a/heat/db/sqlalchemy/api.py +++ b/heat/db/sqlalchemy/api.py @@ -366,6 +366,18 @@ def stack_get(context, stack_id, show_deleted=False, tenant_safe=True, return result +def stack_get_status(context, stack_id): + query = model_query(context, models.Stack) + query = query.options( + orm.load_only("action", "status", "status_reason", "updated_at")) + result = query.filter_by(id=stack_id).first() + if result is None: + raise exception.NotFound(_('Stack with id %s not found') % stack_id) + + return (result.action, result.status, result.status_reason, + result.updated_at) + + def stack_get_all_by_owner_id(context, owner_id): results = soft_delete_aware_query( context, models.Stack).filter_by(owner_id=owner_id).all() diff --git a/heat/engine/resources/stack_resource.py b/heat/engine/resources/stack_resource.py index 4b60d97d6..aa4598abb 100644 --- a/heat/engine/resources/stack_resource.py +++ b/heat/engine/resources/stack_resource.py @@ -30,6 +30,8 @@ from heat.engine import resource from heat.engine import scheduler from heat.engine import stack as parser from heat.engine import template +from heat.objects import stack as stack_object +from heat.objects import stack_lock from heat.rpc import api as rpc_api LOG = logging.getLogger(__name__) @@ -123,24 +125,15 @@ class StackResource(resource.Resource): return False - def nested(self, force_reload=False, show_deleted=False): + def nested(self): """Return a Stack object representing the nested (child) stack. If we catch NotFound exception when loading, return None. - - :param force_reload: Forces reloading from the DB instead of returning - the locally cached Stack object - :param show_deleted: Returns the stack even if it's been deleted """ - if force_reload: - self._nested = None - if self._nested is None and self.resource_id is not None: try: self._nested = parser.Stack.load(self.context, - self.resource_id, - show_deleted=show_deleted, - force_reload=force_reload) + self.resource_id) except exception.NotFound: return None @@ -337,19 +330,23 @@ class StackResource(resource.Resource): raise exception.ResourceFailure(message, self, action=self.action) def check_create_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.CREATE) + return self._check_status_complete(self.CREATE) - def _check_status_complete(self, action, show_deleted=False, - cookie=None): - nested = self.nested(force_reload=True, show_deleted=show_deleted) - if nested is None: - if action == resource.Resource.DELETE: + def _check_status_complete(self, expected_action, cookie=None): + + try: + data = stack_object.Stack.get_status(self.context, + self.resource_id) + except exception.NotFound: + if expected_action == self.DELETE: return True # It's possible the engine handling the create hasn't persisted # the stack to the DB when we first start polling for state return False - if nested.action != action: + action, status, status_reason, updated_time = data + + if action != expected_action: return False # Has the action really started? @@ -368,25 +365,29 @@ class StackResource(resource.Resource): if cookie is not None: prev_state = cookie['previous']['state'] prev_updated_at = cookie['previous']['updated_at'] - if (prev_updated_at == nested.updated_time and - prev_state == nested.state): + if (prev_updated_at == updated_time and + prev_state == (action, status)): return False - if nested.status == resource.Resource.IN_PROGRESS: + if status == self.IN_PROGRESS: return False - elif nested.status == resource.Resource.COMPLETE: - return True - elif nested.status == resource.Resource.FAILED: - raise exception.ResourceFailure(nested.status_reason, self, + elif status == self.COMPLETE: + ret = stack_lock.StackLock.get_engine_id(self.resource_id) is None + if ret: + # Reset nested, to indicate we changed status + self._nested = None + return ret + elif status == self.FAILED: + raise exception.ResourceFailure(status_reason, self, action=action) else: raise exception.ResourceUnknownStatus( - resource_status=nested.status, - status_reason=nested.status_reason, + resource_status=status, + status_reason=status_reason, result=_('Stack unknown status')) def check_adopt_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.ADOPT) + return self._check_status_complete(self.ADOPT) def update_with_template(self, child_template, user_params=None, timeout_mins=None): @@ -430,7 +431,7 @@ class StackResource(resource.Resource): return cookie def check_update_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.UPDATE, + return self._check_status_complete(self.UPDATE, cookie=cookie) def delete_nested(self): @@ -450,8 +451,7 @@ class StackResource(resource.Resource): return self.delete_nested() def check_delete_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.DELETE, - show_deleted=True) + return self._check_status_complete(self.DELETE) def handle_suspend(self): stack = self.nested() @@ -465,7 +465,7 @@ class StackResource(resource.Resource): self.rpc_client().stack_suspend(self.context, dict(stack_identity)) def check_suspend_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.SUSPEND) + return self._check_status_complete(self.SUSPEND) def handle_resume(self): stack = self.nested() @@ -479,7 +479,7 @@ class StackResource(resource.Resource): self.rpc_client().stack_resume(self.context, dict(stack_identity)) def check_resume_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.RESUME) + return self._check_status_complete(self.RESUME) def handle_check(self): stack = self.nested() @@ -494,7 +494,7 @@ class StackResource(resource.Resource): self.rpc_client().stack_check(self.context, dict(stack_identity)) def check_check_complete(self, cookie=None): - return self._check_status_complete(resource.Resource.CHECK) + return self._check_status_complete(self.CHECK) def prepare_abandon(self): self.abandon_in_progress = True diff --git a/heat/objects/stack.py b/heat/objects/stack.py index c995f5ba0..6eed7f1d1 100644 --- a/heat/objects/stack.py +++ b/heat/objects/stack.py @@ -205,3 +205,8 @@ class Stack( @classmethod def encrypt_hidden_parameters(cls, tmpl): raw_template.RawTemplate.encrypt_hidden_parameters(tmpl) + + @classmethod + def get_status(cls, context, stack_id): + """Return action and status for the given stack.""" + return db_api.stack_get_status(context, stack_id) diff --git a/heat/tests/db/test_sqlalchemy_api.py b/heat/tests/db/test_sqlalchemy_api.py index d9c59aa9c..c351090de 100644 --- a/heat/tests/db/test_sqlalchemy_api.py +++ b/heat/tests/db/test_sqlalchemy_api.py @@ -424,6 +424,22 @@ class SqlAlchemyTest(common.HeatTestCase): st = db_api.stack_get(self.ctx, UUID1, show_deleted=True) self.assertEqual(UUID1, st.id) + def test_stack_get_status(self): + stack = self._setup_test_stack('stack', UUID1)[1] + + st = db_api.stack_get_status(self.ctx, UUID1) + self.assertEqual(('CREATE', 'IN_PROGRESS', '', None), st) + + stack.delete() + st = db_api.stack_get_status(self.ctx, UUID1) + self.assertEqual( + ('DELETE', 'COMPLETE', + 'Stack DELETE completed successfully', None), + st) + + self.assertRaises(exception.NotFound, + db_api.stack_get_status, self.ctx, UUID2) + def test_stack_get_show_deleted_context(self): stack = self._setup_test_stack('stack', UUID1)[1] diff --git a/heat/tests/test_stack_resource.py b/heat/tests/test_stack_resource.py index 84d098723..057266442 100644 --- a/heat/tests/test_stack_resource.py +++ b/heat/tests/test_stack_resource.py @@ -25,6 +25,8 @@ from heat.common import template_format from heat.engine.resources import stack_resource from heat.engine import stack as parser from heat.engine import template as templatem +from heat.objects import stack as stack_object +from heat.objects import stack_lock from heat.tests import common from heat.tests import generic_resource as generic_rsrc from heat.tests import utils @@ -452,34 +454,17 @@ class StackResourceTest(StackResourceBaseTest): self.parent_resource.resource_id = 319 self.m.StubOutWithMock(parser.Stack, 'load') parser.Stack.load(self.parent_resource.context, - self.parent_resource.resource_id, - show_deleted=False, - force_reload=False).AndReturn('s') + self.parent_resource.resource_id).AndReturn('s') self.m.ReplayAll() self.parent_resource.nested() self.m.VerifyAll() - def test_load_nested_force_reload(self): - self.parent_resource._nested = 'write-over-me' - self.parent_resource.resource_id = 319 - self.m.StubOutWithMock(parser.Stack, 'load') - parser.Stack.load(self.parent_resource.context, - self.parent_resource.resource_id, - show_deleted=False, - force_reload=True).AndReturn('ok') - self.m.ReplayAll() - self.parent_resource.nested(force_reload=True) - self.assertEqual('ok', self.parent_resource._nested) - self.m.VerifyAll() - def test_load_nested_non_exist(self): self.parent_resource._nested = None self.parent_resource.resource_id = '90-8' self.m.StubOutWithMock(parser.Stack, 'load') parser.Stack.load(self.parent_resource.context, - self.parent_resource.resource_id, - show_deleted=False, - force_reload=False).AndRaise( + self.parent_resource.resource_id).AndRaise( exception.NotFound) self.m.ReplayAll() @@ -490,32 +475,6 @@ class StackResourceTest(StackResourceBaseTest): self.parent_resource._nested = 'gotthis' self.assertEqual('gotthis', self.parent_resource.nested()) - def test_load_nested_force_reload_ok(self): - self.parent_resource._nested = mock.MagicMock() - self.parent_resource.resource_id = '90-8' - self.m.StubOutWithMock(parser.Stack, 'load') - parser.Stack.load(self.parent_resource.context, - self.parent_resource.resource_id, - show_deleted=False, - force_reload=True).AndReturn('s') - self.m.ReplayAll() - st = self.parent_resource.nested(force_reload=True) - self.assertEqual('s', st) - self.m.VerifyAll() - - def test_load_nested_force_reload_none(self): - self.parent_resource._nested = mock.MagicMock() - self.parent_resource.resource_id = '90-8' - self.m.StubOutWithMock(parser.Stack, 'load') - parser.Stack.load(self.parent_resource.context, - self.parent_resource.resource_id, - show_deleted=False, - force_reload=True).AndRaise( - exception.NotFound) - self.m.ReplayAll() - self.assertIsNone(self.parent_resource.nested(force_reload=True)) - self.m.VerifyAll() - def test_delete_nested_none_nested_stack(self): self.parent_resource._nested = None self.assertIsNone(self.parent_resource.delete_nested()) @@ -713,22 +672,18 @@ class StackResourceAttrTest(StackResourceBaseTest): class StackResourceCheckCompleteTest(StackResourceBaseTest): scenarios = [ - ('create', dict(action='create', show_deleted=False)), - ('update', dict(action='update', show_deleted=False)), - ('suspend', dict(action='suspend', show_deleted=False)), - ('resume', dict(action='resume', show_deleted=False)), - ('delete', dict(action='delete', show_deleted=True)), + ('create', dict(action='create')), + ('update', dict(action='update')), + ('suspend', dict(action='suspend')), + ('resume', dict(action='resume')), + ('delete', dict(action='delete')), ] def setUp(self): super(StackResourceCheckCompleteTest, self).setUp() - self.nested = mock.MagicMock() - self.nested.name = 'nested-stack' - self.parent_resource.nested = mock.MagicMock(return_value=self.nested) - self.parent_resource._nested = self.nested - setattr(self.nested, self.action.upper(), self.action.upper()) - self.nested.action = self.action.upper() - self.nested.COMPLETE = 'COMPLETE' + self.status = [self.action.upper(), None, None, None] + self.mock_status = self.patchobject(stack_object.Stack, 'get_status') + self.mock_status.return_value = self.status def test_state_ok(self): """Test case when check_create_complete should return True. @@ -736,12 +691,17 @@ class StackResourceCheckCompleteTest(StackResourceBaseTest): check_create_complete should return True create task is done and the nested stack is in (,COMPLETE) state. """ - self.nested.status = 'COMPLETE' + self.mock_lock = self.patchobject(stack_lock.StackLock, + 'get_engine_id') + self.mock_lock.return_value = None + self.status[1] = 'COMPLETE' complete = getattr(self.parent_resource, 'check_%s_complete' % self.action) self.assertIs(True, complete(None)) - self.parent_resource.nested.assert_called_once_with( - show_deleted=self.show_deleted, force_reload=True) + self.mock_status.assert_called_once_with( + self.parent_resource.context, self.parent_resource.resource_id) + self.mock_lock.assert_called_once_with( + self.parent_resource.resource_id) def test_state_err(self): """Test case when check_create_complete should raise error. @@ -749,20 +709,20 @@ class StackResourceCheckCompleteTest(StackResourceBaseTest): check_create_complete should raise error when create task is done but the nested stack is not in (,COMPLETE) state """ - self.nested.status = 'FAILED' + self.status[1] = 'FAILED' reason = ('Resource %s failed: ValueError: ' 'resources.%s: broken on purpose' % ( self.action.upper(), 'child_res')) exp_path = 'resources.test.resources.child_res' exp = 'ValueError: %s: broken on purpose' % exp_path - self.nested.status_reason = reason + self.status[2] = reason complete = getattr(self.parent_resource, 'check_%s_complete' % self.action) exc = self.assertRaises(exception.ResourceFailure, complete, None) self.assertEqual(exp, six.text_type(exc)) - self.parent_resource.nested.assert_called_once_with( - show_deleted=self.show_deleted, force_reload=True) + self.mock_status.assert_called_once_with( + self.parent_resource.context, self.parent_resource.resource_id) def test_state_unknown(self): """Test case when check_create_complete should raise error. @@ -770,30 +730,29 @@ class StackResourceCheckCompleteTest(StackResourceBaseTest): check_create_complete should raise error when create task is done but the nested stack is not in (,COMPLETE) state """ - self.nested.status = 'WTF' - self.nested.status_reason = 'broken on purpose' + self.status[1] = 'WTF' + self.status[2] = 'broken on purpose' complete = getattr(self.parent_resource, 'check_%s_complete' % self.action) self.assertRaises(exception.ResourceUnknownStatus, complete, None) - self.parent_resource.nested.assert_called_once_with( - show_deleted=self.show_deleted, force_reload=True) + self.mock_status.assert_called_once_with( + self.parent_resource.context, self.parent_resource.resource_id) def test_in_progress(self): - self.nested.status = 'IN_PROGRESS' + self.status[1] = 'IN_PROGRESS' complete = getattr(self.parent_resource, 'check_%s_complete' % self.action) self.assertFalse(complete(None)) - self.parent_resource.nested.assert_called_once_with( - show_deleted=self.show_deleted, force_reload=True) + self.mock_status.assert_called_once_with( + self.parent_resource.context, self.parent_resource.resource_id) def test_update_not_started(self): if self.action != 'update': # only valid for updates at the moment. return - self.nested.status = 'COMPLETE' - self.nested.state = ('UPDATE', 'COMPLETE') - self.nested.updated_time = 'test' + self.status[1] = 'COMPLETE' + self.status[3] = 'test' cookie = {'previous': {'state': ('UPDATE', 'COMPLETE'), 'updated_at': 'test'}} @@ -801,16 +760,16 @@ class StackResourceCheckCompleteTest(StackResourceBaseTest): 'check_%s_complete' % self.action) self.assertFalse(complete(cookie=cookie)) - self.parent_resource.nested.assert_called_once_with( - show_deleted=self.show_deleted, force_reload=True) + self.mock_status.assert_called_once_with( + self.parent_resource.context, self.parent_resource.resource_id) def test_wrong_action(self): - self.nested.action = 'COMPLETE' + self.status[0] = 'COMPLETE' complete = getattr(self.parent_resource, 'check_%s_complete' % self.action) self.assertFalse(complete(None)) - self.parent_resource.nested.assert_called_once_with( - show_deleted=self.show_deleted, force_reload=True) + self.mock_status.assert_called_once_with( + self.parent_resource.context, self.parent_resource.resource_id) class WithTemplateTest(StackResourceBaseTest):