Merge "Handle resource plugins without default_client_name"

heat/engine/clients/default_client_plugin.py

@@ -0,0 +1,25 @@
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from heat.engine.clients import client_plugin
+
+
+class DefaultClientPlugin(client_plugin.ClientPlugin):
+    """A ClientPlugin that has no client.
+
+    This is provided so that Resource can make use of the is_not_found() and
+    is_conflict() methods even if the resource plugin has not specified a
+    client plugin.
+    """
+    def _create(self, version=None):
+        return super(DefaultClientPlugin, self)._create(version)

heat/engine/resource.py

@@ -33,6 +33,7 @@ from heat.common import timeutils
 from heat.engine import attributes
 from heat.engine.cfn import template as cfn_tmpl
 from heat.engine import clients
+from heat.engine.clients import default_client_plugin
 from heat.engine import environment
 from heat.engine import event
 from heat.engine import function
@@ -770,6 +771,21 @@ class Resource(status.ResourceStatus):
         assert client_name, "Must specify client name"
         return self.stack.clients.client_plugin(client_name)
 
+    def _default_client_plugin(self):
+        """Always return a client plugin.
+
+        This will be the client_plugin if the resource has defined a
+        default_client_name, or a no-op plugin if it does not. Thus, the
+        result of this call always has e.g. is_not_found() and is_conflict()
+        methods.
+        """
+        cp = None
+        if self.default_client_name:
+            cp = self.client_plugin()
+        if cp is None:
+            cp = default_client_plugin.DefaultClientPlugin(self.context)
+        return cp
+
     @classmethod
     def is_service_available(cls, context):
         # NOTE(kanagaraj-manickam): return True to satisfy the cases like
@@ -1164,7 +1180,7 @@ class Resource(status.ResourceStatus):
                 self.resource_id = self.external_id
                 self._show_resource()
             except Exception as ex:
-                if self.client_plugin().is_not_found(ex):
+                if self._default_client_plugin().is_not_found(ex):
                     error_message = (_("Invalid external resource: Resource "
                                        "%(external_id)s (%(type)s) can not "
                                        "be found.") %
@@ -1929,15 +1945,11 @@ class Resource(status.ResourceStatus):
     def handle_delete(self):
         """Default implementation; should be overridden by resources."""
         if self.entity and self.resource_id is not None:
-            try:
+            with self._default_client_plugin().ignore_not_found:
                 obj = getattr(self.client(), self.entity)
                 obj.delete(self.resource_id)
-            except Exception as ex:
+                return self.resource_id
-                if self.default_client_name is not None:
+        return None
-                    self.client_plugin().ignore_not_found(ex)
-                    return None
-                raise
-            return self.resource_id
 
     @scheduler.wrappertask
     def delete(self):
@@ -1950,10 +1962,8 @@ class Resource(status.ResourceStatus):
         def should_retry(exc):
             if count >= retry_limit:
                 return False
-            if self.default_client_name:
+            return (self._default_client_plugin().is_conflict(exc) or
-                return (self.client_plugin().is_conflict(exc) or
+                    isinstance(exc, exception.PhysicalResourceExists))
-                        isinstance(exc, exception.PhysicalResourceExists))
-            return isinstance(exc, exception.PhysicalResourceExists)
 
         action = self.DELETE
 
@@ -2156,30 +2166,20 @@ class Resource(status.ResourceStatus):
         if attr in self.base_attributes_schema:
             # check resource_id, because usually it is required for getting
             # information about resource
-            if not self.resource_id:
+            if self.resource_id is not None:
-                return None
+                with self._default_client_plugin().ignore_not_found:
-            try:
+                    return getattr(self, '_{0}_resource'.format(attr))()
-                return getattr(self, '_{0}_resource'.format(attr))()
-            except Exception as ex:
-                if self.default_client_name is not None:
-                    self.client_plugin().ignore_not_found(ex)
-                    return None
-                raise
         else:
-            try:
+            with self._default_client_plugin().ignore_not_found:
                 return self._resolve_attribute(attr)
-            except Exception as ex:
+        return None
-                if self.default_client_name is not None:
-                    self.client_plugin().ignore_not_found(ex)
-                    return None
-                raise
 
     def _show_resource(self):
         """Default implementation; should be overridden by resources.
 
         :returns: the map of resource information or None
         """
-        if self.entity:
+        if self.entity and self.default_client_name is not None:
             try:
                 obj = getattr(self.client(), self.entity)
                 resource = obj.get(self.resource_id)
@@ -2200,8 +2200,7 @@ class Resource(status.ResourceStatus):
         try:
             resource_data = self._show_resource()
         except Exception as ex:
-            if (self.default_client_name is not None and
+            if self._default_client_plugin().is_not_found(ex):
-                    self.client_plugin().is_not_found(ex)):
                 raise exception.EntityNotFound(
                     entity='Resource', name=self.name)
             raise

heat/tests/openstack/manila/test_share_network.py

@@ -62,7 +62,7 @@ class ManilaShareNetworkTest(common.HeatTestCase):
         self.client = mock.Mock()
         self.patchobject(share_network.ManilaShareNetwork, 'client',
                          return_value=self.client)
-        self.client_plugin = mock.Mock()
+        self.client_plugin = mock.MagicMock()
 
         def resolve_neutron(resource_type, name):
             return name

heat/tests/openstack/mistral/test_external_resource.py

@@ -67,7 +67,6 @@ class TestMistralExternalResource(common.HeatTestCase):
         self.patchobject(external_resource.MistralExternalResource,
                          'client',
                          return_value=self.mistral)
-        self.patchobject(client, 'mistral_base')
         self.patchobject(client.MistralClientPlugin, '_create')
         self.client = client.MistralClientPlugin(self.ctx)
 

heat/tests/openstack/mistral/test_workflow.py

@@ -388,8 +388,6 @@ class TestMistralWorkflow(common.HeatTestCase):
                                               '_create_user'))
         self.patches.append(mock.patch.object(signal_responder.SignalResponder,
                                               '_create_keypair'))
