[PATCH 0/2] iotests: complicate run_job with this one weird trick?

[PATCH 0/2] iotests: complicate run_job with this one weird trick?

[John Snow]

Requires: 20200225005641.5478-1-jsnow@redhat.com

I'm kidding, but do treat this as an RFC. I doodled a little code
change and wasn't sure if it was appropriate because it's not really a
simplification.

The basic idea is to make a generic job runtime manager and allow
callers to subclass the manager. Then, instead of adding callback
arguments to the function all the time, we have à la carte customization
of the loop.

To showcase this a little bit, I removed the pre_finalization argument
and made existing callers use a custom JobRunner; and then converted
test 040 to use this style of job runner.

Letmeknowwhatchathink.

John Snow (2):
  iotests: add JobRunner class
  iotests: modify test 040 to use JobRunner

 tests/qemu-iotests/040        |  51 +++++-----
 tests/qemu-iotests/255        |   9 +-
 tests/qemu-iotests/257        |  12 ++-
 tests/qemu-iotests/287        |  19 +++-
 tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++----------
 5 files changed, 183 insertions(+), 84 deletions(-)

-- 
2.21.1

[PATCH 1/2] iotests: add JobRunner class

[John Snow]

The idea is that instead of increasing the arguments to job_run all the
time, create a more general-purpose job runner that can be subclassed to
do interesting things with.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/255        |   9 +-
 tests/qemu-iotests/257        |  12 ++-
 tests/qemu-iotests/287        |  19 +++-
 tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++----------
 4 files changed, 158 insertions(+), 58 deletions(-)

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 4a4818bafb..513e9ebb58 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -71,8 +71,13 @@ with iotests.FilePath('t.qcow2') as disk_path, \
     result = vm.qmp_log('block-commit', job_id='job0', auto_finalize=False,
                         device='overlay', top_node='mid')
 
-    vm.run_job('job0', auto_finalize=False, pre_finalize=start_requests,
-                auto_dismiss=True)
+    class TestJobRunner(iotests.JobRunner):
+        def on_pending(self, event):
+            start_requests()
+            super().on_pending(event)
+
+    runner = TestJobRunner(vm, 'job0', auto_finalize=False, auto_dismiss=True)
+    runner.run()
 
     vm.shutdown()
 
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index 2a81f9e30c..e73b0c20b3 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -265,9 +265,15 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
                 ebitmap.clear()
             ebitmap.dirty_group(2)
 
-        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
-                   pre_finalize=_callback,
-                   cancel=(failure == 'simulated'))
+        class TestJobRunner(iotests.JobRunner):
+            def on_pending(self, event):
+                _callback()
+                super().on_pending(event)
+
+        runner = TestJobRunner(vm, job, cancel=(failure == 'simulated'),
+                               auto_finalize=False, auto_dismiss=True)
+        runner.run()
+
         bitmaps = vm.query_bitmaps()
         log({'bitmaps': bitmaps}, indent=2)
         log('')
diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
index 0ab58dc011..f06e6ff084 100755
--- a/tests/qemu-iotests/287
+++ b/tests/qemu-iotests/287
@@ -165,13 +165,22 @@ def test_bitmap_populate(config):
                 if not config.disabled:
                     ebitmap.dirty_group(2)
 
+
+        class TestJobRunner(iotests.JobRunner):
+            def on_pending(self, event):
+                if config.mid_writes:
+                    perform_writes(drive0, 2)
+                    if not config.disabled:
+                        ebitmap.dirty_group(2)
+                super().on_pending(event)
+
         job = populate(drive0, 'target', 'bitpop0')
         assert job['return'] == {'return': {}}
-        vm.run_job(job['id'],
-                   auto_dismiss=job['auto-dismiss'],
-                   auto_finalize=job['auto-finalize'],
-                   pre_finalize=pre_finalize,
-                   cancel=config.cancel)
+        job_runner = TestJobRunner(vm, job['id'],
+                                   auto_dismiss=job['auto-dismiss'],
+                                   auto_finalize=job['auto-finalize'],
+                                   cancel=config.cancel)
+        job_runner.run()
         log('')
 
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3390fab021..37a8b4d649 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -460,6 +460,130 @@ def remote_filename(path):
     else:
         raise Exception("Protocol %s not supported" % (imgproto))
 
