Add 'callback' to 'wait_for_delete', 'wait_for_status'

This is helpful for OSC. We also use the opportunity to clean up the
tests for these two functions since they were fairly janky.

Change-Id: I559e6341b15041cb40fe208439da44c66b7cc6ca
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

openstack/block_storage/v2/_proxy.py

@@ -610,7 +610,13 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
         backup.reset(self, status)
 
     def wait_for_status(
-        self, res, status='available', failures=None, interval=2, wait=120
+        self,
+        res,
+        status='available',
+        failures=None,
+        interval=2,
+        wait=120,
+        callback=None,
     ):
         """Wait for a resource to be in a particular status.
 
@@ -624,6 +630,9 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
             checks. Default to 2.
         :param wait: Maximum number of seconds to wait before the change.
             Default to 120.
+        :param callback: A callback function. This will be called with a single
+            value, progress.
+
         :returns: The resource is returned on success.
         :raises: :class:`~openstack.exceptions.ResourceTimeout` if transition
             to the desired status failed to occur in specified seconds.
@@ -634,10 +643,16 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
         """
         failures = ['error'] if failures is None else failures
         return resource.wait_for_status(
-            self, res, status, failures, interval, wait
+            self,
+            res,
+            status,
+            failures,
+            interval,
+            wait,
+            callback=callback,
         )
 
-    def wait_for_delete(self, res, interval=2, wait=120):
+    def wait_for_delete(self, res, interval=2, wait=120, callback=None):
         """Wait for a resource to be deleted.
 
         :param res: The resource to wait on to be deleted.
@@ -646,11 +661,20 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
             checks. Default to 2.
         :param wait: Maximum number of seconds to wait before the change.
             Default to 120.
+        :param callback: A callback function. This will be called with a single
+            value, progress.
+
         :returns: The resource is returned on success.
         :raises: :class:`~openstack.exceptions.ResourceTimeout` if transition
             to delete failed to occur in the specified seconds.
         """
-        return resource.wait_for_delete(self, res, interval, wait)
+        return resource.wait_for_delete(
+            self,
+            res,
+            interval,
+            wait,
+            callback=callback,
+        )
 
     def get_quota_set(self, project, usage=False, **query):
         """Show QuotaSet information for the project

openstack/block_storage/v3/_proxy.py

@@ -1584,6 +1584,7 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
         failures=None,
         interval=2,
         wait=120,
+        callback=None,
     ):
         """Wait for a resource to be in a particular status.
 
@@ -1596,6 +1597,9 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
             checks. Default to 2.
         :param wait: Maximum number of seconds to wait before the change.
             Default to 120.
+        :param callback: A callback function. This will be called with a single
+            value, progress.
+
         :returns: The resource is returned on success.
         :raises: :class:`~openstack.exceptions.ResourceTimeout` if transition
             to the desired status failed to occur in specified seconds.
@@ -1606,10 +1610,16 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
         """
         failures = ['error'] if failures is None else failures
         return resource.wait_for_status(
-            self, res, status, failures, interval, wait
+            self,
+            res,
+            status,
+            failures,
+            interval,
+            wait,
+            callback=callback,
         )
 
-    def wait_for_delete(self, res, interval=2, wait=120):
+    def wait_for_delete(self, res, interval=2, wait=120, callback=None):
         """Wait for a resource to be deleted.
 
         :param res: The resource to wait on to be deleted.
@@ -1618,11 +1628,20 @@ class Proxy(_base_proxy.BaseBlockStorageProxy):
             checks. Default to 2.
         :param int wait: Maximum number of seconds to wait before the change.
             Default to 120.
+        :param callback: A callback function. This will be called with a single
+            value, progress.
+
         :returns: The resource is returned on success.
         :raises: :class:`~openstack.exceptions.ResourceTimeout` if transition
             to delete failed to occur in the specified seconds.
         """
-        return resource.wait_for_delete(self, res, interval, wait)
+        return resource.wait_for_delete(
+            self,
+            res,
+            interval,
+            wait,
+            callback=callback,
+        )
 
     def _get_cleanup_dependencies(self):
         return {'block_storage': {'before': []}}

openstack/compute/v2/_proxy.py

@@ -2468,6 +2468,7 @@ class Proxy(proxy.Proxy):
         failures=None,
         interval=2,
         wait=120,
+        callback=None,
     ):
         """Wait for a server to be in a particular status.
 
