Re: [Qemu-devel] [PATCH V2 3/3] qemu-iotests: Add 088 new test for drive-mirror-replace.

[Benoît Canet]

The Thursday 13 Mar 2014 à 22:31:01 (+0100), Max Reitz wrote :
> On 11.03.2014 22:53, Benoît Canet wrote:
> >Tests for drive-mirror-replace whose purpose is to enable quorum file 
> >mirroring
> >and replacement after failure.
> >
> >Signed-off-by: Benoit Canet <address@hidden>
> >---
> >  tests/qemu-iotests/041        |  34 +------
> >  tests/qemu-iotests/088        | 221 
> > ++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/088.out    |   5 +
> >  tests/qemu-iotests/group      |   1 +
> >  tests/qemu-iotests/iotests.py |  33 +++++++
> >  5 files changed, 261 insertions(+), 33 deletions(-)
> >  create mode 100755 tests/qemu-iotests/088
> >  create mode 100644 tests/qemu-iotests/088.out
> >
> >diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> >index ec470b2..10535a6 100755
> >--- a/tests/qemu-iotests/041
> >+++ b/tests/qemu-iotests/041
> >@@ -21,45 +21,13 @@
> >  import time
> >  import os
> >  import iotests
> >-from iotests import qemu_img, qemu_io
> >+from iotests import qemu_img, qemu_io, ImageMirroringTestCase
> >  backing_img = os.path.join(iotests.test_dir, 'backing.img')
> >  target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
> >  test_img = os.path.join(iotests.test_dir, 'test.img')
> >  target_img = os.path.join(iotests.test_dir, 'target.img')
> >-class ImageMirroringTestCase(iotests.QMPTestCase):
> >-    '''Abstract base class for image mirroring test cases'''
> >-
> >-    def wait_ready(self, drive='drive0'):
> >-        '''Wait until a block job BLOCK_JOB_READY event'''
> >-        ready = False
> >-        while not ready:
> >-            for event in self.vm.get_qmp_events(wait=True):
> >-                if event['event'] == 'BLOCK_JOB_READY':
> >-                    self.assert_qmp(event, 'data/type', 'mirror')
> >-                    self.assert_qmp(event, 'data/device', drive)
> >-                    ready = True
> >-
> >-    def wait_ready_and_cancel(self, drive='drive0'):
> >-        self.wait_ready(drive)
> >-        event = self.cancel_and_wait()
> >-        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> >-        self.assert_qmp(event, 'data/type', 'mirror')
> >-        self.assert_qmp(event, 'data/offset', self.image_len)
> >-        self.assert_qmp(event, 'data/len', self.image_len)
> >-
> >-    def complete_and_wait(self, drive='drive0', wait_ready=True):
> >-        '''Complete a block job and wait for it to finish'''
> >-        if wait_ready:
> >-            self.wait_ready()
> >-
> >-        result = self.vm.qmp('block-job-complete', device=drive)
> >-        self.assert_qmp(result, 'return', {})
> >-
> >-        event = self.wait_until_completed()
> >-        self.assert_qmp(event, 'data/type', 'mirror')
> >-
> >  class TestSingleDrive(ImageMirroringTestCase):
> >      image_len = 1 * 1024 * 1024 # MB
> >diff --git a/tests/qemu-iotests/088 b/tests/qemu-iotests/088
> >new file mode 100755
> >index 0000000..326d307
> >--- /dev/null
> >+++ b/tests/qemu-iotests/088
> >@@ -0,0 +1,221 @@
> >+#!/usr/bin/env python
> >+#
> >+# Tests for Quorum image replacement
> >+#
> >+# Copyright (C) 2014 Nodalink, EURL.
> >+#
> >+# based on 041
> >+#
> >+# This program is free software; you can redistribute it and/or modify
> >+# it under the terms of the GNU General Public License as published by
> >+# the Free Software Foundation; either version 2 of the License, or
> >+# (at your option) any later version.
> >+#
> >+# This program is distributed in the hope that it will be useful,
> >+# but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+# GNU General Public License for more details.
> >+#
> >+# You should have received a copy of the GNU General Public License
> >+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >+#
> >+
> >+import subprocess
> >+import time
> >+import os
> >+import iotests
> >+from iotests import qemu_img, qemu_img_args
> >+from iotests import ImageMirroringTestCase
> >+
> >+quorum_img1 = os.path.join(iotests.test_dir, 'quorum1.img')
> >+quorum_img2 = os.path.join(iotests.test_dir, 'quorum2.img')
> >+quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
> >+quorum_backup_img = os.path.join(iotests.test_dir, 'quorum_backup.img')
> >+quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
> >+quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
> >+
> >+class TestRepairQuorum(ImageMirroringTestCase):
> >+    """ This class test quorum file repair using drive-mirror.
> >+        It's mostly a fork of TestSingleDrive. """
> >+    image_len = 1 * 1024 * 1024 # MB
> >+    IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
> >+
> >+    def setUp(self):
> >+        self.vm = iotests.VM()
> >+
> >+        # Add each individual quorum images
> >+        for i in self.IMAGES:
> >+            qemu_img('create', '-f', iotests.imgfmt, i,
> >+                     str(self.image_len))
> >+            # Assign a node name to each quorum image in order to manipulate
> >+            # them
> >+            opts = "node-name=img%i" % self.IMAGES.index(i)
> >+            self.vm = self.vm.add_drive(i, opts)
> >+
> >+        self.vm.launch()
> >+
> >+        #assemble the quorum block device from the individual files
> >+        args = { "options" : { "driver": "quorum", "id": "quorum0",
> >+                 "vote-threshold": 2, "children": [ "img0", "img1", "img2" 
> >] } }
> >+        result = self.vm.qmp("blockdev-add", **args)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+
> >+    def tearDown(self):
> >+        self.vm.shutdown()
> >+        for i in self.IMAGES + [ quorum_repair_img ]:
> >+            # Do a try/except because the test may have deleted some images
> >+            try:
> >+                os.remove(i)
> >+            except OSError:
> >+                pass
> >+
> >+    def test_complete(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             target_reference='img1', new_node_name='img11')
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        event = self.wait_until_completed(drive='quorum0')
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        result = self.vm.qmp('query-named-block-nodes')
> >+        self.assert_qmp(result, 'return[0]/file', quorum_repair_img)
> >+        # TODO: a better test requiring some QEMU infrastructure will be 
> >added
> >+        #       to check that this file is really driven by quorum
> >+        self.vm.shutdown()
> >+        self.assertTrue(iotests.compare_images(quorum_img2, 
> >quorum_repair_img),
> >+                        'target image does not match source after 
> >mirroring')
> 
> Considering you didn't write anything to the images, this assertion
> failing would be rather surprising. Perhaps you could write some
> test data, so the drive-mirror blockjob actually does something? (or
> maybe I'm mistaken and there is indeed some data)

I use sync=full so there is some data.

> 
> >+
> >+    def test_device_not_active(self):
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum12',
> >+                             target_reference='img1', new_node_name='img11')
> >+        self.assert_qmp(result, 'error/class', 'DeviceNotActive')
> 
> Hm, this tests what happens if the block device given as the
> mirroring source does not exist - I could imagine another test for
> what happens if the block device does exist, but there is no
> (mirroring) blockjob running. Without any blockjob running, the
> error path in the newly added code is exactly the same, however
> (find_block_job() fails); and to test what happens if a different
> blockjob type is running ("Can only be used on a drive-mirror block
> job" should be returned) would probably be more difficult.
> 
> Thus, this test alone probably suffices.
> 
> >+        self.vm.shutdown()
> >+
> >+    def test_sync_no_full(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='none',
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             target_reference='img1', new_node_name='img11')
> >+        self.assert_qmp(result, 'error/class', 'GenericError')
> >+
> >+        event = self.cancel_and_wait(drive='quorum0', resume=True)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        self.vm.shutdown()
> >+
> >+    def test_sync_empty_reference(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='none',
> 
> Since this test is not about the sync mode, this should most likely
> be sync='full' again, I guess. The same applies to all the following
> test functions.

true, thanks
> 
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             target_reference='', new_node_name='img11')
> >+        self.assert_qmp(result, 'error/class', 'GenericError')
> >+
> >+        event = self.cancel_and_wait(drive='quorum0', resume=True)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        self.vm.shutdown()
> >+
> >+    def test_sync_no_reference(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='none',
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             new_node_name='img11')
> >+        self.assert_qmp(result, 'error/class', 'GenericError')
> >+
> >+        event = self.cancel_and_wait(drive='quorum0', resume=True)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        self.vm.shutdown()
> >+
> >+    def test_sync_no_node_name(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='none',
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             target_reference='img1')
> >+        self.assert_qmp(result, 'error/class', 'GenericError')
> >+
> >+        event = self.cancel_and_wait(drive='quorum0', resume=True)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        self.vm.shutdown()
> >+
> >+    def test_sync_empty_node_name(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='none',
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             target_reference='img1', new_node_name='')
> >+        self.assert_qmp(result, 'error/class', 'GenericError')
> >+
> >+        event = self.cancel_and_wait(drive='quorum0', resume=True)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        self.vm.shutdown()
> >+
> >+    def test_source_and_dest_equal(self):
> >+        self.assert_no_active_block_jobs()
> >+
> >+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='none',
> >+                             target=quorum_repair_img, 
> >format=iotests.imgfmt)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        self.wait_ready(drive='quorum0')
> >+
> >+        result = self.vm.qmp('drive-mirror-replace', device='quorum0',
> >+                             target_reference='quorum0', 
> >new_node_name='img11')
> >+        self.assert_qmp(result, 'error/class', 'GenericError')
> >+
> >+        event = self.cancel_and_wait(drive='quorum0', resume=True)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >+        self.vm.shutdown()
> 
> In these above test functions, you maybe could even compare the
> error message in order to make sure it's always appropriate. But the
> most important thing is that these are reported as errors, hence I'm
> fine with the GenericError assertions.
> 
> >+
> >+if __name__ == '__main__':
> >+    p1 = subprocess.Popen(qemu_img_args, stdout=subprocess.PIPE)
> >+    p2 = subprocess.Popen(["grep", "quorum"], stdin=p1.stdout, 
> >stdout=subprocess.PIPE)
> >+    p1.stdout.close()  # Allow p1 to receive a SIGsubprocess.PIPE if p2 
> >exits.
> >+    has_quorum = len(p2.communicate()[0]) != 0
> >+    if has_quorum:
> >+        iotests.main(supported_fmts=['qcow2', 'qed'])
> >+    else:
> >+        f = open("088.out", "r")
> >+        print(f.read().strip())
> >+        f.close()
> 
> I think I'd prefer calling iotests.notrun() here, if possible,
> rather than giving the impression this test succeeded when it was in
> fact not run at all.
> 
> I can live with leaving the other issues unchanged, but for this I
> do request a change or at least an explanation before giving a
> reviewed-by. ;-)

