From dcbf5e1899127e80aa37f437ac6462b454035d6e Mon Sep 17 00:00:00 2001 From: Stan Lagun Date: Tue, 6 Sep 2016 23:40:54 -0700 Subject: [PATCH] Raise exception on call of method of destroyed object In theory using destruction dependencies one can obtain a new reference to an object that is about to be destroyed. In this case any attempt to call a method on such object should result in exception being raised. This guarantees that the nothing can be done on the object after its .destroy method was executed. Also because the HeatStack instance is owned by the CloudRegion it is going to be destroyed prior to it. But CloudRegion needs to call $stack.delete() in its .destroy method and this will cause error. As a solution HeatStack made not to be owned by the region and instead be an independent object with destruction dependency on it which guarantees that it will be destroyed after the CloudRegion. Targets-blueprint: dependency-driven-resource-deallocation Change-Id: I5650ea672d5f121be69669f27dd5f513fbdd5c44 --- meta/io.murano/Classes/CloudRegion.yaml | 4 +++- meta/io.murano/Classes/Environment.yaml | 4 ---- murano/dsl/exceptions.py | 6 ++++++ murano/dsl/executor.py | 2 ++ murano/engine/system/heat_stack.py | 11 +++-------- murano/tests/unit/dsl/meta/TestGC.yaml | 17 +++++++++++++++++ murano/tests/unit/dsl/test_gc.py | 8 ++++++++ murano/tests/unit/test_heat_stack.py | 17 +++++++---------- 8 files changed, 46 insertions(+), 23 deletions(-) diff --git a/meta/io.murano/Classes/CloudRegion.yaml b/meta/io.murano/Classes/CloudRegion.yaml index babc4102..4ed34362 100644 --- a/meta/io.murano/Classes/CloudRegion.yaml +++ b/meta/io.murano/Classes/CloudRegion.yaml @@ -42,9 +42,11 @@ Methods: - $.setAttr(generatedStackName, $generatedStackName) - $this.agentListener: new(sys:AgentListener, $this, name => $generatedStackName) - $stackDescriptionFormat: 'This stack was generated by Murano for environment {0} (ID: {1}) - region {2}' - - $this.stack: new(sys:HeatStack, $this, + - $this.stack: new(sys:HeatStack, + regionName => $.name, name => 'murano-' + $generatedStackName, description => $stackDescriptionFormat.format($._environment.name, id($._environment), $.name)) + - sys:GC.subscribeDestruction($this, $this.stack) - $this.securityGroupManager: coalesce($.defaultNetworks.environment, $.defaultNetworks.flat)?. diff --git a/meta/io.murano/Classes/Environment.yaml b/meta/io.murano/Classes/Environment.yaml index 19b5ae8e..28fb368a 100644 --- a/meta/io.murano/Classes/Environment.yaml +++ b/meta/io.murano/Classes/Environment.yaml @@ -119,10 +119,6 @@ Methods: Body: - $.applications.pselect($.deploy()) - destroy: - Body: - - $.stack.delete() - _assignRegions: Body: - If: $.region = null diff --git a/murano/dsl/exceptions.py b/murano/dsl/exceptions.py index f320aa4f..5bf24316 100644 --- a/murano/dsl/exceptions.py +++ b/murano/dsl/exceptions.py @@ -174,3 +174,9 @@ class InvalidLhsTargetError(Exception): class InvalidInheritanceError(Exception): pass + + +class ObjectDestroyedError(Exception): + def __init__(self, obj): + super(ObjectDestroyedError, self).__init__( + 'Object {0} is already destroyed'.format(obj)) diff --git a/murano/dsl/executor.py b/murano/dsl/executor.py index 4036d009..6a975223 100644 --- a/murano/dsl/executor.py +++ b/murano/dsl/executor.py @@ -110,6 +110,8 @@ class MuranoDslExecutor(object): context = self.create_method_context(obj_context, method) if isinstance(this, dsl_types.MuranoObject): + if this.destroyed: + raise dsl_exceptions.ObjectDestroyedError(this) this = this.real_this if method.arguments_scheme is not None: diff --git a/murano/engine/system/heat_stack.py b/murano/engine/system/heat_stack.py index 9676dbc1..5256f2a6 100644 --- a/murano/engine/system/heat_stack.py +++ b/murano/engine/system/heat_stack.py @@ -41,7 +41,7 @@ class HeatStackError(Exception): @dsl.name('io.murano.system.HeatStack') class HeatStack(object): - def __init__(self, this, name, description=None, region_name=None): + def __init__(self, name, description=None, region_name=None): self._name = name self._template = None self._parameters = {} @@ -51,7 +51,6 @@ class HeatStack(object): self._description = description self._last_stack_timestamps = (None, None) self._tags = '' - self._owner = this.find_owner('io.murano.CloudRegion') self._region_name = region_name @staticmethod @@ -63,9 +62,7 @@ class HeatStack(object): @property def _client(self): - region = self._region_name or ( - None if self._owner is None else self._owner['name']) - return self._get_client(region) + return self._get_client(self._region_name) @staticmethod @session_local_storage.execution_session_memoize @@ -75,9 +72,7 @@ class HeatStack(object): def _get_token_client(self): ks_session = auth_utils.get_token_client_session(conf=CONF.heat) - region = self._region_name or ( - None if self._owner is None else self._owner['name']) - return self._create_client(ks_session, region) + return self._create_client(ks_session, self._region_name) def current(self): if self._template is not None: diff --git a/murano/tests/unit/dsl/meta/TestGC.yaml b/murano/tests/unit/dsl/meta/TestGC.yaml index 20da0ed5..505bd7b0 100644 --- a/murano/tests/unit/dsl/meta/TestGC.yaml +++ b/murano/tests/unit/dsl/meta/TestGC.yaml @@ -60,3 +60,20 @@ Methods: Contract: $.class(TestGCNode).notNull() Body: - trace('Destroy ' + $obj.value) + + testCallOnDestroyedObject: + Body: + - $val: new(TestGCNode, value => X) + - sys:GC.subscribeDestruction($val, $this, _handler2) + - $val: null + - sys:GC.collect() + - $this.destroyed.foo() + + _handler2: + Arguments: + - obj: + Contract: $.class(TestGCNode).notNull() + Body: + - $obj.foo() + - $this.destroyed: $obj + diff --git a/murano/tests/unit/dsl/test_gc.py b/murano/tests/unit/dsl/test_gc.py index 8a0068dd..f334407d 100644 --- a/murano/tests/unit/dsl/test_gc.py +++ b/murano/tests/unit/dsl/test_gc.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +from murano.dsl import exceptions + from murano.engine.system import garbage_collector from murano.tests.unit.dsl.foundation import object_model as om from murano.tests.unit.dsl.foundation import test_case @@ -56,3 +58,9 @@ class TestGC(test_case.DslTestCase): self.assertEqual( ['Destroy A', 'Destroy B', 'Destruction of B', 'B', 'A'], self.traces) + + def test_call_on_destroyed_object(self): + self.assertRaises( + exceptions.ObjectDestroyedError, + self.runner.testCallOnDestroyedObject) + self.assertEqual(['foo', 'X'], self.traces) diff --git a/murano/tests/unit/test_heat_stack.py b/murano/tests/unit/test_heat_stack.py index 0e0a8d0e..70ff12a2 100644 --- a/murano/tests/unit/test_heat_stack.py +++ b/murano/tests/unit/test_heat_stack.py @@ -36,8 +36,6 @@ class TestHeatStack(base.MuranoTestCase): return_value=self.heat_client_mock) heat_stack.HeatStack._get_client = mock.Mock( return_value=self.heat_client_mock) - self._this = mock.MagicMock() - self._this.owner = None @mock.patch(CLS_NAME + '._wait_state') @mock.patch(CLS_NAME + '._get_status') @@ -46,8 +44,7 @@ class TestHeatStack(base.MuranoTestCase): status_get.return_value = 'NOT_FOUND' wait_st.return_value = {} - hs = heat_stack.HeatStack( - self._this, 'test-stack', 'Generated by TestHeatStack') + hs = heat_stack.HeatStack('test-stack', 'Generated by TestHeatStack') hs._template = {'resources': {'test': 1}} hs._files = {} hs._hot_environment = '' @@ -56,7 +53,7 @@ class TestHeatStack(base.MuranoTestCase): hs.push() hs = heat_stack.HeatStack( - self._this, 'test-stack', 'Generated by TestHeatStack') + 'test-stack', 'Generated by TestHeatStack') hs._template = {'resources': {'test': 1}} hs._files = {} hs._parameters = {} @@ -86,7 +83,7 @@ class TestHeatStack(base.MuranoTestCase): status_get.return_value = 'NOT_FOUND' wait_st.return_value = {} - hs = heat_stack.HeatStack(self._this, 'test-stack', None) + hs = heat_stack.HeatStack('test-stack', None) hs._template = {'resources': {'test': 1}} hs._files = {} hs._hot_environment = '' @@ -116,7 +113,7 @@ class TestHeatStack(base.MuranoTestCase): status_get.return_value = 'NOT_FOUND' wait_st.return_value = {} - hs = heat_stack.HeatStack(self._this, 'test-stack', None) + hs = heat_stack.HeatStack('test-stack', None) hs._description = None hs._template = {'resources': {'test': 1}} hs._files = {"heatFile": "file"} @@ -147,7 +144,7 @@ class TestHeatStack(base.MuranoTestCase): status_get.return_value = 'NOT_FOUND' wait_st.return_value = {} - hs = heat_stack.HeatStack(self._this, 'test-stack', None) + hs = heat_stack.HeatStack('test-stack', None) hs._description = None hs._template = {'resources': {'test': 1}} hs._files = {"heatFile": "file"} @@ -176,7 +173,7 @@ class TestHeatStack(base.MuranoTestCase): """Template version other than expected should cause error.""" hs = heat_stack.HeatStack( - self._this, 'test-stack', 'Generated by TestHeatStack') + 'test-stack', 'Generated by TestHeatStack') hs._template = {'resources': {'test': 1}} invalid_template = { @@ -215,7 +212,7 @@ class TestHeatStack(base.MuranoTestCase): wait_st.return_value = {} CONF.set_override('stack_tags', ['test-murano', 'murano-tag'], 'heat', enforce_type=True) - hs = heat_stack.HeatStack(self._this, 'test-stack', None) + hs = heat_stack.HeatStack('test-stack', None) hs._description = None hs._template = {'resources': {'test': 1}} hs._files = {}