Move _check_active to nova client plugin

The method was practically identical in Instance and Server resources,
with only difference in reported resource type as part of error message.

Some mocks that mocked this method directly on resource object were
modified accordingly in unit tests.

The new `_check_active` is made to accept both Nova server objects and server
UUIDs/strings. This will allow changing methods of server resources
(and tests!) one-by-one.

Added a new method `fetch_server` that tolerates intermittent failures
when fetching a fresh server object (similar to existing `refresh_server`).

Unit tests for `_check_active` and `fetch_server` added.

Related-Bug: #1393268
Change-Id: I1b63dbd9182cc8519f43ff859a408d49789263dd

heat/engine/clients/os/nova.py

@@ -35,6 +35,7 @@ from heat.common.i18n import _LI
 from heat.common.i18n import _LW
 from heat.engine.clients import client_plugin
 from heat.engine import constraints
+from heat.engine import resource
 from heat.engine import scheduler
 
 LOG = logging.getLogger(__name__)
@@ -104,6 +105,46 @@ class NovaClientPlugin(client_plugin.ClientPlugin):
         return (isinstance(ex, exceptions.ClientException) and
                 http_status == 422)
 
+    def get_server(self, server):
+        """Return fresh server object.
+
+        Substitutes Nova's NotFound for Heat's EntityNotFound,
+        to be returned to user as HTTP error.
+        """
+        try:
+            return self.client().servers.get(server)
+        except exceptions.NotFound as ex:
+            LOG.warn(_LW('Server (%(server)s) not found: %(ex)s'),
+                     {'server': server, 'ex': ex})
+            raise exception.EntityNotFound(entity='Server', name=server)
+
+    def fetch_server(self, server_id):
+        """
+        Fetch fresh server object from Nova.
+
+        Log warnings and return None for non-critical API errors.
+        Use this method in various ``check_*_complete`` resource methods,
+        where intermittent errors can be tolerated.
+        """
+        server = None
+        try:
+            server = self.client().servers.get(server_id)
+        except exceptions.OverLimit as exc:
+            LOG.warn(_LW("Received an OverLimit response when "
+                         "fetching server (%(id)s) : %(exception)s"),
+                     {'id': server_id,
+                      'exception': exc})
+        except exceptions.ClientException as exc:
+            if ((getattr(exc, 'http_status', getattr(exc, 'code', None)) in
+                 (500, 503))):
+                LOG.warn(_LW("Received the following exception when "
+                         "fetching server (%(id)s) : %(exception)s"),
+                         {'id': server_id,
+                          'exception': exc})
+            else:
+                raise
+        return server
+
     def refresh_server(self, server):
         '''
         Refresh server's attributes and log warnings for non-critical
@@ -145,6 +186,47 @@ class NovaClientPlugin(client_plugin.ClientPlugin):
         # Some clouds append extra (STATUS) strings to the status, strip it
         return server.status.split('(')[0]
 
+    def _check_active(self, server, res_name='Server'):
+        """Check server status.
+
+        Accepts both server IDs and server objects.
+        Returns True if server is ACTIVE,
+        raises errors when server has an ERROR or unknown to Heat status,
+        returns False otherwise.
+
+        :param res_name: name of the resource to use in the exception message
+
+        """
+        # not checking with is_uuid_like as most tests use strings e.g. '1234'
+        if isinstance(server, six.string_types):
+            server = self.fetch_server(server)
+            if server is None:
+                return False
+            else:
+                status = self.get_status(server)
+        else:
+            status = self.get_status(server)
+            if status != 'ACTIVE':
+                self.refresh_server(server)
+                status = self.get_status(server)
+
+        if status in self.deferred_server_statuses:
+            return False
+        elif status == 'ACTIVE':
+            return True
+        elif status == 'ERROR':
+            fault = getattr(server, 'fault', {})
+            raise resource.ResourceInError(
+                resource_status=status,
+                status_reason=_("Message: %(message)s, Code: %(code)s") % {
+                    'message': fault.get('message', _('Unknown')),
+                    'code': fault.get('code', _('Unknown'))
+                })
+        else:
+            raise resource.ResourceUnknownStatus(
+                resource_status=server.status,
+                result=_('%s is not active') % res_name)
+
     def get_flavor_id(self, flavor):
         '''
         Get the id for the specified flavor name.
@@ -419,14 +501,6 @@ echo -e '%s\tALL=(ALL)\tNOPASSWD: ALL' >> /etc/sudoers
                 if len(server.networks[n]) > 0:
                     return server.networks[n][0]
 
-    def get_server(self, server):
-        try:
-            return self.client().servers.get(server)
-        except exceptions.NotFound as ex:
-            LOG.warn(_LW('Server (%(server)s) not found: %(ex)s'),
-                     {'server': server, 'ex': ex})
-            raise exception.EntityNotFound(entity='Server', name=server)
-
     def absolute_limits(self):
         """Return the absolute limits as a dictionary."""
         limits = self.client().limits.get()

