From bfdee58d2e8d60cb87545a62bd9d2d9c1b372a04 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 9 Apr 2013 20:01:23 +0200 Subject: [PATCH] Pass data from handle_create() to check_active() All resources that implement check_active() will require some state to be retained from the call to handle_create(). Saving this as state in the Resource object results in repeated, ugly, and potentially error-prone code. Instead, allow a subclass-defined state object returned from handle_create() to be passed to check_active(). This ensures that the state is limited in scope to where it is meaningful (during the create operation), and that it will be garbage-collected at the appropriate time, even if an unexpected exception occurs e.g. because a thread is cancelled. Change-Id: I9d690b44a066aaf33970562a2b9a55c633a7d4e8 --- heat/engine/resource.py | 10 ++++++---- heat/engine/resources/autoscaling.py | 6 +++--- heat/engine/resources/instance.py | 2 +- heat/tests/test_autoscaling.py | 8 ++++---- heat/tests/test_instance_group.py | 16 +++++++++------- heat/tests/test_metadata_refresh.py | 4 ++-- 6 files changed, 25 insertions(+), 21 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index ccd5d133..92aabe6d 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -316,9 +316,10 @@ class Resource(object): try: self.properties.validate() self.state_set(self.CREATE_IN_PROGRESS) + create_data = None if callable(getattr(self, 'handle_create', None)): - self.handle_create() - while not self.check_active(): + create_data = self.handle_create() + while not self.check_active(create_data): eventlet.sleep(1) except greenlet.GreenletExit: raise @@ -329,12 +330,13 @@ class Resource(object): else: self.state_set(self.CREATE_COMPLETE) - def check_active(self): + def check_active(self, create_data): ''' Check if the resource is active (ready to move to the CREATE_COMPLETE state). By default this happens as soon as the handle_create() method has completed successfully, but subclasses may customise this by - overriding this function. + overriding this function. The return value of handle_create() is + passed in to this function each time it is called. ''' return True diff --git a/heat/engine/resources/autoscaling.py b/heat/engine/resources/autoscaling.py index 31914023..53e3367a 100644 --- a/heat/engine/resources/autoscaling.py +++ b/heat/engine/resources/autoscaling.py @@ -82,7 +82,7 @@ class InstanceGroup(resource.Resource): def handle_create(self): self.resize(int(self.properties['Size']), raise_on_error=True) - def check_active(self): + def check_active(self, create_data=None): active = all(i.check_active(override=False) for i in self._activating) if active: self._activating = [] @@ -143,14 +143,14 @@ class InstanceGroup(resource.Resource): def state_set(self, new_state, reason="state changed"): self._store_or_update(new_state, reason) - def check_active(self, override=True): + def check_active(self, create_data=None, override=True): ''' By default, report that the instance is active so that we won't wait for it in create(). ''' if override: return True - return super(GroupedInstance, self).check_active() + return super(GroupedInstance, self).check_active(create_data) conf = self.properties['LaunchConfigurationName'] instance_definition = self.stack.t['Resources'][conf] diff --git a/heat/engine/resources/instance.py b/heat/engine/resources/instance.py index d35ce890..315ae123 100644 --- a/heat/engine/resources/instance.py +++ b/heat/engine/resources/instance.py @@ -313,7 +313,7 @@ class Instance(resource.Resource): self._server_status = server.status - def check_active(self): + def check_active(self, create_data=None): if self._server_status == 'ACTIVE': return True diff --git a/heat/tests/test_autoscaling.py b/heat/tests/test_autoscaling.py index 57e7e1a1..1bf063af 100644 --- a/heat/tests/test_autoscaling.py +++ b/heat/tests/test_autoscaling.py @@ -86,13 +86,13 @@ class AutoScalingTest(unittest.TestCase): def _stub_create(self, num): self.m.StubOutWithMock(eventlet, 'sleep') - self.m.StubOutWithMock(instance.Instance, 'create') + self.m.StubOutWithMock(instance.Instance, 'handle_create') self.m.StubOutWithMock(instance.Instance, 'check_active') for x in range(num): - instance.Instance.create().AndReturn(None) - instance.Instance.check_active().AndReturn(False) + instance.Instance.handle_create().AndReturn(None) + instance.Instance.check_active(None).AndReturn(False) eventlet.sleep(mox.IsA(int)).AndReturn(None) - instance.Instance.check_active().MultipleTimes().AndReturn(True) + instance.Instance.check_active(None).MultipleTimes().AndReturn(True) def _stub_lb_reload(self, expected_list, unset=True): if unset: diff --git a/heat/tests/test_instance_group.py b/heat/tests/test_instance_group.py index be9d3d38..55d64516 100644 --- a/heat/tests/test_instance_group.py +++ b/heat/tests/test_instance_group.py @@ -23,6 +23,7 @@ from nose.plugins.attrib import attr from heat.tests.v1_1 import fakes from heat.common import context +from heat.common import exception from heat.common import template_format from heat.engine.resources import autoscaling as asc from heat.engine.resources import instance @@ -65,13 +66,13 @@ class InstanceGroupTest(unittest.TestCase): def _stub_create(self, num): self.m.StubOutWithMock(eventlet, 'sleep') - self.m.StubOutWithMock(instance.Instance, 'create') + self.m.StubOutWithMock(instance.Instance, 'handle_create') self.m.StubOutWithMock(instance.Instance, 'check_active') for x in range(num): - instance.Instance.create().AndReturn(None) - instance.Instance.check_active().AndReturn(False) + instance.Instance.handle_create().AndReturn(None) + instance.Instance.check_active(None).AndReturn(False) eventlet.sleep(mox.IsA(int)).AndReturn(None) - instance.Instance.check_active().MultipleTimes().AndReturn(True) + instance.Instance.check_active(None).MultipleTimes().AndReturn(True) def create_instance_group(self, t, stack, resource_name): resource = asc.InstanceGroup(resource_name, @@ -113,12 +114,13 @@ class InstanceGroupTest(unittest.TestCase): t['Resources']['JobServerGroup'], stack) - self.m.StubOutWithMock(instance.Instance, 'create') - instance.Instance.create().AndReturn('ImageNotFound: bla') + self.m.StubOutWithMock(instance.Instance, 'handle_create') + not_found = exception.ImageNotFound(image_name='bla') + instance.Instance.handle_create().AndRaise(not_found) self.m.ReplayAll() - self.assertEqual(resource.create(), 'ImageNotFound: bla') + self.assertNotEqual(resource.create(), None) self.assertEqual(asc.InstanceGroup.CREATE_FAILED, resource.state) self.m.VerifyAll() diff --git a/heat/tests/test_metadata_refresh.py b/heat/tests/test_metadata_refresh.py index 4309010c..c0d4928d 100644 --- a/heat/tests/test_metadata_refresh.py +++ b/heat/tests/test_metadata_refresh.py @@ -152,7 +152,7 @@ class MetadataRefreshTest(unittest.TestCase): self.m.StubOutWithMock(instance.Instance, 'handle_create') self.m.StubOutWithMock(instance.Instance, 'check_active') instance.Instance.handle_create().AndReturn(None) - instance.Instance.check_active().AndReturn(True) + instance.Instance.check_active(None).AndReturn(True) self.m.StubOutWithMock(instance.Instance, 'FnGetAtt') return stack @@ -214,7 +214,7 @@ class WaitCondMetadataUpdateTest(unittest.TestCase): self.m.StubOutWithMock(instance.Instance, 'handle_create') self.m.StubOutWithMock(instance.Instance, 'check_active') instance.Instance.handle_create().MultipleTimes().AndReturn(None) - instance.Instance.check_active().AndReturn(True) + instance.Instance.check_active(None).AndReturn(True) self.m.StubOutWithMock(wc.WaitConditionHandle, 'keystone') wc.WaitConditionHandle.keystone().MultipleTimes().AndReturn(self.fc) -- 2.45.2