[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration |
Date: |
Tue, 21 Jul 2020 10:33:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
On 20.07.20 20:02, Vladimir Sementsov-Ogievskiy wrote:
> 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?
Who needs that.
Will fix. O:)
[...]
>> +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
I think mypy complained for some other variables, so I decided to just
type everything. *shrug*
[...]
>> + # 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
Ah, sure.
[...]
>> + # 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.
Why, if you don’t mind me asking?
I’m always afraid of needlessly adding to the runtime of the test. But
it’s not like I couldn’t go for e.g. a sleep(0.1).
>> +
>> + 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.
Why not, I can add a wait_for_migration_state() to patch 2 and then use
that here.
>> +
>> + self.check_bitmap(bitmap_name_valid)
[...]
>> + 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)
Nice, thanks!
>> +
>> + self.assertEqual('\n'.join(hmp_mapping) + '\n',
>> + self.to_hmp_mapping(mapping))
>> + else:
>> + self.assert_qmp(result, 'error/desc', error)
[...]
>> + def test_migratee_node_is_not_mapped_on_src(self) -> None:
>
> is migratee a mistake or an abbreviation for migrate-error ? :)
I meant it as “the node that is being migrated”. As in “employee =
someone who’s employed” or “callee = something that’s being called”.
>> + 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 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)
Thanks, I’ll take them to heart.
signature.asc
Description: OpenPGP digital signature