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.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