+
+class JobRunner:
+    def __init__(self, vm, job,
+                 use_log=True,
+                 cancel=False,
+                 auto_finalize=True,
+                 auto_dismiss=False):
+        self._vm = vm
+        self._id = job
+        self.logging = use_log
+        self.cancel = cancel
+
+        self._auto_finalize = auto_finalize
+        self._auto_dismiss = auto_dismiss
+        self._exited = False
+        self._error = None
+
+        match_device = {'data': {'device': job}}
+        match_id = {'data': {'id': job}}
+        self._events = {
+            'BLOCK_JOB_COMPLETED': match_device,
+            'BLOCK_JOB_CANCELLED': match_device,
+            'BLOCK_JOB_ERROR': match_device,
+            'BLOCK_JOB_READY': match_device,
+            'BLOCK_JOB_PENDING': match_id,
+            'JOB_STATUS_CHANGE': match_id
+        }
+
+        self._dispatch = {
+            'created': self.on_create,
+            'running': self.on_run,
+            'paused': self.on_pause,
+            'ready': self.on_ready,
+            'standby': self.on_standby,
+            'waiting': self.on_waiting,
+            'pending': self.on_pending,
+            'aborting': self.on_abort,
+            'concluded': self.on_conclude,
+            'null': self.on_null,
+        }
+
+    # Job events -- state changes.
+
+    def on_create(self, event):
+        pass
+
+    def on_run(self, event):
+        pass
+
+    def on_pause(self, event):
+        pass
+
+    def on_ready(self, event):
+        if self.logging:
+            self._vm.qmp_log('job-complete', id=self._id)
+        else:
+            self._vm.qmp('job-complete', id=self._id)
+
+    def on_standby(self, event):
+        pass
+
+    def on_waiting(self, event):
+        pass
+
+    def on_pending(self, event):
+        if self._auto_finalize:
+            return
+
+        if self.cancel:
+            if self.logging:
+                self._vm.qmp_log('job-cancel', id=self._id)
+            else:
+                self._vm.qmp('job-cancel', id=self._id)
+        else:
+            if self.logging:
+                self._vm.qmp_log('job-finalize', id=self._id)
+            else:
+                self._vm.qmp('job-finalize', id=self._id)
+
+    def on_abort(self, event):
+        result = self._vm.qmp('query-jobs')
+        for j in result['return']:
+            if j['id'] == self._id:
+                self.error = j['error']
+                if self.logging:
+                    log('Job failed: %s' % (j['error']))
+
+    def on_conclude(self, event):
+        if self._auto_dismiss:
+            return
+
+        if self.logging:
+            self._vm.qmp_log('job-dismiss', id=self._id)
+        else:
+            self._vm.qmp('job-dismiss', id=self._id)
+
+    def on_null(self, event):
+        self._exited = True
+
+    # Macro events -- QAPI events
+
+    def on_change(self, event):
+        status = event['data']['status']
+        assert status in self._dispatch
+        self._dispatch[status](event)
+
+    def on_block_job_event(self, event):
+        if self.logging:
+            log(event)
+
+    def _event(self, event):
+        assert event['event'] in self._events.keys()
+        if event['event'] == 'JOB_STATUS_CHANGE':
+            self.on_change(event)
+        else:
+            self.on_block_job_event(event)
+
+    def run(self, wait=60.0):
+        while not self._exited:
+            raw_event = self._vm.events_wait(self._events, timeout=wait)
+            self._event(filter_qmp_event(raw_event))
+        return self._error
+
+
 class VM(qtest.QEMUQtestMachine):
     '''A QEMU VM'''
 
