Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

[John Snow <jsnow@redhat.com>]

On 6/20/19 2:35 PM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/257     |  412 +++++++
>>  tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/group   |    1 +
>>  3 files changed, 2612 insertions(+)
>>  create mode 100755 tests/qemu-iotests/257
>>  create mode 100644 tests/qemu-iotests/257.out
> 
> This test is actually quite nicely written.
> 

Thanks!

> I like that I don’t have to read the reference output but can just grep
> for “error”.
> 

Me too!! Actually, doing the math for what to expect and verifying the
output by hand was becoming a major burden, so partially this test
infrastructure was my attempt to procedurally verify that the results I
was seeing were what made sense.

At the end of it, I felt it was nice to keep it in there.

> Only minor notes below.
> 
>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>> new file mode 100755
>> index 0000000000..5f7f388504
>> --- /dev/null
>> +++ b/tests/qemu-iotests/257
> 
> [...]
> 
>> +class PatternGroup:
>> +    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
>> +    def __init__(self, patterns):
>> +        self.patterns = patterns
>> +
>> +    def bits(self, granularity):
>> +        """Calculate the unique bits dirtied by this pattern grouping"""
>> +        res = set()
>> +        for pattern in self.patterns:
>> +            lower = math.floor(pattern.offset / granularity)
>> +            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
>> +            res = res | set(range(lower, upper + 1))
> 
> Why you’d do floor((x - 1) / y) + 1 has confused me quite a while.
> Until I realized that oh yeah, Python’s range() is a right-open
> interval.  I don’t like Python’s range().
> 

It confuses me constantly, but it's really meant to be used for
iterating over lengths. This is somewhat of an abuse of it. I always
test it out in a console first before using it, just in case.

