Re: [Qemu-devel] [PATCH v12 14/17] iotests: add simple incremental backup case

[John Snow <jsnow@redhat.com>]

On 02/11/2015 04:40 PM, Max Reitz wrote:
> On 2015-02-09 at 20:35, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/112        | 120
>> +++++++++++++++++++++++++++++++++++++++++-
>>   tests/qemu-iotests/112.out    |   4 +-
>>   tests/qemu-iotests/iotests.py |  18 ++++---
>>   3 files changed, 133 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
>> index 7985cd1..31431ad 100644
>> --- a/tests/qemu-iotests/112
>> +++ b/tests/qemu-iotests/112
>> @@ -22,18 +22,49 @@
>>   import os
>>   import iotests
>> +from iotests import qemu_img, qemu_io, create_image
>
> Is this really necessary? For me, it works without, too; and if it is
> necessary, shouldn't it be part of patch 13?
>

Maybe not? It's baggage from copying 056 as a template.

>>   def io_write_patterns(img, patterns):
>>       for pattern in patterns:
>>           iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
>> +class Bitmap:
>> +    def __init__(self, name, node):
>> +        self.name = name
>> +        self.node = node
>> +        self.pattern = os.path.join(iotests.test_dir,
>> +                                    '%s.backup.%%s.img' % name)
>
> Shouldn't this be %%i? %%s works for me, but I think %%i might look better.
>

It's probably more semantically appropriate, yes.

>> +        self.num = 0
>> +        self.backups = list()
>> +
>> +    def new_target(self, num=None):
>> +        if num is None:
>> +            num = self.num
>> +        self.num = num + 1
>> +        target = self.pattern % num
>
> I hope nobody has a % in the test path...
>

Oh, good point.

> Although it does look nice, maybe just self.base =
> os.path.join(iotests.test_dir, '%s.backup.img.' % name) and target =
> self.base + str(num) is more robust (putting the '.img' at the end is a
> bonus).
>
>> +        self.backups.append(target)
>> +        return target
>> +
>> +    def last_target(self):
>> +        return self.backups[-1]
>> +
>> +    def del_target(self):
>> +        os.remove(self.backups.pop())
>> +        self.num -= 1
>> +
>> +    def __del__(self):
>> +        for backup in self.backups:
>> +            try:
>> +                os.remove(backup)
>> +            except OSError:
>> +                pass
>>   class TestIncrementalBackup(iotests.QMPTestCase):
>>       def setUp(self):
>>           self.bitmaps = list()
>>           self.files = list()
>> -        self.vm = iotests.VM()
>> +        self.vm = iotests.VM().add_args(['-S'])
>
> Isn't -machine accel=qtest (which is specified by default) enough?

Yes, this is actually somewhat vestigial from a standalone version of 
this that would tolerate an arbitrary image. I left it in because I 
thought it was harmless and it served as an illustration about the 
inferred timing of the commands being tested.

That said, it /can/ be removed. Its only purpose is illustrative.

>
>>           self.test_img = os.path.join(iotests.test_dir, 'base.img')
>>           self.full_bak = os.path.join(iotests.test_dir, 'backup.img')
>>           self.foo_img = os.path.join(iotests.test_dir, 'foo.bar')
>> @@ -58,6 +89,93 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>               iotests.qemu_img('create', '-f', fmt, img, size)
>>           self.files.append(img)
>> +
>> +    def create_full_backup(self, drive='drive0'):
>> +        res = self.vm.qmp('drive-backup', device=drive,
>> +                          sync='full', format=iotests.imgfmt,
>> +                          target=self.full_bak)
>> +        self.assert_qmp(res, 'return', {})
>> +        self.wait_until_completed(drive)
>> +        self.check_full_backup()
>> +        self.files.append(self.full_bak)
>> +
>> +
>> +    def check_full_backup(self):
>> +        self.assertTrue(iotests.compare_images(self.test_img,
>> self.full_bak))
>> +
>> +
>> +    def add_bitmap(self, name, node='drive0'):
>> +        bitmap = Bitmap(name, node)
>> +        self.bitmaps.append(bitmap)
>> +        result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node,
>> +                             name=bitmap.name)
>> +        self.assert_qmp(result, 'return', {})
>> +        return bitmap
>> +
>> +
>> +    def create_incremental(self, bitmap=None, num=None,
>> +                           parent=None, parentFormat=None,
>> validate=True):
>> +        if bitmap is None:
>> +            bitmap = self.bitmaps[-1]
>> +
>> +        # If this is the first incremental backup for a bitmap,
>> +        # use the full backup as a backing image. Otherwise, use
>> +        # the last incremental backup.
>> +        if parent is None:
>> +            if bitmap.num is 0:
>
> Why not == 0?
>

Just having a really good time in python.

I think "is" tests instances; it's like a strict equals. == is 
equivalence as you and I are used to it. In this case, it winds up being 
the same because python has some numerical objects cached, and any 
equation that evaluates to 0 evaluates to the /same/ 0.

== is more correct, though, apparently.

