[V2,3/3] qemu-iotests: Add 088 new test for drive-mirror-replace.

Tests for drive-mirror-replace whose purpose is to enable quorum file mirroring
and replacement after failure.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 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

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 <benoit@irqsave.net>
> ---
>   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)

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

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

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")

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 <benoit@irqsave.net>
> >---
> >  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")
>