Re: [PATCH v2 15/15] iotests: Add (NBD-based) tests for inactive nodes

[Kevin Wolf]

Am 03.02.2025 um 20:49 hat Eric Blake geschrieben:
> On Fri, Jan 31, 2025 at 10:50:51AM +0100, Kevin Wolf wrote:
> > This tests different types of operations on inactive block nodes
> > (including graph changes, block jobs and NBD exports) to make sure that
> > users manually activating and inactivating nodes doesn't break things.
> > 
> > Support for inactive nodes in other export types will have to come with
> > separate test cases because they have different dependencies like blkio
> > or root permissions and we don't want to disable this basic test when
> > they are not fulfilled.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  tests/qemu-iotests/iotests.py |   4 +
> >  tests/qemu-iotests/tests/inactive-node-nbd| 303 ++
> >  .../qemu-iotests/tests/inactive-node-nbd.out  | 239 ++
> >  3 files changed, 546 insertions(+)
> >  create mode 100755 tests/qemu-iotests/tests/inactive-node-nbd
> >  create mode 100644 tests/qemu-iotests/tests/inactive-node-nbd.out
> > 
> 
> > +iotests.log('\nMirror from active source to inactive target')
> > +
> > +iotests.log('disk-fmt active: %s' % node_is_active(vm, 'disk-fmt'))
> > +iotests.log('snap-fmt active: %s' % node_is_active(vm, 'snap-fmt'))
> > +iotests.log('snap2-fmt active: %s' % node_is_active(vm, 'snap2-fmt'))
> > +iotests.log('target-fmt active: %s' % node_is_active(vm, 'target-fmt'))
> > +
> > +# Activating snap2-fmt recursively activates the whole backing chain
> > +vm.qmp_log('blockdev-set-active', node_name='snap2-fmt', active=True)
> > +vm.qmp_log('blockdev-set-active', node_name='target-fmt', active=False)
> 
> Here, you have "Activating ... recursively activates"...
> 
> > +
> > +iotests.log('disk-fmt active: %s' % node_is_active(vm, 'disk-fmt'))
> > +iotests.log('snap-fmt active: %s' % node_is_active(vm, 'snap-fmt'))
> > +iotests.log('snap2-fmt active: %s' % node_is_active(vm, 'snap2-fmt'))
> > +iotests.log('target-fmt active: %s' % node_is_active(vm, 'target-fmt'))
> > +
> > +vm.qmp_log('blockdev-mirror', job_id='job0', device='snap2-fmt',
> > +   target='target-fmt', sync='full',
> > +   filters=[iotests.filter_qmp_generated_node_ids])
> > +
> > +iotests.log('\nBackup from active source to inactive target')
> > +
> > +vm.qmp_log('blockdev-backup', job_id='job0', device='snap2-fmt',
> > +   target='target-fmt', sync='full',
> > +   filters=[iotests.filter_qmp_generated_node_ids])
> > +
> > +iotests.log('\nBackup from inactive source to active target')
> > +
> > +# Activating snap2-fmt recursively inactivates the whole backing chain
> > +vm.qmp_log('blockdev-set-active', node_name='snap2-fmt', active=False)
> > +vm.qmp_log('blockdev-set-active', node_name='target-fmt', active=True)
> 
> ...but here, "Activating ... recursively inactivates".  Is one of
> these statements wrong?

Yes, looks like I updated only half of the comment after copying it. As
you can see in the line after the comment, it sets active=False for
snap2-fmt, so the comment should say "Inactivating ... recursively
inactivates".

Kevin

> Overall a nice barrage of tests, and I can see how adding this many
> tests caused your v2 to fix some bugs that it discovered in v1.
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org
> 

Re: [PATCH v2 15/15] iotests: Add (NBD-based) tests for inactive nodes

[Eric Blake]

On Fri, Jan 31, 2025 at 10:50:51AM +0100, Kevin Wolf wrote:
> This tests different types of operations on inactive block nodes
> (including graph changes, block jobs and NBD exports) to make sure that
> users manually activating and inactivating nodes doesn't break things.
> 
> Support for inactive nodes in other export types will have to come with
> separate test cases because they have different dependencies like blkio
> or root permissions and we don't want to disable this basic test when
> they are not fulfilled.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/iotests.py |   4 +
>  tests/qemu-iotests/tests/inactive-node-nbd| 303 ++
>  .../qemu-iotests/tests/inactive-node-nbd.out  | 239 ++
>  3 files changed, 546 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/inactive-node-nbd
>  create mode 100644 tests/qemu-iotests/tests/inactive-node-nbd.out
> 

> +iotests.log('\nMirror from active source to inactive target')
> +
> +iotests.log('disk-fmt active: %s' % node_is_active(vm, 'disk-fmt'))
> +iotests.log('snap-fmt active: %s' % node_is_active(vm, 'snap-fmt'))
> +iotests.log('snap2-fmt active: %s' % node_is_active(vm, 'snap2-fmt'))
> +iotests.log('target-fmt active: %s' % node_is_active(vm, 'target-fmt'))
> +
> +# Activating snap2-fmt recursively activates the whole backing chain
> +vm.qmp_log('blockdev-set-active', node_name='snap2-fmt', active=True)
> +vm.qmp_log('blockdev-set-active', node_name='target-fmt', active=False)

Here, you have "Activating ... recursively activates"...

