30.06.2018 00:04, John Snow wrote:

On 06/29/2018 01:58 PM, Eric Blake wrote:
On 06/29/2018 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
From: Fam Zheng <f...@redhat.com>

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 <f...@redhat.com>
[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 <vsement...@virtuozzo.com>
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?

Yup...

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

Mine was indeed inspired by Fam's version, but I opted to rewrite it
instead of continue with the python unit tests approach. This is why I
lost the commit message, existing copyright text, etc.

I can re-add him as author credit to my version, or at least mention in
the commit message that it was based on a proposed test that he wrote.

I'm sorry if that was a faux pas.

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.

And before we do that, getting to the bottom of patch 2/3 in this series
is in order.

+++ 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.

My version started with a blank test which was based on an unmerged
test, so it didn't have anything useful to reference as a "source." If
we keep the python unit test style here, keeping this line is fine.

+
+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:'

Because this version will use the cleanup infrastructure as part of the
unit tests, unlike the python scripting approach which needs to manage
its own cleanup.

+        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.

I felt like the hex aided clarity to show which cluster we were
targeting. I'm not very good at reading raw byte values.

+
+        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.

Yes, I think the "python script" style is superior because you can get
debug printfs from a test build of QEMU into the diff output instead of
having to edit the iotests infrastructure to stop deleting the logs so
you can read them.

infrastructure for python unittest based tests definitely should be improved..


+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

I would prefer to use "my" version as a base, but we can modify it to
use the fleecing filter if that winds up being requisite. Your patch 1/3
is better, though.

Ok, no objections

--
Best regards,
Vladimir

Reply via email to