* [PATCH v4 0/3] iotests: add JobRunner framework @ 2020-05-14 2:25 John Snow 2020-05-14 2:25 ` [PATCH v4 1/3] qmp.py: change event_wait to use a dict John Snow ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: John Snow @ 2020-05-14 2:25 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa, John Snow 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. Is it a simplification? No. Is it cool? Maybe. Did it remove the duplicated job-running code in 040? yes. As of V4, I'm not sure I really want to pursue this at the moment; it does feel a bit harder to use than the old interface; though it's more "complete". I might look into refining the idea and trying to include it directly in python/qemu; and providing some more 'bespoke' wrappers in iotests that make using it in the test framework a little less cumbersome. Regardless, I am posting it to the list for archiving for now. v4: - Rebased on current master. - Converted new usages is test 055. V3: - Rebased on logging series v8 - Converted 155's new usage of job_run V2: - Rebased on logging series; logging conditionals are pretty now. - Inlined callback login in 257 - No longer based on bitmap-populate job (no test 287) - Moved super() call to the beginning of test 040's callback - Added docstrings and type annotations John Snow (3): qmp.py: change event_wait to use a dict iotests: add JobRunner class iotests: modify test 040 to use JobRunner python/qemu/machine.py | 10 +- tests/qemu-iotests/040 | 51 +++++---- tests/qemu-iotests/055 | 17 +-- tests/qemu-iotests/155 | 11 +- tests/qemu-iotests/255 | 9 +- tests/qemu-iotests/257 | 54 +++++---- tests/qemu-iotests/260 | 5 +- tests/qemu-iotests/iotests.py | 206 +++++++++++++++++++++++++--------- tests/qemu-iotests/pylintrc | 11 ++ 9 files changed, 258 insertions(+), 116 deletions(-) -- 2.21.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-05-14 2:25 [PATCH v4 0/3] iotests: add JobRunner framework John Snow @ 2020-05-14 2:25 ` John Snow 2020-05-14 14:47 ` Kevin Wolf 2020-05-14 2:25 ` [PATCH v4 2/3] iotests: add JobRunner class John Snow 2020-05-14 2:25 ` [PATCH v4 3/3] iotests: modify test 040 to use JobRunner John Snow 2 siblings, 1 reply; 15+ messages in thread From: John Snow @ 2020-05-14 2:25 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa, John Snow It's easier to work with than a list of tuples, because we can check the keys for membership. Signed-off-by: John Snow <jsnow@redhat.com> --- python/qemu/machine.py | 10 +++++----- tests/qemu-iotests/040 | 12 ++++++------ tests/qemu-iotests/260 | 5 +++-- tests/qemu-iotests/iotests.py | 16 ++++++++-------- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/python/qemu/machine.py b/python/qemu/machine.py index b9a98e2c86..eaedc25172 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -478,21 +478,21 @@ def event_wait(self, name, timeout=60.0, match=None): timeout: QEMUMonitorProtocol.pull_event timeout parameter. match: Optional match criteria. See event_match for details. """ - return self.events_wait([(name, match)], timeout) + return self.events_wait({name: match}, timeout) def events_wait(self, events, timeout=60.0): """ events_wait waits for and returns a named event from QMP with a timeout. - events: a sequence of (name, match_criteria) tuples. + events: a mapping containing {name: match_criteria}. The match criteria are optional and may be None. See event_match for details. timeout: QEMUMonitorProtocol.pull_event timeout parameter. """ def _match(event): - for name, match in events: - if event['event'] == name and self.event_match(event, match): - return True + name = event['event'] + if name in events: + return self.event_match(event, events[name]) return False # Search cached events diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index 32c82b4ec6..90b59081ff 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase): 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), - ] + events = { + 'BLOCK_JOB_COMPLETED': match_device, + 'BLOCK_JOB_CANCELLED': match_device, + 'BLOCK_JOB_ERROR': match_device, + 'BLOCK_JOB_READY': match_device, + } completed = False log = [] diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260 index 804a7addb9..729f031122 100755 --- a/tests/qemu-iotests/260 +++ b/tests/qemu-iotests/260 @@ -67,8 +67,9 @@ def test(persistent, restart): vm.qmp_log('block-commit', device='drive0', top=top, filters=[iotests.filter_qmp_testfiles]) - ev = vm.events_wait((('BLOCK_JOB_READY', None), - ('BLOCK_JOB_COMPLETED', None))) + ev = vm.events_wait({ + 'BLOCK_JOB_READY': None, + 'BLOCK_JOB_COMPLETED': None }) log(filter_qmp_event(ev)) if (ev['event'] == 'BLOCK_JOB_COMPLETED'): vm.shutdown() diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 6c0e781af7..aada94f4f9 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -635,14 +635,14 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False, """ 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) - ] + 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)) -- 2.21.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-05-14 2:25 ` [PATCH v4 1/3] qmp.py: change event_wait to use a dict John Snow @ 2020-05-14 14:47 ` Kevin Wolf 2020-05-14 15:07 ` John Snow 0 siblings, 1 reply; 15+ messages in thread From: Kevin Wolf @ 2020-05-14 14:47 UTC (permalink / raw) To: John Snow; +Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa Am 14.05.2020 um 04:25 hat John Snow geschrieben: > It's easier to work with than a list of tuples, because we can check the > keys for membership. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > python/qemu/machine.py | 10 +++++----- > tests/qemu-iotests/040 | 12 ++++++------ > tests/qemu-iotests/260 | 5 +++-- > tests/qemu-iotests/iotests.py | 16 ++++++++-------- > 4 files changed, 22 insertions(+), 21 deletions(-) I think you need to convert scripts/simplebench/bench_block_job.py, too. > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index b9a98e2c86..eaedc25172 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -478,21 +478,21 @@ def event_wait(self, name, timeout=60.0, match=None): > timeout: QEMUMonitorProtocol.pull_event timeout parameter. > match: Optional match criteria. See event_match for details. > """ > - return self.events_wait([(name, match)], timeout) > + return self.events_wait({name: match}, timeout) > > def events_wait(self, events, timeout=60.0): > """ > events_wait waits for and returns a named event from QMP with a timeout. > > - events: a sequence of (name, match_criteria) tuples. > + events: a mapping containing {name: match_criteria}. > The match criteria are optional and may be None. > See event_match for details. timeout: > QEMUMonitorProtocol.pull_event timeout parameter. > """ > def _match(event): > - for name, match in events: > - if event['event'] == name and self.event_match(event, match): > - return True > + name = event['event'] > + if name in events: > + return self.event_match(event, events[name]) This part confused me a bit for a second. Of course, that's probably mostly just me, but I feel 'events' isn't a good name any more when the values of the dict are match strings rather than events. > return False > > # Search cached events > diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 > index 32c82b4ec6..90b59081ff 100755 > --- a/tests/qemu-iotests/040 > +++ b/tests/qemu-iotests/040 > @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase): > > 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), > - ] > + events = { > + 'BLOCK_JOB_COMPLETED': match_device, > + 'BLOCK_JOB_CANCELLED': match_device, > + 'BLOCK_JOB_ERROR': match_device, > + 'BLOCK_JOB_READY': match_device, > + } > > completed = False > log = [] > diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260 > index 804a7addb9..729f031122 100755 > --- a/tests/qemu-iotests/260 > +++ b/tests/qemu-iotests/260 > @@ -67,8 +67,9 @@ def test(persistent, restart): > > vm.qmp_log('block-commit', device='drive0', top=top, > filters=[iotests.filter_qmp_testfiles]) > - ev = vm.events_wait((('BLOCK_JOB_READY', None), > - ('BLOCK_JOB_COMPLETED', None))) > + ev = vm.events_wait({ > + 'BLOCK_JOB_READY': None, > + 'BLOCK_JOB_COMPLETED': None }) So, I'm not sure if this is nitpicking or rather bikeshedding, but having the closing brackets on the next line would be more consistent with the other instances in this patch. > log(filter_qmp_event(ev)) > if (ev['event'] == 'BLOCK_JOB_COMPLETED'): > vm.shutdown() Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-05-14 14:47 ` Kevin Wolf @ 2020-05-14 15:07 ` John Snow 2020-05-14 15:59 ` Kevin Wolf 0 siblings, 1 reply; 15+ messages in thread From: John Snow @ 2020-05-14 15:07 UTC (permalink / raw) To: Kevin Wolf Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa On 5/14/20 10:47 AM, Kevin Wolf wrote: > Am 14.05.2020 um 04:25 hat John Snow geschrieben: >> It's easier to work with than a list of tuples, because we can check the >> keys for membership. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> python/qemu/machine.py | 10 +++++----- >> tests/qemu-iotests/040 | 12 ++++++------ >> tests/qemu-iotests/260 | 5 +++-- >> tests/qemu-iotests/iotests.py | 16 ++++++++-------- >> 4 files changed, 22 insertions(+), 21 deletions(-) > > I think you need to convert scripts/simplebench/bench_block_job.py, too. Oh, right -- that one is new since I did this. A lot of these scripts need to be moved over into the python/ directory and managed under the same pylint/mypy regime as everything else. A *ton* of our scripts are in various states of disrepair. > >> diff --git a/python/qemu/machine.py b/python/qemu/machine.py >> index b9a98e2c86..eaedc25172 100644 >> --- a/python/qemu/machine.py >> +++ b/python/qemu/machine.py >> @@ -478,21 +478,21 @@ def event_wait(self, name, timeout=60.0, match=None): >> timeout: QEMUMonitorProtocol.pull_event timeout parameter. >> match: Optional match criteria. See event_match for details. >> """ >> - return self.events_wait([(name, match)], timeout) >> + return self.events_wait({name: match}, timeout) >> >> def events_wait(self, events, timeout=60.0): >> """ >> events_wait waits for and returns a named event from QMP with a timeout. >> >> - events: a sequence of (name, match_criteria) tuples. >> + events: a mapping containing {name: match_criteria}. >> The match criteria are optional and may be None. >> See event_match for details. timeout: >> QEMUMonitorProtocol.pull_event timeout parameter. >> """ >> def _match(event): >> - for name, match in events: >> - if event['event'] == name and self.event_match(event, match): >> - return True >> + name = event['event'] >> + if name in events: >> + return self.event_match(event, events[name]) > > This part confused me a bit for a second. Of course, that's probably > mostly just me, but I feel 'events' isn't a good name any more when the > values of the dict are match strings rather than events. > This is honestly a really bad function. When I was trying to type everything, this one was at the bottom of the pile and it was the worst. It needs an overhaul. In my 32 patch series, I left the "match" types as "Any" pretty much everywhere, because it's such a laissez-faire series of functions. I'll keep the feedback in mind. >> return False >> >> # Search cached events >> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 >> index 32c82b4ec6..90b59081ff 100755 >> --- a/tests/qemu-iotests/040 >> +++ b/tests/qemu-iotests/040 >> @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase): >> >> 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), >> - ] >> + events = { >> + 'BLOCK_JOB_COMPLETED': match_device, >> + 'BLOCK_JOB_CANCELLED': match_device, >> + 'BLOCK_JOB_ERROR': match_device, >> + 'BLOCK_JOB_READY': match_device, >> + } >> >> completed = False >> log = [] >> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260 >> index 804a7addb9..729f031122 100755 >> --- a/tests/qemu-iotests/260 >> +++ b/tests/qemu-iotests/260 >> @@ -67,8 +67,9 @@ def test(persistent, restart): >> >> vm.qmp_log('block-commit', device='drive0', top=top, >> filters=[iotests.filter_qmp_testfiles]) >> - ev = vm.events_wait((('BLOCK_JOB_READY', None), >> - ('BLOCK_JOB_COMPLETED', None))) >> + ev = vm.events_wait({ >> + 'BLOCK_JOB_READY': None, >> + 'BLOCK_JOB_COMPLETED': None }) > > So, I'm not sure if this is nitpicking or rather bikeshedding, but > having the closing brackets on the next line would be more consistent > with the other instances in this patch. > Nah, it's fine. I'll clean it up. This is pretty close to an RFC series anyway, so I didn't really polish it. (Or, I will try to clean it up. I probably won't work on it again in the near term. I think I just wanted to see if this seemed useful in general to people. As part of maybe moving the python library onto a package, I thought that maybe developing a JobRunner tool would be useful to have in that library. As you can see, I nestled it into iotests.py, though.) >> log(filter_qmp_event(ev)) >> if (ev['event'] == 'BLOCK_JOB_COMPLETED'): >> vm.shutdown() > > Kevin > -- —js ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-05-14 15:07 ` John Snow @ 2020-05-14 15:59 ` Kevin Wolf 2020-05-14 19:31 ` John Snow 0 siblings, 1 reply; 15+ messages in thread From: Kevin Wolf @ 2020-05-14 15:59 UTC (permalink / raw) To: John Snow; +Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa Am 14.05.2020 um 17:07 hat John Snow geschrieben: > > > On 5/14/20 10:47 AM, Kevin Wolf wrote: > > Am 14.05.2020 um 04:25 hat John Snow geschrieben: > >> It's easier to work with than a list of tuples, because we can check the > >> keys for membership. > >> > >> Signed-off-by: John Snow <jsnow@redhat.com> > >> --- > >> python/qemu/machine.py | 10 +++++----- > >> tests/qemu-iotests/040 | 12 ++++++------ > >> tests/qemu-iotests/260 | 5 +++-- > >> tests/qemu-iotests/iotests.py | 16 ++++++++-------- > >> 4 files changed, 22 insertions(+), 21 deletions(-) > > > > I think you need to convert scripts/simplebench/bench_block_job.py, too. > > Oh, right -- that one is new since I did this. A lot of these scripts > need to be moved over into the python/ directory and managed under the > same pylint/mypy regime as everything else. > > A *ton* of our scripts are in various states of disrepair. Is python/ actually supposed to have executable files in it? I thought it was more for libraries. > > > >> diff --git a/python/qemu/machine.py b/python/qemu/machine.py > >> index b9a98e2c86..eaedc25172 100644 > >> --- a/python/qemu/machine.py > >> +++ b/python/qemu/machine.py > >> @@ -478,21 +478,21 @@ def event_wait(self, name, timeout=60.0, match=None): > >> timeout: QEMUMonitorProtocol.pull_event timeout parameter. > >> match: Optional match criteria. See event_match for details. > >> """ > >> - return self.events_wait([(name, match)], timeout) > >> + return self.events_wait({name: match}, timeout) > >> > >> def events_wait(self, events, timeout=60.0): > >> """ > >> events_wait waits for and returns a named event from QMP with a timeout. > >> > >> - events: a sequence of (name, match_criteria) tuples. > >> + events: a mapping containing {name: match_criteria}. > >> The match criteria are optional and may be None. > >> See event_match for details. timeout: > >> QEMUMonitorProtocol.pull_event timeout parameter. > >> """ > >> def _match(event): > >> - for name, match in events: > >> - if event['event'] == name and self.event_match(event, match): > >> - return True > >> + name = event['event'] > >> + if name in events: > >> + return self.event_match(event, events[name]) > > > > This part confused me a bit for a second. Of course, that's probably > > mostly just me, but I feel 'events' isn't a good name any more when the > > values of the dict are match strings rather than events. > > > > This is honestly a really bad function. When I was trying to type > everything, this one was at the bottom of the pile and it was the worst. > > It needs an overhaul. > > In my 32 patch series, I left the "match" types as "Any" pretty much > everywhere, because it's such a laissez-faire series of functions. It would require recursive types, which aren't supported yet. So I guess Any is the best we can do at the moment. > I'll keep the feedback in mind. > > >> return False > >> > >> # Search cached events > >> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 > >> index 32c82b4ec6..90b59081ff 100755 > >> --- a/tests/qemu-iotests/040 > >> +++ b/tests/qemu-iotests/040 > >> @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase): > >> > >> 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), > >> - ] > >> + events = { > >> + 'BLOCK_JOB_COMPLETED': match_device, > >> + 'BLOCK_JOB_CANCELLED': match_device, > >> + 'BLOCK_JOB_ERROR': match_device, > >> + 'BLOCK_JOB_READY': match_device, > >> + } > >> > >> completed = False > >> log = [] > >> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260 > >> index 804a7addb9..729f031122 100755 > >> --- a/tests/qemu-iotests/260 > >> +++ b/tests/qemu-iotests/260 > >> @@ -67,8 +67,9 @@ def test(persistent, restart): > >> > >> vm.qmp_log('block-commit', device='drive0', top=top, > >> filters=[iotests.filter_qmp_testfiles]) > >> - ev = vm.events_wait((('BLOCK_JOB_READY', None), > >> - ('BLOCK_JOB_COMPLETED', None))) > >> + ev = vm.events_wait({ > >> + 'BLOCK_JOB_READY': None, > >> + 'BLOCK_JOB_COMPLETED': None }) > > > > So, I'm not sure if this is nitpicking or rather bikeshedding, but > > having the closing brackets on the next line would be more consistent > > with the other instances in this patch. > > > > Nah, it's fine. I'll clean it up. This is pretty close to an RFC series > anyway, so I didn't really polish it. > > (Or, I will try to clean it up. I probably won't work on it again in the > near term. I think I just wanted to see if this seemed useful in general > to people. Ah, there isn't much missing for this series, though. We don't have to wait for a fix-the-world series when we can incrementally improve things. > As part of maybe moving the python library onto a package, I thought > that maybe developing a JobRunner tool would be useful to have in that > library. As you can see, I nestled it into iotests.py, though.) Let's just do that now, we can always move it somewhere else later. Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-05-14 15:59 ` Kevin Wolf @ 2020-05-14 19:31 ` John Snow 2020-06-16 21:41 ` Eric Blake 0 siblings, 1 reply; 15+ messages in thread From: John Snow @ 2020-05-14 19:31 UTC (permalink / raw) To: Kevin Wolf Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa On 5/14/20 11:59 AM, Kevin Wolf wrote: > Am 14.05.2020 um 17:07 hat John Snow geschrieben: >> >> >> On 5/14/20 10:47 AM, Kevin Wolf wrote: >>> Am 14.05.2020 um 04:25 hat John Snow geschrieben: >>>> It's easier to work with than a list of tuples, because we can check the >>>> keys for membership. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> python/qemu/machine.py | 10 +++++----- >>>> tests/qemu-iotests/040 | 12 ++++++------ >>>> tests/qemu-iotests/260 | 5 +++-- >>>> tests/qemu-iotests/iotests.py | 16 ++++++++-------- >>>> 4 files changed, 22 insertions(+), 21 deletions(-) >>> >>> I think you need to convert scripts/simplebench/bench_block_job.py, too. >> >> Oh, right -- that one is new since I did this. A lot of these scripts >> need to be moved over into the python/ directory and managed under the >> same pylint/mypy regime as everything else. >> >> A *ton* of our scripts are in various states of disrepair. > > Is python/ actually supposed to have executable files in it? I thought > it was more for libraries. > Welllllllllllllll. At the moment it's library only. but one of the things you can do with a library is define executable entry-points into that library. If you haven't cast an eye at that 32 patch series yet, it basically creates a structure like this: > ./python/qemu/lib/[qmp|machine|qtest|accel].py qemu/ forms a PEP420 namespace; the idea is to be able to modularly create and independently package subpackages. qemu/lib forms a proper python package in which there are no executables, just a library, as you say. My idea is that anything under python/*/ ought to form a properly formatted package. So we could, for instance, have a python/qemu/devtools namespace which packages and collects a bunch of our little scripts. Then we could make sure we hit them with the same mypy/pylint/flake8/whatever as the core libraries those scripts are using to keep them in sync better. And, ideally, if they are all using the same kind of paradigms for import and dependency management it will be easier to use them and keep them up to date, etc. For using them as a developer, you could, say, cd ~/src/qemu/python pip3 install --user -e . and install the source packages to your local environment and then have access to e.g. > qmp-shell right on your CLI, without having to fuss with PYTHONPATH or anything else. As you update the source repo, you'll get the new versions of the package living in your python environment automatically. Of course, this maybe has downsides too; so you can always use a virtual environment to adopt a context in which you have these tools. For that, > pip3 install --user pipenv # or use dnf, or apt, w/e. > cd ~/src/qemu/python > pipenv shell > pip install -e . And from here you'll have the dev package installed to a development venv that you can use. *cough* anyway, that's wildly off-topic. Generally, you want to format a library such that you have a callable entry point, maybe named main(). So you'd have some qmp-shell module and it has a main() function. Then, in the setup.py script, you'd define qemu.lib.qmp_shell:main() as an entry point and give it a name like 'qmp-shell'. When pip/setuptools processes your package installation, it'll create a shim for you in e.g. ~/.local/bin/qmp-shell that will just load the library and execute that entrypoint for you. I was thinking I'd do this for all of our python scripts so I could spend my energy on a pylint/mypy test infrastructure *once* and *in one place* and then it would be easier to detect regressions for scripts that don't actually run as part of the test suite. >>> >>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py >>>> index b9a98e2c86..eaedc25172 100644 >>>> --- a/python/qemu/machine.py >>>> +++ b/python/qemu/machine.py >>>> @@ -478,21 +478,21 @@ def event_wait(self, name, timeout=60.0, match=None): >>>> timeout: QEMUMonitorProtocol.pull_event timeout parameter. >>>> match: Optional match criteria. See event_match for details. >>>> """ >>>> - return self.events_wait([(name, match)], timeout) >>>> + return self.events_wait({name: match}, timeout) >>>> >>>> def events_wait(self, events, timeout=60.0): >>>> """ >>>> events_wait waits for and returns a named event from QMP with a timeout. >>>> >>>> - events: a sequence of (name, match_criteria) tuples. >>>> + events: a mapping containing {name: match_criteria}. >>>> The match criteria are optional and may be None. >>>> See event_match for details. timeout: >>>> QEMUMonitorProtocol.pull_event timeout parameter. >>>> """ >>>> def _match(event): >>>> - for name, match in events: >>>> - if event['event'] == name and self.event_match(event, match): >>>> - return True >>>> + name = event['event'] >>>> + if name in events: >>>> + return self.event_match(event, events[name]) >>> >>> This part confused me a bit for a second. Of course, that's probably >>> mostly just me, but I feel 'events' isn't a good name any more when the >>> values of the dict are match strings rather than events. >>> >> >> This is honestly a really bad function. When I was trying to type >> everything, this one was at the bottom of the pile and it was the worst. >> >> It needs an overhaul. >> >> In my 32 patch series, I left the "match" types as "Any" pretty much >> everywhere, because it's such a laissez-faire series of functions. > > It would require recursive types, which aren't supported yet. So I guess > Any is the best we can do at the moment. > It would also do well with a fully schema-validated API if we actually knew exactly what a "QMP Message" was and had some guarantee about exactly what fields and types it had. This is where a generated validator would really help for type-safe SDK tooling. ... We of course do not have that right now, and might not ever, so yeah, 'Any' type is fine -- but I really want to rewrite this function because it does stick out as ugly and hard to read, I admit :( Anything that's hard to type -- by the person who wrote most of it -- is going to be hard to use or understand. Time for a rework. >> I'll keep the feedback in mind. >> >>>> return False >>>> >>>> # Search cached events >>>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 >>>> index 32c82b4ec6..90b59081ff 100755 >>>> --- a/tests/qemu-iotests/040 >>>> +++ b/tests/qemu-iotests/040 >>>> @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase): >>>> >>>> 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), >>>> - ] >>>> + events = { >>>> + 'BLOCK_JOB_COMPLETED': match_device, >>>> + 'BLOCK_JOB_CANCELLED': match_device, >>>> + 'BLOCK_JOB_ERROR': match_device, >>>> + 'BLOCK_JOB_READY': match_device, >>>> + } >>>> >>>> completed = False >>>> log = [] >>>> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260 >>>> index 804a7addb9..729f031122 100755 >>>> --- a/tests/qemu-iotests/260 >>>> +++ b/tests/qemu-iotests/260 >>>> @@ -67,8 +67,9 @@ def test(persistent, restart): >>>> >>>> vm.qmp_log('block-commit', device='drive0', top=top, >>>> filters=[iotests.filter_qmp_testfiles]) >>>> - ev = vm.events_wait((('BLOCK_JOB_READY', None), >>>> - ('BLOCK_JOB_COMPLETED', None))) >>>> + ev = vm.events_wait({ >>>> + 'BLOCK_JOB_READY': None, >>>> + 'BLOCK_JOB_COMPLETED': None }) >>> >>> So, I'm not sure if this is nitpicking or rather bikeshedding, but >>> having the closing brackets on the next line would be more consistent >>> with the other instances in this patch. >>> >> >> Nah, it's fine. I'll clean it up. This is pretty close to an RFC series >> anyway, so I didn't really polish it. >> >> (Or, I will try to clean it up. I probably won't work on it again in the >> near term. I think I just wanted to see if this seemed useful in general >> to people. > > Ah, there isn't much missing for this series, though. We don't have to > wait for a fix-the-world series when we can incrementally improve > things. > Alright, I'll try to hit it halfway -- I spent some time thinking about a "full" job running framework but ran into some dead-ends I wasn't too happy with, and wasn't convinced this was a simplification of any kind. Still, seeing part of the job running code get duplicated in 040 was a motivation to try and provide some universal job-running monster that would be extensible for nearly any task. Unfortunately that complexity does generally make the calling sites look worse, so I cooled off on the idea since. So I did intend this as an RFC, because I'm not really 100% happy with the design. >> As part of maybe moving the python library onto a package, I thought >> that maybe developing a JobRunner tool would be useful to have in that >> library. As you can see, I nestled it into iotests.py, though.) > > Let's just do that now, we can always move it somewhere else later. > I assume you mean "Let's just put it in iotests.py for now." If we do decide to take that 32 patch series to formalize a qemu lib, it will not be hard to start moving things from here to there. I will probably take an eye to our iotests and see what functionality gets duplicated or used a lot and try to push more things down into the stack where possible. The QMP event handling stuff in particular seems needlessly split between qmp.py, machine.py, iotests.py, etc. > Kevin > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-05-14 19:31 ` John Snow @ 2020-06-16 21:41 ` Eric Blake 2020-06-17 2:49 ` John Snow 0 siblings, 1 reply; 15+ messages in thread From: Eric Blake @ 2020-06-16 21:41 UTC (permalink / raw) To: John Snow, Kevin Wolf Cc: qemu-devel, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz On 5/14/20 2:31 PM, John Snow wrote: >>> >>> Nah, it's fine. I'll clean it up. This is pretty close to an RFC series >>> anyway, so I didn't really polish it. >>> >>> (Or, I will try to clean it up. I probably won't work on it again in the >>> near term. I think I just wanted to see if this seemed useful in general >>> to people. >> >> Ah, there isn't much missing for this series, though. We don't have to >> wait for a fix-the-world series when we can incrementally improve >> things. >> > > Alright, I'll try to hit it halfway -- I spent some time thinking about > a "full" job running framework but ran into some dead-ends I wasn't too > happy with, and wasn't convinced this was a simplification of any kind. > > Still, seeing part of the job running code get duplicated in 040 was a > motivation to try and provide some universal job-running monster that > would be extensible for nearly any task. > > Unfortunately that complexity does generally make the calling sites look > worse, so I cooled off on the idea since. > > So I did intend this as an RFC, because I'm not really 100% happy with > the design. I noticed that the block-dirty-bitmap-populate series depends on this one; is it going to be simpler for me to fix the few things that Kevin pointed out here, or to wait for you to post a v5 of this series, or to rewrite the iotest in that series to not depend on JobRunner after all? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] qmp.py: change event_wait to use a dict 2020-06-16 21:41 ` Eric Blake @ 2020-06-17 2:49 ` John Snow 0 siblings, 0 replies; 15+ messages in thread From: John Snow @ 2020-06-17 2:49 UTC (permalink / raw) To: Eric Blake, Kevin Wolf Cc: qemu-devel, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz On 6/16/20 5:41 PM, Eric Blake wrote: > On 5/14/20 2:31 PM, John Snow wrote: > >>>> >>>> Nah, it's fine. I'll clean it up. This is pretty close to an RFC series >>>> anyway, so I didn't really polish it. >>>> >>>> (Or, I will try to clean it up. I probably won't work on it again in >>>> the >>>> near term. I think I just wanted to see if this seemed useful in >>>> general >>>> to people. >>> >>> Ah, there isn't much missing for this series, though. We don't have to >>> wait for a fix-the-world series when we can incrementally improve >>> things. >>> >> >> Alright, I'll try to hit it halfway -- I spent some time thinking about >> a "full" job running framework but ran into some dead-ends I wasn't too >> happy with, and wasn't convinced this was a simplification of any kind. >> >> Still, seeing part of the job running code get duplicated in 040 was a >> motivation to try and provide some universal job-running monster that >> would be extensible for nearly any task. >> >> Unfortunately that complexity does generally make the calling sites look >> worse, so I cooled off on the idea since. >> >> So I did intend this as an RFC, because I'm not really 100% happy with >> the design. > > I noticed that the block-dirty-bitmap-populate series depends on this > one; is it going to be simpler for me to fix the few things that Kevin > pointed out here, or to wait for you to post a v5 of this series, or to > rewrite the iotest in that series to not depend on JobRunner after all? > It should be pretty trivial (I think) to just rebase the bitpop job on top of mainline QEMU without needing this, I'd recommend doing that. I started porting the job runner to the standalone qemu package instead and it's going to take me longer to do that than it would be to just not use this patchset for the bitpop test. If you ping me on IRC tomorrow (Sorry) I can wean the dependency myself. --js ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/3] iotests: add JobRunner class 2020-05-14 2:25 [PATCH v4 0/3] iotests: add JobRunner framework John Snow 2020-05-14 2:25 ` [PATCH v4 1/3] qmp.py: change event_wait to use a dict John Snow @ 2020-05-14 2:25 ` John Snow 2020-05-14 15:40 ` Kevin Wolf 2020-05-14 2:25 ` [PATCH v4 3/3] iotests: modify test 040 to use JobRunner John Snow 2 siblings, 1 reply; 15+ messages in thread From: John Snow @ 2020-05-14 2:25 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa, 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. pylint note: the 'callbacks' option guards against unused warning arguments in functions designated as callbacks. It does not currently guard against "no-self-use" though; hence a once-off ignore. mypy note: QapiEvent is only a weak alias; it's fully interchangable with the type it's declared as. In the future, we may wish to tighten these types. For now, this communicates the rough shape of the type and (more importantly) the intent. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qemu-iotests/055 | 17 +-- tests/qemu-iotests/155 | 11 +- tests/qemu-iotests/255 | 9 +- tests/qemu-iotests/257 | 54 +++++---- tests/qemu-iotests/iotests.py | 206 +++++++++++++++++++++++++--------- tests/qemu-iotests/pylintrc | 11 ++ 6 files changed, 225 insertions(+), 83 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index 4d3744b0d3..7e13585a94 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -115,19 +115,22 @@ class TestSingleDrive(iotests.QMPTestCase): self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img) def do_test_resize_blockdev_backup(self, device, node): - def pre_finalize(): - result = self.vm.qmp('block_resize', device=device, size=65536) - self.assert_qmp(result, 'error/class', 'GenericError') - - result = self.vm.qmp('block_resize', node_name=node, size=65536) - self.assert_qmp(result, 'error/class', 'GenericError') + class JobRunner(iotests.TestJobRunner): + def on_pending(self, event): + qmp = self._vm.qmp + result = qmp('block_resize', device=device, size=65536) + self.test.assert_qmp(result, 'error/class', 'GenericError') + result = qmp('block_resize', node_name=node, size=65536) + self.test.assert_qmp(result, 'error/class', 'GenericError') + super().on_pending(event) result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0', target='drive1', sync='full', auto_finalize=False, auto_dismiss=False) self.assert_qmp(result, 'return', {}) - self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize) + runner = JobRunner(self.vm, 'job0', test=self, auto_finalize=False) + runner.run() def test_source_resize_blockdev_backup(self): self.do_test_resize_blockdev_backup('drive0', 'source') diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 index cb371d4649..1aa6cfefb8 100755 --- a/tests/qemu-iotests/155 +++ b/tests/qemu-iotests/155 @@ -163,6 +163,12 @@ class BaseClass(iotests.QMPTestCase): self.assert_qmp_absent(node, 'image/backing-image') +class MirrorJob(iotests.TestJobRunner): + def on_pending(self, event): + self.test.openBacking() + super().on_pending(event) + + # Class variables for controlling its behavior: # # cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror @@ -188,8 +194,9 @@ class MirrorBaseClass(BaseClass): self.assert_qmp(result, 'return', {}) - self.vm.run_job('mirror-job', auto_finalize=False, - pre_finalize=self.openBacking, auto_dismiss=True) + job = MirrorJob(self.vm, 'mirror-job', test=self, + auto_finalize=False, auto_dismiss=True) + job.run() def testFull(self): self.runMirror('full') diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255 index 8f08f741da..74487548fa 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 JobRunner(iotests.JobRunner): + def on_pending(self, event): + start_requests() + super().on_pending(event) + + runner = JobRunner(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 004a433b8b..2933e2670b 100755 --- a/tests/qemu-iotests/257 +++ b/tests/qemu-iotests/257 @@ -352,30 +352,40 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None): job = backup(drive0, 1, bsync1, msync_mode, bitmap="bitmap0", bitmap_mode=bsync_mode) - def _callback(): - """Issue writes while the job is open to test bitmap divergence.""" - # Note: when `failure` is 'intermediate', this isn't called. - log('') - bitmaps = perform_writes(drive0, 2, filter_node_name='backup-top') - # Named bitmap (static, should be unchanged) - ebitmap.compare(vm.get_bitmap(drive0.node, 'bitmap0', - bitmaps=bitmaps)) - # Anonymous bitmap (dynamic, shows new writes) - anonymous = EmulatedBitmap() - anonymous.dirty_group(2) - anonymous.compare(vm.get_bitmap(drive0.node, '', recording=True, - bitmaps=bitmaps)) - # Simulate the order in which this will happen: - # group 1 gets cleared first, then group two gets written. - if ((bsync_mode == 'on-success' and not failure) or - (bsync_mode == 'always')): - ebitmap.clear() - ebitmap.dirty_group(2) + class JobRunner(iotests.JobRunner): + def on_pending(self, event): + """ + Issue writes while the job is open to test bitmap divergence. + """ + + # Note: when `failure` is 'intermediate', this isn't called. + log('') + bitmaps = perform_writes(drive0, 2, + filter_node_name='backup-top') + # Named bitmap (static, should be unchanged) + ebitmap.compare(vm.get_bitmap(drive0.node, 'bitmap0', + bitmaps=bitmaps)) + # Anonymous bitmap (dynamic, shows new writes) + anonymous = EmulatedBitmap() + anonymous.dirty_group(2) + anonymous.compare(vm.get_bitmap(drive0.node, '', recording=True, + bitmaps=bitmaps)) + + # Simulate the order in which this will happen: + # group 1 gets cleared first, then group two gets written. + if ((bsync_mode == 'on-success' and not failure) or + (bsync_mode == 'always')): + ebitmap.clear() + ebitmap.dirty_group(2) + + super().on_pending(event) + + + runner = JobRunner(vm, job, cancel=(failure == 'simulated'), + auto_finalize=False, auto_dismiss=True) + runner.run() - vm.run_job(job, auto_dismiss=True, auto_finalize=False, - pre_finalize=_callback, - cancel=(failure == 'simulated')) bitmaps = vm.query_bitmaps() log({'bitmaps': bitmaps}, indent=2) log('') diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index aada94f4f9..6b9b35acb7 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -40,6 +40,7 @@ # Type Aliases QMPResponse = Dict[str, Any] +QapiEvent = Dict[str, Any] # Use this logger for logging messages directly from the iotests module @@ -489,6 +490,141 @@ def remote_filename(path): else: raise Exception("Protocol %s not supported" % (imgproto)) + +class JobRunner: + """ + JobRunner offers a job-lifetime management framework. + + It can be used as-is for a no-frills run-to-completion module, + or subclassed to gain access to per-event callbacks for + customizable behavior. + + :param vm: The VM the job is running on + :param job: Job ID of a recently created job + :param cancel: When true, cancels the job prior to finalization. + :param auto_finalize: True if the job was configured to finalize itself. + :param auto_dismiss: True if the job will dismiss itself post-finalization. + """ + def __init__(self, + vm: 'VM', + job: str, + cancel: bool = False, + auto_finalize: bool = True, + auto_dismiss: bool = False): + self._vm = vm + self._id = job + self.cancel = cancel + + self._auto_finalize = auto_finalize + self._auto_dismiss = auto_dismiss + self._exited = False + self._error: Optional[str] = None + + match_device = {'data': {'device': self._id}} + match_id = {'data': {'id': self._id}} + + # Listen for these events with these parameters: + 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, + } + + # These are Job state change callbacks. + # Subclass and override these for custom workflows. + + def on_create(self, event: QapiEvent) -> None: + pass + + def on_run(self, event: QapiEvent) -> None: + pass + + def on_pause(self, event: QapiEvent) -> None: + pass + + def on_ready(self, event: QapiEvent) -> None: + self._vm.qmp_log('job-complete', id=self._id) + + def on_standby(self, event: QapiEvent) -> None: + pass + + def on_waiting(self, event: QapiEvent) -> None: + pass + + def on_pending(self, event: QapiEvent) -> None: + if self._auto_finalize: + return + + if self.cancel: + self._vm.qmp_log('job-cancel', id=self._id) + else: + self._vm.qmp_log('job-finalize', id=self._id) + + def on_abort(self, event: QapiEvent) -> None: + result = self._vm.qmp('query-jobs') + for j in result['return']: + if j['id'] == self._id: + self._error = j['error'] + log('Job failed: %s' % (j['error'])) + + def on_conclude(self, event: QapiEvent) -> None: + if self._auto_dismiss: + return + + self._vm.qmp_log('job-dismiss', id=self._id) + + def on_null(self, event: QapiEvent) -> None: + self._exited = True + + # Macro events -- QAPI events. + # These are callbacks for individual top-level events. + + def on_change(self, event: QapiEvent) -> None: + status = event['data']['status'] + assert status in self._dispatch + self._dispatch[status](event) + + def on_block_job_event(self, event: QapiEvent) -> None: + # pylint: disable=no-self-use + log(event) + + def event(self, event: QapiEvent) -> None: + 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: float = 60.0) -> Optional[str]: + """ + Run the event loop for this job. + + :param wait: Timeout in seconds specifying how long to wait + for an event. Defaults to 60.0. + :return: Error string on failure, Nothing on success. + """ + 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''' @@ -615,60 +751,21 @@ def qmp_log(self, cmd, filters=(), indent=None, **kwargs): log(result, filters, indent=indent) return result - # 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, wait=60.0): + def run_job(self, job, **kwargs) -> Optional[str]: """ run_job moves a job from creation through to dismissal. - :param job: String. ID of recently-launched job - :param auto_finalize: Bool. True if the job was launched with - 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 wait: Float. Timeout value specifying how long to wait for any - event, in seconds. Defaults to 60.0. + :param job: Job ID of a recently created job. + :param kwargs: See JobRunner.__init__() and JobRunner.run(). + + :return: Error string on failure, Nothing on success. """ - 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': - 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'] - 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: - self.qmp_log('job-cancel', id=job) - else: - self.qmp_log('job-finalize', id=job) - elif status == 'concluded' and not auto_dismiss: - self.qmp_log('job-dismiss', id=job) - elif status == 'null': - return error + if 'wait' in kwargs: + run_kwargs = {'wait': kwargs.pop('wait')} + else: + run_kwargs = {} + job_runner = JobRunner(self, job, **kwargs) + return job_runner.run(**run_kwargs) # Returns None on success, and an error string on failure def blockdev_create(self, options, job_id='job0', filters=None): @@ -980,6 +1077,15 @@ def case_skip(self, reason): self.skipTest(reason) +class TestJobRunner(JobRunner): + """ + JobRunner intended for use within a QMPTestCase. + """ + def __init__(self, *args, test: QMPTestCase, **kwargs): + super().__init__(*args, **kwargs) + self.test = test + + def notrun(reason): '''Skip this test suite''' # Each test in qemu-iotests has a number ("seq") diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc index 5481afe528..df602e02b1 100644 --- a/tests/qemu-iotests/pylintrc +++ b/tests/qemu-iotests/pylintrc @@ -17,9 +17,20 @@ disable=invalid-name, too-many-lines, too-many-locals, too-many-public-methods, + too-many-instance-attributes, # These are temporary, and should be removed: missing-docstring, + +[VARIABLES] + +# List of strings which can identify a callback function by name. A callback +# name must start or end with one of those strings. +callbacks=cb_, + _cb, + on_, + + [FORMAT] # Maximum number of characters on a single line. -- 2.21.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] iotests: add JobRunner class 2020-05-14 2:25 ` [PATCH v4 2/3] iotests: add JobRunner class John Snow @ 2020-05-14 15:40 ` Kevin Wolf 2020-05-14 19:32 ` John Snow 0 siblings, 1 reply; 15+ messages in thread From: Kevin Wolf @ 2020-05-14 15:40 UTC (permalink / raw) To: John Snow; +Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa Am 14.05.2020 um 04:25 hat John Snow geschrieben: > 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. > > pylint note: the 'callbacks' option guards against unused warning > arguments in functions designated as callbacks. It does not currently > guard against "no-self-use" though; hence a once-off ignore. > > mypy note: QapiEvent is only a weak alias; it's fully interchangable > with the type it's declared as. In the future, we may wish to tighten > these types. For now, this communicates the rough shape of the type and > (more importantly) the intent. > > Signed-off-by: John Snow <jsnow@redhat.com> > + # Listen for these events with these parameters: > + 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 > + } The old code had a trailing comma here in case we need to add more events later. Anyway: Reviewed-by: Kevin Wolf <kwolf@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] iotests: add JobRunner class 2020-05-14 15:40 ` Kevin Wolf @ 2020-05-14 19:32 ` John Snow 0 siblings, 0 replies; 15+ messages in thread From: John Snow @ 2020-05-14 19:32 UTC (permalink / raw) To: Kevin Wolf Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa On 5/14/20 11:40 AM, Kevin Wolf wrote: > Am 14.05.2020 um 04:25 hat John Snow geschrieben: >> 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. >> >> pylint note: the 'callbacks' option guards against unused warning >> arguments in functions designated as callbacks. It does not currently >> guard against "no-self-use" though; hence a once-off ignore. >> >> mypy note: QapiEvent is only a weak alias; it's fully interchangable >> with the type it's declared as. In the future, we may wish to tighten >> these types. For now, this communicates the rough shape of the type and >> (more importantly) the intent. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > >> + # Listen for these events with these parameters: >> + 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 >> + } > > The old code had a trailing comma here in case we need to add more > events later. Anyway: > > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > Whoops. I favor those too, so I'll put it back. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/3] iotests: modify test 040 to use JobRunner 2020-05-14 2:25 [PATCH v4 0/3] iotests: add JobRunner framework John Snow 2020-05-14 2:25 ` [PATCH v4 1/3] qmp.py: change event_wait to use a dict John Snow 2020-05-14 2:25 ` [PATCH v4 2/3] iotests: add JobRunner class John Snow @ 2020-05-14 2:25 ` John Snow 2020-05-14 15:53 ` Kevin Wolf 2 siblings, 1 reply; 15+ messages in thread From: John Snow @ 2020-05-14 2:25 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa, 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..e2ef3bb812 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): + super().on_pause(event) + result = self._vm.qmp('block-job-resume', device=self._id) + self.test.assert_qmp(result, 'return', {}) + + 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(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', 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] 15+ messages in thread
* Re: [PATCH v4 3/3] iotests: modify test 040 to use JobRunner 2020-05-14 2:25 ` [PATCH v4 3/3] iotests: modify test 040 to use JobRunner John Snow @ 2020-05-14 15:53 ` Kevin Wolf 2020-05-14 19:37 ` John Snow 0 siblings, 1 reply; 15+ messages in thread From: Kevin Wolf @ 2020-05-14 15:53 UTC (permalink / raw) To: John Snow; +Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa Am 14.05.2020 um 04:25 hat John Snow geschrieben: > Instead of having somewhat reproduced it for itself. > > Signed-off-by: John Snow <jsnow@redhat.com> I think you should pass auto_dismiss=True to the JobRunner, or (probably preferable) change prepare_and_start_job() to start the job with auto_dismiss=False. Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] iotests: modify test 040 to use JobRunner 2020-05-14 15:53 ` Kevin Wolf @ 2020-05-14 19:37 ` John Snow 2020-05-15 9:46 ` Kevin Wolf 0 siblings, 1 reply; 15+ messages in thread From: John Snow @ 2020-05-14 19:37 UTC (permalink / raw) To: Kevin Wolf Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa On 5/14/20 11:53 AM, Kevin Wolf wrote: > Am 14.05.2020 um 04:25 hat John Snow geschrieben: >> Instead of having somewhat reproduced it for itself. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > > I think you should pass auto_dismiss=True to the JobRunner, or (probably > preferable) change prepare_and_start_job() to start the job with > auto_dismiss=False. > > Kevin > okay, I'll try that out and see if I like it. Wild tangents, as is my normal: I also think it would be neat, in some sense, to provide a job creation abstraction where creating the QMP command in python also creates the runner with the right parameters based on how you initialized it. I've not given these even a proper three minutes think, but some generalized interface for managing the creation of jobs to use in concert with the job runner would be slick. (What reminds me of this is needing to remember and understand if I started something with auto_dismiss or not, which jobs it defaults to which for, etc. Streamlining the creation and runner could be slick for faster test-writing in normative cases.) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] iotests: modify test 040 to use JobRunner 2020-05-14 19:37 ` John Snow @ 2020-05-15 9:46 ` Kevin Wolf 0 siblings, 0 replies; 15+ messages in thread From: Kevin Wolf @ 2020-05-15 9:46 UTC (permalink / raw) To: John Snow; +Cc: Eduardo Habkost, Max Reitz, qemu-devel, qemu-block, Cleber Rosa Am 14.05.2020 um 21:37 hat John Snow geschrieben: > > > On 5/14/20 11:53 AM, Kevin Wolf wrote: > > Am 14.05.2020 um 04:25 hat John Snow geschrieben: > >> Instead of having somewhat reproduced it for itself. > >> > >> Signed-off-by: John Snow <jsnow@redhat.com> > > > > I think you should pass auto_dismiss=True to the JobRunner, or (probably > > preferable) change prepare_and_start_job() to start the job with > > auto_dismiss=False. > > > > Kevin > > > > okay, I'll try that out and see if I like it. > > > Wild tangents, as is my normal: > > I also think it would be neat, in some sense, to provide a job creation > abstraction where creating the QMP command in python also creates the > runner with the right parameters based on how you initialized it. > > I've not given these even a proper three minutes think, but some > generalized interface for managing the creation of jobs to use in > concert with the job runner would be slick. JobRunner could have a static method that starts a job (it would take the same options as qmp() and forward everything to qmp(), except that it parses auto_* first) and returns a JobRunner object that you can run later. > (What reminds me of this is needing to remember and understand if I > started something with auto_dismiss or not, which jobs it defaults to > which for, etc. Streamlining the creation and runner could be slick for > faster test-writing in normative cases.) Yes. In general, I think we should keep tests simple (even if that means some duplication) and avoid overengineering testing infrastructure because we could well sink more time there than we'll ever get back, but I guess small and simple wrappers like in this case can't hurt. Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-06-17 2:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-14 2:25 [PATCH v4 0/3] iotests: add JobRunner framework John Snow 2020-05-14 2:25 ` [PATCH v4 1/3] qmp.py: change event_wait to use a dict John Snow 2020-05-14 14:47 ` Kevin Wolf 2020-05-14 15:07 ` John Snow 2020-05-14 15:59 ` Kevin Wolf 2020-05-14 19:31 ` John Snow 2020-06-16 21:41 ` Eric Blake 2020-06-17 2:49 ` John Snow 2020-05-14 2:25 ` [PATCH v4 2/3] iotests: add JobRunner class John Snow 2020-05-14 15:40 ` Kevin Wolf 2020-05-14 19:32 ` John Snow 2020-05-14 2:25 ` [PATCH v4 3/3] iotests: modify test 040 to use JobRunner John Snow 2020-05-14 15:53 ` Kevin Wolf 2020-05-14 19:37 ` John Snow 2020-05-15 9:46 ` Kevin Wolf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.