[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 11/12] iotests: add test 257 for bitmap-mode bac
From: |
John Snow |
Subject: |
Re: [Qemu-block] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups |
Date: |
Thu, 20 Jun 2019 15:08:34 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
On 6/20/19 2:35 PM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> 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.
- Re: [Qemu-block] [PATCH 08/12] iotests: add testing shim for script-style python tests, (continued)
[Qemu-block] [PATCH 05/12] hbitmap: enable merging across granularities, John Snow, 2019/06/19
[Qemu-block] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups, John Snow, 2019/06/19
[Qemu-block] [PATCH 03/12] block/backup: add 'never' policy to bitmap sync mode, John Snow, 2019/06/19