| Message ID | 20190920152804.12875-21-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix check_to_replace_node() | expand |
20.09.2019 18:28, Max Reitz wrote: > Add two tests to see that you cannot replace a Quorum child with the > mirror job while the child is in use by a different parent. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/041 | 57 +++++++++++++++++++++++++++++++++++++- > tests/qemu-iotests/041.out | 4 +-- > 2 files changed, 58 insertions(+), 3 deletions(-) > > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 > index 20ae9750b7..148dc47ce4 100755 > --- a/tests/qemu-iotests/041 > +++ b/tests/qemu-iotests/041 > @@ -34,6 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img') > quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img') > quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img') > > +nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock') > + > class TestSingleDrive(iotests.QMPTestCase): > image_len = 1 * 1024 * 1024 # MB > qmp_cmd = 'drive-mirror' > @@ -901,7 +903,8 @@ class TestRepairQuorum(iotests.QMPTestCase): > > def tearDown(self): > self.vm.shutdown() > - for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]: > + for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file, > + nbd_sock_path ]: > # Do a try/except because the test may have deleted some images > try: > os.remove(i) > @@ -1075,6 +1078,58 @@ class TestRepairQuorum(iotests.QMPTestCase): > self.assert_has_block_node("repair0", quorum_repair_img) > self.vm.assert_block_path('quorum0/children.1', 'repair0') > > + ''' > + Check that we cannot replace a Quorum child when it has other > + parents. > + ''' you constantly use ''', when PEP8 recommends """ for doc-strings. I can't complain, as our python code is something not related to PEP8 unfortunately.. > + def test_with_other_parent(self): don't we need if not iotests.supports_quorum(): return like in neighbors? > + result = self.vm.qmp('nbd-server-start', > + addr={ > + 'type': 'unix', > + 'data': {'path': nbd_sock_path} > + }) > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('nbd-server-add', device='img1') > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0', > + sync='full', node_name='repair0', replaces='img1', > + target=quorum_repair_img, format=iotests.imgfmt) > + self.assert_qmp(result, 'error/desc', > + "Cannot replace 'img1' by a node mirrored from " > + "'quorum0', because it cannot be guaranteed that doing " > + "so would not lead to an abrupt change of visible data") > + > + ''' > + The same as test_with_other_parent(), but add the NBD server only > + when the mirror job is already running. > + ''' > + def test_with_other_parents_after_mirror_start(self): > + result = self.vm.qmp('nbd-server-start', > + addr={ > + 'type': 'unix', > + 'data': {'path': nbd_sock_path} > + }) > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0', > + sync='full', node_name='repair0', replaces='img1', > + target=quorum_repair_img, format=iotests.imgfmt) > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('nbd-server-add', device='img1') > + self.assert_qmp(result, 'return', {}) > + > + # The full error message goes to stderr, so we unfortunately > + # cannot check it here We can, iotests 169 and 245 do it with help of vm.get_log() > + self.complete_and_wait('mirror', > + completion_error='Operation not permitted') > + > + # Should not have been replaced > + self.vm.assert_block_path('quorum0/children.1', 'img1') > + > + > # Test mirroring with a source that does not have any parents (not even a > # BlockBackend) > class TestOrphanedSource(iotests.QMPTestCase): > diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out > index f496be9197..ffc779b4d1 100644 > --- a/tests/qemu-iotests/041.out > +++ b/tests/qemu-iotests/041.out > @@ -1,5 +1,5 @@ > -........................................................................................... > +............................................................................................. > ---------------------------------------------------------------------- > -Ran 91 tests > +Ran 93 tests > > OK > With supports_quorum checked like in neighbor test-cases (or use @iotests.skip_if_unsupported): Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 26.09.19 16:30, Vladimir Sementsov-Ogievskiy wrote: > 20.09.2019 18:28, Max Reitz wrote: >> Add two tests to see that you cannot replace a Quorum child with the >> mirror job while the child is in use by a different parent. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/041 | 57 +++++++++++++++++++++++++++++++++++++- >> tests/qemu-iotests/041.out | 4 +-- >> 2 files changed, 58 insertions(+), 3 deletions(-) >> >> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 >> index 20ae9750b7..148dc47ce4 100755 >> --- a/tests/qemu-iotests/041 >> +++ b/tests/qemu-iotests/041 >> @@ -34,6 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img') >> quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img') >> quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img') >> >> +nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock') >> + >> class TestSingleDrive(iotests.QMPTestCase): >> image_len = 1 * 1024 * 1024 # MB >> qmp_cmd = 'drive-mirror' >> @@ -901,7 +903,8 @@ class TestRepairQuorum(iotests.QMPTestCase): >> >> def tearDown(self): >> self.vm.shutdown() >> - for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]: >> + for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file, >> + nbd_sock_path ]: >> # Do a try/except because the test may have deleted some images >> try: >> os.remove(i) >> @@ -1075,6 +1078,58 @@ class TestRepairQuorum(iotests.QMPTestCase): >> self.assert_has_block_node("repair0", quorum_repair_img) >> self.vm.assert_block_path('quorum0/children.1', 'repair0') >> >> + ''' >> + Check that we cannot replace a Quorum child when it has other >> + parents. >> + ''' > > you constantly use ''', when PEP8 recommends """ for doc-strings. I can't > complain, as our python code is something not related to PEP8 unfortunately.. I just use what I see in existing code. (And additionally, in scripting languages I tend to just use what works.) >> + def test_with_other_parent(self): > > don't we need > if not iotests.supports_quorum(): > return > like in neighbors? Good point. Or a decorator, probably. >> + result = self.vm.qmp('nbd-server-start', >> + addr={ >> + 'type': 'unix', >> + 'data': {'path': nbd_sock_path} >> + }) >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('nbd-server-add', device='img1') >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0', >> + sync='full', node_name='repair0', replaces='img1', >> + target=quorum_repair_img, format=iotests.imgfmt) >> + self.assert_qmp(result, 'error/desc', >> + "Cannot replace 'img1' by a node mirrored from " >> + "'quorum0', because it cannot be guaranteed that doing " >> + "so would not lead to an abrupt change of visible data") >> + >> + ''' >> + The same as test_with_other_parent(), but add the NBD server only >> + when the mirror job is already running. >> + ''' >> + def test_with_other_parents_after_mirror_start(self): >> + result = self.vm.qmp('nbd-server-start', >> + addr={ >> + 'type': 'unix', >> + 'data': {'path': nbd_sock_path} >> + }) >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0', >> + sync='full', node_name='repair0', replaces='img1', >> + target=quorum_repair_img, format=iotests.imgfmt) >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('nbd-server-add', device='img1') >> + self.assert_qmp(result, 'return', {}) >> + >> + # The full error message goes to stderr, so we unfortunately >> + # cannot check it here > > We can, iotests 169 and 245 do it with help of vm.get_log() Uh, thanks, I’ll look into it. Max >> + self.complete_and_wait('mirror', >> + completion_error='Operation not permitted') >> + >> + # Should not have been replaced >> + self.vm.assert_block_path('quorum0/children.1', 'img1') >> + >> + >> # Test mirroring with a source that does not have any parents (not even a >> # BlockBackend) >> class TestOrphanedSource(iotests.QMPTestCase): >> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out >> index f496be9197..ffc779b4d1 100644 >> --- a/tests/qemu-iotests/041.out >> +++ b/tests/qemu-iotests/041.out >> @@ -1,5 +1,5 @@ >> -........................................................................................... >> +............................................................................................. >> ---------------------------------------------------------------------- >> -Ran 91 tests >> +Ran 93 tests >> >> OK >> > > With supports_quorum checked like in neighbor test-cases (or use @iotests.skip_if_unsupported): > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index 20ae9750b7..148dc47ce4 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -34,6 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img') quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img') quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img') +nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock') + class TestSingleDrive(iotests.QMPTestCase): image_len = 1 * 1024 * 1024 # MB qmp_cmd = 'drive-mirror' @@ -901,7 +903,8 @@ class TestRepairQuorum(iotests.QMPTestCase): def tearDown(self): self.vm.shutdown() - for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]: + for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file, + nbd_sock_path ]: # Do a try/except because the test may have deleted some images try: os.remove(i) @@ -1075,6 +1078,58 @@ class TestRepairQuorum(iotests.QMPTestCase): self.assert_has_block_node("repair0", quorum_repair_img) self.vm.assert_block_path('quorum0/children.1', 'repair0') + ''' + Check that we cannot replace a Quorum child when it has other + parents. + ''' + def test_with_other_parent(self): + result = self.vm.qmp('nbd-server-start', + addr={ + 'type': 'unix', + 'data': {'path': nbd_sock_path} + }) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('nbd-server-add', device='img1') + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0', + sync='full', node_name='repair0', replaces='img1', + target=quorum_repair_img, format=iotests.imgfmt) + self.assert_qmp(result, 'error/desc', + "Cannot replace 'img1' by a node mirrored from " + "'quorum0', because it cannot be guaranteed that doing " + "so would not lead to an abrupt change of visible data") + + ''' + The same as test_with_other_parent(), but add the NBD server only + when the mirror job is already running. + ''' + def test_with_other_parents_after_mirror_start(self): + result = self.vm.qmp('nbd-server-start', + addr={ + 'type': 'unix', + 'data': {'path': nbd_sock_path} + }) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0', + sync='full', node_name='repair0', replaces='img1', + target=quorum_repair_img, format=iotests.imgfmt) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('nbd-server-add', device='img1') + self.assert_qmp(result, 'return', {}) + + # The full error message goes to stderr, so we unfortunately + # cannot check it here + self.complete_and_wait('mirror', + completion_error='Operation not permitted') + + # Should not have been replaced + self.vm.assert_block_path('quorum0/children.1', 'img1') + + # Test mirroring with a source that does not have any parents (not even a # BlockBackend) class TestOrphanedSource(iotests.QMPTestCase): diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out index f496be9197..ffc779b4d1 100644 --- a/tests/qemu-iotests/041.out +++ b/tests/qemu-iotests/041.out @@ -1,5 +1,5 @@ -........................................................................................... +............................................................................................. ---------------------------------------------------------------------- -Ran 91 tests +Ran 93 tests OK
Add two tests to see that you cannot replace a Quorum child with the mirror job while the child is in use by a different parent. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/041 | 57 +++++++++++++++++++++++++++++++++++++- tests/qemu-iotests/041.out | 4 +-- 2 files changed, 58 insertions(+), 3 deletions(-)