curtin-dev team mailing list archive

[Chris Peterson <mp+481223@xxxxxxxxxxxxxxxxxx>]

Review: Approve

I think this makes sense to me. Is there any reason we shouldn't just fix the return statement now?

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):

nit(non-blocking): I think you could add corresponding fields to the dictionaries in the mocked `udevadm_info` function and not have to mock `curtin.block.get_major_minor`?

> +            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.