Ok

> 
> Max
> 
> >diff --git a/tests/qemu-iotests/088.out b/tests/qemu-iotests/088.out
> >new file mode 100644
> >index 0000000..594c16f
> >--- /dev/null
> >+++ b/tests/qemu-iotests/088.out
> >@@ -0,0 +1,5 @@
> >+........
> >+----------------------------------------------------------------------
> >+Ran 8 tests
> >+
> >+OK
> >diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> >index b96c6bc..e590d09 100644
> >--- a/tests/qemu-iotests/group
> >+++ b/tests/qemu-iotests/group
> >@@ -89,3 +89,4 @@
> >  085 rw auto quick
> >  086 rw auto quick
> >  087 rw auto quick
> >+088 rw auto
> >diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >index e4fa9af..a803943 100644
> >--- a/tests/qemu-iotests/iotests.py
> >+++ b/tests/qemu-iotests/iotests.py
> >@@ -272,6 +272,39 @@ class QMPTestCase(unittest.TestCase):
> >          self.assert_no_active_block_jobs()
> >          return event
> >+# made common here for 041 and 088
> >+class ImageMirroringTestCase(QMPTestCase):
> >+    '''Abstract base class for image mirroring test cases'''
> >+
> >+    def wait_ready(self, drive='drive0'):
> >+        '''Wait until a block job BLOCK_JOB_READY event'''
> >+        ready = False
> >+        while not ready:
> >+            for event in self.vm.get_qmp_events(wait=True):
> >+                if event['event'] == 'BLOCK_JOB_READY':
> >+                    self.assert_qmp(event, 'data/type', 'mirror')
> >+                    self.assert_qmp(event, 'data/device', drive)
> >+                    ready = True
> >+
> >+    def wait_ready_and_cancel(self, drive='drive0'):
> >+        self.wait_ready(drive=drive)
> >+        event = self.cancel_and_wait(drive=drive)
> >+        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+        self.assert_qmp(event, 'data/offset', self.image_len)
> >+        self.assert_qmp(event, 'data/len', self.image_len)
> >+
> >+    def complete_and_wait(self, drive='drive0', wait_ready=True):
> >+        '''Complete a block job and wait for it to finish'''
> >+        if wait_ready:
> >+            self.wait_ready(drive=drive)
> >+
> >+        result = self.vm.qmp('block-job-complete', device=drive)
> >+        self.assert_qmp(result, 'return', {})
> >+
> >+        event = self.wait_until_completed(drive=drive)
> >+        self.assert_qmp(event, 'data/type', 'mirror')
> >+
> >  def notrun(reason):
> >      '''Skip this test suite'''
> >      # Each test in qemu-iotests has a number ("seq")
>