| Message ID | 20190920152804.12875-23-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: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/041 | 152 +++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/041.out | 4 +- > 2 files changed, 154 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 > index e4cc829fe2..6ea4764ae8 100755 > --- a/tests/qemu-iotests/041 > +++ b/tests/qemu-iotests/041 > @@ -1265,6 +1265,158 @@ class TestReplaces(iotests.QMPTestCase): > > self.vm.assert_block_path('filter0/file', 'target') > > + ''' > + See what happens when the @sync/@replaces configuration dictates > + creating a loop. > + ''' > + def test_loop(self): > + qemu_img('create', '-f', iotests.imgfmt, test_img, str(1 * 1024 * 1024)) > + > + # Dummy group so we can create a NOP filter > + result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg0') > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('blockdev-add', **{ > + 'driver': 'throttle', > + 'node-name': 'source', > + 'throttle-group': 'tg0', > + 'file': { > + 'driver': iotests.imgfmt, > + 'node-name': 'filtered', > + 'file': { > + 'driver': 'file', > + 'filename': test_img > + } > + } > + }) > + self.assert_qmp(result, 'return', {}) > + > + # Block graph is now: > + # source[throttle] --file--> filtered[qcow2] --file--> ... or qed, actually > + > + result = self.vm.qmp('drive-mirror', job_id='mirror', device='source', > + target=target_img, format=iotests.imgfmt, > + node_name='target', sync='none', > + replaces='filtered') > + > + ''' > + Block graph before mirror exits would be (ignoring mirror_top): > + source[throttle] --file--> filtered[qcow2] --file--> ... > + target[qcow2] --file--> ... > + > + Then, because of sync=none and drive-mirror in absolute-paths mode, > + the source is attached to the target: > + source[throttle] --file--> filtered[qcow2] --file--> ... > + ^ | > + backing > + | > + target[qcow2] --file--> ... > + > + Replacing filtered by target would yield: > + source[throttle] --file--> target[qcow2] --file--> ... > + ^ | > + +------- backing --------+ > + > + I.e., a loop. bdrv_replace_node() detects this and simply > + does not let source's file link point to target. However, > + that means that target cannot really replace source. > + > + drive-mirror should detect this and not allow this case. > + ''' > + > + self.assert_qmp(result, 'error/desc', > + "Replacing 'filtered' by 'target' with this sync " + \ > + "mode would result in a loop, because the former " + \ > + "would be a child of the latter's backing file " + \ > + "('source') after the mirror job") > + > + ''' > + Test what happens when there would be no loop with the pre-mirror > + configuration, but something changes during the mirror job that asks > + for a loop to be created during completion. > + ''' > + def test_loop_during_mirror(self): > + qemu_img('create', '-f', iotests.imgfmt, test_img, str(1 * 1024 * 1024)) > + > + result = self.vm.qmp('blockdev-add', **{ > + 'driver': 'null-co', > + 'node-name': 'common-base', > + 'read-zeroes': True, why do you need read-zeroes? > + 'size': 1 * 1024 * 1024 > + }) > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('blockdev-add', **{ > + 'driver': 'copy-on-read', > + 'node-name': 'source', > + 'file': 'common-base' > + }) > + self.assert_qmp(result, 'return', {}) Hmm, why don't you create them both in one query? > + > + ''' the following is hard to read without some hint like, "We are going to ..." > + x-blockdev-change can only add children to a quorum node that > + have no parent yet, so we need an intermediate node between > + target and common-base that has no parents other than target. > + We cannot use any parent that would forward the RESIZE > + permission (because the job takes it, but unshares it on the > + source), so we make it a backing child of a qcow2 node. > + Unfortunately, we cannot add backing files to Quorum nodes > + (because of an op blocker), so we put another raw node between > + the qcow2 node and common-base. > + ''' > + result = self.vm.qmp('blockdev-add', **{ > + 'driver': 'qcow2', > + 'node-name': 'base-parent', > + 'file': { > + 'driver': 'file', > + 'filename': test_img > + }, > + 'backing': { > + 'driver': 'raw', > + 'file': 'common-base' > + } > + }) > + > + # Add a quorum node with a single child, we will add > + # base-parent to prepare a loop later > + result = self.vm.qmp('blockdev-add', **{ > + 'driver': 'quorum', It would be good to skip test-cases if quorum unsupported, like other test-cases with quorum. > + 'node-name': 'target', > + 'vote-threshold': 1, > + 'children': [ > + { > + 'driver': 'null-co', > + 'read-zeroes': True > + } > + ] > + }) > + self.assert_qmp(result, 'return', {}) It would be nice to comment out current block graph here... > + > + result = self.vm.qmp('blockdev-mirror', job_id='mirror', > + device='source', target='target', sync='full', > + replaces='common-base') > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp('x-blockdev-change', > + parent='target', node='base-parent') > + self.assert_qmp(result, 'return', {}) > + > + ''' and here, like you do in previous test-case. And here it even nicer, as this test-case is more complex. > + This asks for this configuration post-mirror: > + > + source -> target (replaced common-base) -> base-parent > + ^ | > + | v > + +----------------- [raw] > + > + bdrv_replace_node() would not allow such a configuration, but > + we should not pretend we can create it, so the mirror job > + should fail during completion. > + ''' > + > + self.complete_and_wait('mirror', > + completion_error='Operation not permitted') > + > if __name__ == '__main__': > iotests.main(supported_fmts=['qcow2', 'qed'], > supported_protocols=['file']) > diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out > index 877b76fd31..20a8158b99 100644 > --- a/tests/qemu-iotests/041.out > +++ b/tests/qemu-iotests/041.out > @@ -1,5 +1,5 @@ > -.............................................................................................. > +................................................................................................ > ---------------------------------------------------------------------- > -Ran 94 tests > +Ran 96 tests > > OK >
On 26.09.19 17:03, Vladimir Sementsov-Ogievskiy wrote: > 20.09.2019 18:28, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/041 | 152 +++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/041.out | 4 +- >> 2 files changed, 154 insertions(+), 2 deletions(-) >> >> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 >> index e4cc829fe2..6ea4764ae8 100755 >> --- a/tests/qemu-iotests/041 >> +++ b/tests/qemu-iotests/041 >> @@ -1265,6 +1265,158 @@ class TestReplaces(iotests.QMPTestCase): >> >> self.vm.assert_block_path('filter0/file', 'target') >> >> + ''' >> + See what happens when the @sync/@replaces configuration dictates >> + creating a loop. >> + ''' >> + def test_loop(self): >> + qemu_img('create', '-f', iotests.imgfmt, test_img, str(1 * 1024 * 1024)) >> + >> + # Dummy group so we can create a NOP filter >> + result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg0') >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('blockdev-add', **{ >> + 'driver': 'throttle', >> + 'node-name': 'source', >> + 'throttle-group': 'tg0', >> + 'file': { >> + 'driver': iotests.imgfmt, >> + 'node-name': 'filtered', >> + 'file': { >> + 'driver': 'file', >> + 'filename': test_img >> + } >> + } >> + }) >> + self.assert_qmp(result, 'return', {}) >> + >> + # Block graph is now: >> + # source[throttle] --file--> filtered[qcow2] --file--> ... > > or qed, actually Yep. >> + >> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='source', >> + target=target_img, format=iotests.imgfmt, >> + node_name='target', sync='none', >> + replaces='filtered') >> + >> + ''' >> + Block graph before mirror exits would be (ignoring mirror_top): >> + source[throttle] --file--> filtered[qcow2] --file--> ... >> + target[qcow2] --file--> ... >> + >> + Then, because of sync=none and drive-mirror in absolute-paths mode, >> + the source is attached to the target: >> + source[throttle] --file--> filtered[qcow2] --file--> ... >> + ^ > | >> + backing >> + | >> + target[qcow2] --file--> ... >> + >> + Replacing filtered by target would yield: >> + source[throttle] --file--> target[qcow2] --file--> ... >> + ^ | >> + +------- backing --------+ >> + >> + I.e., a loop. bdrv_replace_node() detects this and simply >> + does not let source's file link point to target. However, >> + that means that target cannot really replace source. >> + >> + drive-mirror should detect this and not allow this case. >> + ''' >> + >> + self.assert_qmp(result, 'error/desc', >> + "Replacing 'filtered' by 'target' with this sync " + \ >> + "mode would result in a loop, because the former " + \ >> + "would be a child of the latter's backing file " + \ >> + "('source') after the mirror job") >> + >> + ''' >> + Test what happens when there would be no loop with the pre-mirror >> + configuration, but something changes during the mirror job that asks >> + for a loop to be created during completion. >> + ''' >> + def test_loop_during_mirror(self): >> + qemu_img('create', '-f', iotests.imgfmt, test_img, str(1 * 1024 * 1024)) >> + >> + result = self.vm.qmp('blockdev-add', **{ >> + 'driver': 'null-co', >> + 'node-name': 'common-base', >> + 'read-zeroes': True, > > why do you need read-zeroes? It’s my understanding that we’d better always set it to true. >> + 'size': 1 * 1024 * 1024 >> + }) >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('blockdev-add', **{ >> + 'driver': 'copy-on-read', >> + 'node-name': 'source', >> + 'file': 'common-base' >> + }) >> + self.assert_qmp(result, 'return', {}) > > Hmm, why don't you create them both in one query? No good reason, I think. >> + >> + ''' > > the following is hard to read without some hint like, "We are going to ..." I’ll see what I can come up with. >> + x-blockdev-change can only add children to a quorum node that >> + have no parent yet, so we need an intermediate node between >> + target and common-base that has no parents other than target. >> + We cannot use any parent that would forward the RESIZE >> + permission (because the job takes it, but unshares it on the >> + source), so we make it a backing child of a qcow2 node. >> + Unfortunately, we cannot add backing files to Quorum nodes >> + (because of an op blocker), so we put another raw node between >> + the qcow2 node and common-base. >> + ''' >> + result = self.vm.qmp('blockdev-add', **{ >> + 'driver': 'qcow2', >> + 'node-name': 'base-parent', >> + 'file': { >> + 'driver': 'file', >> + 'filename': test_img >> + }, >> + 'backing': { >> + 'driver': 'raw', >> + 'file': 'common-base' >> + } >> + }) >> + >> + # Add a quorum node with a single child, we will add >> + # base-parent to prepare a loop later >> + result = self.vm.qmp('blockdev-add', **{ >> + 'driver': 'quorum', > > It would be good to skip test-cases if quorum unsupported, like other test-cases > with quorum. Will do. >> + 'node-name': 'target', >> + 'vote-threshold': 1, >> + 'children': [ >> + { >> + 'driver': 'null-co', >> + 'read-zeroes': True >> + } >> + ] >> + }) >> + self.assert_qmp(result, 'return', {}) > > It would be nice to comment out current block graph here... OK. >> + >> + result = self.vm.qmp('blockdev-mirror', job_id='mirror', >> + device='source', target='target', sync='full', >> + replaces='common-base') >> + self.assert_qmp(result, 'return', {}) >> + >> + result = self.vm.qmp('x-blockdev-change', >> + parent='target', node='base-parent') >> + self.assert_qmp(result, 'return', {}) >> + >> + ''' > > and here, like you do in previous test-case. And here it even nicer, as this test-case > is more complex. OK. >> + This asks for this configuration post-mirror: >> + >> + source -> target (replaced common-base) -> base-parent >> + ^ | >> + | v >> + +----------------- [raw] >> + >> + bdrv_replace_node() would not allow such a configuration, but >> + we should not pretend we can create it, so the mirror job >> + should fail during completion. >> + ''' >> + >> + self.complete_and_wait('mirror', >> + completion_error='Operation not permitted') >> + >> if __name__ == '__main__': >> iotests.main(supported_fmts=['qcow2', 'qed'], >> supported_protocols=['file']) >> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out >> index 877b76fd31..20a8158b99 100644 >> --- a/tests/qemu-iotests/041.out >> +++ b/tests/qemu-iotests/041.out >> @@ -1,5 +1,5 @@ >> -.............................................................................................. >> +................................................................................................ >> ---------------------------------------------------------------------- >> -Ran 94 tests >> +Ran 96 tests >> >> OK >> > >
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index e4cc829fe2..6ea4764ae8 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -1265,6 +1265,158 @@ class TestReplaces(iotests.QMPTestCase): self.vm.assert_block_path('filter0/file', 'target') + ''' + See what happens when the @sync/@replaces configuration dictates + creating a loop. + ''' + def test_loop(self): + qemu_img('create', '-f', iotests.imgfmt, test_img, str(1 * 1024 * 1024)) + + # Dummy group so we can create a NOP filter + result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg0') + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('blockdev-add', **{ + 'driver': 'throttle', + 'node-name': 'source', + 'throttle-group': 'tg0', + 'file': { + 'driver': iotests.imgfmt, + 'node-name': 'filtered', + 'file': { + 'driver': 'file', + 'filename': test_img + } + } + }) + self.assert_qmp(result, 'return', {}) + + # Block graph is now: + # source[throttle] --file--> filtered[qcow2] --file--> ... + + result = self.vm.qmp('drive-mirror', job_id='mirror', device='source', + target=target_img, format=iotests.imgfmt, + node_name='target', sync='none', + replaces='filtered') + + ''' + Block graph before mirror exits would be (ignoring mirror_top): + source[throttle] --file--> filtered[qcow2] --file--> ... + target[qcow2] --file--> ... + + Then, because of sync=none and drive-mirror in absolute-paths mode, + the source is attached to the target: + source[throttle] --file--> filtered[qcow2] --file--> ... + ^ + backing + | + target[qcow2] --file--> ... + + Replacing filtered by target would yield: + source[throttle] --file--> target[qcow2] --file--> ... + ^ | + +------- backing --------+ + + I.e., a loop. bdrv_replace_node() detects this and simply + does not let source's file link point to target. However, + that means that target cannot really replace source. + + drive-mirror should detect this and not allow this case. + ''' + + self.assert_qmp(result, 'error/desc', + "Replacing 'filtered' by 'target' with this sync " + \ + "mode would result in a loop, because the former " + \ + "would be a child of the latter's backing file " + \ + "('source') after the mirror job") + + ''' + Test what happens when there would be no loop with the pre-mirror + configuration, but something changes during the mirror job that asks + for a loop to be created during completion. + ''' + def test_loop_during_mirror(self): + qemu_img('create', '-f', iotests.imgfmt, test_img, str(1 * 1024 * 1024)) + + result = self.vm.qmp('blockdev-add', **{ + 'driver': 'null-co', + 'node-name': 'common-base', + 'read-zeroes': True, + 'size': 1 * 1024 * 1024 + }) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('blockdev-add', **{ + 'driver': 'copy-on-read', + 'node-name': 'source', + 'file': 'common-base' + }) + self.assert_qmp(result, 'return', {}) + + ''' + x-blockdev-change can only add children to a quorum node that + have no parent yet, so we need an intermediate node between + target and common-base that has no parents other than target. + We cannot use any parent that would forward the RESIZE + permission (because the job takes it, but unshares it on the + source), so we make it a backing child of a qcow2 node. + Unfortunately, we cannot add backing files to Quorum nodes + (because of an op blocker), so we put another raw node between + the qcow2 node and common-base. + ''' + result = self.vm.qmp('blockdev-add', **{ + 'driver': 'qcow2', + 'node-name': 'base-parent', + 'file': { + 'driver': 'file', + 'filename': test_img + }, + 'backing': { + 'driver': 'raw', + 'file': 'common-base' + } + }) + + # Add a quorum node with a single child, we will add + # base-parent to prepare a loop later + result = self.vm.qmp('blockdev-add', **{ + 'driver': 'quorum', + 'node-name': 'target', + 'vote-threshold': 1, + 'children': [ + { + 'driver': 'null-co', + 'read-zeroes': True + } + ] + }) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('blockdev-mirror', job_id='mirror', + device='source', target='target', sync='full', + replaces='common-base') + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('x-blockdev-change', + parent='target', node='base-parent') + self.assert_qmp(result, 'return', {}) + + ''' + This asks for this configuration post-mirror: + + source -> target (replaced common-base) -> base-parent + ^ | + | v + +----------------- [raw] + + bdrv_replace_node() would not allow such a configuration, but + we should not pretend we can create it, so the mirror job + should fail during completion. + ''' + + self.complete_and_wait('mirror', + completion_error='Operation not permitted') + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2', 'qed'], supported_protocols=['file']) diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out index 877b76fd31..20a8158b99 100644 --- a/tests/qemu-iotests/041.out +++ b/tests/qemu-iotests/041.out @@ -1,5 +1,5 @@ -.............................................................................................. +................................................................................................ ---------------------------------------------------------------------- -Ran 94 tests +Ran 96 tests OK
Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/041 | 152 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/041.out | 4 +- 2 files changed, 154 insertions(+), 2 deletions(-)