curtin-dev team mailing list archive

[Olivier Gayot <mp+481223@xxxxxxxxxxxxxxxxxx>]

> Is there any reason we shouldn't just fix the return statement now?

Honestly I'm not super comfortable doing this change as part of this MP.

It won't solve bug 2094979 since Ubuntu Desktop ISOs don't have multipath-tools installed - so the code will still flow through _legacy_detect_multipath.

And the code is quite confusing to me so I'm a bit worried about potential regressions:

 * I'm only 90% sure we're actually missing a return statement. It /feels/ right to me but..
 * There's a non-zero chance that some things have been working by accident because of the missing return statement (assuming it's unintentional)
 * There's no existing unit test.
 * I'm not sure which of `return None` and `return False` is better. When detect_multipath uses _legacy_detect_multipath, it returns a boolean. But earlier return statements return a string/devpath. I would have to check more in depth what the callers expect.

Diff comments:

> diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
> index 2818fe4..a82131f 100644
> --- a/tests/unittests/test_block.py
> +++ b/tests/unittests/test_block.py
> @@ -940,4 +940,96 @@ class TestResize(CiTestCase):
>              self.assertEqual({'a', 'b'}, block.get_resize_fstypes())
>  
>  
> +class TestGetMajorMinor(CiTestCase):
> +    @mock.patch('curtin.block.udevadm_info')
> +    def test_disk(self, m_info):
> +        m_info.return_value = {'DEVTYPE': 'disk', 'MAJOR': '8', 'MINOR': '0'}
> +        self.assertEqual((8, 0), block.get_major_minor("/dev/sda"))
> +
> +    @mock.patch('curtin.block.udevadm_info')
> +    def test_partition(self, m_info):
> +        m_info.return_value = {
> +            'DEVTYPE': 'partition',
> +            'MAJOR': '8',
> +            'MINOR': '3'}
> +        self.assertEqual((8, 3), block.get_major_minor("/dev/sda"))
> +
> +
> +@mock.patch('curtin.block.blkid')
> +@mock.patch('curtin.block.get_devices_for_mp')
> +@mock.patch('curtin.block.udevadm_info')
> +@mock.patch('curtin.block.rescan_block_devices', new=mock.Mock())
> +class TestLegacyDetectMultipath(CiTestCase):
> +    def test_simple_no_multipath(self, m_udevadm_info, m_devices_for_mp,
> +                                 m_blkid):
> +        m_blkid.return_value = {
> +            '/dev/nvme0n1p3': {
> +                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
> +            }, '/dev/nvme0n1p1': {
> +                'UUID': '50BA-7B2F',
> +            }, '/dev/nvme0n1p2': {
> +                'UUID': 'F1EA-BF0A',
> +            },
> +        }
> +        m_devices_for_mp.return_value = "/dev/nvme0n1p1"
> +        m_udevadm_info.return_value = {}
> +
> +        self.assertFalse(block._legacy_detect_multipath())
> +
> +    def test_isms_raid(self, m_udevadm_info, m_devices_for_mp, m_blkid):
> +        '''In LP: #2094979, we have an IMSM RAID device (mirrored). After
> +           installing to the RAID device and running partprobe, /dev/nvme0n1
> +           and /dev/nvme1n1 appear to be clones of /dev/md126. This made the
> +           legacy multipath detection code think that there are multipath
> +           devices, since partiions of /dev/md126 share the same UUID as
> +           partitions in /dev/nvme0n1 and /dev/nvme1n1. Ensure we don't detect
> +           those as multipath anymore.'''
> +        m_blkid.return_value = {
> +            '/dev/nvme0n1p1': {
> +                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
> +            }, '/dev/nvme1n1p1': {
> +                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
> +            }, '/dev/md126p1': {
> +                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
> +            },
> +        }
> +        m_devices_for_mp.return_value = '/dev/md126p1'
> +
> +        def udevadm_info(devpath):
> +            if devpath == '/dev/md126p1':
> +                return {
> +                    'DEVTYPE': 'partition',
> +                    'MD_CONTAINER': '/dev/md/imsm0',
> +                    'ID_PART_ENTRY_DISK': '8:3',
> +                }
> +            elif devpath == '/dev/nvme0n1p1':
> +                return {
> +                    'DEVTYPE': 'partition',
> +                    'ID_PART_ENTRY_DISK': '8:1',
> +                }
> +            elif devpath == '/dev/nvme1n1p1':
> +                return {
> +                    'DEVTYPE': 'partition',
> +                    'ID_PART_ENTRY_DISK': '8:2',
> +                }
> +            raise ValueError
> +
> +        def get_major_minor(devpath):

done, thanks.

> +            if devpath == '/dev/nvme0n1':
> +                return 8, 1
> +            elif devpath == '/dev/nvme1n1':
> +                return 8, 2
> +            raise ValueError
> +
> +        m_udevadm_info.side_effect = udevadm_info
> +
> +        p_get_major_minor = mock.patch('curtin.block.get_major_minor',
> +                                       side_effect=get_major_minor)
> +        p_md_get_all_devices = mock.patch(
> +            'curtin.block.md_get_all_devices_list',
> +            return_value=['/dev/nvme0n1', '/dev/nvme1n1'])
> +
> +        with p_get_major_minor, p_md_get_all_devices:
> +            self.assertFalse(block._legacy_detect_multipath())
> +
>  # vi: ts=4 expandtab syntax=python


-- 
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/481223
Your team curtin developers is subscribed to branch curtin:master.