>> +                parent = self.full_bak
>> +            else:
>> +                parent = self.bitmaps[-1].last_target()
>
> Is this intentional or should this be bitmap.last_target()? In this
> patch, no caller of create_incremental() gives any parameter, so there's
> no difference.
>

Vestigial from the standalone edition. 'bitmap' should be preferred over 
bitmaps[-1], yes.

>> +
>> +        target = bitmap.new_target(num)
>> +        self.img_create(target, iotests.imgfmt, parent=parent)
>> +
>> +        result = self.vm.qmp('drive-backup', device=bitmap.node,
>> +                             sync='dirty-bitmap', bitmap=bitmap.name,
>> +                             format=iotests.imgfmt, target=target,
>> +                             mode='existing')
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        event = self.wait_until_completed(bitmap.node,
>> check_offset=validate)
>> +        if validate:
>> +            return self.check_incremental(target)
>> +
>> +
>> +    def check_incremental(self, target=None):
>> +        if target is None:
>> +            target = self.bitmaps[-1].last_target()
>> +        self.assertTrue(iotests.compare_images(self.test_img, target))
>> +        return True
>> +
>> +
>> +    def hmp_io_writes(self, drive, patterns):
>> +        for pattern in patterns:
>> +            self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern)
>> +        self.vm.hmp_qemu_io(drive, 'flush')
>> +
>> +
>> +    # Create a bitmap and full backup before VM execution begins,
>> +    # then create a series of incremental backups during execution.
>> +    def test_incremental_simple(self):
>> +        self.create_full_backup()
>> +        self.add_bitmap('bitmap0', 'drive0')
>> +        self.vm.hmp('c')
>
> self.vm.qmp('continue') works, too, but as I said above, I think
> accel=qtest should be enough so you may not need this at all.
>
> (I'm saying this because we all know that QMP > HMP (obviously))
>
>
> So I had some small questions which I think I want to have answers
> before giving an R-b, but the test logic looks good to me.
>
> Max
>

Explanation above covers this; a more human-focused demi-interactive 
version preceded this iotest.

I'll excise it and just use comments to create the same documentation 
effect.

>> +        # Sanity: Create a "hollow" incremental backup
>> +        self.create_incremental()
>> +        # Three writes: One complete overwrite, one new segment,
>> +        # and one partial overlap.
>> +        self.hmp_io_writes('drive0', (('0xab', 0, 512),
>> +                                      ('0xfe', '16M', '256k'),
>> +                                      ('0x64', '32736k', '64k')))
>> +        self.create_incremental()
>> +        # Three more writes, one of each kind, like above
>> +        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
>> +                                      ('0x55', '8M', '352k'),
>> +                                      ('0x78', '15872k', '1M')))
>> +        self.create_incremental()
>> +        return True
>> +
>> +
>>       def test_sync_dirty_bitmap_missing(self):
>>           self.assert_no_active_block_jobs()
>>           self.files.append(self.foo_img)
>> diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
>> index fbc63e6..8d7e996 100644
>> --- a/tests/qemu-iotests/112.out
>> +++ b/tests/qemu-iotests/112.out
>> @@ -1,5 +1,5 @@
>> -..
>> +...
>>   ----------------------------------------------------------------------
>> -Ran 2 tests
>> +Ran 3 tests
>>   OK
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 241b5ee..6bff935 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -88,6 +88,11 @@ class VM(object):
>>                        '-display', 'none', '-vga', 'none']
>>           self._num_drives = 0
>> +    # Add arbitrary arguments to the command-line.
>> +    def add_args(self, arglist):
>> +        self._args = self._args + arglist
>> +        return self
>> +
>>       # This can be used to add an unused monitor instance.
>>       def add_monitor_telnet(self, ip, port):
>>           args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port)
>> @@ -109,23 +114,24 @@ class VM(object):
>>           self._num_drives += 1
>>           return self
>> +    def hmp(self, args):
>> +        return self.qmp('human-monitor-command',
>> +                           command_line=args)
>> +
>>       def pause_drive(self, drive, event=None):
>>           '''Pause drive r/w operations'''
>>           if not event:
>>               self.pause_drive(drive, "read_aio")
>>               self.pause_drive(drive, "write_aio")
>>               return
>> -        self.qmp('human-monitor-command',
>> -                    command_line='qemu-io %s "break %s bp_%s"' %
>> (drive, event, drive))
>> +        self.hmp('qemu-io %s "break %s bp_%s"' % (drive, event, drive))
>>       def resume_drive(self, drive):
>> -        self.qmp('human-monitor-command',
>> -                    command_line='qemu-io %s "remove_break bp_%s"' %
>> (drive, drive))
>> +        self.hmp('qemu-io %s "remove_break bp_%s"' % (drive, drive))
>>       def hmp_qemu_io(self, drive, cmd):
>>           '''Write to a given drive using an HMP command'''
>> -        return self.qmp('human-monitor-command',
>> -                        command_line='qemu-io %s "%s"' % (drive, cmd))
>> +        return self.hmp('qemu-io %s "%s"' % (drive, cmd))
>>       def add_fd(self, fd, fdset, opaque, opts=''):
>>           '''Pass a file descriptor to the VM'''
>