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.