@@ -585,7 +709,7 @@ def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
 
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-                pre_finalize=None, cancel=False, use_log=True, wait=60.0):
+                cancel=False, use_log=True, wait=60.0):
         """
         run_job moves a job from creation through to dismissal.
 
@@ -594,59 +718,15 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
                               auto_finalize. Defaults to True.
         :param auto_dismiss: Bool. True if the job was launched with
                              auto_dismiss=True. Defaults to False.
-        :param pre_finalize: Callback. A callable that takes no arguments to be
-                             invoked prior to issuing job-finalize, if any.
         :param cancel: Bool. When true, cancels the job after the pre_finalize
                        callback.
         :param use_log: Bool. When false, does not log QMP messages.
         :param wait: Float. Timeout value specifying how long to wait for any
                      event, in seconds. Defaults to 60.0.
         """
-        match_device = {'data': {'device': job}}
-        match_id = {'data': {'id': job}}
-        events = {
-            'BLOCK_JOB_COMPLETED': match_device,
-            'BLOCK_JOB_CANCELLED': match_device,
-            'BLOCK_JOB_ERROR': match_device,
-            'BLOCK_JOB_READY': match_device,
-            'BLOCK_JOB_PENDING': match_id,
-            'JOB_STATUS_CHANGE': match_id,
-        }
-        error = None
-        while True:
-            ev = filter_qmp_event(self.events_wait(events, timeout=wait))
-            if ev['event'] != 'JOB_STATUS_CHANGE':
-                if use_log:
-                    log(ev)
-                continue
-            status = ev['data']['status']
-            if status == 'aborting':
-                result = self.qmp('query-jobs')
-                for j in result['return']:
-                    if j['id'] == job:
-                        error = j['error']
-                        if use_log:
-                            log('Job failed: %s' % (j['error']))
-            elif status == 'ready':
-                self.qmp_log('job-complete', id=job)
-            elif status == 'pending' and not auto_finalize:
-                if pre_finalize:
-                    pre_finalize()
-                if cancel and use_log:
-                    self.qmp_log('job-cancel', id=job)
-                elif cancel:
-                    self.qmp('job-cancel', id=job)
-                elif use_log:
-                    self.qmp_log('job-finalize', id=job)
-                else:
-                    self.qmp('job-finalize', id=job)
-            elif status == 'concluded' and not auto_dismiss:
-                if use_log:
-                    self.qmp_log('job-dismiss', id=job)
-                else:
-                    self.qmp('job-dismiss', id=job)
-            elif status == 'null':
-                return error
+        job_runner = JobRunner(self, job, use_log, cancel,
+                               auto_finalize, auto_dismiss)
+        return job_runner.run(wait=wait)
 
     # Returns None on success, and an error string on failure
     def blockdev_create(self, options, job_id='job0', filters=None):
-- 
2.21.1

[PATCH 2/2] iotests: modify test 040 to use JobRunner

[John Snow]

Instead of having somewhat reproduced it for itself.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/040 | 51 +++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 90b59081ff..579dafc797 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase):
                           file=('top-dbg' if top_debug else 'top-file'),
                           backing='mid-fmt')
 
+
+    class TestJobRunner(iotests.JobRunner):
+        expected_events = ('BLOCK_JOB_COMPLETED',
+                           'BLOCK_JOB_ERROR',
+                           'BLOCK_JOB_READY')
+
+        def __init__(self, *args, test, **kwargs):
+            super().__init__(*args, **kwargs)
+            self.log = []
+            self.test = test
+
+        def on_pause(self, event):
+            result = self._vm.qmp('block-job-resume', device=self._id)
+            self.test.assert_qmp(result, 'return', {})
+            super().on_pause(event)
+
+        def on_block_job_event(self, event):
+            if event['event'] not in self.expected_events:
+                self.test.fail("Unexpected event: %s" % event)
+            super().on_block_job_event(event)
+            self.log.append(iotests.filter_qmp_event(event))
+
     def run_job(self, expected_events, error_pauses_job=False):
