| Message ID | 20210212232134.1462756-4-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/5] migration: dirty-bitmap: Use struct for alias map inner members | expand |
Am 13.02.2021 um 00:21 hat Eric Blake geschrieben: > From: Peter Krempa <pkrempa@redhat.com> > > Verify that the modification of the bitmap persistence over migration > which is controlled via BitmapMigrationBitmapAliasTransform works > properly. > > Based on TestCrossAliasMigration > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > [eblake: Adjust test for explicit read_zeroes=False] > Signed-off-by: Eric Blake <eblake@redhat.com> This breaks 297: --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out +++ 297.out.bad @@ -1,2 +1,8 @@ === pylint === +************* Module 300 +300:605:0: C0301: Line too long (80/79) (line-too-long) +300:677:0: C0301: Line too long (98/79) (line-too-long) === mypy === +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str" +Found 1 error in 1 file (checked 1 source file) +
On 2/15/21 6:31 AM, Kevin Wolf wrote: > Am 13.02.2021 um 00:21 hat Eric Blake geschrieben: >> From: Peter Krempa <pkrempa@redhat.com> >> >> Verify that the modification of the bitmap persistence over migration >> which is controlled via BitmapMigrationBitmapAliasTransform works >> properly. >> >> Based on TestCrossAliasMigration >> >> Signed-off-by: Peter Krempa <pkrempa@redhat.com> >> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> [eblake: Adjust test for explicit read_zeroes=False] >> Signed-off-by: Eric Blake <eblake@redhat.com> > > This breaks 297: > > --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out > +++ 297.out.bad > @@ -1,2 +1,8 @@ > === pylint === > +************* Module 300 > +300:605:0: C0301: Line too long (80/79) (line-too-long) > +300:677:0: C0301: Line too long (98/79) (line-too-long) These two are easy fixes (add line breaks for shorter lines), but this: > === mypy === > +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str" > +Found 1 error in 1 file (checked 1 source file) is beyond my skill. The typing at line 33: BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] is insufficient to allow our new 'transform' member in the new transform_mapping() -> Block BitmapMapping near line 677: 'bitmaps': [ { 'name': 'bmap-a', 'alias': 'bmap-a', 'transform': { 'persistent': True } }, but I'm not sure how to tell python the right type it should be. John?
Am 15.02.2021 um 17:46 hat Eric Blake geschrieben: > On 2/15/21 6:31 AM, Kevin Wolf wrote: > > Am 13.02.2021 um 00:21 hat Eric Blake geschrieben: > >> From: Peter Krempa <pkrempa@redhat.com> > >> > >> Verify that the modification of the bitmap persistence over migration > >> which is controlled via BitmapMigrationBitmapAliasTransform works > >> properly. > >> > >> Based on TestCrossAliasMigration > >> > >> Signed-off-by: Peter Krempa <pkrempa@redhat.com> > >> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > >> [eblake: Adjust test for explicit read_zeroes=False] > >> Signed-off-by: Eric Blake <eblake@redhat.com> > > > > This breaks 297: > > > > --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out > > +++ 297.out.bad > > @@ -1,2 +1,8 @@ > > === pylint === > > +************* Module 300 > > +300:605:0: C0301: Line too long (80/79) (line-too-long) > > +300:677:0: C0301: Line too long (98/79) (line-too-long) > > These two are easy fixes (add line breaks for shorter lines), but this: > > > === mypy === > > +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str" > > +Found 1 error in 1 file (checked 1 source file) > > is beyond my skill. The typing at line 33: > > BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] > > is insufficient to allow our new 'transform' member in the new > transform_mapping() -> Block BitmapMapping near line 677: > > 'bitmaps': [ > { > 'name': 'bmap-a', > 'alias': 'bmap-a', > 'transform': > { > 'persistent': True > } > }, > > but I'm not sure how to tell python the right type it should be. John? To be honest, this looks sufficiently like JSON that I would just go for List[Dict[str, Any]] (as long as recursive types don't work), but if you really want to have an explicit type, I think you'd have to replace the rightmost str with Union[str, Dict[str, bool]] to allow both. Kevin
On 2/15/21 11:09 AM, Kevin Wolf wrote: >>> This breaks 297: >>> === mypy === >>> +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str" >>> +Found 1 error in 1 file (checked 1 source file) >> >> is beyond my skill. The typing at line 33: >> >> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] >> >> is insufficient to allow our new 'transform' member in the new >> transform_mapping() -> Block BitmapMapping near line 677: >> >> 'bitmaps': [ >> { >> 'name': 'bmap-a', >> 'alias': 'bmap-a', >> 'transform': >> { >> 'persistent': True >> } >> }, >> >> but I'm not sure how to tell python the right type it should be. John? > > To be honest, this looks sufficiently like JSON that I would just go for > List[Dict[str, Any]] (as long as recursive types don't work), but if you > really want to have an explicit type, I think you'd have to replace the > rightmost str with Union[str, Dict[str, bool]] to allow both. Indeed, I played with it before reading your response, and came up with this. Shall I turn it into a formal patch? diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300 index 63036f6a6e13..7501bd1018e2 100755 --- i/tests/qemu-iotests/300 +++ w/tests/qemu-iotests/300 @@ -30,7 +30,10 @@ import iotests # pylint: disable=wrong-import-order import qemu -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] +BlockBitmapMapping = List[Dict[str, + Union[str, + List[Dict[str, + Union[str, Dict[str, bool]]]]]]] mig_sock = os.path.join(iotests.sock_dir, 'mig_sock') @@ -602,7 +605,8 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration): class TestAliasTransformMigration(TestDirtyBitmapMigration): """ - Tests the 'transform' option which modifies bitmap persistence on migration. + Tests the 'transform' option which modifies bitmap persistence on + migration. """ src_node_name = 'node-a' @@ -674,7 +678,8 @@ class TestAliasTransformMigration(TestDirtyBitmapMigration): bitmaps = self.vm_b.query_bitmaps() for node in bitmaps: - bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for bmap in bitmaps[node])) + bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) + for bmap in bitmaps[node])) self.assertEqual(bitmaps, {'node-a': [('bmap-a', True), ('bmap-b', False)],
On 2/15/21 1:25 PM, Eric Blake wrote: > -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] > +BlockBitmapMapping = List[Dict[str, > + Union[str, > + List[Dict[str, > + Union[str, Dict[str, > bool]]]]]]] That looks *very* beefy. Is the Union because that union is valid for every key, or because every key has a potentially different value that is specific to that key? if it's the latter, I'd ditch the Union and just go with: Dict[str, object], or Dict[str, Any] object: will allow any type, but keeps strict checking enabled. If you try to use that value later on without a cast, mypy will warn you if you are using it in a manner not guaranteed by the "object" type. Can be useful if you are passing values to a function that already does RTTI to determine behavior. Any: Also allows any type, but enables gradual typing. If you later "assume" the type of this value, mypy will say nothing. Can be useful when you've just got a job to do and the right tool would have been a recursive type or a TypedDict (unavailable in Python 3.6.) --js
On 2/15/21 1:00 PM, John Snow wrote: > On 2/15/21 1:25 PM, Eric Blake wrote: >> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] >> +BlockBitmapMapping = List[Dict[str, >> + Union[str, >> + List[Dict[str, >> + Union[str, Dict[str, >> bool]]]]]]] > > That looks *very* beefy. > > Is the Union because that union is valid for every key, or because every > key has a potentially different value that is specific to that key? > > if it's the latter, I'd ditch the Union and just go with: > > Dict[str, object], or > Dict[str, Any] > > object: will allow any type, but keeps strict checking enabled. If you > try to use that value later on without a cast, mypy will warn you if you > are using it in a manner not guaranteed by the "object" type. Can be > useful if you are passing values to a function that already does RTTI to > determine behavior. We're in luck; both 297 and 300 still pass with this applied on top of my previous attempt: diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300 index 7501bd1018e2..adb927629747 100755 --- i/tests/qemu-iotests/300 +++ w/tests/qemu-iotests/300 @@ -22,7 +22,7 @@ import os import random import re -from typing import Dict, List, Optional, Union +from typing import Dict, List, Optional import iotests @@ -30,10 +30,7 @@ import iotests # pylint: disable=wrong-import-order import qemu -BlockBitmapMapping = List[Dict[str, - Union[str, - List[Dict[str, - Union[str, Dict[str, bool]]]]]]] +BlockBitmapMapping = List[Dict[str, object]] mig_sock = os.path.join(iotests.sock_dir, 'mig_sock') > > Any: Also allows any type, but enables gradual typing. If you later > "assume" the type of this value, mypy will say nothing. Can be useful > when you've just got a job to do and the right tool would have been a > recursive type or a TypedDict (unavailable in Python 3.6.) I'm not too worried about needing to further enhance the type-checking on an individual iotest.
On 2/15/21 3:26 PM, Eric Blake wrote: > On 2/15/21 1:00 PM, John Snow wrote: >> On 2/15/21 1:25 PM, Eric Blake wrote: >>> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]] >>> +BlockBitmapMapping = List[Dict[str, >>> + Union[str, >>> + List[Dict[str, >>> + Union[str, Dict[str, >>> bool]]]]]]] >> >> That looks *very* beefy. >> >> Is the Union because that union is valid for every key, or because every >> key has a potentially different value that is specific to that key? >> >> if it's the latter, I'd ditch the Union and just go with: >> >> Dict[str, object], or >> Dict[str, Any] >> >> object: will allow any type, but keeps strict checking enabled. If you >> try to use that value later on without a cast, mypy will warn you if you >> are using it in a manner not guaranteed by the "object" type. Can be >> useful if you are passing values to a function that already does RTTI to >> determine behavior. > > We're in luck; both 297 and 300 still pass with this applied on top of > my previous attempt: > > diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300 > index 7501bd1018e2..adb927629747 100755 > --- i/tests/qemu-iotests/300 > +++ w/tests/qemu-iotests/300 > @@ -22,7 +22,7 @@ > import os > import random > import re > -from typing import Dict, List, Optional, Union > +from typing import Dict, List, Optional > > import iotests > > @@ -30,10 +30,7 @@ import iotests > # pylint: disable=wrong-import-order > import qemu > > -BlockBitmapMapping = List[Dict[str, > - Union[str, > - List[Dict[str, > - Union[str, Dict[str, > bool]]]]]]] > +BlockBitmapMapping = List[Dict[str, object]] > nice :) > mig_sock = os.path.join(iotests.sock_dir, 'mig_sock') > > > >> >> Any: Also allows any type, but enables gradual typing. If you later >> "assume" the type of this value, mypy will say nothing. Can be useful >> when you've just got a job to do and the right tool would have been a >> recursive type or a TypedDict (unavailable in Python 3.6.) > > I'm not too worried about needing to further enhance the type-checking > on an individual iotest. > Yes, Agreed. I have been going very "overboard" with the python and QAPI types, but I consider those libraries that might have need of such pedantic types. The iotests themselves? eh. Just figured I'd give you a range of options to choose from and you'd pick the best one. --js PS: I really really really really really wish that 3.6 had TypedDict.
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300 index 43264d883d79..63036f6a6e13 100755 --- a/tests/qemu-iotests/300 +++ b/tests/qemu-iotests/300 @@ -600,6 +600,99 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration): self.verify_dest_has_all_bitmaps() self.verify_dest_error(None) +class TestAliasTransformMigration(TestDirtyBitmapMigration): + """ + Tests the 'transform' option which modifies bitmap persistence on migration. + """ + + 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', + read_zeroes=False) + self.assert_qmp(result, 'return', {}) + + result = self.vm_b.qmp('blockdev-add', + node_name='node-a', driver='null-co', + read_zeroes=False) + 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 transform_mapping() -> BlockBitmapMapping: + return [ + { + 'node-name': 'node-a', + 'alias': 'node-a', + 'bitmaps': [ + { + 'name': 'bmap-a', + 'alias': 'bmap-a', + 'transform': + { + 'persistent': True + } + }, + { + 'name': 'bmap-b', + 'alias': 'bmap-b' + } + ] + }, + { + 'node-name': 'node-b', + 'alias': 'node-b', + 'bitmaps': [ + { + 'name': 'bmap-a', + 'alias': 'bmap-a' + }, + { + 'name': 'bmap-b', + 'alias': 'bmap-b' + } + ] + } + ] + + def verify_dest_bitmap_state(self) -> None: + bitmaps = self.vm_b.query_bitmaps() + + for node in bitmaps: + bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for bmap in bitmaps[node])) + + self.assertEqual(bitmaps, + {'node-a': [('bmap-a', True), ('bmap-b', False)], + 'node-b': [('bmap-a', False), ('bmap-b', False)]}) + + def test_transform_on_src(self) -> None: + self.set_mapping(self.vm_a, self.transform_mapping()) + + self.migrate() + self.verify_dest_bitmap_state() + self.verify_dest_error(None) + + def test_transform_on_dst(self) -> None: + self.set_mapping(self.vm_b, self.transform_mapping()) + + self.migrate() + self.verify_dest_bitmap_state() + self.verify_dest_error(None) if __name__ == '__main__': iotests.main(supported_protocols=['file']) diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out index cafb8161f7b1..12e9ab7d571b 100644 --- a/tests/qemu-iotests/300.out +++ b/tests/qemu-iotests/300.out @@ -1,5 +1,5 @@ -..................................... +....................................... ---------------------------------------------------------------------- -Ran 37 tests +Ran 39 tests OK