@@ -2484,6 +2485,10 @@ class Proxy(proxy.Proxy):
         :param wait: Maximum number of seconds to wait before the change.
             Default to 120.
         :type wait: int
+        :param callback: A callback function. This will be called with a single
+            value, progress, which is a percentage value from 0-100.
+        :type callback: callable
+
         :returns: The resource is returned on success.
         :raises: :class:`~openstack.exceptions.ResourceTimeout` if transition
             to the desired status failed to occur in specified seconds.
@@ -2500,9 +2505,10 @@ class Proxy(proxy.Proxy):
             failures,
             interval,
             wait,
+            callback=callback,
         )
 
-    def wait_for_delete(self, res, interval=2, wait=120):
+    def wait_for_delete(self, res, interval=2, wait=120, callback=None):
         """Wait for a resource to be deleted.
 
         :param res: The resource to wait on to be deleted.
@@ -2511,11 +2517,14 @@ class Proxy(proxy.Proxy):
             checks. Default to 2.
         :param wait: Maximum number of seconds to wait before the change.
             Default to 120.
+        :param callback: A callback function. This will be called with a single
+            value, progress, which is a percentage value from 0-100.
+
         :returns: The resource is returned on success.
         :raises: :class:`~openstack.exceptions.ResourceTimeout` if transition
             to delete failed to occur in the specified seconds.
         """
-        return resource.wait_for_delete(self, res, interval, wait)
+        return resource.wait_for_delete(self, res, interval, wait, callback)
 
     def _get_cleanup_dependencies(self):
         return {

openstack/resource.py

@@ -2329,6 +2329,7 @@ def wait_for_status(
     interval=None,
     wait=None,
     attribute='status',
+    callback=None,
 ):
     """Wait for the resource to be in a particular status.
 
@@ -2345,6 +2346,9 @@ def wait_for_status(
     :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.
+    :param callback: A callback function. This will be called with a single
+        value, progress. This is API specific but is generally a percentage
+        value from 0-100.
 
     :return: The updated resource.
     :raises: :class:`~openstack.exceptions.ResourceTimeout` transition
@@ -2372,7 +2376,6 @@ def wait_for_status(
         timeout=wait, message=msg, wait=interval
     ):
         resource = resource.fetch(session, skip_cache=True)
-
         if not resource:
             raise exceptions.ResourceFailure(
                 "{name} went away while waiting for {status}".format(
@@ -2399,8 +2402,12 @@ def wait_for_status(
             new_status,
         )
 
+        if callback:
+            progress = getattr(resource, 'progress', None) or 0
+            callback(progress)
 
-def wait_for_delete(session, resource, interval, wait):
+
+def wait_for_delete(session, resource, interval, wait, callback=None):
     """Wait for the resource to be deleted.
 
     :param session: The session to use for making this request.
@@ -2409,6 +2416,9 @@ def wait_for_delete(session, resource, interval, wait):
     :type resource: :class:`~openstack.resource.Resource`
     :param interval: Number of seconds to wait between checks.
     :param wait: Maximum number of seconds to wait for the delete.
+    :param callback: A callback function. This will be called with a single
+        value, progress. This is API specific but is generally a percentage
+        value from 0-100.
 
     :return: Method returns self on success.
     :raises: :class:`~openstack.exceptions.ResourceTimeout` transition
@@ -2430,3 +2440,7 @@ def wait_for_delete(session, resource, interval, wait):
                 return resource
         except exceptions.NotFoundException:
             return orig_resource
+
+        if callback:
+            progress = getattr(resource, 'progress', None) or 0
+            callback(progress)

openstack/tests/unit/block_storage/v2/test_proxy.py

@@ -121,6 +121,7 @@ class TestVolume(TestVolumeProxy):
             self.proxy.wait_for_status,
             method_args=[value],
             expected_args=[self.proxy, value, 'available', ['error'], 2, 120],
+            expected_kwargs={'callback': None},
         )
 
 

openstack/tests/unit/block_storage/v3/test_proxy.py

@@ -123,6 +123,7 @@ class TestVolume(TestVolumeProxy):
             self.proxy.wait_for_status,
             method_args=[value],
             expected_args=[self.proxy, value, 'available', ['error'], 2, 120],
