* [PATCH 0/2] iotests: complicate run_job with this one weird trick? @ 2020-02-26 0:44 John Snow 2020-02-26 0:44 ` [PATCH 1/2] iotests: add JobRunner class John Snow 2020-02-26 0:44 ` [PATCH 2/2] iotests: modify test 040 to use JobRunner John Snow 0 siblings, 2 replies; 9+ messages in thread From: John Snow @ 2020-02-26 0:44 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] iotests: add JobRunner class 2020-02-26 0:44 [PATCH 0/2] iotests: complicate run_job with this one weird trick? John Snow @ 2020-02-26 0:44 ` John Snow 2020-02-26 11:18 ` Max Reitz 2020-02-26 0:44 ` [PATCH 2/2] iotests: modify test 040 to use JobRunner John Snow 1 sibling, 1 reply; 9+ messages in thread From: John Snow @ 2020-02-26 0:44 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: add JobRunner class 2020-02-26 0:44 ` [PATCH 1/2] iotests: add JobRunner class John Snow @ 2020-02-26 11:18 ` Max Reitz 2020-02-26 17:58 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Max Reitz @ 2020-02-26 11:18 UTC (permalink / raw) To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: add JobRunner class 2020-02-26 11:18 ` Max Reitz @ 2020-02-26 17:58 ` John Snow 2020-02-27 11:44 ` Max Reitz 0 siblings, 1 reply; 9+ messages in thread From: John Snow @ 2020-02-26 17:58 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: add JobRunner class 2020-02-26 17:58 ` John Snow @ 2020-02-27 11:44 ` Max Reitz 2020-03-03 21:32 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Max Reitz @ 2020-02-27 11:44 UTC (permalink / raw) To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: add JobRunner class 2020-02-27 11:44 ` Max Reitz @ 2020-03-03 21:32 ` John Snow 0 siblings, 0 replies; 9+ messages in thread From: John Snow @ 2020-03-03 21:32 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] iotests: modify test 040 to use JobRunner 2020-02-26 0:44 [PATCH 0/2] iotests: complicate run_job with this one weird trick? John Snow 2020-02-26 0:44 ` [PATCH 1/2] iotests: add JobRunner class John Snow @ 2020-02-26 0:44 ` John Snow 2020-02-26 11:31 ` Max Reitz 1 sibling, 1 reply; 9+ messages in thread From: John Snow @ 2020-02-26 0:44 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iotests: modify test 040 to use JobRunner 2020-02-26 0:44 ` [PATCH 2/2] iotests: modify test 040 to use JobRunner John Snow @ 2020-02-26 11:31 ` Max Reitz 2020-02-26 18:04 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Max Reitz @ 2020-02-26 11:31 UTC (permalink / raw) To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iotests: modify test 040 to use JobRunner 2020-02-26 11:31 ` Max Reitz @ 2020-02-26 18:04 ` John Snow 0 siblings, 0 replies; 9+ messages in thread From: John Snow @ 2020-02-26 18:04 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-03 21:33 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-26 0:44 [PATCH 0/2] iotests: complicate run_job with this one weird trick? John Snow 2020-02-26 0:44 ` [PATCH 1/2] iotests: add JobRunner class John Snow 2020-02-26 11:18 ` Max Reitz 2020-02-26 17:58 ` John Snow 2020-02-27 11:44 ` Max Reitz 2020-03-03 21:32 ` John Snow 2020-02-26 0:44 ` [PATCH 2/2] iotests: modify test 040 to use JobRunner John Snow 2020-02-26 11:31 ` Max Reitz 2020-02-26 18:04 ` John Snow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).