> (Yes, you’re right, this is better to read than just ceil(x / y).
> Because it reminds people like me that range() is weird.)
> 
>> +        return res
>> +
>> +GROUPS = [
>> +    PatternGroup([
>> +        # Batch 0: 4 clusters
>> +        mkpattern('0x49', 0x0000000),
>> +        mkpattern('0x6c', 0x0100000),   # 1M
>> +        mkpattern('0x6f', 0x2000000),   # 32M
>> +        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
>> +    PatternGroup([
>> +        # Batch 1: 6 clusters (3 new)
>> +        mkpattern('0x65', 0x0000000),   # Full overwrite
>> +        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
>> +        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
>> +        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
>> +    PatternGroup([
>> +        # Batch 2: 7 clusters (3 new)
>> +        mkpattern('0x74', 0x0010000),   # Adjacent-right
>> +        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
>> +        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
>> +        mkpattern('0x67', 0x3fe0000,
>> +                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
>> +    PatternGroup([
>> +        # Batch 3: 8 clusters (5 new)
>> +        # Carefully chosen such that nothing re-dirties the one cluster
>> +        # that copies out successfully before failure in Group #1.
>> +        mkpattern('0xaa', 0x0010000,
>> +                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
>> +        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
>> +        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
>> +        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
>> +    ]
> 
> I’d place this four spaces to the left.  But maybe placing it here is
> proper Python indentation, while moving it to the left would be C
> indentation.
> 

Either is fine, I think. In this case it affords us more room for the
commentary on the bit ranges. Maybe it's not necessary, but at least
personally I get woozy looking at the bit patterns.

>> +class Drive:
>> +    """Represents, vaguely, a drive attached to a VM.
>> +    Includes format, graph, and device information."""
>> +
>> +    def __init__(self, path, vm=None):
>> +        self.path = path
>> +        self.vm = vm
>> +        self.fmt = None
>> +        self.size = None
>> +        self.node = None
>> +        self.device = None
>> +
>> +    @property
>> +    def name(self):
>> +        return self.node or self.device
>> +
>> +    def img_create(self, fmt, size):
>> +        self.fmt = fmt
>> +        self.size = size
>> +        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
>> +
>> +    def create_target(self, name, fmt, size):
>> +        basename = os.path.basename(self.path)
>> +        file_node_name = "file_{}".format(basename)
>> +        vm = self.vm
>> +
>> +        log(vm.command('blockdev-create', job_id='bdc-file-job',
>> +                       options={
>> +                           'driver': 'file',
>> +                           'filename': self.path,
>> +                           'size': 0,
>> +                       }))
>> +        vm.run_job('bdc-file-job')
>> +        log(vm.command('blockdev-add', driver='file',
>> +                       node_name=file_node_name, filename=self.path))
>> +
>> +        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
>> +                       options={
>> +                           'driver': fmt,
>> +                           'file': file_node_name,
>> +                           'size': size,
>> +                       }))
>> +        vm.run_job('bdc-fmt-job')
>> +        log(vm.command('blockdev-add', driver=fmt,
>> +                       node_name=name,
>> +                       file=file_node_name))
>> +        self.fmt = fmt
>> +        self.size = size
>> +        self.node = name
> 
> It’s cool that you use blockdev-create here, but would it not have been
> easier to just use self.img_create() + blockdev-add?
> 
> I mean, there’s no point in changing it now, I’m just wondering.
> 

Mostly just because I already wrote this for the last test, and we
already test incremental backups the other way. I figured this would
just be nice for code coverage purposes, and also because using the
blockdev interfaces exclusively does tend to reveal little gotchas
everywhere.

I also kind of want to refactor a lot of my tests and share some of the
common code. I was tinkering with the idea of adding some common objects
to iotests, like "Drive" "Bitmap" and "Backup".

That's why there's a kind of superfluous "Drive" object here.

>> +
>> +def query_bitmaps(vm):
>> +    res = vm.qmp("query-block")
>> +    return {"bitmaps": {device['device'] or device['qdev']:
>> +                        device.get('dirty-bitmaps', []) for
>> +                        device in res['return']}}
> 
> Python’s just not for me if I find this syntax unintuitive and
> confusing, hu?
> 
> [...]
> 

Sorry. It's a very functional-esque way of processing iterables.

I've been doing a lot of FP stuff lately and that skews what I find
readable...

It's a dict comprehension of the form:

{key: value for atom in iterable}

Key here is either the device or qdev name,
the value is the 'dirty-bitmaps' field of the atom, and
res['return'] is the iterable.

Effectively it turns a list of devices into a dict keyed by the device
name, and the only field (if any) was its dirty-bitmap object.

However, in explaining this I do notice I have a bug -- the default
value for the bitmap object ought to be {}, not [].

This is code that should become common testing code too, as I've
re-written it a few times now...

>> +def bitmap_comparison(bitmap, groups=None, want=0):
>> +    """
>> +    Print a nice human-readable message checking that this bitmap has as
>> +    many bits set as we expect it to.
>> +    """
>> +    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
>> +
>> +    if groups:
>> +        want = calculate_bits(groups)
>> +    have = int(bitmap['count'] / bitmap['granularity'])
> 
> Or just bitmap['count'] // bitmap['granularity']?
> 
> [...]
> 

I forget that exists. If you prefer that, OK.

>> +def test_bitmap_sync(bsync_mode, failure=None):
> 
> [...]
> 
>> +        log('--- Preparing image & VM ---\n')
>> +        drive0 = Drive(img_path, vm=vm)
>> +        drive0.img_create(iotests.imgfmt, SIZE)
>> +        vm.add_device('virtio-scsi-pci,id=scsi0')
> 
> Judging from 238, this should be virtio-scsi-ccw on s390-ccw-virtio.
> 
> (This is the reason I cannot give an R-b.)
> 
> [...]
> 

Oh, good catch. Alright.

>> +        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
>> +                   pre_finalize=_callback,
>> +                   cancel=failure == 'simulated')
> 
> I’d prefer “cancel=(failure == 'simulated')”.  (Or spaces around =).
> 
> Also in other places elsewhere that are of the form x=y where y contains
> spaces.
> 
> [...]
> 

OK; I might need to make a pylintrc file to allow that style. Python is
VERY aggressively tuned to omitting parentheses.

(I do actually try to run pylint on my python patches now to make sure I
am targeting SOME kind of style. I realize this is not standardized in
this project.)

>> +def main():
>> +    for bsync_mode in ("never", "conditional", "always"):
>> +        for failure in ("simulated", None):
>> +            test_bitmap_sync(bsync_mode, failure)
>> +
>> +    for bsync_mode in ("never", "conditional", "always"):
>> +        test_bitmap_sync(bsync_mode, "intermediate")
> 
> Why are these separate?  Couldn’t you just iterate over
> ("simulated", None, "intermediate")?
> 
> Max
> 

Haha, oops! That's a holdover from when those two routines weren't
unified. I wrote it like this while I was unifying them to avoid
reordering the test output, which helped me polish the merging of the
routines.

You are right, though, it should be one loop.