+            expected_kwargs={'callback': None},
         )
 
 

openstack/tests/unit/compute/v2/test_proxy.py

@@ -963,6 +963,7 @@ class TestCompute(TestComputeProxy):
             self.proxy.wait_for_server,
             method_args=[value],
             expected_args=[self.proxy, value, 'ACTIVE', ['ERROR'], 2, 120],
+            expected_kwargs={'callback': None},
         )
 
     def test_server_resize(self):

openstack/tests/unit/test_resource.py

@@ -12,6 +12,7 @@
 
 import itertools
 import json
+import logging
 from unittest import mock
 
 from keystoneauth1 import adapter
@@ -3421,14 +3422,57 @@ class TestResourceFind(base.TestCase):
             )
 
 
-class TestWaitForStatus(base.TestCase):
+class TestWait(base.TestCase):
+    def setUp(self):
+        super().setUp()
+
+        handler = logging.StreamHandler(self._log_stream)
+        formatter = logging.Formatter('%(asctime)s %(name)-32s %(message)s')
+        handler.setFormatter(formatter)
+
+        logger = logging.getLogger('openstack.iterate_timeout')
+        logger.setLevel(logging.DEBUG)
+        logger.addHandler(handler)
+
+    @staticmethod
+    def _fake_resource(statuses=None, progresses=None, *, attribute='status'):
+        if statuses is None:
+            statuses = ['building', 'building', 'building', 'active']
+
+        def fetch(*args, **kwargs):
+            # when we get to the last status, keep returning that
+            if statuses:
+                setattr(fake_resource, attribute, statuses.pop(0))
+
+            if progresses:
+                fake_resource.progress = progresses.pop(0)
+
+            return fake_resource
+
+        spec = ['id', attribute, 'fetch']
+        if progresses:
+            spec.append('progress')
+
+        fake_resource = mock.Mock(spec=spec)
+        setattr(fake_resource, attribute, statuses.pop(0))
+        fake_resource.fetch.side_effect = fetch
+
+        return fake_resource
+
+
+class TestWaitForStatus(TestWait):
     def test_immediate_status(self):
         status = "loling"
         res = mock.Mock(spec=['id', 'status'])
         res.status = status
 
         result = resource.wait_for_status(
-            self.cloud.compute, res, status, "failures", "interval", "wait"
+            self.cloud.compute,
+            res,
+            status,
+            None,
+            interval=1,
+            wait=1,
         )
 
         self.assertEqual(res, result)
@@ -3439,7 +3483,12 @@ class TestWaitForStatus(base.TestCase):
         res.status = status
 
         result = resource.wait_for_status(
-            self.cloud.compute, res, 'lOling', "failures", "interval", "wait"
+            self.cloud.compute,
+            res,
+            'lOling',
+            None,
+            interval=1,
+            wait=1,
         )
 
         self.assertEqual(res, result)
@@ -3453,127 +3502,131 @@ class TestWaitForStatus(base.TestCase):
             self.cloud.compute,
             res,
             status,
-            "failures",
+            None,
-            "interval",
+            interval=1,
-            "wait",
+            wait=1,
             attribute='mood',
         )
 
         self.assertEqual(res, result)
 
-    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(spec=['id', 'fetch', attribute])
-            setattr(res, attribute, status)
-            resources.append(res)
-        for index, res in enumerate(resources[:-1]):
-            res.fetch.return_value = resources[index + 1]
-        return resources
-
     def test_status_match(self):
         status = "loling"
 
         # other gets past the first check, two anothers gets through
         # the sleep loop, and the third matches
-        resources = self._resources_from_statuses(
+        statuses = ["first", "other", "another", "another", status]
-            "first", "other", "another", "another", status
+        res = self._fake_resource(statuses)
-        )
 
         result = resource.wait_for_status(
-            mock.Mock(), resources[0], status, None, 1, 5
+            mock.Mock(),
+            res,
+            status,
+            None,
+            interval=1,
+            wait=5,
         )
 
-        self.assertEqual(result, resources[-1])
+        self.assertEqual(result, res)
 
     def test_status_match_with_none(self):
         status = "loling"
 
         # apparently, None is a correct state in some cases
