[1/2] iotests: add JobRunner class
diff mbox series

Message ID 20200226004425.1303-2-jsnow@redhat.com
State New
Headers show
Series
  • iotests: complicate run_job with this one weird trick?
Related show

Commit Message

John Snow Feb. 26, 2020, 12:44 a.m. UTC
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(-)

Comments

Max Reitz Feb. 26, 2020, 11:18 a.m. UTC | #1
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
John Snow Feb. 26, 2020, 5:58 p.m. UTC | #2
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
Max Reitz Feb. 27, 2020, 11:44 a.m. UTC | #3
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
John Snow March 3, 2020, 9:32 p.m. UTC | #4
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

Patch
diff mbox series

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):