-        match_device = {'data': {'device': 'job0'}}
-        events = {
-            'BLOCK_JOB_COMPLETED': match_device,
-            'BLOCK_JOB_CANCELLED': match_device,
-            'BLOCK_JOB_ERROR': match_device,
-            'BLOCK_JOB_READY': match_device,
-        }
-
-        completed = False
-        log = []
-        while not completed:
-            ev = self.vm.events_wait(events, timeout=5.0)
-            if ev['event'] == 'BLOCK_JOB_COMPLETED':
-                completed = True
-            elif ev['event'] == 'BLOCK_JOB_ERROR':
-                if error_pauses_job:
-                    result = self.vm.qmp('block-job-resume', device='job0')
-                    self.assert_qmp(result, 'return', {})
-            elif ev['event'] == 'BLOCK_JOB_READY':
-                result = self.vm.qmp('block-job-complete', device='job0')
-                self.assert_qmp(result, 'return', {})
-            else:
-                self.fail("Unexpected event: %s" % ev)
-            log.append(iotests.filter_qmp_event(ev))
-
+        job = self.TestJobRunner(self.vm, 'job0', use_log=False, test=self)
+        job.run()
         self.maxDiff = None
-        self.assertEqual(expected_events, log)
+        self.assertEqual(expected_events, job.log)
 
     def event_error(self, op, action):
         return {
-- 
2.21.1

Re: [PATCH 1/2] iotests: add JobRunner class

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2900 bytes --]

On 26.02.20 01:44, John Snow wrote:
> The idea is that instead of increasing the arguments to job_run all the
> time, create a more general-purpose job runner that can be subclassed to
> do interesting things with.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/255        |   9 +-
>  tests/qemu-iotests/257        |  12 ++-
>  tests/qemu-iotests/287        |  19 +++-
>  tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++----------
>  4 files changed, 158 insertions(+), 58 deletions(-)

I like it!

[...]

> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
> index 0ab58dc011..f06e6ff084 100755
> --- a/tests/qemu-iotests/287
> +++ b/tests/qemu-iotests/287
> @@ -165,13 +165,22 @@ def test_bitmap_populate(config):
>                  if not config.disabled:
>                      ebitmap.dirty_group(2)
>  
> +
> +        class TestJobRunner(iotests.JobRunner):
> +            def on_pending(self, event):
> +                if config.mid_writes:
> +                    perform_writes(drive0, 2)
> +                    if not config.disabled:
> +                        ebitmap.dirty_group(2)

I actually prefer inlining the pre_finalize() functions (over calling
the existing one), but then we can also remove the original function. :)

> +                super().on_pending(event)
> +
>          job = populate(drive0, 'target', 'bitpop0')
>          assert job['return'] == {'return': {}}
> -        vm.run_job(job['id'],
> -                   auto_dismiss=job['auto-dismiss'],
> -                   auto_finalize=job['auto-finalize'],
> -                   pre_finalize=pre_finalize,
> -                   cancel=config.cancel)
> +        job_runner = TestJobRunner(vm, job['id'],
> +                                   auto_dismiss=job['auto-dismiss'],
> +                                   auto_finalize=job['auto-finalize'],
> +                                   cancel=config.cancel)
> +        job_runner.run()
>          log('')
>  
>  
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3390fab021..37a8b4d649 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -460,6 +460,130 @@ def remote_filename(path):
>      else:
>          raise Exception("Protocol %s not supported" % (imgproto))
>  
> +
> +class JobRunner:

[...]

> +    def on_ready(self, event):
> +        if self.logging:
> +            self._vm.qmp_log('job-complete', id=self._id)
> +        else:
> +            self._vm.qmp('job-complete', id=self._id)

I suppose this is a bug fix.  (The old version always called qmp_log.)

But what about adding a do_qmp method to JobRunner that does the
“if self.logging { self._vm.qmp_log() } else { self._vm.qmp }” part so
we don’t have to inline that everywhere?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] iotests: modify test 040 to use JobRunner

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3280 bytes --]

