| Message ID | 20171207155102.66622-7-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd export qmp interface | expand |
On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > tests/qemu-iotests/201 | 130 +++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/201.out | 5 ++ > tests/qemu-iotests/group | 1 + > 3 files changed, 136 insertions(+) > create mode 100644 tests/qemu-iotests/201 > create mode 100644 tests/qemu-iotests/201.out > pep8 complains: $ pep8 tests/qemu-iotests/201 tests/qemu-iotests/201:31:1: E302 expected 2 blank lines, found 1 tests/qemu-iotests/201:68:17: E128 continuation line under-indented for visual indent tests/qemu-iotests/201:69:17: E128 continuation line under-indented for visual indent I don't mind cleaning those up (if you don't have to respin because of my comments on 5/6). > +import os > +import sys > +import iotests > +import time > +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive > + > +nbd_port = '10900' > +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp' Is this still going to be safe when we improve the testsuite to run multiple tests in parallel? Wouldn't it be safer to dynamically choose the port, instead of hard-coding one? > +disk = os.path.join(iotests.test_dir, 'disk') > + > +class TestNbdServerRemove(iotests.QMPTestCase): > + def setUp(self): > + qemu_img('create', '-f', iotests.imgfmt, disk, '1M') > + > + self.vm = iotests.VM().add_drive(disk) > + self.vm.launch() > + > + address = { > + 'type': 'inet', > + 'data': { > + 'host': 'localhost', > + 'port': nbd_port Again, for a one-shot test, this works; but it doesn't lend itself to parallel testing. Maybe do a loop with incrementing port numbers until one succeeds, if we can reliably detect when a port is already in use? > + def assertConnectFailed(self, qemu_io_output): > + self.assertEqual(filter_qemu_io(qemu_io_output).strip(), > + "can't open device nbd+tcp://localhost:10900/exp: " + Worth parameterizing or filtering this assertion to match the rest of the parameterization of nbd_port? > + "Requested export not available\nserver reported: " + > + "export 'exp' not present") > + > + def do_test_connect_after_remove(self, force=None): > + args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri) > + self.assertReadOk(qemu_io(*args)) > + self.remove_export('exp', force) > + self.assertExportNotFound('exp') > + self.assertConnectFailed(qemu_io(*args)) > + > + def test_connect_after_remove_default(self): > + self.do_test_connect_after_remove() > + > + def test_connect_after_remove_safe(self): > + self.do_test_connect_after_remove(False) > + > + def test_connect_after_remove_force(self): > + self.do_test_connect_after_remove(True) May need updates if my comments on 3/6 result in us having three states rather than just 2 (the difference being whether there is a knob for choosing to let existing clients gracefully disconnect with all pending I/O completed, and/or failing the nbd-server-remove if a client is still connected). > +++ b/tests/qemu-iotests/201.out > @@ -0,0 +1,5 @@ > +....... > +---------------------------------------------------------------------- > +Ran 7 tests I'm not a fan of python tests that are difficult to debug. Your additions to 147 in patch 4/6 are okay (hard to debug, but an incremental addition); but is it possible to rewrite this test in a bit more verbose manner? See test 194 and this message for more details: https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html > + > +OK > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group > index 3e688678dd..15df7678b3 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -197,3 +197,4 @@ > 197 rw auto quick > 198 rw auto > 200 rw auto > +201 rw auto quick >
09.01.2018 23:49, Eric Blake wrote: > On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> tests/qemu-iotests/201 | 130 +++++++++++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/201.out | 5 ++ >> tests/qemu-iotests/group | 1 + >> 3 files changed, 136 insertions(+) >> create mode 100644 tests/qemu-iotests/201 >> create mode 100644 tests/qemu-iotests/201.out >> > pep8 complains: > > $ pep8 tests/qemu-iotests/201 > tests/qemu-iotests/201:31:1: E302 expected 2 blank lines, found 1 > tests/qemu-iotests/201:68:17: E128 continuation line under-indented for > visual indent > tests/qemu-iotests/201:69:17: E128 continuation line under-indented for > visual indent > > I don't mind cleaning those up (if you don't have to respin because of > my comments on 5/6). > >> +import os >> +import sys >> +import iotests >> +import time >> +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive >> + >> +nbd_port = '10900' >> +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp' > Is this still going to be safe when we improve the testsuite to run > multiple tests in parallel? Wouldn't it be safer to dynamically choose > the port, instead of hard-coding one? > >> +disk = os.path.join(iotests.test_dir, 'disk') >> + >> +class TestNbdServerRemove(iotests.QMPTestCase): >> + def setUp(self): >> + qemu_img('create', '-f', iotests.imgfmt, disk, '1M') >> + >> + self.vm = iotests.VM().add_drive(disk) >> + self.vm.launch() >> + >> + address = { >> + 'type': 'inet', >> + 'data': { >> + 'host': 'localhost', >> + 'port': nbd_port > Again, for a one-shot test, this works; but it doesn't lend itself to > parallel testing. Maybe do a loop with incrementing port numbers until > one succeeds, if we can reliably detect when a port is already in use? > >> + def assertConnectFailed(self, qemu_io_output): >> + self.assertEqual(filter_qemu_io(qemu_io_output).strip(), >> + "can't open device nbd+tcp://localhost:10900/exp: " + > Worth parameterizing or filtering this assertion to match the rest of > the parameterization of nbd_port? > >> + "Requested export not available\nserver reported: " + >> + "export 'exp' not present") >> + >> + def do_test_connect_after_remove(self, force=None): >> + args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri) >> + self.assertReadOk(qemu_io(*args)) >> + self.remove_export('exp', force) >> + self.assertExportNotFound('exp') >> + self.assertConnectFailed(qemu_io(*args)) >> + >> + def test_connect_after_remove_default(self): >> + self.do_test_connect_after_remove() >> + >> + def test_connect_after_remove_safe(self): >> + self.do_test_connect_after_remove(False) >> + >> + def test_connect_after_remove_force(self): >> + self.do_test_connect_after_remove(True) > May need updates if my comments on 3/6 result in us having three states > rather than just 2 (the difference being whether there is a knob for > choosing to let existing clients gracefully disconnect with all pending > I/O completed, and/or failing the nbd-server-remove if a client is still > connected). > >> +++ b/tests/qemu-iotests/201.out >> @@ -0,0 +1,5 @@ >> +....... >> +---------------------------------------------------------------------- >> +Ran 7 tests > I'm not a fan of python tests that are difficult to debug. Your > additions to 147 in patch 4/6 are okay (hard to debug, but an > incremental addition); but is it possible to rewrite this test in a bit > more verbose manner? See test 194 and this message for more details: > https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html hmm, what do you mean by "difficult to debug"? This is a usual python unittest based test. And there 3 test cases, sharing same setUp. Do not you say that unittest becomes deprecated in qemu? I think, if we have only one testcase, we may use 194-like approach, but if we have more, it's better to use unittest. > >> + >> +OK >> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group >> index 3e688678dd..15df7678b3 100644 >> --- a/tests/qemu-iotests/group >> +++ b/tests/qemu-iotests/group >> @@ -197,3 +197,4 @@ >> 197 rw auto quick >> 198 rw auto >> 200 rw auto >> +201 rw auto quick >>
On 01/12/2018 05:43 AM, Vladimir Sementsov-Ogievskiy wrote: >>> +++ b/tests/qemu-iotests/201.out >>> @@ -0,0 +1,5 @@ >>> +....... >>> +---------------------------------------------------------------------- >>> +Ran 7 tests >> I'm not a fan of python tests that are difficult to debug. Your >> additions to 147 in patch 4/6 are okay (hard to debug, but an >> incremental addition); but is it possible to rewrite this test in a bit >> more verbose manner? See test 194 and this message for more details: >> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html > > hmm, what do you mean by "difficult to debug"? This is a usual python > unittest based test. And the list archives show several threads of people complaining that ./check failing with a diff that merely shows: -..... +..E.. makes it rather hard to see WHAT test 2 was doing that caused an error instead of a pass, let alone set up a reproduction scenario on JUST the failing test. Yes, a lot of existing iotests use this unittest layout, and on that grounds, I'm not opposed to adding another one; but test 194 really IS easier to debug when something goes wrong. > And there 3 test cases, sharing same setUp. Do not you say that unittest > becomes > deprecated in qemu? I think, if we have only one testcase, we may use > 194-like approach, > but if we have more, it's better to use unittest. Yes, I think a nice goal for improved testing is to write more python-based iotests in the style that uses actual output, and not just the unittest framework, in the test log. It's not a hard requirement as long as no one has converted existing tests, but is food for thought.
12.01.2018 19:54, Eric Blake wrote: > On 01/12/2018 05:43 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> +++ b/tests/qemu-iotests/201.out >>>> @@ -0,0 +1,5 @@ >>>> +....... >>>> +---------------------------------------------------------------------- >>>> +Ran 7 tests >>> I'm not a fan of python tests that are difficult to debug. Your >>> additions to 147 in patch 4/6 are okay (hard to debug, but an >>> incremental addition); but is it possible to rewrite this test in a bit >>> more verbose manner? See test 194 and this message for more details: >>> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html >> hmm, what do you mean by "difficult to debug"? This is a usual python >> unittest based test. > And the list archives show several threads of people complaining that > ./check failing with a diff that merely shows: > > -..... > +..E.. didn't see that. usually, for failed iotests I see -..... +..E.. + some kind of assert-fail in one of testcases so we know in which testcase and in which line it was failed. > > makes it rather hard to see WHAT test 2 was doing that caused an error > instead of a pass, let alone set up a reproduction scenario on JUST the > failing test. Yes, a lot of existing iotests use this unittest layout, > and on that grounds, I'm not opposed to adding another one; but test 194 > really IS easier to debug when something goes wrong. > >> And there 3 test cases, sharing same setUp. Do not you say that unittest >> becomes >> deprecated in qemu? I think, if we have only one testcase, we may use >> 194-like approach, >> but if we have more, it's better to use unittest. > Yes, I think a nice goal for improved testing is to write more > python-based iotests in the style that uses actual output, and not just > the unittest framework, in the test log. It's not a hard requirement as > long as no one has converted existing tests, but is food for thought. > I think, it doesn't mean that we should not use unittest at all, we just need more output with it.
On 01/15/2018 08:40 AM, Vladimir Sementsov-Ogievskiy wrote: >> And the list archives show several threads of people complaining that >> ./check failing with a diff that merely shows: >> >> -..... >> +..E.. > > didn't see that. usually, for failed iotests I see > > -..... > +..E.. > > + some kind of assert-fail in one of testcases Although deciphering the assert-fail is not always trivial, and it is still sorely underdocumented on how to manually reproduce the situation that got to the stackdump. > > so we know in which testcase and in which line it was failed. > >> >> makes it rather hard to see WHAT test 2 was doing that caused an error >> instead of a pass, let alone set up a reproduction scenario on JUST the >> failing test. Yes, a lot of existing iotests use this unittest layout, >> and on that grounds, I'm not opposed to adding another one; but test 194 >> really IS easier to debug when something goes wrong. >> >>> And there 3 test cases, sharing same setUp. Do not you say that unittest >>> becomes >>> deprecated in qemu? I think, if we have only one testcase, we may use >>> 194-like approach, >>> but if we have more, it's better to use unittest. >> Yes, I think a nice goal for improved testing is to write more >> python-based iotests in the style that uses actual output, and not just >> the unittest framework, in the test log. It's not a hard requirement as >> long as no one has converted existing tests, but is food for thought. >> > > I think, it doesn't mean that we should not use unittest at all, we just > need more output with > it. Yes, that's also a potentially viable option.
15.01.2018 18:05, Eric Blake wrote: > On 01/15/2018 08:40 AM, Vladimir Sementsov-Ogievskiy wrote: > >>> And the list archives show several threads of people complaining that >>> ./check failing with a diff that merely shows: >>> >>> -..... >>> +..E.. >> didn't see that. usually, for failed iotests I see >> >> -..... >> +..E.. >> >> + some kind of assert-fail in one of testcases > Although deciphering the assert-fail is not always trivial, and it is > still sorely underdocumented on how to manually reproduce the situation > that got to the stackdump. hmm, restart test? is it documented for 194? I don't see an option to run only one testcase, but unittest supports it, so we just need to add this option to ./check Is it really not trivial with my test? example, inject bug: --- a/tests/qemu-iotests/201 +++ b/tests/qemu-iotests/201 @@ -76,7 +76,7 @@ class TestNbdServerRemove(iotests.QMPTestCase): self.assertEqual(filter_qemu_io(qemu_io_output).strip(), "can't open device nbd+tcp://localhost:10900/exp: " + "Requested export not available\nserver reported: " + - "export 'exp' not present") + "1export 'exp' not present") def do_test_connect_after_remove(self, force=None): args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri) output: -....... +FFF.... +====================================================================== +FAIL: test_connect_after_remove_default (__main__.TestNbdServerRemove) +---------------------------------------------------------------------- +Traceback (most recent call last): + File "201", line 89, in test_connect_after_remove_default + self.do_test_connect_after_remove() + File "201", line 86, in do_test_connect_after_remove + self.assertConnectFailed(qemu_io(*args)) + File "201", line 79, in assertConnectFailed + "1export 'exp' not present") +AssertionError: "can't open device nbd+tcp://localhost:10900/exp: Requested export not available\nserver reported: export 'exp' not present" != "can't open device nbd+tcp://localhost:10900/exp: Requested export not available\nserver reported: 1export 'exp' not present" + [...] - all obvious. We see what happened and where. And the name of the broken testcase. ---- I remember the following problems with iotests, but I do not think that this is a reason to deprecate unittest and go in some custom way. Better is to fix them. It's all are problems of our unittest wrapping, not of python unittest. - asserts and prints are not naturally mixed in final output (isn't it a reason for never using print in these tests?) - no progress, to see the output we should wait until test finished - if qemu crashed, it is hard to understand, in which testcase so, my point is: use unittest. It is a standard library and common way of doing this. And it is already in Qemu iotests. It gives good organization of test code. Maybe, a "plain executable python test" is good for complicated tests, which are actually cant be called "unit test", but which are more like "system wide test", when we actually need only one testcase, but it needs several pages of code. > >> so we know in which testcase and in which line it was failed. >> >>> makes it rather hard to see WHAT test 2 was doing that caused an error >>> instead of a pass, let alone set up a reproduction scenario on JUST the >>> failing test. Yes, a lot of existing iotests use this unittest layout, >>> and on that grounds, I'm not opposed to adding another one; but test 194 >>> really IS easier to debug when something goes wrong. >>> >>>> And there 3 test cases, sharing same setUp. Do not you say that unittest >>>> becomes >>>> deprecated in qemu? I think, if we have only one testcase, we may use >>>> 194-like approach, >>>> but if we have more, it's better to use unittest. >>> Yes, I think a nice goal for improved testing is to write more >>> python-based iotests in the style that uses actual output, and not just >>> the unittest framework, in the test log. It's not a hard requirement as >>> long as no one has converted existing tests, but is food for thought. >>> >> I think, it doesn't mean that we should not use unittest at all, we just >> need more output with >> it. > Yes, that's also a potentially viable option. >
diff --git a/tests/qemu-iotests/201 b/tests/qemu-iotests/201 new file mode 100644 index 0000000000..28d0324195 --- /dev/null +++ b/tests/qemu-iotests/201 @@ -0,0 +1,130 @@ +#!/usr/bin/env python +# +# Tests for qmp command nbd-server-remove. +# +# Copyright (c) 2017 Virtuozzo International GmbH +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +import os +import sys +import iotests +import time +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive + +nbd_port = '10900' +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp' +disk = os.path.join(iotests.test_dir, 'disk') + +class TestNbdServerRemove(iotests.QMPTestCase): + def setUp(self): + qemu_img('create', '-f', iotests.imgfmt, disk, '1M') + + self.vm = iotests.VM().add_drive(disk) + self.vm.launch() + + address = { + 'type': 'inet', + 'data': { + 'host': 'localhost', + 'port': nbd_port + } + } + + result = self.vm.qmp('nbd-server-start', addr=address) + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp('nbd-server-add', device='drive0', name='exp') + self.assert_qmp(result, 'return', {}) + + def tearDown(self): + self.vm.shutdown() + os.remove(disk) + + def remove_export(self, name, force=None): + if force is None: + result = self.vm.qmp('nbd-server-remove', name=name) + else: + result = self.vm.qmp('nbd-server-remove', name=name, force=force) + self.assert_qmp(result, 'return', {}) + + def assertExportNotFound(self, name): + result = self.vm.qmp('nbd-server-remove', name=name) + self.assert_qmp(result, 'error/desc', "Export 'exp' is not found") + + def assertReadOk(self, qemu_io_output): + self.assertEqual(filter_qemu_io(qemu_io_output).strip(), + 'read 512/512 bytes at offset 0\n' + + '512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)') + + def assertReadFailed(self, qemu_io_output): + self.assertEqual(filter_qemu_io(qemu_io_output).strip(), + 'read failed: Input/output error') + + def assertConnectFailed(self, qemu_io_output): + self.assertEqual(filter_qemu_io(qemu_io_output).strip(), + "can't open device nbd+tcp://localhost:10900/exp: " + + "Requested export not available\nserver reported: " + + "export 'exp' not present") + + def do_test_connect_after_remove(self, force=None): + args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri) + self.assertReadOk(qemu_io(*args)) + self.remove_export('exp', force) + self.assertExportNotFound('exp') + self.assertConnectFailed(qemu_io(*args)) + + def test_connect_after_remove_default(self): + self.do_test_connect_after_remove() + + def test_connect_after_remove_safe(self): + self.do_test_connect_after_remove(False) + + def test_connect_after_remove_force(self): + self.do_test_connect_after_remove(True) + + def do_test_remove_during_connect(self, force=None): + qio = QemuIoInteractive('-r', '-f', 'raw', nbd_uri) + self.assertReadOk(qio.cmd('read 0 512')) + self.remove_export('exp', force) + if force: + self.assertReadFailed(qio.cmd('read 0 512')) + self.assertExportNotFound('exp') + else: + self.assertReadOk(qio.cmd('read 0 512')) + qio.close() + self.assertExportNotFound('exp') + + def test_remove_during_connect_default(self): + self.do_test_remove_during_connect() + + def test_remove_during_connect_safe(self): + self.do_test_remove_during_connect(False) + + def test_remove_during_connect_force(self): + self.do_test_remove_during_connect(True) + + def test_remove_during_connect_safe_force(self): + qio = QemuIoInteractive('-r', '-f', 'raw', nbd_uri) + self.assertReadOk(qio.cmd('read 0 512')) + self.remove_export('exp', False) + self.assertReadOk(qio.cmd('read 0 512')) + self.remove_export('exp', True) + self.assertExportNotFound('exp') + self.assertReadFailed(qio.cmd('read 0 512')) + qio.close() + + +if __name__ == '__main__': + iotests.main() diff --git a/tests/qemu-iotests/201.out b/tests/qemu-iotests/201.out new file mode 100644 index 0000000000..2f7d3902f2 --- /dev/null +++ b/tests/qemu-iotests/201.out @@ -0,0 +1,5 @@ +....... +---------------------------------------------------------------------- +Ran 7 tests + +OK diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 3e688678dd..15df7678b3 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -197,3 +197,4 @@ 197 rw auto quick 198 rw auto 200 rw auto +201 rw auto quick
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- tests/qemu-iotests/201 | 130 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/201.out | 5 ++ tests/qemu-iotests/group | 1 + 3 files changed, 136 insertions(+) create mode 100644 tests/qemu-iotests/201 create mode 100644 tests/qemu-iotests/201.out