[Kimchi-devel] [PATCH 3/5] AsyncTask: Improve continuous status feedback

[Aline Manera]

On 10/28/2014 04:48 AM, Zhou Zheng Sheng wrote:
> One of the advantage of AsyncTask is that the background task can call
> "_status_cb()" multiple times to report the interim progress. However
> this is not properly implemented in "_status_cb()". The correct handling
> should be as follow.
>
> cb(message, True)
>    Means task finished succesfully.
>
> cb(message, False)
>    Means task failed.
>
> cb(message)
> cb(message, None)
>    Means task is ongoing and the invocation is to provide progress feedback.
>
> The current implementation fails to distinguish "cb(message, False)" and
> "cb(message)". This patch fixes the problem and adds a new test case.
>
> This patch also improves the "_wait_task()" methods in tests, adding a
> regular status report to allow developer know it's waiting for some task
> but not stuck on something. There is also two methods in tests that start a
> AsyncTask but do not wait it till finish, this will interfere with other
> tests, so the patch adds the missing "_wait_task()" invocation for them.
>
> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> ---
>   src/kimchi/asynctask.py |  4 ++--
>   tests/test_mockmodel.py | 14 ++++++++++++++
>   tests/test_model.py     | 31 ++++++++++++++++++++++++++-----
>   tests/test_rest.py      |  7 +++++++
>   4 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/src/kimchi/asynctask.py b/src/kimchi/asynctask.py
> index 99e7a64..f8e55c0 100644
> --- a/src/kimchi/asynctask.py
> +++ b/src/kimchi/asynctask.py
> @@ -49,9 +49,9 @@ class AsyncTask(object):
>               self._save_helper()
>               return
>
> -        if success:
> +        if success == True:
>               self.status = 'finished'
> -        else:
> +        elif success ==False:
>               self.status = 'failed'
>           self.message = message
>           self._save_helper()
> diff --git a/tests/test_mockmodel.py b/tests/test_mockmodel.py
> index 8de7ce9..3158c48 100644
> --- a/tests/test_mockmodel.py
> +++ b/tests/test_mockmodel.py
> @@ -27,6 +27,7 @@ import unittest
>   import kimchi.mockmodel
>   from utils import get_free_port, patch_auth, request, run_server
>   from kimchi.control.base import Collection, Resource
> +from kimchi.utils import kimchi_log
>
>
>   test_server = None
> @@ -174,3 +175,16 @@ class MockModelTests(unittest.TestCase):
>           task = model.host_swupdate()
>           task_params = [u'id', u'message', u'status', u'target_uri']
>           self.assertEquals(sorted(task_params), sorted(task.keys()))
> +        self._wait_task(model, task['id'])

> +
> +    def _wait_task(self, model, taskid, timeout=5):
> +        for i in range(0, timeout):
> +            task_info = model.task_lookup(taskid)
> +            if task_info['status'] == "running":
> +                kimchi_log.info("Waiting task %s, message: %s",
> +                                task_info['id'], task_info['message'])
> +                time.sleep(1)
> +            else:
> +                return
> +        kimchi_log.error("Timeout while process long-run task, "
> +                         "try to increase timeout value.")

We could move it to tests/utils.py to avoid duplicating code when a Task 
needs to be watched.

> diff --git a/tests/test_model.py b/tests/test_model.py
> index e407fe5..c0e8cb5 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -42,7 +42,7 @@ from kimchi.exception import InvalidParameter, NotFoundError, OperationFailed
>   from kimchi.iscsi import TargetClient
>   from kimchi.model import model
>   from kimchi.rollbackcontext import RollbackContext
> -from kimchi.utils import add_task
> +from kimchi.utils import add_task, kimchi_log
>
>
>   invalid_repository_urls = ['www.fedora.org',       # missing protocol
> @@ -285,8 +285,9 @@ class ModelTests(unittest.TestCase):
>                         'capacity': 1024,
>                         'allocation': 512,
>                         'format': 'qcow2'}
> -            inst.storagevolumes_create(pool, params)
> +            task_id = inst.storagevolumes_create(pool, params)['id']
>               rollback.prependDefer(inst.storagevolume_delete, pool, vol)
> +            self._wait_task(inst, task_id)
>
>               vm_name = 'kimchi-cdrom'
>               params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso}
> @@ -1107,6 +1108,13 @@ class ModelTests(unittest.TestCase):
>               except:
>                   cb("Exception raised", False)
>
> +        def continuous_ops(cb, params):
> +            cb("step 1 OK")
> +            time.sleep(2)
> +            cb("step 2 OK")
> +            time.sleep(2)
> +            cb("step 3 OK", params.get('result', True))
> +
>           inst = model.Model('test:///default',
>                              objstore_loc=self.tmp_store)
>           taskid = add_task('', quick_op, inst.objstore, 'Hello')
> @@ -1131,6 +1139,12 @@ class ModelTests(unittest.TestCase):
>                             inst.task_lookup(taskid)['message'])
>           self.assertEquals('failed', inst.task_lookup(taskid)['status'])
>
> +        taskid = add_task('', continuous_ops, inst.objstore,
> +                          {'result': True})
> +        self.assertEquals('running', inst.task_lookup(taskid)['status'])
> +        self._wait_task(inst, taskid, timeout=10)
> +        self.assertEquals('finished', inst.task_lookup(taskid)['status'])
> +
>       # This wrapper function is needed due to the new backend messaging in
>       # vm model. vm_poweroff and vm_delete raise exception if vm is not found.
>       # These functions are called after vm has been deleted if test finishes
> @@ -1253,9 +1267,16 @@ class ModelTests(unittest.TestCase):
>                       raise e
>
>       def _wait_task(self, model, taskid, timeout=5):
> -            for i in range(0, timeout):
> -                if model.task_lookup(taskid)['status'] == 'running':
> -                    time.sleep(1)
> +        for i in range(0, timeout):
> +            task_info = model.task_lookup(taskid)
> +            if task_info['status'] == "running":
> +                kimchi_log.info("Waiting task %s, message: %s",
> +                                taskid, task_info['message'])
> +                time.sleep(1)
> +            else:
> +                return
> +        kimchi_log.error("Timeout while process long-run task, "
> +                         "try to increase timeout value.")
>
>       def test_get_distros(self):
>           inst = model.Model('test:///default',
> diff --git a/tests/test_rest.py b/tests/test_rest.py
> index 8de0a9c..649790b 100644
> --- a/tests/test_rest.py
> +++ b/tests/test_rest.py
> @@ -37,6 +37,7 @@ import kimchi.mockmodel
>   import kimchi.server
>   from kimchi.config import paths
>   from kimchi.rollbackcontext import RollbackContext
> +from kimchi.utils import kimchi_log
>   from utils import get_free_port, patch_auth, request
>   from utils import run_server
>
> @@ -1535,7 +1536,13 @@ class RestTests(unittest.TestCase):
>           for i in range(0, timeout):
>               task = json.loads(self.request('/tasks/%s' % taskid).read())
>               if task['status'] == 'running':
> +                kimchi_log.info("Waiting task %d, message: %s",
> +                                taskid, task['message'])
>                   time.sleep(1)
> +            else:
> +                return
> +        kimchi_log.error("Timeout while process long-run task, "
> +                         "try to increase timeout value.")
>
>       def test_tasks(self):
>           id1 = model.add_task('/tasks/1', self._async_op)