From 4e8a1f6ae96f3c4cf83fea8b3faa5dfce2104349 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 25 May 2018 10:28:22 +0200 Subject: [PATCH] Several improvements to resource.wait_for_status * Clarify that interval and wait can be None (already supported by the underlying interval_timeout function). * Fix incorrect :returns: docstring. * Allow the caller to override which attribute to use. (ironic nodes have provision_state and power_state, not status). * Add some logging when looping (the underlying interval_timeout call does it, but just "Waiting %d seconds" is not enough in the context of a big caller application). * Use lower() on the initial check for status to be consistent with how it is treated inside the loop (correctly handling None, which is a possible state according to the CI). Change-Id: Icd58fdab1438b738d3ce6cd3162bb7445e9a2bf9 --- openstack/resource.py | 35 ++++++++++---- openstack/tests/unit/test_resource.py | 68 +++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 12 deletions(-) diff --git a/openstack/resource.py b/openstack/resource.py index 2801c33df..d7d49e73c 100644 --- a/openstack/resource.py +++ b/openstack/resource.py @@ -37,6 +37,7 @@ import itertools from keystoneauth1 import adapter from requests import structures +from openstack import _log from openstack import exceptions from openstack import format from openstack import utils @@ -1047,21 +1048,31 @@ class Resource(object): "No %s found for %s" % (cls.__name__, name_or_id)) -def wait_for_status(session, resource, status, failures, interval, wait): +def _normalize_status(status): + if status is not None: + status = status.lower() + return status + + +def wait_for_status(session, resource, status, failures, interval=None, + wait=None, attribute='status'): """Wait for the resource to be in a particular status. :param session: The session to use for making this request. :type session: :class:`~keystoneauth1.adapter.Adapter` :param resource: The resource to wait on to reach the status. The resource - must have a status attribute. + must have a status attribute specified via ``attribute``. :type resource: :class:`~openstack.resource.Resource` :param status: Desired status of the resource. :param list failures: Statuses that would indicate the transition failed such as 'ERROR'. Defaults to ['ERROR']. :param interval: Number of seconds to wait between checks. + Set to ``None`` to use the default interval. :param wait: Maximum number of seconds to wait for transition. + Set to ``None`` to wait forever. + :param attribute: Name of the resource attribute that contains the status. - :return: Method returns self on success. + :return: The updated resource. :raises: :class:`~openstack.exceptions.ResourceTimeout` transition to status failed to occur in wait seconds. :raises: :class:`~openstack.exceptions.ResourceFailure` resource @@ -1069,7 +1080,10 @@ def wait_for_status(session, resource, status, failures, interval, wait): :raises: :class:`~AttributeError` if the resource does not have a status attribute """ - if resource.status == status: + log = _log.setup_logging(__name__) + + current_status = getattr(resource, attribute) + if _normalize_status(current_status) == status.lower(): return resource if failures is None: @@ -1090,13 +1104,18 @@ def wait_for_status(session, resource, status, failures, interval, wait): raise exceptions.ResourceFailure( "{name} went away while waiting for {status}".format( name=name, status=status)) - new_status = resource.status - if new_status.lower() == status.lower(): + + new_status = getattr(resource, attribute) + normalized_status = _normalize_status(new_status) + if normalized_status == status.lower(): return resource - if resource.status.lower() in failures: + elif normalized_status in failures: raise exceptions.ResourceFailure( "{name} transitioned to failure state {status}".format( - name=name, status=resource.status)) + name=name, status=new_status)) + + log.debug('Still waiting for resource %s to reach state %s, ' + 'current state is %s', name, status, new_status) def wait_for_delete(session, resource, interval, wait): diff --git a/openstack/tests/unit/test_resource.py b/openstack/tests/unit/test_resource.py index 090677c52..41ed10959 100644 --- a/openstack/tests/unit/test_resource.py +++ b/openstack/tests/unit/test_resource.py @@ -1771,7 +1771,7 @@ class TestWaitForStatus(base.TestCase): def test_immediate_status(self): status = "loling" - res = mock.Mock() + res = mock.Mock(spec=['id', 'status']) res.status = status result = resource.wait_for_status( @@ -1779,11 +1779,34 @@ class TestWaitForStatus(base.TestCase): self.assertTrue(result, res) - def _resources_from_statuses(self, *statuses): + def test_immediate_status_case(self): + status = "LOLing" + res = mock.Mock(spec=['id', 'status']) + res.status = status + + result = resource.wait_for_status( + "session", res, 'lOling', "failures", "interval", "wait") + + self.assertTrue(result, res) + + def test_immediate_status_different_attribute(self): + status = "loling" + res = mock.Mock(spec=['id', 'mood']) + res.mood = status + + result = resource.wait_for_status( + "session", res, status, "failures", "interval", "wait", + attribute='mood') + + self.assertTrue(result, res) + + def _resources_from_statuses(self, *statuses, **kwargs): + attribute = kwargs.pop('attribute', 'status') + assert not kwargs, 'Unexpected keyword arguments: %s' % kwargs resources = [] for status in statuses: - res = mock.Mock() - res.status = status + res = mock.Mock(spec=['id', 'get', attribute]) + setattr(res, attribute, status) resources.append(res) for index, res in enumerate(resources[:-1]): res.get.return_value = resources[index + 1] @@ -1802,6 +1825,31 @@ class TestWaitForStatus(base.TestCase): self.assertEqual(result, resources[-1]) + def test_status_match_with_none(self): + status = "loling" + + # apparently, None is a correct state in some cases + resources = self._resources_from_statuses( + None, "other", None, "another", status) + + result = resource.wait_for_status( + mock.Mock(), resources[0], status, None, 1, 5) + + self.assertEqual(result, resources[-1]) + + def test_status_match_different_attribute(self): + status = "loling" + + resources = self._resources_from_statuses( + "first", "other", "another", "another", status, + attribute='mood') + + result = resource.wait_for_status( + mock.Mock(), resources[0], status, None, 1, 5, + attribute='mood') + + self.assertEqual(result, resources[-1]) + def test_status_fails(self): failure = "crying" @@ -1812,6 +1860,18 @@ class TestWaitForStatus(base.TestCase): resource.wait_for_status, mock.Mock(), resources[0], "loling", [failure], 1, 5) + def test_status_fails_different_attribute(self): + failure = "crying" + + resources = self._resources_from_statuses("success", "other", failure, + attribute='mood') + + self.assertRaises( + exceptions.ResourceFailure, + resource.wait_for_status, + mock.Mock(), resources[0], "loling", [failure.upper()], 1, 5, + attribute='mood') + def test_timeout(self): status = "loling" res = mock.Mock()