heat/engine/resources/aws/ec2/instance.py

@@ -588,7 +588,7 @@ class Instance(resource.Resource):
 
     def check_create_complete(self, cookie):
         server, volume_attach_task = cookie
-        return (self._check_active(server) and
+        return (self.client_plugin()._check_active(server, 'Instance') and
                 self._check_volume_attached(server, volume_attach_task))
 
     def _check_volume_attached(self, server, volume_attach_task):
@@ -599,32 +599,6 @@ class Instance(resource.Resource):
         else:
             return volume_attach_task.step()
 
-    def _check_active(self, server):
-        cp = self.client_plugin()
-        status = cp.get_status(server)
-        if status != 'ACTIVE':
-            cp.refresh_server(server)
-            status = cp.get_status(server)
-
-        if status == 'ACTIVE':
-            return True
-
-        if status in cp.deferred_server_statuses:
-            return False
-
-        if status == 'ERROR':
-            fault = getattr(server, 'fault', {})
-            raise resource.ResourceInError(
-                resource_status=status,
-                status_reason=_("Message: %(message)s, Code: %(code)s") % {
-                    'message': fault.get('message', _('Unknown')),
-                    'code': fault.get('code', _('Unknown'))
-                })
-
-        raise resource.ResourceUnknownStatus(
-            resource_status=server.status,
-            result=_('Instance is not active'))
-
     def volumes(self):
         """
         Return an iterator over (volume_id, device) tuples for all volumes
@@ -645,7 +619,7 @@ class Instance(resource.Resource):
 
     def handle_check(self):
         server = self.nova().servers.get(self.resource_id)
-        if not self._check_active(server):
+        if not self.client_plugin()._check_active(server, 'Instance'):
             raise exception.Error(_("Instance is not ACTIVE (was: %s)") %
                                   server.status.strip())
 
@@ -869,7 +843,7 @@ class Instance(resource.Resource):
             return server
 
     def check_resume_complete(self, server):
-        return self._check_active(server)
+        return self.client_plugin()._check_active(server, 'Instance')
 
 
 def resource_mapping():

heat/engine/resources/openstack/nova/server.py

@@ -722,31 +722,7 @@ class Server(stack_user.StackUser):
         return server
 
     def check_create_complete(self, server):
-        return self._check_active(server)
-
-    def _check_active(self, server):
-        cp = self.client_plugin()
-        status = cp.get_status(server)
-        if status != 'ACTIVE':
-            cp.refresh_server(server)
-            status = cp.get_status(server)
-
-        if status in cp.deferred_server_statuses:
-            return False
-        elif status == 'ACTIVE':
-            return True
-        elif status == 'ERROR':
-            fault = getattr(server, 'fault', {})
-            raise resource.ResourceInError(
-                resource_status=status,
-                status_reason=_("Message: %(message)s, Code: %(code)s") % {
-                    'message': fault.get('message', _('Unknown')),
-                    'code': fault.get('code', _('Unknown'))
-                })
-        else:
-            raise resource.ResourceUnknownStatus(
-                resource_status=server.status,
-                result=_('Server is not active'))
+        return self.client_plugin()._check_active(server)
 
     def handle_check(self):
         server = self.client().servers.get(self.resource_id)
@@ -1452,7 +1428,7 @@ class Server(stack_user.StackUser):
             return server
 
     def check_resume_complete(self, server):
-        return self._check_active(server)
+        return self.client_plugin()._check_active(server)
 
     def handle_snapshot(self):
         image_id = self.nova().servers.create_image(

heat/tests/aws/test_instance.py

@@ -449,7 +449,8 @@ class InstancesTest(common.HeatTestCase):
         instance = instances.Instance('instance_create_image',
                                       res_definitions['WebServer'], stack)
         instance.nova = mock.Mock()
-        instance._check_active = mock.Mock(return_value=True)
+        self.patchobject(nova.NovaClientPlugin, '_check_active',
+                         return_value=True)
 
         self.assertIsNone(instance.handle_check())
 
@@ -461,7 +462,8 @@ class InstancesTest(common.HeatTestCase):
                                       res_definitions['WebServer'], stack)
         instance.nova = mock.Mock()
         instance.nova.return_value.servers.get.return_value.status = 'foo'
-        instance._check_active = mock.Mock(return_value=False)
+        self.patchobject(nova.NovaClientPlugin, '_check_active',
+                         return_value=False)
 
         exc = self.assertRaises(exception.Error, instance.handle_check)
         self.assertIn('foo', six.text_type(exc))

heat/tests/test_nova_client.py

@@ -22,6 +22,7 @@ import six
 
 from heat.common import exception
 from heat.engine.clients.os import nova
+from heat.engine import resource
 from heat.tests import common
 from heat.tests.nova import fakes as fakes_nova
 from heat.tests import utils
@@ -172,7 +173,7 @@ class NovaClientPluginTests(NovaClientPluginTestCase):
         self.assertEqual('ACTIVE', observed)
 
 
-class NovaUtilsRefreshServerTests(NovaClientPluginTestCase):
+class NovaClientPluginRefreshServerTests(NovaClientPluginTestCase):
     msg = ("ClientException: The server has either erred or is "
            "incapable of performing the requested operation.")
 
@@ -205,7 +206,106 @@ class NovaUtilsRefreshServerTests(NovaClientPluginTestCase):
         server.get.assert_called_once_with()
 
 
-class NovaUtilsUserdataTests(NovaClientPluginTestCase):
+class NovaClientPluginFetchServerTests(NovaClientPluginTestCase):
+
+    server = mock.Mock()
+    # set explicitly as id and name has internal meaning in mock.Mock
+    server.id = '1234'
+    server.name = 'test_fetch_server'
+    msg = ("ClientException: The server has either erred or is "
+           "incapable of performing the requested operation.")
+    scenarios = [
+        ('successful_refresh', dict(
+            value=server,
+            e_raise=False)),
+        ('overlimit_error', dict(
+            value=nova_exceptions.OverLimit(413, "limit reached"),
+            e_raise=False)),
+        ('500_error', dict(
+            value=nova_exceptions.ClientException(500, msg),
+            e_raise=False)),
+        ('503_error', dict(
+            value=nova_exceptions.ClientException(503, msg),
+            e_raise=False)),
+        ('unhandled_exception', dict(
+            value=nova_exceptions.ClientException(501, msg),
+            e_raise=True)),
+    ]
+
+    def test_fetch_server(self):
+        self.nova_client.servers.get.side_effect = [self.value]
+        if self.e_raise:
+            self.assertRaises(nova_exceptions.ClientException,
+                              self.nova_plugin.fetch_server, self.server.id)
+        elif isinstance(self.value, mock.Mock):
+            self.assertEqual(self.value,
+                             self.nova_plugin.fetch_server(self.server.id))
+        else:
+            self.assertIsNone(self.nova_plugin.fetch_server(self.server.id))
+
+        self.nova_client.servers.get.assert_called_once_with(self.server.id)
+
+
+class NovaClientPluginCheckActiveTests(NovaClientPluginTestCase):
+
+    scenarios = [
+        ('active', dict(
+            status='ACTIVE',
+            e_raise=False)),
+        ('deferred', dict(
+            status='BUILD',
+            e_raise=False)),
+        ('error', dict(
+            status='ERROR',
+            e_raise=resource.ResourceInError)),
+        ('unknown', dict(
+            status='VIKINGS!',
+            e_raise=resource.ResourceUnknownStatus))
+    ]
+
+    def setUp(self):
+        super(NovaClientPluginCheckActiveTests, self).setUp()
+        self.server = mock.Mock()
+        self.server.id = '1234'
+        self.server.status = self.status
+        self.r_mock = self.patchobject(self.nova_plugin, 'refresh_server',
+                                       return_value=None)
+        self.f_mock = self.patchobject(self.nova_plugin, 'fetch_server',
+                                       return_value=self.server)
+
+    def test_check_active_with_object(self):
+        if self.e_raise:
+            self.assertRaises(self.e_raise,
+                              self.nova_plugin._check_active, self.server)
+            self.r_mock.assert_called_once_with(self.server)
+        elif self.status in self.nova_plugin.deferred_server_statuses:
+            self.assertFalse(self.nova_plugin._check_active(self.server))
+            self.r_mock.assert_called_once_with(self.server)
+        else:
+            self.assertTrue(self.nova_plugin._check_active(self.server))
+            self.assertEqual(0, self.r_mock.call_count)
+        self.assertEqual(0, self.f_mock.call_count)
+
+    def test_check_active_with_string(self):
+        if self.e_raise:
+            self.assertRaises(self.e_raise,
+                              self.nova_plugin._check_active, self.server.id)
+        elif self.status in self.nova_plugin.deferred_server_statuses:
+            self.assertFalse(self.nova_plugin._check_active(self.server.id))
+        else:
+            self.assertTrue(self.nova_plugin._check_active(self.server.id))
+
+        self.f_mock.assert_called_once_with(self.server.id)
+        self.assertEqual(0, self.r_mock.call_count)
+
+    def test_check_active_with_string_unavailable(self):
+        self.f_mock.return_value = None
+        self.assertFalse(self.nova_plugin._check_active(self.server.id))
+        self.f_mock.assert_called_once_with(self.server.id)
+        self.assertEqual(0, self.r_mock.call_count)
+
+
+class NovaClientPluginUserdataTests(NovaClientPluginTestCase):
 
     def test_build_userdata(self):
         """Tests the build_userdata function."""
@@ -253,7 +353,7 @@ class NovaUtilsUserdataTests(NovaClientPluginTestCase):
         self.assertIn("custominstanceuser", data)
 
 
-class NovaUtilsMetadataTests(NovaClientPluginTestCase):
+class NovaClientPluginMetadataTests(NovaClientPluginTestCase):
 
     def test_serialize_string(self):
         original = {'test_key': 'simple string value'}