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

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

[Vladimir Sementsov-Ogievskiy]

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)
+
+iotests.qemu_img('bitmap', '--add', base ,'bitmap0')
+# 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"
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
new file mode 100644
index 0000000000..c28af82c75
--- /dev/null
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
@@ -0,0 +1,6 @@
+Trying to remove persistent bitmap from r-o base node, should fail:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "base"}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'bitmap0' is readonly and cannot be modified"}}
+Remove persistent bitmap from base node reopened to RW:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "base"}}
+{"return": {}}
-- 
2.29.2

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

[Max Reitz]

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.

> +# 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')

?

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

Max

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