-        self.patches.append(mock.patch.object(client,
-                                              'mistral_base'))
         self.patches.append(mock.patch.object(client.MistralClientPlugin,
                                               '_create'))
         for patch in self.patches:

heat/tests/test_resource.py

@@ -372,8 +372,8 @@ class ResourceTest(common.HeatTestCase):
             'test_resource', 'GenericResourceType',
             external_id=external_id)
         res = generic_rsrc.GenericResource('test_resource', tmpl, self.stack)
-        res.client_plugin = mock.Mock()
+        res._default_client_plugin = mock.Mock()
-        self.patchobject(res.client_plugin, 'is_not_found',
+        self.patchobject(res._default_client_plugin(), 'is_not_found',
                          return_value=True)
         self.patchobject(res, '_show_resource', side_effect=Exception())
         e = self.assertRaises(exception.StackValidationFailed,
@@ -2508,6 +2508,9 @@ class ResourceTest(common.HeatTestCase):
         stack = self.create_resource_for_attributes_tests()
         res = stack['res']
 
+        class MyException(Exception):
+            pass
+
         with mock.patch.object(res, '_show_resource') as show_attr:
             # return None, if resource_id is None
             self.assertIsNone(res.FnGetAtt('show'))
@@ -2522,12 +2525,10 @@ class ResourceTest(common.HeatTestCase):
 
             # clean resolved_values
             res.attributes.reset_resolved_values()
-            with mock.patch.object(res, 'client_plugin') as client_plugin:
+            # generate error during calling _show_resource
-                # generate error during calling _show_resource
+            show_attr.side_effect = [MyException]
-                show_attr.side_effect = [Exception]
+            self.assertRaises(MyException, res.FnGetAtt, 'show')
-                self.assertIsNone(res.FnGetAtt('show'))
+            self.assertEqual(2, show_attr.call_count)
-                self.assertEqual(2, show_attr.call_count)
-                self.assertEqual(1, client_plugin.call_count)
 
     def test_resolve_attributes_stuff_custom_attribute(self):
         # check path with resolve_attribute
@@ -2535,16 +2536,17 @@ class ResourceTest(common.HeatTestCase):
         res = stack['res']
         res.default_client_name = 'foo'
 
+        class MyException(Exception):
+            pass
+
         with mock.patch.object(res, '_resolve_attribute') as res_attr:
-            res_attr.side_effect = ['Works', Exception]
+            res_attr.side_effect = ['Works', MyException]
             self.assertEqual('Works', res.FnGetAtt('Foo'))
             res_attr.assert_called_once_with('Foo')
 
             # clean resolved_values
             res.attributes.reset_resolved_values()
-            with mock.patch.object(res, 'client_plugin') as client_plugin:
+            self.assertRaises(MyException, res.FnGetAtt, 'Foo')
-                self.assertIsNone(res.FnGetAtt('Foo'))
-                self.assertEqual(1, client_plugin.call_count)
 
     def test_resolve_attributes_no_default_client_name(self):
         class MyException(Exception):
@@ -2571,6 +2573,7 @@ class ResourceTest(common.HeatTestCase):
         # set entity and recheck
         res.resource_id = 'test_resource_id'
         res.entity = 'test'
+        res.default_client_name = 'whatever'
 
         # mock getting resource info
         res.client = mock.Mock()
@@ -3906,6 +3909,7 @@ class ResourceAvailabilityTest(common.HeatTestCase):
         delete = mock.Mock()
         res.client().entity.delete = delete
         res.entity = 'entity'
+        res.default_client_name = 'something'
         res.resource_id = '12345'
 
         self.assertEqual('12345', res.handle_delete())
@@ -3918,26 +3922,26 @@ class ResourceAvailabilityTest(common.HeatTestCase):
         snippet = rsrc_defn.ResourceDefinition('aresource',
                                                'OS::Heat::None')
         res = resource.Resource('aresource', snippet, self.stack)
+        res.entity = 'entity'
+        res.default_client_name = 'foo'
+        res.resource_id = '12345'
 
         FakeClient = collections.namedtuple('Client', ['entity'])
         client = FakeClient(collections.namedtuple('entity', ['delete']))
+        client_plugin = res._default_client_plugin()
 
-        class FakeClientPlugin(object):
+        def is_not_found(ex):
-            def ignore_not_found(self, ex):
+            return isinstance(ex, exception.NotFound)
-                if not isinstance(ex, exception.NotFound):
-                    raise ex
 
+        client_plugin.is_not_found = mock.Mock(side_effect=is_not_found)
         self.patchobject(resource.Resource, 'client', return_value=client)
-        self.patchobject(resource.Resource, 'client_plugin',
-                         return_value=FakeClientPlugin())
         delete = mock.Mock()
         delete.side_effect = [exception.NotFound()]
         res.client().entity.delete = delete
-        res.entity = 'entity'
-        res.resource_id = '12345'
-        res.default_client_name = 'foo'
 
-        self.assertIsNone(res.handle_delete())
+        with mock.patch.object(res, '_default_client_plugin',
+                               return_value=client_plugin):
+            self.assertIsNone(res.handle_delete())
         delete.assert_called_once_with('12345')
 
     def test_handle_delete_raise_error(self):
@@ -3947,25 +3951,26 @@ class ResourceAvailabilityTest(common.HeatTestCase):
         snippet = rsrc_defn.ResourceDefinition('aresource',
                                                'OS::Heat::None')
         res = resource.Resource('aresource', snippet, self.stack)
+        res.entity = 'entity'
+        res.default_client_name = 'something'
+        res.resource_id = '12345'
 
         FakeClient = collections.namedtuple('Client', ['entity'])
         client = FakeClient(collections.namedtuple('entity', ['delete']))
+        client_plugin = res._default_client_plugin()
 
-        class FakeClientPlugin(object):
+        def is_not_found(ex):
-            def ignore_not_found(self, ex):
+            return isinstance(ex, exception.NotFound)
-                if not isinstance(ex, exception.NotFound):
-                    raise ex
 
+        client_plugin.is_not_found = mock.Mock(side_effect=is_not_found)
         self.patchobject(resource.Resource, 'client', return_value=client)
-        self.patchobject(resource.Resource, 'client_plugin',
-                         return_value=FakeClientPlugin())
         delete = mock.Mock()
         delete.side_effect = [exception.Error('boom!')]
         res.client().entity.delete = delete
-        res.entity = 'entity'
-        res.resource_id = '12345'
 
-        ex = self.assertRaises(exception.Error, res.handle_delete)
+        with mock.patch.object(res, '_default_client_plugin',
+                               return_value=client_plugin):
+            ex = self.assertRaises(exception.Error, res.handle_delete)
         self.assertEqual('boom!', six.text_type(ex))
         delete.assert_called_once_with('12345')
 
@@ -4003,6 +4008,7 @@ class ResourceAvailabilityTest(common.HeatTestCase):
         delete = mock.Mock()
         res.client().entity.delete = delete
         res.entity = 'entity'
+        res.default_client_name = 'something'
         res.resource_id = None
 
         self.assertIsNone(res.handle_delete())