Re: [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Image fleecing test case 222

[Eric Blake]

On 06/29/2018 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Fam Zheng <address@hidden>
>
> This tests the workflow of creating a lightweight point-in-time snapshot
> with blockdev-backup command, and exporting it with built-in NBD server.
>
> It's tested that any post-snapshot writing to the original device
> doesn't change data seen in NBD target.
>
> Signed-off-by: Fam Zheng <address@hidden>
> [vsementsov: add -f iotests.imgfmt to qemu_io, fix target_img to be
> always qcow2, add -r to qemu_io for nbd, add fleecing-filter layer,
> wrap long lines]
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

For my own records: Fam's most recent post was:
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03895.html
[Qemu-devel] [PATCH v20 15/15] qemu-iotests: Image fleecing test case 089

Wow - has it really been FOUR YEARS since we first proposed this?

I'm trying to figure out why that original post was abandoned; it looks 
like:
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04333.html

split that v20 series into stuff that was ready (which made it in back 
then), and stuff that was still undergoing comments (which then stalled 
and got lost in the shuffle until now).

My next question: how does this compare to John's recent patch?  And how 
much of Fam's original work did John borrow (that self.patterns array 
looks pretty common, now that I'm looking at multiple mails - although 
John's version lacks any commit message body):
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08594.html

And given the iterative improvements that have gone into John's version 
(such as checking that the fleece image reads overwritten zeroes just as 
correctly as overwritten data), we definitely need to merge the two 
approaches into one patch.

> +++ b/tests/qemu-iotests/222
> @@ -0,0 +1,112 @@
> +#!/usr/bin/env python
> +#
> +# Tests for image fleecing (point in time snapshot export to NBD)
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#

Interesting copyright line; it matches Fam's original post (and the fact 
that you kept him as author); whereas John's version had the humorous:

+# Copyright (C) 2018 Red Hat, Inc.
+# John helped, too.

> +# Based on 055.

Is this information still useful? John dropped it.

> +
> +test_img = os.path.join(iotests.test_dir, 'test.img')
> +target_img = os.path.join(iotests.test_dir, 'target.img')
> +nbd_sock = os.path.join(iotests.test_dir, 'nbd.sock')
> +
> +class TestImageFleecing(iotests.QMPTestCase):
> +    image_len = 64 * 1024 * 1024 # MB
> +
> +    def setUp(self):
> +        # Write data to the image so we can compare later
> +        qemu_img('create', '-f', iotests.imgfmt, test_img,
> +                 str(TestImageFleecing.image_len))

Difference in style between nested methods of a class, vs. directly 
using 'with iotests.FilePath(..., iotests.VM() as vm:'

> +        self.patterns = [
> +                ("0x5d", "0", "64k"),
> +                ("0xd5", "1M", "64k"),
> +                ("0xdc", "32M", "64k"),
> +                ("0xdc", "67043328", "64k")]

John spelled it:
 ("0xcd", "0x3ff0000", "64k"]  # 64M - 64K

The 0xdc vs. 0xcd doesn't matter all that much, but having distinct 
patterns in distinct ranges is kind of nice if we have to later inspect 
a file.

> +
> +        for p in self.patterns:
> +            qemu_io('-f', iotests.imgfmt, '-c', 'write -P%s %s %s' % p,
> +                    test_img)
> +
> +        qemu_img('create', '-f', 'qcow2', target_img,
> +                 str(TestImageFleecing.image_len))
> +
> +        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm.launch()
> +
> +        self.overwrite_patterns = [
> +                ("0xa0", "0", "64k"),
> +                ("0x0a", "1M", "64k"),
> +                ("0x55", "32M", "64k"),
> +                ("0x56", "67043328", "64k")]
> +
> +        self.nbd_uri = "nbd+unix:///drive1?socket=%s" % nbd_sock
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(test_img)
> +        os.remove(target_img)

I'm not seeing anything that stops the NBD server; John's version added 
that at my insistence.

> +
> +    def verify_patterns(self):
> +        for p in self.patterns:
> +            self.assertEqual(
> +                    -1,
> +                    qemu_io(self.nbd_uri, '-r', '-c', 'read -P%s %s %s' % p)
> +                        .find("verification failed"),
> +                    "Failed to verify pattern: %s %s %s" % p)
> +
> +    def test_image_fleecing(self):
> +        result = self.vm.qmp("blockdev-add", **{
> +            "driver": "fleecing-filter",
> +            "node-name": "drive1",
> +            "file": {
> +                "driver": "qcow2",
> +                "file": {
> +                    "driver": "file",
> +                    "filename": target_img,
> +                    },
> +                "backing": "drive0",
> +            }
> +            })
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp(
> +                "nbd-server-start",
> +                **{"addr": { "type": "unix", "data": { "path": nbd_sock } } })
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp("blockdev-backup", device="drive0",
> +                             target="drive1", sync="none")
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp("nbd-server-add", device="drive1")
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.verify_patterns()
> +
> +        for p in self.overwrite_patterns:
> +            self.vm.hmp_qemu_io("drive0", "write -P%s %s %s" % p)
> +
> +        self.verify_patterns()
> +
> +        self.cancel_and_wait(resume=True)
> +        self.assert_no_active_block_jobs()
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=['raw', 'qcow2'])
> diff --git a/tests/qemu-iotests/222.out b/tests/qemu-iotests/222.out
> new file mode 100644
> index 0000000000..ae1213e6f8
> --- /dev/null
> +++ b/tests/qemu-iotests/222.out
> @@ -0,0 +1,5 @@
> +.
> +----------------------------------------------------------------------
> +Ran 1 tests
> +

I also much prefer the output style that resulted in John's version.

> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index eea75819d2..8019a9f721 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -220,3 +220,4 @@
>   218 rw auto quick
>   219 rw auto
>   221 rw auto quick
> +222 rw auto quick

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org