Re: [PATCH] iotests: add test for removing persistent bitmap from backing file

[Vladimir Sementsov-Ogievskiy]

29.03.2021 15:56, Max Reitz wrote:
> On 17.03.21 17:02, Vladimir Sementsov-Ogievskiy wrote:
> > Just demonstrate one of x-blockdev-reopen usecases. We can't simply
> > remove persistent bitmap from RO node (for example from backing file),
> > as we need to remove it from the image too. So, we should reopen the
> > node first.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   .../tests/remove-bitmap-from-backing          | 68 +++++++++++++++++++
> >   .../tests/remove-bitmap-from-backing.out      |  6 ++
> >   2 files changed, 74 insertions(+)
> >   create mode 100755 tests/qemu-iotests/tests/remove-bitmap-from-backing
> >   create mode 100644 tests/qemu-iotests/tests/remove-bitmap-from-backing.out
> >
> > diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing 
> > b/tests/qemu-iotests/tests/remove-bitmap-from-backing
> > new file mode 100755
> > index 0000000000..121860d035
> > --- /dev/null
> > +++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
> > @@ -0,0 +1,68 @@
> > +#!/usr/bin/env python3
> > +#
> > +# Test removing persistent bitmap from backing
> > +#
> > +# Copyright (c) 2021 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 iotests
> > +from iotests import log, qemu_img_create
> > +
> > +iotests.script_initialize(supported_fmts=['qcow2'])
> > +
> > +top, base = iotests.file_path('top', 'base')
> > +size = '1M'
> > +
> > +qemu_img_create('-f', iotests.imgfmt, base, size)
> > +qemu_img_create('-f', iotests.imgfmt, '-b', base, top, size)
>
> I think qemu-img create nowadays complains when you use -b without -F. Also, 
> I’d prefer an assert around this (i.e. assert qemu_img_create() == 0).
>
> > +
> > +iotests.qemu_img('bitmap', '--add', base ,'bitmap0')
>
> s/ ,/, /
>
> Which makes me notice that 297 doesn’t yet check tests/.  I’ll send a patch.
>
> Also, again, an assert == 0 might be nice.
>
> And then I wonder why you import qemu_img_create from iotests, but not qemu_img 
> and qemu_img_pipe.

Hmm, at least qemu_img is used only once. On the other hand, it's probably 
better to import them all, readability would become better.

> > +# Just assert that our method of checking bitmaps in the image works.
> > +assert 'bitmaps' in iotests.qemu_img_pipe('info', base)
> > +
> > +vm = iotests.VM().add_drive(top, 'backing.node-name=base')
> > +vm.launch()
> > +
> > +log('Trying to remove persistent bitmap from r-o base node, should fail:')
> > +vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
> > +
> > +new_base_opts = {
> > +    'node-name': 'base',
> > +    'driver': 'qcow2',
> > +    'file': {
> > +        'driver': 'file',
> > +        'filename':  base
> > +    },
> > +    'read-only': False
> > +}
> > +
> > +# Don't want to bother with filtering qmp_log for reopen command
> > +result = vm.qmp('x-blockdev-reopen', **new_base_opts)
> > +if result != {'return': {}}:
> > +    log('Failed to reopen: ' + str(result))
> > +
> > +log('Remove persistent bitmap from base node reopened to RW:')
> > +vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
> > +
> > +new_base_opts['read-only'] = True
> > +result = vm.qmp('x-blockdev-reopen', **new_base_opts)
> > +if result != {'return': {}}:
> > +    log('Failed to reopen: ' + str(result))
> > +
> > +vm.shutdown()
> > +
> > +if 'bitmaps' in iotests.qemu_img_pipe('info', base):
> > +    "Hmm, bitmap is still in the base image. That's wrong"
>
> With 297 covering tests/, it complains about this “pointless string statement”. 
>  Shouldn’t this be something like
>
> log('ERROR: Bitmap is still in the base image')

Oops yes:)

> ?
>
> Apart from that more syntax-y stuff, the test looks good to me.

Thanks, I'll resend.

Side notes:

Actually I had to implement a downstream-only argument for 
block-dirty-bitmap-remove command, that allows to temporary reopen the node, 
because:

1. in Rhel7 based downstream, we don't have x-blockdev-reopen at all
2. in Rhel8 based downstream, we are still in pre-blockdev era, and 
x-blockdev-reopen doesn't work as expected (it doesn't want to work with 
generated node-names)

And I have a request of doing it in a qmp transaction.. It's very nice to do 
complicated bitmap manipulations in transactions, because management code 
(libvirt) becomes a lot simpler..
Probably I should publish my patch.

On the one hand, it seems that, I should not: upstream way is using 
blockdev-reopen.

Still, transaction support seems simpler to implement for temporary reopening to RW than 
for the whole blockdev-reopen.. Or may be not? blockdev-reopen is already 
transaction-backed, may be we could support qmp transactions. Anyway, my "update 
permissions update" series should be merged first..

Also, working with this all by hand, I found it not-comfortable to specify all 
blockdev-options to just reopen to RW.. May be for libvirt it's not a problem 
if it tracks all options in the own node graph model.


--
Best regards,
Vladimir