> +
> +iotests.log('disk-fmt active: %s' % node_is_active(vm, 'disk-fmt'))
> +iotests.log('snap-fmt active: %s' % node_is_active(vm, 'snap-fmt'))
> +iotests.log('snap2-fmt active: %s' % node_is_active(vm, 'snap2-fmt'))
> +iotests.log('target-fmt active: %s' % node_is_active(vm, 'target-fmt'))
> +
> +vm.qmp_log('blockdev-mirror', job_id='job0', device='snap2-fmt',
> +   target='target-fmt', sync='full',
> +   filters=[iotests.filter_qmp_generated_node_ids])
> +
> +iotests.log('\nBackup from active source to inactive target')
> +
> +vm.qmp_log('blockdev-backup', job_id='job0', device='snap2-fmt',
> +   target='target-fmt', sync='full',
> +   filters=[iotests.filter_qmp_generated_node_ids])
> +
> +iotests.log('\nBackup from inactive source to active target')
> +
> +# Activating snap2-fmt recursively inactivates the whole backing chain
> +vm.qmp_log('blockdev-set-active', node_name='snap2-fmt', active=False)
> +vm.qmp_log('blockdev-set-active', node_name='target-fmt', active=True)

...but here, "Activating ... recursively inactivates".  Is one of
these statements wrong?

Overall a nice barrage of tests, and I can see how adding this many
tests caused your v2 to fix some bugs that it discovered in v1.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[PATCH v2 15/15] iotests: Add (NBD-based) tests for inactive nodes

[Kevin Wolf]

This tests different types of operations on inactive block nodes
(including graph changes, block jobs and NBD exports) to make sure that
users manually activating and inactivating nodes doesn't break things.

Support for inactive nodes in other export types will have to come with
separate test cases because they have different dependencies like blkio
or root permissions and we don't want to disable this basic test when
they are not fulfilled.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py |   4 +
 tests/qemu-iotests/tests/inactive-node-nbd| 303 ++
 .../qemu-iotests/tests/inactive-node-nbd.out  | 239 ++
 3 files changed, 546 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/inactive-node-nbd
 create mode 100644 tests/qemu-iotests/tests/inactive-node-nbd.out

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9c9c908983..7292c8b342 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -913,6 +913,10 @@ def add_incoming(self, addr):
 self._args.append(addr)
 return self
 
+def add_paused(self):
+self._args.append('-S')
+return self
+
 def hmp(self, command_line: str, use_log: bool = False) -> QMPMessage:
 cmd = 'human-monitor-command'
 kwargs: Dict[str, Any] = {'command-line': command_line}
diff --git a/tests/qemu-iotests/tests/inactive-node-nbd 
b/tests/qemu-iotests/tests/inactive-node-nbd
new file mode 100755
index 00..2279f7c1e1
--- /dev/null
+++ b/tests/qemu-iotests/tests/inactive-node-nbd
@@ -0,0 +1,303 @@
+#!/usr/bin/env python3
+# group: rw quick
+#
+# Copyright (C) Red Hat, Inc.
+#
+# 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 .
+#
+# Creator/Owner: Kevin Wolf 
+
+import iotests
+
+from iotests import QemuIoInteractive
+from iotests import filter_qemu_io, filter_qtest, filter_qmp_testfiles
+
+iotests.script_initialize(supported_fmts=['generic'],
+  supported_protocols=['file'],
+  supported_platforms=['linux'])
+
+def get_export(node_name='disk-fmt', allow_inactive=None):
+exp = {
+'id': 'exp0',
+'type': 'nbd',
+'node-name': node_name,
+'writable': True,
+}
+
+if allow_inactive is not None:
+exp['allow-inactive'] = allow_inactive
+
+return exp
+
+def node_is_active(_vm, node_name):
+nodes = _vm.cmd('query-named-block-nodes', flat=True)
+node = next(n for n in nodes if n['node-name'] == node_name)
+return node['active']
+
+with iotests.FilePath('disk.img') as path, \
+ iotests.FilePath('snap.qcow2') as snap_path, \
+ iotests.FilePath('snap2.qcow2') as snap2_path, \
+ iotests.FilePath('target.img') as target_path, \
+ iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock, \
+ iotests.VM() as vm:
+
+img_size = '10M'
+
+iotests.log('Preparing disk...')
+iotests.qemu_img_create('-f', iotests.imgfmt, path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, target_path, img_size)
+
+iotests.qemu_img_create('-f', 'qcow2', '-b', path, '-F', iotests.imgfmt,
+snap_path)
+iotests.qemu_img_create('-f', 'qcow2', '-b', snap_path, '-F', 'qcow2',
+snap2_path)
+
+iotests.log('Launching VM...')
+vm.add_blockdev(f'file,node-name=disk-file,filename={path}')
+vm.add_blockdev(f'{iotests.imgfmt},file=disk-file,node-name=disk-fmt,'
+ 'active=off')
+vm.add_blockdev(f'file,node-name=target-file,filename={target_path}')
+vm.add_blockdev(f'{iotests.imgfmt},file=target-file,node-name=target-fmt')
+vm.add_blockdev(f'file,node-name=snap-file,filename={snap_path}')
+vm.add_blockdev(f'file,node-name=snap2-file,filename={snap2_path}')
+
+# Actually running the VM activates all images
+vm.add_paused()
+
+vm.launch()
+vm.qmp_log('nbd-server-start',
+addr={'type': 'unix', 'data':{'path': nbd_sock}},
+filters=[filter_qmp_testfiles])
+
+iotests.log('\n=== Creating export of inactive node ===')
+
+iotests.log('\nExports activate nodes without allow-inactive')
+iotests.log('disk-fmt active: %s' % node_is_active(vm, 'disk-fmt'))
+vm.qmp_log('block-export-add', **get_export())
+iotests.log('disk-fmt active: %s' % node_is_