On 26.02.20 01:44, John Snow wrote:
> Instead of having somewhat reproduced it for itself.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/040 | 51 +++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index 90b59081ff..579dafc797 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase):
>                            file=('top-dbg' if top_debug else 'top-file'),
>                            backing='mid-fmt')
>  
> +
> +    class TestJobRunner(iotests.JobRunner):
> +        expected_events = ('BLOCK_JOB_COMPLETED',
> +                           'BLOCK_JOB_ERROR',
> +                           'BLOCK_JOB_READY')
> +
> +        def __init__(self, *args, test, **kwargs):
> +            super().__init__(*args, **kwargs)
> +            self.log = []
> +            self.test = test
> +
> +        def on_pause(self, event):
> +            result = self._vm.qmp('block-job-resume', device=self._id)
> +            self.test.assert_qmp(result, 'return', {})
> +            super().on_pause(event)

Not that it functionally matters, but I suppose I’d call
super().on_pause() before resuming (because the job isn’t exactly paused
afterwards).

> +
> +        def on_block_job_event(self, event):
> +            if event['event'] not in self.expected_events:
> +                self.test.fail("Unexpected event: %s" % event)
> +            super().on_block_job_event(event)
> +            self.log.append(iotests.filter_qmp_event(event))

Hasn’t the event been through filter_qmp_event() already?

Max

> +
>      def run_job(self, expected_events, error_pauses_job=False):
> -        match_device = {'data': {'device': 'job0'}}
> -        events = {
> -            'BLOCK_JOB_COMPLETED': match_device,
> -            'BLOCK_JOB_CANCELLED': match_device,
> -            'BLOCK_JOB_ERROR': match_device,
> -            'BLOCK_JOB_READY': match_device,
> -        }
> -
> -        completed = False
> -        log = []
> -        while not completed:
> -            ev = self.vm.events_wait(events, timeout=5.0)
> -            if ev['event'] == 'BLOCK_JOB_COMPLETED':
> -                completed = True
> -            elif ev['event'] == 'BLOCK_JOB_ERROR':
> -                if error_pauses_job:
> -                    result = self.vm.qmp('block-job-resume', device='job0')
> -                    self.assert_qmp(result, 'return', {})
> -            elif ev['event'] == 'BLOCK_JOB_READY':
> -                result = self.vm.qmp('block-job-complete', device='job0')
> -                self.assert_qmp(result, 'return', {})
> -            else:
> -                self.fail("Unexpected event: %s" % ev)
> -            log.append(iotests.filter_qmp_event(ev))
> -
> +        job = self.TestJobRunner(self.vm, 'job0', use_log=False, test=self)
> +        job.run()
>          self.maxDiff = None
> -        self.assertEqual(expected_events, log)
> +        self.assertEqual(expected_events, job.log)
>  
>      def event_error(self, op, action):
>          return {
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] iotests: add JobRunner class

[John Snow]

On 2/26/20 6:18 AM, Max Reitz wrote:
> On 26.02.20 01:44, John Snow wrote:
>> The idea is that instead of increasing the arguments to job_run all the
>> time, create a more general-purpose job runner that can be subclassed to
>> do interesting things with.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/255        |   9 +-
>>  tests/qemu-iotests/257        |  12 ++-
>>  tests/qemu-iotests/287        |  19 +++-
>>  tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++----------
>>  4 files changed, 158 insertions(+), 58 deletions(-)
> 
> I like it!
> 

High praise!

> [...]
> 
>> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
>> index 0ab58dc011..f06e6ff084 100755
>> --- a/tests/qemu-iotests/287
>> +++ b/tests/qemu-iotests/287
>> @@ -165,13 +165,22 @@ def test_bitmap_populate(config):
>>                  if not config.disabled:
>>                      ebitmap.dirty_group(2)
>>  
>> +
>> +        class TestJobRunner(iotests.JobRunner):
>> +            def on_pending(self, event):
>> +                if config.mid_writes:
>> +                    perform_writes(drive0, 2)
>> +                    if not config.disabled:
>> +                        ebitmap.dirty_group(2)
> 
> I actually prefer inlining the pre_finalize() functions (over calling
> the existing one), but then we can also remove the original function. :)
> 

Not sure I understand you correctly. You're saying you prefer this
strategy where I inline the logic vs others where I call out to a function?

If so, I agree if only for purity -- the function looks and acts like a
callback instead of a callback-that-calls-another-callback.

