[v2,4/4] iotests: Mirror with different source/target size
diff mbox series

Message ID 20200511135825.219437-5-kwolf@redhat.com
State New
Headers show
Series
  • mirror: Make sure that source and target size match
Related show

Commit Message

Kevin Wolf May 11, 2020, 1:58 p.m. UTC
This tests that the mirror job catches situations where the target node
has a different size than the source node. It must also forbid resize
operations when the job is already running.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200507145228.323412-4-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/041     | 45 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 47 insertions(+), 2 deletions(-)

Comments

Max Reitz May 11, 2020, 3:42 p.m. UTC | #1
On 11.05.20 15:58, Kevin Wolf wrote:
> This tests that the mirror job catches situations where the target node
> has a different size than the source node. It must also forbid resize
> operations when the job is already running.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20200507145228.323412-4-kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/041     | 45 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  2 files changed, 47 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>
Vladimir Sementsov-Ogievskiy May 13, 2020, 11:17 a.m. UTC | #2
11.05.2020 16:58, Kevin Wolf wrote:
> This tests that the mirror job catches situations where the target node
> has a different size than the source node. It must also forbid resize
> operations when the job is already running.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20200507145228.323412-4-kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/041     | 45 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/041.out |  4 ++--
>   2 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 1812dd8479..601c756117 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -240,6 +240,49 @@ class TestSingleBlockdev(TestSingleDrive):


Hmm, probably resize tests would be good in the TestSingleDrive, to cover drive-mirror too. Still, there shouldn't be any difference.. So, I don't think it worth doing.

>                                target=self.qmp_target)
>           self.assert_qmp(result, 'error/class', 'GenericError')
>   
> +    def do_test_resize(self, device, node):
> +        def pre_finalize():

[..]

> +    def do_test_target_size(self, size):
> +        result = self.vm.qmp('block_resize', node_name=self.qmp_target,
> +                             size=size)
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp(self.qmp_cmd, job_id='job0',
> +                             device='drive0', sync='full', auto_dismiss=False,
> +                             target=self.qmp_target)
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.run_job('job0')
> +        self.assertEqual(result, 'Source and target image have different sizes')

Hmm, and this proves, that we are not very good with handling early errors. A lot better would be to fail earlier, on blockdev-mirror command. And, as shown by your previous series, backup works better in this case. But again, it's for another series.
Kevin Wolf May 13, 2020, 2:21 p.m. UTC | #3
Am 13.05.2020 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 11.05.2020 16:58, Kevin Wolf wrote:
> > This tests that the mirror job catches situations where the target node
> > has a different size than the source node. It must also forbid resize
> > operations when the job is already running.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Message-Id: <20200507145228.323412-4-kwolf@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> > ---
> >   tests/qemu-iotests/041     | 45 ++++++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/041.out |  4 ++--
> >   2 files changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> > index 1812dd8479..601c756117 100755
> > --- a/tests/qemu-iotests/041
> > +++ b/tests/qemu-iotests/041
> > @@ -240,6 +240,49 @@ class TestSingleBlockdev(TestSingleDrive):
> 
> 
> Hmm, probably resize tests would be good in the TestSingleDrive, to
> cover drive-mirror too. Still, there shouldn't be any difference.. So,
> I don't think it worth doing.

At first, I tried to do a single implementation that works for both, but
it wasn't easily possible. I'm not sure any more if it was the only
problem, but at least drive-mirror targets don't have a known node-name,
so we would have to guess it from query-named-block-nodes. Not
impossible, but maybe not worth it...

> >                                target=self.qmp_target)
> >           self.assert_qmp(result, 'error/class', 'GenericError')
> > +    def do_test_resize(self, device, node):
> > +        def pre_finalize():
> 
> [..]
> 
> > +    def do_test_target_size(self, size):
> > +        result = self.vm.qmp('block_resize', node_name=self.qmp_target,
> > +                             size=size)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        result = self.vm.qmp(self.qmp_cmd, job_id='job0',
> > +                             device='drive0', sync='full', auto_dismiss=False,
> > +                             target=self.qmp_target)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        result = self.vm.run_job('job0')
> > +        self.assertEqual(result, 'Source and target image have different sizes')
> 
> Hmm, and this proves, that we are not very good with handling early
> errors. A lot better would be to fail earlier, on blockdev-mirror
> command. And, as shown by your previous series, backup works better in
> this case. But again, it's for another series.

At least it shows that errp does make it to the client. :-)

I'm not sure which way is currently better. QMP handlers are still
synchronous code that blocks the rest of QEMU, so doing things only
inside the coroutine does have some benefits. Maybe with coroutine QMP
handlers this will become less of a concern, though.

Kevin

Patch
diff mbox series

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 1812dd8479..601c756117 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -240,6 +240,49 @@  class TestSingleBlockdev(TestSingleDrive):
                              target=self.qmp_target)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
+    def do_test_resize(self, device, node):
+        def pre_finalize():
+            if device:
+                result = self.vm.qmp('block_resize', device=device, size=65536)
+                self.assert_qmp(result, 'error/class', 'GenericError')
+
+            result = self.vm.qmp('block_resize', node_name=node, size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp(self.qmp_cmd, job_id='job0', device='drive0',
+                             sync='full', target=self.qmp_target,
+                             auto_finalize=False, auto_dismiss=False)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.run_job('job0', auto_finalize=False,
+                                 pre_finalize=pre_finalize)
+        self.assertEqual(result, None)
+
+    def test_source_resize(self):
+        self.do_test_resize('drive0', 'top')
+
+    def test_target_resize(self):
+        self.do_test_resize(None, self.qmp_target)
+
+    def do_test_target_size(self, size):
+        result = self.vm.qmp('block_resize', node_name=self.qmp_target,
+                             size=size)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp(self.qmp_cmd, job_id='job0',
+                             device='drive0', sync='full', auto_dismiss=False,
+                             target=self.qmp_target)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.run_job('job0')
+        self.assertEqual(result, 'Source and target image have different sizes')
+
+    def test_small_target(self):
+        self.do_test_target_size(self.image_len // 2)
+
+    def test_large_target(self):
+        self.do_test_target_size(self.image_len * 2)
+
     test_large_cluster = None
     test_image_not_found = None
     test_small_buffer2 = None
@@ -251,6 +294,8 @@  class TestSingleDriveZeroLength(TestSingleDrive):
 
 class TestSingleBlockdevZeroLength(TestSingleBlockdev):
     image_len = 0
+    test_small_target = None
+    test_large_target = None
 
 class TestSingleDriveUnalignedLength(TestSingleDrive):
     image_len = 1025 * 1024
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 877b76fd31..53abe11d73 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@ 
-..............................................................................................
+........................................................................................................
 ----------------------------------------------------------------------
-Ran 94 tests
+Ran 104 tests
 
 OK