Re: [PATCH v5 16/16] iotests/image-fleecing: test push backup with fleecing

[Vladimir Sementsov-Ogievskiy]

03.03.2022 13:58, Hanna Reitz wrote:
> On 28.02.22 12:39, Vladimir Sementsov-Ogievskiy wrote:
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/tests/image-fleecing     | 120 ++++++++++++++------
> >   tests/qemu-iotests/tests/image-fleecing.out |  63 ++++++++++
> >   2 files changed, 151 insertions(+), 32 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/tests/image-fleecing 
> > b/tests/qemu-iotests/tests/image-fleecing
> > index 33995612be..89c79af698 100755
> > --- a/tests/qemu-iotests/tests/image-fleecing
> > +++ b/tests/qemu-iotests/tests/image-fleecing
>
> [...]
>
> > @@ -170,6 +196,20 @@ def do_test(use_cbw, use_snapshot_access_filter, 
> > base_img_path,
> >           log(cmd)
> >           log(vm.hmp_qemu_io(qom_path, cmd, qdev=True))
> > +    if push_backup:
> > +        # Check that previous operations were done during backup, not after
> > +        result = vm.qmp('query-block-jobs')
> > +        if len(result['return']) != 1:
> > +            log('Backup finished too fast, COW is not tested')
>
> I don’t understand why this log is here, its message sounds like “case not 
> run”, but first this logged message will make the whole test fail...

This log means that test doesn't test what it should. If that happens, we'll 
need to adjust disk size, backup speed, or something like this. I hope, that 
will not happen, at least it works for me )

> > +
> > +        result = vm.qmp('block-job-set-speed', device='push-backup', speed=0)
> > +        assert result == {'return': {}}
>
> ...and then this will fail, too.
>
> Either this is a hard failure, then the log shouldn’t include “COW is not 
> tested” (because it is tested, and the case has failed); or it’s a casenotrun, 
> and then nothing should be logged (the message should be appended to 
> .casenotrun), and the block-job-set-speed call and waiting for 
> BLOCK_JOB_COMPLETED should only be done when the job is still in the job list.

OK, I understand. What about this:

# Check that backup is not finished yet. If it is, it's possible that backup
# finished even before guest write, and we didn't actually test
# copy-before-write operation. If this happen, we'll need to adjust storage
# size or backup speed or something like this.
assert len(result['return'] == 1

> > +
> > +        log(vm.event_wait(name='BLOCK_JOB_COMPLETED',
> > +                          match={'data': {'device': 'push-backup'}}),
> > +                          filters=[iotests.filter_qmp_event])
> > +        log(vm.qmp('blockdev-del', node_name='target'))
> > +
> >       log('')
> >       log('--- Verifying Data ---')
> >       log('')
> > @@ -177,15 +217,19 @@ def do_test(use_cbw, use_snapshot_access_filter, 
> > base_img_path,
> >       for p in patterns + zeroes:
> >           cmd = 'read -P%s %s %s' % p
> >           log(cmd)
> > -        out, ret = qemu_io_pipe_and_status('-r', '-f', 'raw', '-c', cmd, 
> > nbd_uri)
> > -        if ret != 0:
> > -            print(out)
> > +        if push_backup:
> > +            assert qemu_io_silent('-r', '-c', cmd, target_img_path) == 0
> > +        else:
> > +            out, ret = qemu_io_pipe_and_status('-r', '-f', 'raw', '-c', cmd, 
> > nbd_uri)
> > +            if ret != 0:
> > +                print(out)
>
> The existing principle of “print qemu-io’s output on error” seemed perfectly 
> fine to me.  Why not continue using it?
>
> (e.g. like
>
> args = ['-r', '-c', cmd]
> if push_backup:
>      args += [target_img_path]
> else:
>      args += ['-f', 'raw', nbd_uri]
> out, ret = qemu_io_pipe_and_status(*args)
>
> )

I don't remember why did I changed it. Your variant seems good.

> >       log('')
> >       log('--- Cleanup ---')
> >       log('')
> > -    log(vm.qmp('nbd-server-stop'))
> > +    if not push_backup:
> > +        log(vm.qmp('nbd-server-stop'))
> >       if use_cbw:
> >           if use_snapshot_access_filter:
> > +read -P0xcd 0x3ff0000 64k
> > +
> > +Done


--
Best regards,
Vladimir