-        resources = self._resources_from_statuses(
+        statuses = [None, "other", None, "another", status]
-            None, "other", None, "another", status
+        res = self._fake_resource(statuses)
-        )
 
         result = resource.wait_for_status(
-            mock.Mock(), resources[0], status, None, 1, 5
+            mock.Mock(),
+            res,
+            status,
+            None,
+            interval=1,
+            wait=5,
         )
 
-        self.assertEqual(result, resources[-1])
+        self.assertEqual(result, res)
 
     def test_status_match_none(self):
         status = None
 
         # apparently, None can be expected status in some cases
-        resources = self._resources_from_statuses(
+        statuses = ["first", "other", "another", "another", status]
-            "first", "other", "another", "another", status
+        res = self._fake_resource(statuses)
-        )
 
         result = resource.wait_for_status(
-            mock.Mock(), resources[0], status, None, 1, 5
+            mock.Mock(),
+            res,
+            status,
+            None,
+            interval=1,
+            wait=5,
         )
 
-        self.assertEqual(result, resources[-1])
+        self.assertEqual(result, res)
 
     def test_status_match_different_attribute(self):
         status = "loling"
 
-        resources = self._resources_from_statuses(
+        statuses = ["first", "other", "another", "another", status]
-            "first", "other", "another", "another", status, attribute='mood'
+        res = self._fake_resource(statuses, attribute='mood')
-        )
 
         result = resource.wait_for_status(
-            mock.Mock(), resources[0], status, None, 1, 5, attribute='mood'
+            mock.Mock(),
+            res,
+            status,
+            None,
+            interval=1,
+            wait=5,
+            attribute='mood',
         )
 
-        self.assertEqual(result, resources[-1])
+        self.assertEqual(result, res)
 
     def test_status_fails(self):
         failure = "crying"
 
-        resources = self._resources_from_statuses("success", "other", failure)
+        statuses = ["success", "other", failure]
+        res = self._fake_resource(statuses)
 
         self.assertRaises(
             exceptions.ResourceFailure,
             resource.wait_for_status,
             mock.Mock(),
-            resources[0],
+            res,
             "loling",
             [failure],
-            1,
+            interval=1,
-            5,
+            wait=5,
         )
 
     def test_status_fails_different_attribute(self):
         failure = "crying"
 
-        resources = self._resources_from_statuses(
+        statuses = ["success", "other", failure]
-            "success", "other", failure, attribute='mood'
+        res = self._fake_resource(statuses, attribute='mood')
-        )
 
         self.assertRaises(
             exceptions.ResourceFailure,
             resource.wait_for_status,
             mock.Mock(),
-            resources[0],
+            res,
             "loling",
             [failure.upper()],
-            1,
+            interval=1,
-            5,
+            wait=5,
             attribute='mood',
         )
 
     def test_timeout(self):
         status = "loling"
-        res = mock.Mock()
 
         # The first "other" gets past the first check, and then three
         # pairs of "other" statuses run through the sleep counter loop,
         # after which time should be up. This is because we have a
         # one second interval and three second waiting period.
         statuses = ["other"] * 7
-        type(res).status = mock.PropertyMock(side_effect=statuses)
+        res = self._fake_resource(statuses)
 
         self.assertRaises(
             exceptions.ResourceTimeout,
@@ -3587,9 +3640,8 @@ class TestWaitForStatus(base.TestCase):
         )
 
     def test_no_sleep(self):
-        res = mock.Mock()
         statuses = ["other"]
-        type(res).status = mock.PropertyMock(side_effect=statuses)
+        res = self._fake_resource(statuses)
 
         self.assertRaises(
             exceptions.ResourceTimeout,
@@ -3598,20 +3650,63 @@ class TestWaitForStatus(base.TestCase):
             res,
             "status",
             None,
-            0,
+            interval=0,
-            -1,
+            wait=-1,
         )
 
+    def test_callback(self):
+        """Callback is called with 'progress' attribute."""
+        statuses = ['building', 'building', 'building', 'building', 'active']
+        progresses = [0, 25, 50, 100]
+        res = self._fake_resource(statuses=statuses, progresses=progresses)
 
