Re: [PATCH 1/2] tests: add migrate-during-backup

[Vladimir Sementsov-Ogievskiy]

10.09.2021 17:18, Hanna Reitz wrote:
> On 10.09.21 13:00, Vladimir Sementsov-Ogievskiy wrote:
> > Add a simple test which tries to run migration during backup.
> > bdrv_inactivate_all() should fail. But due to bug (see next commit with
> > fix) it doesn't, nodes are inactivated and continued backup crashes
> > on assertion "assert(!(bs->open_flags & BDRV_O_INACTIVE));" in
> > bdrv_co_write_req_prepare().
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   .../qemu-iotests/tests/migrate-during-backup  | 87 +++++++++++++++++++
> >   .../tests/migrate-during-backup.out           |  5 ++
> >   2 files changed, 92 insertions(+)
> >   create mode 100755 tests/qemu-iotests/tests/migrate-during-backup
> >   create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out
> >
> > diff --git a/tests/qemu-iotests/tests/migrate-during-backup 
> > b/tests/qemu-iotests/tests/migrate-during-backup
> > new file mode 100755
> > index 0000000000..c3b7f1983d
> > --- /dev/null
> > +++ b/tests/qemu-iotests/tests/migrate-during-backup
> > @@ -0,0 +1,87 @@
> > +#!/usr/bin/env python3
> > +# group: migration disabled
> > +#
> > +# Copyright (c) 2021 Virtuozzo International GmbH
> > +#
> > +# 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 os
> > +import iotests
> > +from iotests import qemu_img_create, qemu_io
> > +
> > +
> > +disk_a = os.path.join(iotests.test_dir, 'disk_a')
> > +disk_b = os.path.join(iotests.test_dir, 'disk_b')
> > +size = '1M'
> > +mig_file = os.path.join(iotests.test_dir, 'mig_file')
> > +mig_cmd = 'exec: cat > ' + mig_file
> > +
> > +
> > +class TestMigrateDuringBackup(iotests.QMPTestCase):
> > +    def tearDown(self):
> > +        self.vm.shutdown()
> > +        os.remove(disk_a)
> > +        os.remove(disk_b)
> > +        os.remove(mig_file)
> > +
> > +    def setUp(self):
> > +        qemu_img_create('-f', iotests.imgfmt, disk_a, size)
> > +        qemu_img_create('-f', iotests.imgfmt, disk_b, size)
> > +        qemu_io('-c', f'write 0 {size}', disk_a)
> > +
> > +        self.vm = iotests.VM().add_drive(disk_a)
> > +        self.vm.launch()
> > +        result = self.vm.qmp('blockdev-add', {
> > +            'node-name': 'target',
> > +            'driver': iotests.imgfmt,
> > +            'file': {
> > +                'driver': 'file',
> > +                'filename': disk_b
> > +            }
> > +        })
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +    def test_migrate(self):
> > +        result = self.vm.qmp('blockdev-backup', device='drive0',
> > +                             target='target', sync='full',
> > +                             speed=1, x_perf={
> > +                                 'max-workers': 1,
> > +                                 'max-chunk': 64 * 1024
> > +                             })
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        result = self.vm.qmp('job-pause', id='drive0')
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        result = self.vm.qmp('migrate-set-capabilities',
> > +                             capabilities=[{'capability': 'events',
> > +                                            'state': True}])
> > +        self.assert_qmp(result, 'return', {})
> > +        result = self.vm.qmp('migrate', uri=mig_cmd)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        self.vm.events_wait((('MIGRATION', {'data': {'status': 'completed'}}),
> > +                             ('MIGRATION', {'data': {'status': 'failed'}})))
>
> So the migration failing is the result we expect here, right? Perhaps we should 
> then have a loop that waits for MIGRATION events, and breaks on both 
> status=completed and status=failed, but logs an error if the migration 
> completes unexpectedly.
>
> While I’ll give a
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
> either way, I’d like to know your opinion on this still.

If migration finishes with "completed" backup will crash (current behavior).
So, if we just check that nothing crashes, we are OK with both completed and 
failed results.

If think more about that all...

Actually, nothing wrong if both migration and backup succeeded. It becomes 
possible if we don't inactivate backup target node. And actually we don't need 
that for migration.

On the other hand, in Qemu we are generaly OK with reading from inactivated node. But 
that's wrong: if I understand correctly, "inactive" means that file may be 
modified by external program (for example, by Qemu process on target for shared 
migration). In this perspective, we can't do migration during backup.

On one more hand, for non-shared migration it's absolutely possible to support 
migration during backup: you just need not stop source immediately after 
migration but wait for backup completion.


So, just to be more concrete, I can suggest to amend the following


diff --git a/tests/qemu-iotests/tests/migrate-during-backup 
b/tests/qemu-iotests/tests/migrate-during-backup
index c3b7f1983d..9bb7ebed3a 100755
--- a/tests/qemu-iotests/tests/migrate-during-backup
+++ b/tests/qemu-iotests/tests/migrate-during-backup
@@ -72,8 +72,13 @@ class TestMigrateDuringBackup(iotests.QMPTestCase):
         result = self.vm.qmp('migrate', uri=mig_cmd)
         self.assert_qmp(result, 'return', {})
 
-        self.vm.events_wait((('MIGRATION', {'data': {'status': 'completed'}}),
-                             ('MIGRATION', {'data': {'status': 'failed'}})))
+        e = self.vm.events_wait((('MIGRATION',
+                                  {'data': {'status': 'completed'}}),
+                                 ('MIGRATION',
+                                  {'data': {'status': 'failed'}})))
+
+        # Don't assert that e is 'failed' now: this way we'll miss
+        # possible crash when backup continues :)
 
         result = self.vm.qmp('block-job-set-speed', device='drive0',
                              speed=0)
@@ -81,6 +86,11 @@ class TestMigrateDuringBackup(iotests.QMPTestCase):
         result = self.vm.qmp('job-resume', id='drive0')
         self.assert_qmp(result, 'return', {})
 
+        # For future: if something changes so that both migration
+        # and backup pass, let's not miss that moment, as it may
+        # be a bug as well as an improvement.
+        self.assert_qmp(e, 'data/status', 'failed')
+
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'],




--
Best regards,
Vladimir