>> +                super().on_pending(event)
>> +
>>          job = populate(drive0, 'target', 'bitpop0')
>>          assert job['return'] == {'return': {}}
>> -        vm.run_job(job['id'],
>> -                   auto_dismiss=job['auto-dismiss'],
>> -                   auto_finalize=job['auto-finalize'],
>> -                   pre_finalize=pre_finalize,
>> -                   cancel=config.cancel)
>> +        job_runner = TestJobRunner(vm, job['id'],
>> +                                   auto_dismiss=job['auto-dismiss'],
>> +                                   auto_finalize=job['auto-finalize'],
>> +                                   cancel=config.cancel)
>> +        job_runner.run()
>>          log('')
>>  
>>  
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 3390fab021..37a8b4d649 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -460,6 +460,130 @@ def remote_filename(path):
>>      else:
>>          raise Exception("Protocol %s not supported" % (imgproto))
>>  
>> +
>> +class JobRunner:
> 
> [...]
> 
>> +    def on_ready(self, event):
>> +        if self.logging:
>> +            self._vm.qmp_log('job-complete', id=self._id)
>> +        else:
>> +            self._vm.qmp('job-complete', id=self._id)
> 
> I suppose this is a bug fix.  (The old version always called qmp_log.)
> 

Technically yes. It was needed for 040.

> But what about adding a do_qmp method to JobRunner that does the
> “if self.logging { self._vm.qmp_log() } else { self._vm.qmp }” part so
> we don’t have to inline that everywhere?
> 
> Max
> 

I'll just clean up the logging series I had to do it at a more
fundamental level.

Just testing the temperature of the water.

--js

Re: [PATCH 2/2] iotests: modify test 040 to use JobRunner

[John Snow]

On 2/26/20 6:31 AM, Max Reitz wrote:
> On 26.02.20 01:44, John Snow wrote:
>> Instead of having somewhat reproduced it for itself.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/040 | 51 +++++++++++++++++++++---------------------
>>  1 file changed, 25 insertions(+), 26 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 90b59081ff..579dafc797 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040
>> @@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase):
>>                            file=('top-dbg' if top_debug else 'top-file'),
>>                            backing='mid-fmt')
>>  
>> +
>> +    class TestJobRunner(iotests.JobRunner):
>> +        expected_events = ('BLOCK_JOB_COMPLETED',
>> +                           'BLOCK_JOB_ERROR',
>> +                           'BLOCK_JOB_READY')
>> +
>> +        def __init__(self, *args, test, **kwargs):
>> +            super().__init__(*args, **kwargs)
>> +            self.log = []
>> +            self.test = test
>> +
>> +        def on_pause(self, event):
>> +            result = self._vm.qmp('block-job-resume', device=self._id)
>> +            self.test.assert_qmp(result, 'return', {})
>> +            super().on_pause(event)
> 
> Not that it functionally matters, but I suppose I’d call
> super().on_pause() before resuming (because the job isn’t exactly paused
> afterwards).
> 

Reasonable detail to consider.

It's also likely valid to just *omit* calling the base class pause event
when overriding behavior -- If we decide to send resume commands in the
future, we'll want to avoid sending conflicting/duplicate events.

In this case, the base event is just a NOP so it doesn't matter, but it
probably is good hygiene to avoid changing the state *and then* calling
the base class.

So I think this is a valid observation that should be worked into the
docstring for the JobRunner class on how best to make use of it.

>> +
>> +        def on_block_job_event(self, event):
>> +            if event['event'] not in self.expected_events:
>> +                self.test.fail("Unexpected event: %s" % event)
>> +            super().on_block_job_event(event)
>> +            self.log.append(iotests.filter_qmp_event(event))
> 
> Hasn’t the event been through filter_qmp_event() already?
> 

Oh, yeah. When I converted 040 here I just kind of shoehorned it onto
the new API in a somewhat mechanical fashion, but you're right.

Re: [PATCH 1/2] iotests: add JobRunner class

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3799 bytes --]

