| Message ID | 20200217150246.29180-13-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix error handling during bitmap postcopy | expand |
On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote: > Mostly, satisfy pep8 complains. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > tests/qemu-iotests/199 | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/tests/qemu-iotests/199 b/tests/qemu-iotests/199 > index 40774eed74..de9ba8d94c 100755 > --- a/tests/qemu-iotests/199 > +++ b/tests/qemu-iotests/199 > @@ -28,8 +28,8 @@ disk_b = os.path.join(iotests.test_dir, 'disk_b') > size = '256G' > fifo = os.path.join(iotests.test_dir, 'mig_fifo') > > -class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): > > +class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): > def tearDown(self): > self.vm_a.shutdown() > self.vm_b.shutdown() > @@ -54,7 +54,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): > > result = self.vm_a.qmp('block-dirty-bitmap-add', node='drive0', > name='bitmap', granularity=granularity) > - self.assert_qmp(result, 'return', {}); > + self.assert_qmp(result, 'return', {}) > > s = 0 > while s < write_size: > @@ -71,7 +71,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): > > result = self.vm_a.qmp('block-dirty-bitmap-clear', node='drive0', > name='bitmap') > - self.assert_qmp(result, 'return', {}); > + self.assert_qmp(result, 'return', {}) > s = 0 > while s < write_size: > self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk)) > @@ -104,15 +104,16 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): > self.vm_b.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk)) > s += 0x10000 > > - result = self.vm_b.qmp('query-block'); > + result = self.vm_b.qmp('query-block') > while len(result['return'][0]['dirty-bitmaps']) > 1: > time.sleep(2) > - result = self.vm_b.qmp('query-block'); > + result = self.vm_b.qmp('query-block') > > result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256', > node='drive0', name='bitmap') > > - self.assert_qmp(result, 'return/sha256', sha256); > + self.assert_qmp(result, 'return/sha256', sha256) > + > > if __name__ == '__main__': > iotests.main(supported_fmts=['qcow2'], supported_cache_modes=['none'], > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
On 2/17/20 9:02 AM, Vladimir Sementsov-Ogievskiy wrote: > Mostly, satisfy pep8 complains. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > tests/qemu-iotests/199 | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) With none of your series applied, I get: $ ./check -qcow2 199 ... 199 not run [16:52:34] [16:52:34] not suitable for this cache mode: writeback Not run: 199 Passed all 0 iotests 199 fail [16:53:37] [16:53:37] output mismatch (see 199.out.bad) --- /home/eblake/qemu/tests/qemu-iotests/199.out 2020-07-23 16:48:56.275529368 -0500 +++ /home/eblake/qemu/build/tests/qemu-iotests/199.out.bad 2020-07-23 16:53:37.728416207 -0500 @@ -1,5 +1,13 @@ -. +E +====================================================================== +ERROR: test_postcopy (__main__.TestDirtyBitmapPostcopyMigration) +---------------------------------------------------------------------- +Traceback (most recent call last): + File "199", line 41, in setUp + os.mkfifo(fifo) +FileExistsError: [Errno 17] File exists + ---------------------------------------------------------------------- Ran 1 tests -OK +FAILED (errors=1) Failures: 199 Failed 1 of 1 iotests Ah, 'scratch/mig_fifo' was left over from some other aborted run of the test. I removed that file (which implies it might be nice if the test handled that automatically, instead of making me do it), and tried again; now I got the desired: 199 pass [17:00:34] [17:01:48] 74s Passed all 1 iotests After trying to rebase your series, I once again got failures, but that could mean I botched the rebase (since quite a few of the code patches earlier in the series were non-trivially changed). If you send a v3 (which would be really nice!), I'd hoist this and 13/22 first in the series, to get to a point where testing 199 works, to then make it easier to demonstrate what the rest of the 199 enhancements do in relation to the non-iotest patches. But I like that you separated the 199 improvements from the code - testing-wise, it's easy to apply the iotests patches first, make sure it fails, then apply the code patches, and make sure it passes, to prove that the enhanced test now covers what the code fixes did.
24.07.2020 01:03, Eric Blake wrote: > On 2/17/20 9:02 AM, Vladimir Sementsov-Ogievskiy wrote: >> Mostly, satisfy pep8 complains. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> tests/qemu-iotests/199 | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) > > With none of your series applied, I get: > > $ ./check -qcow2 199 > ... > 199 not run [16:52:34] [16:52:34] not suitable for this cache mode: writeback > Not run: 199 > Passed all 0 iotests > 199 fail [16:53:37] [16:53:37] output mismatch (see 199.out.bad) > --- /home/eblake/qemu/tests/qemu-iotests/199.out 2020-07-23 16:48:56.275529368 -0500 > +++ /home/eblake/qemu/build/tests/qemu-iotests/199.out.bad 2020-07-23 16:53:37.728416207 -0500 > @@ -1,5 +1,13 @@ > -. > +E > +====================================================================== > +ERROR: test_postcopy (__main__.TestDirtyBitmapPostcopyMigration) > +---------------------------------------------------------------------- > +Traceback (most recent call last): > + File "199", line 41, in setUp > + os.mkfifo(fifo) > +FileExistsError: [Errno 17] File exists > + > ---------------------------------------------------------------------- > Ran 1 tests > > -OK > +FAILED (errors=1) > Failures: 199 > Failed 1 of 1 iotests > > Ah, 'scratch/mig_fifo' was left over from some other aborted run of the test. I removed that file (which implies it might be nice if the test handled that automatically, instead of making me do it), and tried again; now I got the desired: > > 199 pass [17:00:34] [17:01:48] 74s > Passed all 1 iotests > > > After trying to rebase your series, I once again got failures, but that could mean I botched the rebase (since quite a few of the code patches earlier in the series were non-trivially changed).> If you send a v3 (which would be really nice!), I'd hoist this and 13/22 first in the series, to get to a point where testing 199 works, to then make it easier to demonstrate what the rest of the 199 enhancements do in relation to the non-iotest patches. But I like that you separated the 199 improvements from the code - testing-wise, it's easy to apply the iotests patches first, make sure it fails, then apply the code patches, and make sure it passes, to prove that the enhanced test now covers what the code fixes did. > A bit off-topic: Yes, that's our usual scheme: test goes after bug-fix, so careful reviewers always have to apply patches in different order to check is there a real bug-fix.. And the only benefit of such scheme - not to break git bisect. Still, I think we can do better: - apply test first, just put it into "bug" group - check script should ignore "bug" group (unless it explicitly specified, or direct test run) - bug-fix patch removes test from "bug" group So bisect is not broken and we achieve native ordering: problem exists (test fails) -> fixing -> no problem (test pass). I think, I'll add "bug" group as a follow up to my "[PATCH v4 0/9] Rework iotests/check", which I really hope will land one day. PS: I've successfully rebased the series, and test pass. I'll now fix all review notes and resend soon.
diff --git a/tests/qemu-iotests/199 b/tests/qemu-iotests/199 index 40774eed74..de9ba8d94c 100755 --- a/tests/qemu-iotests/199 +++ b/tests/qemu-iotests/199 @@ -28,8 +28,8 @@ disk_b = os.path.join(iotests.test_dir, 'disk_b') size = '256G' fifo = os.path.join(iotests.test_dir, 'mig_fifo') -class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): +class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): def tearDown(self): self.vm_a.shutdown() self.vm_b.shutdown() @@ -54,7 +54,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): result = self.vm_a.qmp('block-dirty-bitmap-add', node='drive0', name='bitmap', granularity=granularity) - self.assert_qmp(result, 'return', {}); + self.assert_qmp(result, 'return', {}) s = 0 while s < write_size: @@ -71,7 +71,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): result = self.vm_a.qmp('block-dirty-bitmap-clear', node='drive0', name='bitmap') - self.assert_qmp(result, 'return', {}); + self.assert_qmp(result, 'return', {}) s = 0 while s < write_size: self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk)) @@ -104,15 +104,16 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase): self.vm_b.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk)) s += 0x10000 - result = self.vm_b.qmp('query-block'); + result = self.vm_b.qmp('query-block') while len(result['return'][0]['dirty-bitmaps']) > 1: time.sleep(2) - result = self.vm_b.qmp('query-block'); + result = self.vm_b.qmp('query-block') result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256', node='drive0', name='bitmap') - self.assert_qmp(result, 'return/sha256', sha256); + self.assert_qmp(result, 'return/sha256', sha256) + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2'], supported_cache_modes=['none'],
Mostly, satisfy pep8 complains. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- tests/qemu-iotests/199 | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)