-class TestWaitForDelete(base.TestCase):
+        callback = mock.Mock()
-    def test_success(self):
+
+        result = resource.wait_for_status(
+            mock.Mock(),
+            res,
+            'active',
+            None,
+            interval=0.1,
+            wait=1,
+            callback=callback,
+        )
+
+        self.assertEqual(result, res)
+        callback.assert_has_calls([mock.call(x) for x in progresses])
+
+    def test_callback_without_progress(self):
+        """Callback is called with 0 if 'progress' attribute is missing."""
+        statuses = ['building', 'building', 'building', 'building', 'active']
+        res = self._fake_resource(statuses=statuses)
+
+        callback = mock.Mock()
+
+        result = resource.wait_for_status(
+            mock.Mock(),
+            res,
+            'active',
+            None,
+            interval=0.1,
+            wait=1,
+            callback=callback,
+        )
+
+        self.assertEqual(result, res)
+        # there are 5 statuses but only 3 callback calls since the initial
+        # status and final status don't result in calls
+        callback.assert_has_calls([mock.call(0)] * 3)
+
+
+class TestWaitForDelete(TestWait):
+    def test_success_not_found(self):
         response = mock.Mock()
         response.headers = {}
         response.status_code = 404
         res = mock.Mock()
         res.fetch.side_effect = [
-            None,
+            res,
-            None,
+            res,
             exceptions.ResourceNotFound('Not Found', response),
         ]
 
@@ -3619,6 +3714,59 @@ class TestWaitForDelete(base.TestCase):
 
         self.assertEqual(result, res)
 
+    def test_status(self):
+        """Successful deletion indicated by status."""
+        statuses = ['active', 'deleting', 'deleting', 'deleting', 'deleted']
+        res = self._fake_resource(statuses=statuses)
+
+        result = resource.wait_for_delete(
+            mock.Mock(),
+            res,
+            interval=0.1,
+            wait=1,
+        )
+
+        self.assertEqual(result, res)
+
+    def test_callback(self):
+        """Callback is called with 'progress' attribute."""
+        statuses = ['active', 'deleting', 'deleting', 'deleting', 'deleted']
+        progresses = [0, 25, 50, 100]
+        res = self._fake_resource(statuses=statuses, progresses=progresses)
+
+        callback = mock.Mock()
+
+        result = resource.wait_for_delete(
+            mock.Mock(),
+            res,
+            interval=1,
+            wait=5,
+            callback=callback,
+        )
+
+        self.assertEqual(result, res)
+        callback.assert_has_calls([mock.call(x) for x in progresses])
+
+    def test_callback_without_progress(self):
+        """Callback is called with 0 if 'progress' attribute is missing."""
+        statuses = ['active', 'deleting', 'deleting', 'deleting', 'deleted']
+        res = self._fake_resource(statuses=statuses)
+
+        callback = mock.Mock()
+
+        result = resource.wait_for_delete(
+            mock.Mock(),
+            res,
+            interval=1,
+            wait=5,
+            callback=callback,
+        )
+
+        self.assertEqual(result, res)
+        # there are 5 statuses but only 3 callback calls since the initial
+        # status and final status don't result in calls
+        callback.assert_has_calls([mock.call(0)] * 3)
+
     def test_timeout(self):
         res = mock.Mock()
         res.status = 'ACTIVE'

releasenotes/notes/wait_for_status_delete_callback_param-68d30161e23340bb.yaml

@@ -0,0 +1,8 @@
+---
+features:
+  - |
+    The ``Resource.wait_for_status``, ``Resource.wait_for_delete``, and related
+    proxy wrappers now accept a ``callback`` argument that can be used to pass
+    a callback function. When provided, the wait function will attempt to
+    retrieve a ``progress`` value from the resource in question and pass it to
+    the callback function each time it iterates.

tox.ini

@@ -15,7 +15,7 @@ setenv =
     LANG=en_US.UTF-8
     LANGUAGE=en_US:en
     LC_ALL=C
-    OS_LOG_CAPTURE={env:OS_LOG_CAPTURE:false}
+    OS_LOG_CAPTURE={env:OS_LOG_CAPTURE:true}
     OS_STDOUT_CAPTURE={env:OS_STDOUT_CAPTURE:true}
     OS_STDERR_CAPTURE={env:OS_STDERR_CAPTURE:true}
 deps =