On 26.02.20 18:58, John Snow wrote:
> 
> 
> On 2/26/20 6:18 AM, Max Reitz wrote:
>> On 26.02.20 01:44, John Snow wrote:
>>> The idea is that instead of increasing the arguments to job_run all the
>>> time, create a more general-purpose job runner that can be subclassed to
>>> do interesting things with.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  tests/qemu-iotests/255        |   9 +-
>>>  tests/qemu-iotests/257        |  12 ++-
>>>  tests/qemu-iotests/287        |  19 +++-
>>>  tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++----------
>>>  4 files changed, 158 insertions(+), 58 deletions(-)
>>
>> I like it!
>>
> 
> High praise!
> 
>> [...]
>>
>>> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
>>> index 0ab58dc011..f06e6ff084 100755
>>> --- a/tests/qemu-iotests/287
>>> +++ b/tests/qemu-iotests/287
>>> @@ -165,13 +165,22 @@ def test_bitmap_populate(config):
>>>                  if not config.disabled:
>>>                      ebitmap.dirty_group(2)
>>>  
>>> +
>>> +        class TestJobRunner(iotests.JobRunner):
>>> +            def on_pending(self, event):
>>> +                if config.mid_writes:
>>> +                    perform_writes(drive0, 2)
>>> +                    if not config.disabled:
>>> +                        ebitmap.dirty_group(2)
>>
>> I actually prefer inlining the pre_finalize() functions (over calling
>> the existing one), but then we can also remove the original function. :)
>>
> 
> Not sure I understand you correctly. You're saying you prefer this
> strategy where I inline the logic vs others where I call out to a function?

Yes.

> If so, I agree if only for purity -- the function looks and acts like a
> callback instead of a callback-that-calls-another-callback.

That was my thinking.

>>> +                super().on_pending(event)
>>> +
>>>          job = populate(drive0, 'target', 'bitpop0')
>>>          assert job['return'] == {'return': {}}
>>> -        vm.run_job(job['id'],
>>> -                   auto_dismiss=job['auto-dismiss'],
>>> -                   auto_finalize=job['auto-finalize'],
>>> -                   pre_finalize=pre_finalize,
>>> -                   cancel=config.cancel)
>>> +        job_runner = TestJobRunner(vm, job['id'],
>>> +                                   auto_dismiss=job['auto-dismiss'],
>>> +                                   auto_finalize=job['auto-finalize'],
>>> +                                   cancel=config.cancel)
>>> +        job_runner.run()
>>>          log('')
>>>  
>>>  
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3390fab021..37a8b4d649 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -460,6 +460,130 @@ def remote_filename(path):
>>>      else:
>>>          raise Exception("Protocol %s not supported" % (imgproto))
>>>  
>>> +
>>> +class JobRunner:
>>
>> [...]
>>
>>> +    def on_ready(self, event):
>>> +        if self.logging:
>>> +            self._vm.qmp_log('job-complete', id=self._id)
>>> +        else:
>>> +            self._vm.qmp('job-complete', id=self._id)
>>
>> I suppose this is a bug fix.  (The old version always called qmp_log.)
>>
> 
> Technically yes. It was needed for 040.
> 
>> But what about adding a do_qmp method to JobRunner that does the
>> “if self.logging { self._vm.qmp_log() } else { self._vm.qmp }” part so
>> we don’t have to inline that everywhere?
>>
>> Max
>>
> 
> I'll just clean up the logging series I had to do it at a more
> fundamental level.

OK.  So you’re looking to basically get VM.qmp() to log automatically if
necessary?  (or maybe qmp_log() to not log unless necessary)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] iotests: add JobRunner class

[John Snow]

On 2/27/20 6:44 AM, Max Reitz wrote:
>> I'll just clean up the logging series I had to do it at a more
>> fundamental level.
> OK.  So you’re looking to basically get VM.qmp() to log automatically if
> necessary?  (or maybe qmp_log() to not log unless necessary)
> 
> Max
> 

Yes. If this series looks good I will just rebase it on top of the
logging series. Then self.logging goes away and the calls to qmp_log et
al just become unconditional.

Then we can enable/disable "all QMP logging" globally instead of
individually throughout the iotests shared codebase.

--js