From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Peter Krempa <pkrempa@redhat.com>,
Juan Quintela <quintela@redhat.com>, John Snow <jsnow@redhat.com>,
qemu-devel@nongnu.org,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration
Date: Mon, 20 Jul 2020 21:02:49 +0300 [thread overview]
Message-ID: <4cada895-7634-4b15-d03b-9f0f72995895@virtuozzo.com> (raw)
In-Reply-To: <20200716135303.319502-4-mreitz@redhat.com>
16.07.2020 16:53, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/300 | 511 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/300.out | 5 +
> tests/qemu-iotests/group | 1 +
> 3 files changed, 517 insertions(+)
> create mode 100755 tests/qemu-iotests/300
> create mode 100644 tests/qemu-iotests/300.out
>
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> new file mode 100755
> index 0000000000..68714b7167
> --- /dev/null
> +++ b/tests/qemu-iotests/300
> @@ -0,0 +1,511 @@
> +#!/usr/bin/env python3
> +#
> +# Tests for dirty bitmaps migration with node aliases
copyright?
> +#
> +# 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 random
> +from typing import Dict, List, Optional, Union
> +import iotests
> +import qemu
> +
> +BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
> +
> +assert iotests.sock_dir is not None
> +mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
> +
> +class TestDirtyBitmapMigration(iotests.QMPTestCase):
> + src_node_name: str = ''
> + dst_node_name: str = ''
> + src_bmap_name: str = ''
> + dst_bmap_name: str = ''
Hmm, I hope, typing actually not needed with such an obvious initialization
> +
> + def setUp(self) -> None:
> + self.vm_a = iotests.VM(path_suffix='-a')
> + self.vm_a.add_blockdev(f'node-name={self.src_node_name},' \
> + 'driver=null-co')
Hmm, you don't specify disk size.. How can it work? Aha, it's 1G by default. OK.
> + self.vm_a.launch()
> +
> + self.vm_b = iotests.VM(path_suffix='-b')
> + self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' \
> + 'driver=null-co')
> + self.vm_b.add_incoming(f'unix:{mig_sock}')
> + self.vm_b.launch()
> +
> + result = self.vm_a.qmp('block-dirty-bitmap-add',
> + node=self.src_node_name,
> + name=self.src_bmap_name)
> + self.assert_qmp(result, 'return', {})
> +
> + # Dirty some random megabytes
> + for _ in range(9):
> + mb_ofs = random.randrange(1024)
> + self.vm_a.hmp_qemu_io(self.src_node_name, 'write %dM 1M' % mb_ofs)
May be, use f-string for consistency
> +
> + result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
> + node=self.src_node_name,
> + name=self.src_bmap_name)
> + self.bitmap_hash_reference = result['return']['sha256']
> +
> + caps = [{'capability': name, 'state': True} \
> + for name in ('dirty-bitmaps', 'events')]
> +
> + for vm in (self.vm_a, self.vm_b):
> + result = vm.qmp('migrate-set-capabilities', capabilities=caps)
> + self.assert_qmp(result, 'return', {})
> +
> + def tearDown(self) -> None:
> + self.vm_a.shutdown()
> + self.vm_b.shutdown()
> + try:
> + os.remove(mig_sock)
> + except OSError:
> + pass
> +
> + def check_bitmap(self, bitmap_name_valid: bool) -> None:
> + result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
> + node=self.dst_node_name,
> + name=self.dst_bmap_name)
> + if bitmap_name_valid:
> + self.assert_qmp(result, 'return/sha256',
> + self.bitmap_hash_reference)
> + else:
> + self.assert_qmp(result, 'error/desc',
> + f"Dirty bitmap '{self.dst_bmap_name}' not found")
> +
> + def migrate(self, migration_success: bool = True,
> + bitmap_name_valid: bool = True) -> None:
> + result = self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
> + self.assert_qmp(result, 'return', {})
> +
> + status = None
> + while status not in ('completed', 'failed'):
> + status = self.vm_a.event_wait('MIGRATION')['data']['status']
> +
> + self.assertEqual(status == 'completed', migration_success)
> + if status == 'failed':
> + # Wait until the migration has been cleaned up
> + # (Otherwise, bdrv_close_all() will abort because the
> + # dirty bitmap migration code still holds a reference to
> + # the BDS)
> + # (Unfortunately, there does not appear to be a nicer way
> + # of waiting until a failed migration has been cleaned up)
> + timeout_msg = 'Timeout waiting for migration to be cleaned up'
> + with iotests.Timeout(30, timeout_msg):
> + while os.path.exists(mig_sock):
> + pass
> +
> + # Dropping src_node_name will only work once the
> + # bitmap migration code has released it
> + while 'error' in self.vm_a.qmp('blockdev-del',
> + node_name=self.src_node_name):
> + pass
Somehow I would feel calmer with s/pass/time.sleep(0.5)/ in such loops.
> +
> + return
> +
> + self.vm_a.wait_for_runstate('postmigrate')
> + self.vm_b.wait_for_runstate('running')
Actually, bitmaps migration may continue in postcopy, so more correct would be to wait for completed status for migration on target. Still, shouldn't be a big difference when migrate small bitmap data.
> +
> + self.check_bitmap(bitmap_name_valid)
> +
> + @staticmethod
> + def mapping(node_name: str, node_alias: str,
> + bitmap_name: str, bitmap_alias: str) \
> + -> BlockBitmapMapping:
> + return [{
> + 'node-name': node_name,
> + 'alias': node_alias,
> + 'bitmaps': [{
> + 'name': bitmap_name,
> + 'alias': bitmap_alias
> + }]
> + }]
> +
> + def set_mapping(self, vm: iotests.VM, mapping: BlockBitmapMapping,
> + error: Optional[str] = None) \
> + -> None:
> + """
> + Invoke migrate-set-parameters on @vm to set the given @mapping.
> + Check for success if @error is None, or verify the error message
> + if it is not.
> + On success, verify that "info migrate_parameters" on HMP returns
> + our mapping. (Just to check its formatting code.)
> + """
> + result = vm.qmp('migrate-set-parameters',
> + block_bitmap_mapping=mapping)
> +
> + if error is None:
> + self.assert_qmp(result, 'return', {})
> +
> + result = vm.qmp('human-monitor-command',
> + command_line='info migrate_parameters')
> +
> + hmp_mapping: List[str] = []
> + for line in result['return'].split('\n'):
> + line = line.rstrip()
> +
> + if hmp_mapping == []:
> + if line == 'block-bitmap-mapping:':
> + hmp_mapping.append(line)
> + continue
> +
> + if line.startswith(' '):
> + hmp_mapping.append(line)
> + else:
> + break
Let me try:
hmp_mapping = re.search(r'^block-bitmap-mapping:.*(\n .*)*', result['return'], flags=re.MULTILINE)
> +
> + self.assertEqual('\n'.join(hmp_mapping) + '\n',
> + self.to_hmp_mapping(mapping))
> + else:
> + self.assert_qmp(result, 'error/desc', error)
> +
> + @staticmethod
> + def to_hmp_mapping(mapping: BlockBitmapMapping) -> str:
> + result = 'block-bitmap-mapping:\n'
> +
> + for node in mapping:
> + result += f" '{node['node-name']}' -> '{node['alias']}'\n"
> +
> + assert isinstance(node['bitmaps'], list)
> + for bitmap in node['bitmaps']:
> + result += f" '{bitmap['name']}' -> '{bitmap['alias']}'\n"
> +
> + return result
> +
> +
> +class TestAliasMigration(TestDirtyBitmapMigration):
> + src_node_name = 'node0'
> + dst_node_name = 'node0'
> + src_bmap_name = 'bmap0'
> + dst_bmap_name = 'bmap0'
> +
> + def test_migration_without_alias(self) -> None:
> + self.migrate(self.src_node_name == self.dst_node_name,
> + self.src_bmap_name == self.dst_bmap_name)
> +
> + # Expect abnormal shutdown of the destination VM on migration failure
> + if self.src_node_name != self.dst_node_name:
> + try:
> + self.vm_b.shutdown()
> + except qemu.machine.AbnormalShutdown:
> + pass
> +
> + def test_alias_on_src_migration(self) -> None:
> + mapping = self.mapping(self.src_node_name, self.dst_node_name,
> + self.src_bmap_name, self.dst_bmap_name)
> +
> + self.set_mapping(self.vm_a, mapping)
> + self.migrate()
> +
> + def test_alias_on_dst_migration(self) -> None:
> + mapping = self.mapping(self.dst_node_name, self.src_node_name,
> + self.dst_bmap_name, self.src_bmap_name)
> +
> + self.set_mapping(self.vm_b, mapping)
> + self.migrate()
> +
> + def test_alias_on_both_migration(self) -> None:
> + src_map = self.mapping(self.src_node_name, 'node-alias',
> + self.src_bmap_name, 'bmap-alias')
> +
> + dst_map = self.mapping(self.dst_node_name, 'node-alias',
> + self.dst_bmap_name, 'bmap-alias')
> +
> + self.set_mapping(self.vm_a, src_map)
> + self.set_mapping(self.vm_b, dst_map)
> + self.migrate()
> +
> +
> +class TestNodeAliasMigration(TestAliasMigration):
> + src_node_name = 'node-src'
> + dst_node_name = 'node-dst'
> +
> +
> +class TestBitmapAliasMigration(TestAliasMigration):
> + src_bmap_name = 'bmap-src'
> + dst_bmap_name = 'bmap-dst'
> +
> +
> +class TestFullAliasMigration(TestAliasMigration):
> + src_node_name = 'node-src'
> + dst_node_name = 'node-dst'
> + src_bmap_name = 'bmap-src'
> + dst_bmap_name = 'bmap-dst'
> +
> +
> +class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
> + src_node_name = 'node0'
> + dst_node_name = 'node0'
> + src_bmap_name = 'bmap0'
> + dst_bmap_name = 'bmap0'
> +
> + """
> + Note that mapping nodes or bitmaps that do not exist is not an error.
> + """
> +
> + def test_non_injective_node_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [
> + {
> + 'node-name': 'node0',
> + 'alias': 'common-alias',
> + 'bitmaps': [{
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + }]
> + },
> + {
> + 'node-name': 'node1',
> + 'alias': 'common-alias',
> + 'bitmaps': [{
> + 'name': 'bmap1',
> + 'alias': 'bmap-alias1'
> + }]
> + }
> + ]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The node alias common-alias is used twice')
> +
> + def test_non_injective_bitmap_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': 'node0',
> + 'alias': 'node-alias0',
> + 'bitmaps': [
> + {
> + 'name': 'bmap0',
> + 'alias': 'common-alias'
> + },
> + {
> + 'name': 'bmap1',
> + 'alias': 'common-alias'
> + }
> + ]
> + }]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The bitmap alias node-alias0/common-alias is used ' \
> + 'twice')
> +
> + def test_ambiguous_node_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [
> + {
> + 'node-name': 'node0',
> + 'alias': 'node-alias0',
> + 'bitmaps': [{
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + }]
> + },
> + {
> + 'node-name': 'node0',
> + 'alias': 'node-alias1',
> + 'bitmaps': [{
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + }]
> + }
> + ]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The node name node0 is mapped twice')
> +
> + def test_ambiguous_bitmap_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': 'node0',
> + 'alias': 'node-alias0',
> + 'bitmaps': [
> + {
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + },
> + {
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias1'
> + }
> + ]
> + }]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The bitmap node0/bmap0 is mapped twice')
> +
> + def test_migratee_node_is_not_mapped_on_src(self) -> None:
is migratee a mistake or an abbreviation for migrate-error ? :)
> + self.set_mapping(self.vm_a, [])
> + # Should just ignore all bitmaps on unmapped nodes
> + self.migrate(True, False)
> +
> + def test_migratee_node_is_not_mapped_on_dst(self) -> None:
> + self.set_mapping(self.vm_b, [])
> + self.migrate(False, False)
> +
> + # Expect abnormal shutdown of the destination VM on migration failure
> + try:
> + self.vm_b.shutdown()
> + except qemu.machine.AbnormalShutdown:
> + pass
> +
> + self.assertIn(f"Unknown node alias '{self.src_node_name}'",
> + self.vm_b.get_log())
> +
> + def test_migratee_bitmap_is_not_mapped_on_src(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': self.src_node_name,
> + 'alias': self.dst_node_name,
> + 'bitmaps': []
> + }]
> +
> + self.set_mapping(self.vm_a, mapping)
> + # Should just ignore all unmapped bitmaps
> + self.migrate(True, False)
> +
> + def test_migratee_bitmap_is_not_mapped_on_dst(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': self.dst_node_name,
> + 'alias': self.src_node_name,
> + 'bitmaps': []
> + }]
> +
> + self.set_mapping(self.vm_b, mapping)
> + self.migrate(False, False)
> +
> + # Expect abnormal shutdown of the destination VM on migration failure
> + try:
> + self.vm_b.shutdown()
> + except qemu.machine.AbnormalShutdown:
> + pass
> +
> + self.assertIn(f"Unknown bitmap alias '{self.src_bmap_name}' on node " \
> + f"'{self.dst_node_name}' (alias '{self.src_node_name}')",
> + self.vm_b.get_log())
> +
> + def test_non_wellformed_node_alias(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': self.src_bmap_name,
> + 'alias': '123-foo',
> + 'bitmaps': []
> + }]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The node alias 123-foo is not well-formed')
> +
> +
> +class TestCrossAliasMigration(TestDirtyBitmapMigration):
> + """
> + Swap aliases, both to see that qemu does not get confused, and
> + that we can migrate multiple things at once.
> +
> + So we migrate this:
> + node-a.bmap-a -> node-b.bmap-b
> + node-a.bmap-b -> node-b.bmap-a
> + node-b.bmap-a -> node-a.bmap-b
> + node-b.bmap-b -> node-a.bmap-a
> + """
> +
> + src_node_name = 'node-a'
> + dst_node_name = 'node-b'
> + src_bmap_name = 'bmap-a'
> + dst_bmap_name = 'bmap-b'
> +
> + def setUp(self) -> None:
> + TestDirtyBitmapMigration.setUp(self)
> +
> + # Now create another block device and let both have two bitmaps each
> + result = self.vm_a.qmp('blockdev-add',
> + node_name='node-b', driver='null-co')
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm_b.qmp('blockdev-add',
> + node_name='node-a', driver='null-co')
> + self.assert_qmp(result, 'return', {})
> +
> + bmaps_to_add = (('node-a', 'bmap-b'),
> + ('node-b', 'bmap-a'),
> + ('node-b', 'bmap-b'))
> +
> + for (node, bmap) in bmaps_to_add:
> + result = self.vm_a.qmp('block-dirty-bitmap-add',
> + node=node, name=bmap)
> + self.assert_qmp(result, 'return', {})
> +
> + @staticmethod
> + def cross_mapping() -> BlockBitmapMapping:
> + return [
> + {
> + 'node-name': 'node-a',
> + 'alias': 'node-b',
> + 'bitmaps': [
> + {
> + 'name': 'bmap-a',
> + 'alias': 'bmap-b'
> + },
> + {
> + 'name': 'bmap-b',
> + 'alias': 'bmap-a'
> + }
> + ]
> + },
> + {
> + 'node-name': 'node-b',
> + 'alias': 'node-a',
> + 'bitmaps': [
> + {
> + 'name': 'bmap-b',
> + 'alias': 'bmap-a'
> + },
> + {
> + 'name': 'bmap-a',
> + 'alias': 'bmap-b'
> + }
> + ]
> + }
> + ]
> +
> + def verify_dest_has_all_bitmaps(self) -> None:
> + bitmaps = self.vm_a.query_bitmaps()
s/vm_a/vm_b/
Ha! I've already thought, that I'll not find any mistake :)
With it fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
(other my notes up to you)
> +
> + # Extract and sort bitmap names
> + for node in bitmaps:
> + bitmaps[node] = sorted((bmap['name'] for bmap in bitmaps[node]))
> +
> + self.assertEqual(bitmaps,
> + {'node-a': ['bmap-a', 'bmap-b'],
> + 'node-b': ['bmap-a', 'bmap-b']})
> +
> + def test_alias_on_src(self) -> None:
> + self.set_mapping(self.vm_a, self.cross_mapping())
> +
> + # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
> + # that is enough
> + self.migrate()
> +
> + self.verify_dest_has_all_bitmaps()
> +
> + def test_alias_on_dst(self) -> None:
> + self.set_mapping(self.vm_b, self.cross_mapping())
> +
> + # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
> + # that is enough
> + self.migrate()
> +
> + self.verify_dest_has_all_bitmaps()
> +
> +
> +if __name__ == '__main__':
> + iotests.main(supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out
> new file mode 100644
> index 0000000000..6d9bee1a4b
> --- /dev/null
> +++ b/tests/qemu-iotests/300.out
> @@ -0,0 +1,5 @@
> +...........................
> +----------------------------------------------------------------------
> +Ran 27 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index c0710cd99e..6b8910a767 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -307,4 +307,5 @@
> 295 rw
> 296 rw
> 297 meta
> +300 migration
> 301 backing quick
>
--
Best regards,
Vladimir
next prev parent reply other threads:[~2020-07-20 18:03 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-16 13:53 [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Max Reitz
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
2020-07-20 16:31 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:02 ` Max Reitz
2020-07-22 11:07 ` Vladimir Sementsov-Ogievskiy
2020-07-22 9:06 ` Dr. David Alan Gilbert
2020-07-16 13:53 ` [PATCH v2 2/3] iotests.py: Add wait_for_runstate() Max Reitz
2020-07-20 16:46 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:05 ` Max Reitz
2020-07-16 13:53 ` [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration Max Reitz
2020-07-20 18:02 ` Vladimir Sementsov-Ogievskiy [this message]
2020-07-21 8:33 ` Max Reitz
2020-07-22 9:17 ` [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Dr. David Alan Gilbert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4cada895-7634-4b15-d03b-9f0f72995895@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=dgilbert@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=pkrempa@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).