diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 54de8d72a..a4cc60fd7 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -81,6 +81,8 @@ RAID_APPLY_CONFIGURATION_ARGSINFO = { } } +MULTIPATH_ENABLED = None + def _get_device_info(dev, devclass, field): """Get the device info according to device class and field.""" @@ -138,6 +140,36 @@ def _load_ipmi_modules(): il_utils.try_execute('modprobe', 'ipmi_si') +def _load_multipath_modules(): + """Load multipath modules + + This is required to be able to collect multipath information. + + Two separate paths exist, one with a helper utility for Centos/RHEL + and another which is just load the modules, and trust multipathd + will do the needful. + """ + if (os.path.isfile('/usr/sbin/mpathconf') + and not os.path.isfile('/etc/multipath.conf')): + # For Centos/Rhel/Etc which uses mpathconf, this does + # a couple different things, including configuration generation... + # which is not *really* required.. at least *shouldn't* be. + # WARNING(TheJulia): This command explicitly replaces local + # configuration. + il_utils.try_execute('/usr/sbin/mpathconf', '--enable', + '--find_multipaths', 'yes', + '--with_module', 'y', + '--with_multipathd', 'y') + else: + # Ensure modules are loaded. Configuration is not required + # and implied based upon compiled in defaults. + # NOTE(TheJulia): Debian/Ubuntu specifically just document + # using `multipath -t` output to start a new configuration + # file, if needed. + il_utils.try_execute('modprobe', 'dm_multipath') + il_utils.try_execute('modprobe', 'multipath') + + def _check_for_iscsi(): """Connect iSCSI shared connected via iBFT or OF. @@ -181,6 +213,84 @@ def _get_md_uuid(raid_device): return match.group(1) +def _enable_multipath(): + """Initialize multipath IO if possible. + + :returns: True if the multipathd daemon and multipath command to enumerate + devices was scucessfully able to be called. + """ + try: + _load_multipath_modules() + # This might not work, ideally it *should* already be running... + # NOTE(TheJulia): Testing locally, a prior running multipathd, the + # explicit multipathd start just appears to silently exit with a + # result code of 0. + il_utils.execute('multipathd') + # This is mainly to get the system to actually do the needful and + # identify/enumerate paths by combining what it can detect and what + # it already knows. This may be useful, and in theory this should be + # logged in the IPA log should it be needed. + il_utils.execute('multipath', '-ll') + return True + except FileNotFoundError as e: + LOG.warning('Attempted to determine if multipath tools were present. ' + 'Not detected. Error recorded: %s', e) + return False + except processutils.ProcessExecutionError as e: + LOG.warning('Attempted to invoke multipath utilities, but we ' + 'encountered an error: %s', e) + return False + + +def _get_multipath_parent_device(device): + """Check and return a multipath device.""" + if not device: + # if lsblk provides invalid output, this can be None. + return + check_device = os.path.join('/dev', str(device)) + try: + # Explicitly run the check as regardless of if the device is mpath or + # not, multipath tools when using list always exits with a return + # code of 0. + il_utils.execute('multipath', '-c', check_device) + # path check with return an exit code of 1 if you send it a multipath + # device mapper device, like dm-0. + # NOTE(TheJulia): -ll is supposed to load from all available + # information, but may not force a rescan. It may be -f if we need + # that. That being said, it has been about a decade since I was + # running multipath tools on SAN connected gear, so my memory is + # definitely fuzzy. + out, _ = il_utils.execute('multipath', '-ll', check_device) + except processutils.ProcessExecutionError as e: + # FileNotFoundError if the utility does not exist. + # -1 return code if the device is not valid. + LOG.debug('Checked device %(dev)s and determined it was ' + 'not a multipath device. %(error)s', + {'dev': check_device, + 'error': e}) + return + except FileNotFoundError: + # This should never happen, as MULTIPATH_ENABLED would be False + # before this occurs. + LOG.warning('Attempted to check multipathing status, however ' + 'the \'multipath\' binary is missing or not in the ' + 'execution PATH.') + return + # Data format: + # MPATHDEVICENAME dm-0 TYPE,HUMANNAME + # size=56G features='1 retain_attached_hw_handler' hwhandler='0' wp=rw + # `-+- policy='service-time 0' prio=1 status=active + # `- 0:0:0:0 sda 8:0 active ready running + try: + lines = out.splitlines() + mpath_device = lines[0].split(' ')[1] + # give back something like dm-0 so we can log it. + return mpath_device + except IndexError: + # We didn't get any command output, so Nope. + pass + + def get_component_devices(raid_device): """Get the component devices of a Software RAID device. @@ -371,7 +481,8 @@ def _md_scan_and_assemble(): def list_all_block_devices(block_type='disk', ignore_raid=False, ignore_floppy=True, - ignore_empty=True): + ignore_empty=True, + ignore_multipath=False): """List all physical block devices The switches we use for lsblk: P for KEY="value" output, b for size output @@ -388,6 +499,9 @@ def list_all_block_devices(block_type='disk', :param ignore_floppy: Ignore floppy disk devices in the block device list. By default, these devices are filtered out. :param ignore_empty: Whether to ignore disks with size equal 0. + :param ignore_multipath: Whether to ignore devices backing multipath + devices. Default is to consider multipath + devices, if possible. :return: A list of BlockDevices """ @@ -398,6 +512,8 @@ def list_all_block_devices(block_type='disk', return True return False + check_multipath = not ignore_multipath and get_multipath_status() + _udev_settle() # map device names to /dev/disk/by-path symbolic links that points to it @@ -428,7 +544,6 @@ def list_all_block_devices(block_type='disk', '-o{}'.format(','.join(columns)))[0] lines = report.splitlines() context = pyudev.Context() - devices = [] for line in lines: device = {} @@ -450,10 +565,25 @@ def list_all_block_devices(block_type='disk', LOG.debug('Ignoring floppy disk device: %s', line) continue + dev_kname = device.get('KNAME') + if check_multipath: + # Net effect is we ignore base devices, and their base devices + # to what would be the mapped device name which would not pass the + # validation, but would otherwise be match-able. + mpath_parent_dev = _get_multipath_parent_device(dev_kname) + if mpath_parent_dev: + LOG.warning( + "We have identified a multipath device %(device)s, this " + "is being ignored in favor of %(mpath_device)s and its " + "related child devices.", + {'device': dev_kname, + 'mpath_device': mpath_parent_dev}) + continue # Search for raid in the reply type, as RAID is a # disk device, and we should honor it if is present. # Other possible type values, which we skip recording: # lvm, part, rom, loop + if devtype != block_type: if devtype is None or ignore_raid: LOG.debug( @@ -462,7 +592,7 @@ def list_all_block_devices(block_type='disk', {'block_type': block_type, 'line': line}) continue elif ('raid' in devtype - and block_type in ['raid', 'disk']): + and block_type in ['raid', 'disk', 'mpath']): LOG.debug( "TYPE detected to contain 'raid', signifying a " "RAID volume. Found: %s", line) @@ -476,6 +606,11 @@ def list_all_block_devices(block_type='disk', LOG.debug( "TYPE detected to contain 'md', signifying a " "RAID partition. Found: %s", line) + elif devtype == 'mpath' and block_type == 'disk': + LOG.debug( + "TYPE detected to contain 'mpath', " + "signifing a device mapper multipath device. " + "Found: %s", line) else: LOG.debug( "TYPE did not match. Wanted: %(block_type)s but found: " @@ -1001,6 +1136,10 @@ class GenericHardwareManager(HardwareManager): _check_for_iscsi() _md_scan_and_assemble() _load_ipmi_modules() + global MULTIPATH_ENABLED + if MULTIPATH_ENABLED is None: + MULTIPATH_ENABLED = _enable_multipath() + self.wait_for_disks() return HardwareSupport.GENERIC @@ -2787,3 +2926,11 @@ def deduplicate_steps(candidate_steps): deduped_steps[manager].append(winning_step) return deduped_steps + + +def get_multipath_status(): + """Return the status of multipath initialization.""" + # NOTE(TheJulia): Provides a nice place to mock out and simplify testing + # as if we directly try and work with the global var, we will be racing + # tests endlessly. + return MULTIPATH_ENABLED diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index e04fb3bc3..2444e30d1 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -737,6 +737,7 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 mock_append_to_fstab.assert_called_with(self.fake_dir, self.fake_efi_system_part_uuid) + @mock.patch.object(hardware, 'get_multipath_status', lambda *_: False) @mock.patch.object(os.path, 'ismount', lambda *_: False) @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: True) @mock.patch.object(os.path, 'exists', autospec=True) @@ -848,6 +849,7 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 uuid=self.fake_efi_system_part_uuid) self.assertFalse(mock_dispatch.called) + @mock.patch.object(hardware, 'get_multipath_status', lambda *_: False) @mock.patch.object(image, '_efi_boot_setup', lambda *_: False) @mock.patch.object(os.path, 'ismount', lambda *_: False) @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: True) diff --git a/ironic_python_agent/tests/unit/samples/hardware_samples.py b/ironic_python_agent/tests/unit/samples/hardware_samples.py index 660c61b53..e402af45e 100644 --- a/ironic_python_agent/tests/unit/samples/hardware_samples.py +++ b/ironic_python_agent/tests/unit/samples/hardware_samples.py @@ -121,7 +121,10 @@ BLK_DEVICE_TEMPLATE = ( 'KNAME="fd1" MODEL="magic" SIZE="4096" ROTA="1" TYPE="disk" UUID="" ' 'PARTUUID=""\n' 'KNAME="sdf" MODEL="virtual floppy" SIZE="0" ROTA="1" TYPE="disk" UUID="" ' - 'PARTUUID=""' + 'PARTUUID=""\n' + 'KNAME="dm-0" MODEL="NWD-BLP4-1600 " SIZE="1765517033472" ' + ' ROTA="0" TYPE="mpath" UUID="" PARTUUID=""\n' + ) # NOTE(pas-ha) largest device is 1 byte smaller than 4GiB @@ -160,6 +163,49 @@ RAID_BLK_DEVICE_TEMPLATE = ( 'PARTUUID=""' ) +MULTIPATH_BLK_DEVICE_TEMPLATE = ( + 'KNAME="sda" MODEL="INTEL_SSDSC2CT060A3" SIZE="60022480896" ROTA="0" ' + 'TYPE="disk" UUID="" PARTUUID=""\n' + 'KNAME="sda2" MODEL="" SIZE="59162722304" ROTA="0" TYPE="part" ' + 'UUID="f8b55d59-96c3-3982-b129-1b6b2ee8da86" ' + 'PARTUUID="c97c8aac-7796-4433-b1fc-9b5fac43edf3"\n' + 'KNAME="sda3" MODEL="" SIZE="650002432" ROTA="0" TYPE="part" ' + 'UUID="b3b03565-5f13-3c93-b2a6-6d90e25be926" ' + 'PARTUUID="6c85beff-b2bd-4a1c-91b7-8abb5256459d"\n' + 'KNAME="sda1" MODEL="" SIZE="209715200" ROTA="0" TYPE="part" ' + 'UUID="0a83355d-7500-3f5f-9abd-66f6fd03714c" ' + 'PARTUUID="eba28b26-b76a-402c-94dd-0b66a523a485"\n' + 'KNAME="dm-0" MODEL="" SIZE="60022480896" ROTA="0" TYPE="mpath" ' + 'UUID="" PARTUUID=""\n' + 'KNAME="dm-4" MODEL="" SIZE="650002432" ROTA="0" TYPE="part" ' + 'UUID="b3b03565-5f13-3c93-b2a6-6d90e25be926" ' + 'PARTUUID="6c85beff-b2bd-4a1c-91b7-8abb5256459d"\n' + 'KNAME="dm-2" MODEL="" SIZE="209715200" ROTA="0" TYPE="part" ' + 'UUID="0a83355d-7500-3f5f-9abd-66f6fd03714c" ' + 'PARTUUID="eba28b26-b76a-402c-94dd-0b66a523a485"\n' + 'KNAME="dm-3" MODEL="" SIZE="59162722304" ROTA="0" TYPE="part" ' + 'UUID="f8b55d59-96c3-3982-b129-1b6b2ee8da86" ' + 'PARTUUID="c97c8aac-7796-4433-b1fc-9b5fac43edf3"\n' + 'KNAME="sdb" MODEL="INTEL_SSDSC2CT060A3" SIZE="60022480896" ' + 'ROTA="0" TYPE="disk" UUID="" PARTUUID=""\n' + 'KNAME="sdb2" MODEL="" SIZE="59162722304" ROTA="0" TYPE="part" ' + 'UUID="f8b55d59-96c3-3982-b129-1b6b2ee8da86" ' + 'PARTUUID="c97c8aac-7796-4433-b1fc-9b5fac43edf3"\n' + 'KNAME="sdb3" MODEL="" SIZE="650002432" ROTA="0" TYPE="part" ' + 'UUID="b3b03565-5f13-3c93-b2a6-6d90e25be926" ' + 'PARTUUID="6c85beff-b2bd-4a1c-91b7-8abb5256459d"\n' + 'KNAME="sdb1" MODEL="" SIZE="209715200" ROTA="0" TYPE="part" ' + 'UUID="0a83355d-7500-3f5f-9abd-66f6fd03714c" ' + 'PARTUUID="eba28b26-b76a-402c-94dd-0b66a523a485"\n' + 'KNAME="sdc" MODEL="ST1000DM003-1CH162" SIZE="1000204886016" ' + 'ROTA="1" TYPE="disk" UUID="" PARTUUID=""\n' + 'KNAME="sdc1" MODEL="" SIZE="899999072256" ROTA="1" TYPE="part" ' + 'UUID="457f7d3c-9376-4997-89bd-d1a7c8b04060" ' + 'PARTUUID="c9433d2e-3bbc-47b4-92bf-43c1d80f06e0"\n' + 'KNAME="dm-1" MODEL="" SIZE="1000204886016" ROTA="0" TYPE="mpath" ' + 'UUID="" PARTUUID=""\n' +) + PARTUUID_DEVICE_TEMPLATE = ( 'KNAME="sda" MODEL="DRIVE 0" SIZE="1765517033472" ' 'ROTA="1" TYPE="disk" UUID="" PARTUUID=""\n' @@ -1501,7 +1547,6 @@ NVME_CLI_INFO_TEMPLATE_FORMAT_UNSUPPORTED = (""" } """) - SGDISK_INFO_TEMPLATE = (""" Partition GUID code: C12A7328-F81F-11D2-BA4B-00A0C93EC93B (EFI system partition) Partition unique GUID: FAED7408-6D92-4FC6-883B-9069E2274ECA @@ -1511,3 +1556,13 @@ Partition size: 1048576 sectors (512.0 MiB) Attribute flags: 0000000000000000 Partition name: 'EFI System Partition' """) # noqa + +MULTIPATH_VALID_PATH = '%s is a valid multipath device path' +MULTIPATH_INVALID_PATH = '%s is not a valid multipath device path' + +MULTIPATH_LINKS_DM = ( + 'SUPER_FRIENDLY_NAME %s ATA,INTEL SSDSC2CT06\n' + 'size=56G features=\'1 retain_attached_hw_handler\' hwhandler=\'0\' wp=rw\n' # noqa + ' `-+- policy=\'service-time 0\' prio=1 status=active\n' + ' `- 0:0:0:0 device s 8:0 active ready running\n' +) diff --git a/ironic_python_agent/tests/unit/test_agent.py b/ironic_python_agent/tests/unit/test_agent.py index 0c5680289..d90b04140 100644 --- a/ironic_python_agent/tests/unit/test_agent.py +++ b/ironic_python_agent/tests/unit/test_agent.py @@ -538,6 +538,7 @@ class TestBaseAgent(ironic_agent_base.IronicAgentTest): wsgi_server.start.assert_called_once_with() self.agent.heartbeater.start.assert_called_once_with() + @mock.patch.object(hardware, '_enable_multipath', autospec=True) @mock.patch('ironic_python_agent.hardware_managers.cna._detect_cna_card', mock.Mock()) @mock.patch.object(agent.IronicPythonAgent, @@ -548,7 +549,8 @@ class TestBaseAgent(ironic_agent_base.IronicAgentTest): @mock.patch.object(hardware.HardwareManager, 'list_hardware_info', autospec=True) def test_run_with_inspection(self, mock_list_hardware, mock_wsgi, - mock_dispatch, mock_inspector, mock_wait): + mock_dispatch, mock_inspector, mock_wait, + mock_mpath): CONF.set_override('inspection_callback_url', 'http://foo/bar') def set_serve_api(): @@ -589,6 +591,7 @@ class TestBaseAgent(ironic_agent_base.IronicAgentTest): mock_dispatch.call_args_list) self.agent.heartbeater.start.assert_called_once_with() + @mock.patch.object(hardware, '_enable_multipath', autospec=True) @mock.patch('ironic_lib.mdns.get_endpoint', autospec=True) @mock.patch( 'ironic_python_agent.hardware_managers.cna._detect_cna_card', @@ -606,7 +609,8 @@ class TestBaseAgent(ironic_agent_base.IronicAgentTest): mock_dispatch, mock_inspector, mock_wait, - mock_mdns): + mock_mdns, + mock_mpath): mock_mdns.side_effect = lib_exc.ServiceLookupFailure() # If inspection_callback_url is configured and api_url is not when the # agent starts, ensure that the inspection will be called and wsgi @@ -646,6 +650,7 @@ class TestBaseAgent(ironic_agent_base.IronicAgentTest): self.assertTrue(mock_wait.called) self.assertFalse(mock_dispatch.called) + @mock.patch.object(hardware, '_enable_multipath', autospec=True) @mock.patch('ironic_lib.mdns.get_endpoint', autospec=True) @mock.patch( 'ironic_python_agent.hardware_managers.cna._detect_cna_card', @@ -663,7 +668,8 @@ class TestBaseAgent(ironic_agent_base.IronicAgentTest): mock_dispatch, mock_inspector, mock_wait, - mock_mdns): + mock_mdns, + mock_mpath): mock_mdns.side_effect = lib_exc.ServiceLookupFailure() # If both api_url and inspection_callback_url are not configured when # the agent starts, ensure that the inspection will be skipped and wsgi @@ -988,12 +994,13 @@ class TestAdvertiseAddress(ironic_agent_base.IronicAgentTest): self.assertFalse(mock_exec.called) self.assertFalse(mock_gethostbyname.called) + @mock.patch.object(hardware, '_enable_multipath', autospec=True) @mock.patch.object(netutils, 'get_ipv4_addr', autospec=True) @mock.patch('ironic_python_agent.hardware_managers.cna._detect_cna_card', autospec=True) - def test_with_network_interface(self, mock_cna, mock_get_ipv4, mock_exec, - mock_gethostbyname): + def test_with_network_interface(self, mock_cna, mock_get_ipv4, mock_mpath, + mock_exec, mock_gethostbyname): self.agent.network_interface = 'em1' mock_get_ipv4.return_value = '1.2.3.4' mock_cna.return_value = False @@ -1006,12 +1013,14 @@ class TestAdvertiseAddress(ironic_agent_base.IronicAgentTest): self.assertFalse(mock_exec.called) self.assertFalse(mock_gethostbyname.called) + @mock.patch.object(hardware, '_enable_multipath', autospec=True) @mock.patch.object(netutils, 'get_ipv4_addr', autospec=True) @mock.patch('ironic_python_agent.hardware_managers.cna._detect_cna_card', autospec=True) def test_with_network_interface_failed(self, mock_cna, mock_get_ipv4, - mock_exec, mock_gethostbyname): + mock_mpath, mock_exec, + mock_gethostbyname): self.agent.network_interface = 'em1' mock_get_ipv4.return_value = None mock_cna.return_value = False diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 0db923544..1eb0f80a7 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -758,60 +758,238 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.assertEqual('eth1.102', interfaces[4].name) self.assertEqual('eth1.103', interfaces[5].name) + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) @mock.patch.object(il_utils, 'execute', autospec=True) def test_get_os_install_device(self, mocked_execute, mock_cached_node, - mocked_listdir, mocked_readlink): + mocked_listdir, mocked_readlink, + mocked_mpath): mocked_readlink.return_value = '../../sda' mocked_listdir.return_value = ['1:0:0:0'] mock_cached_node.return_value = None - mocked_execute.return_value = (hws.BLK_DEVICE_TEMPLATE, '') + mocked_mpath.return_value = False + mocked_execute.side_effect = [ + (hws.BLK_DEVICE_TEMPLATE, ''), + ] self.assertEqual('/dev/sdb', self.hardware.get_os_install_device()) - mocked_execute.assert_called_once_with( - 'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID') + mock_cached_node.assert_called_once_with() + self.assertEqual(1, mocked_mpath.call_count) + + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) + @mock.patch.object(os, 'readlink', autospec=True) + @mock.patch.object(os, 'listdir', autospec=True) + @mock.patch.object(hardware, 'get_cached_node', autospec=True) + @mock.patch.object(il_utils, 'execute', autospec=True) + def test_get_os_install_device_multipath( + self, mocked_execute, mock_cached_node, + mocked_listdir, mocked_readlink, + mocked_mpath): + mocked_mpath.return_value = True + mocked_readlink.return_value = '../../sda' + mocked_listdir.return_value = ['1:0:0:0'] + mock_cached_node.return_value = None + mocked_execute.side_effect = [ + (hws.MULTIPATH_BLK_DEVICE_TEMPLATE, ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda2', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda3', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda1', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-4 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-2 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-3 + (hws.MULTIPATH_VALID_PATH % '/dev/sdb', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sdb2', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sdb3', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sdb1', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdc'), # sdc + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdc1'), # sdc1 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-1 + ] + expected = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + mock.call('multipath', '-c', '/dev/sda'), + mock.call('multipath', '-ll', '/dev/sda'), + mock.call('multipath', '-c', '/dev/sda2'), + mock.call('multipath', '-ll', '/dev/sda2'), + mock.call('multipath', '-c', '/dev/sda3'), + mock.call('multipath', '-ll', '/dev/sda3'), + mock.call('multipath', '-c', '/dev/sda1'), + mock.call('multipath', '-ll', '/dev/sda1'), + mock.call('multipath', '-c', '/dev/dm-0'), + mock.call('multipath', '-c', '/dev/dm-4'), + mock.call('multipath', '-c', '/dev/dm-2'), + mock.call('multipath', '-c', '/dev/dm-3'), + mock.call('multipath', '-c', '/dev/sdb'), + mock.call('multipath', '-ll', '/dev/sdb'), + mock.call('multipath', '-c', '/dev/sdb2'), + mock.call('multipath', '-ll', '/dev/sdb2'), + mock.call('multipath', '-c', '/dev/sdb3'), + mock.call('multipath', '-ll', '/dev/sdb3'), + mock.call('multipath', '-c', '/dev/sdb1'), + mock.call('multipath', '-ll', '/dev/sdb1'), + mock.call('multipath', '-c', '/dev/sdc'), + mock.call('multipath', '-c', '/dev/sdc1'), + mock.call('multipath', '-c', '/dev/dm-1'), + ] + self.assertEqual('/dev/dm-0', self.hardware.get_os_install_device()) + mocked_execute.assert_has_calls(expected) mock_cached_node.assert_called_once_with() + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) + @mock.patch.object(os, 'readlink', autospec=True) + @mock.patch.object(os, 'listdir', autospec=True) + @mock.patch.object(hardware, 'get_cached_node', autospec=True) + @mock.patch.object(il_utils, 'execute', autospec=True) + def test_get_os_install_device_not_multipath( + self, mocked_execute, mock_cached_node, + mocked_listdir, mocked_readlink, mocked_mpath): + mocked_readlink.return_value = '../../sda' + mocked_listdir.return_value = ['1:0:0:0'] + mocked_mpath.return_value = True + hint = {'size': '>900'} + mock_cached_node.return_value = {'properties': {'root_device': hint}, + 'uuid': 'node1', + 'instance_info': {}} + mocked_execute.side_effect = [ + (hws.MULTIPATH_BLK_DEVICE_TEMPLATE, ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda2', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda3', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sda1', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-4 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-2 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-3 + (hws.MULTIPATH_VALID_PATH % '/dev/sdb', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sdb2', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sdb3', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + (hws.MULTIPATH_VALID_PATH % '/dev/sdb1', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdc'), # sdc + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdc1'), # sdc1 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-1 + ] + expected = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + mock.call('multipath', '-c', '/dev/sda'), + mock.call('multipath', '-ll', '/dev/sda'), + mock.call('multipath', '-c', '/dev/sda2'), + mock.call('multipath', '-ll', '/dev/sda2'), + mock.call('multipath', '-c', '/dev/sda3'), + mock.call('multipath', '-ll', '/dev/sda3'), + mock.call('multipath', '-c', '/dev/sda1'), + mock.call('multipath', '-ll', '/dev/sda1'), + mock.call('multipath', '-c', '/dev/dm-0'), + mock.call('multipath', '-c', '/dev/dm-4'), + mock.call('multipath', '-c', '/dev/dm-2'), + mock.call('multipath', '-c', '/dev/dm-3'), + mock.call('multipath', '-c', '/dev/sdb'), + mock.call('multipath', '-ll', '/dev/sdb'), + mock.call('multipath', '-c', '/dev/sdb2'), + mock.call('multipath', '-ll', '/dev/sdb2'), + mock.call('multipath', '-c', '/dev/sdb3'), + mock.call('multipath', '-ll', '/dev/sdb3'), + mock.call('multipath', '-c', '/dev/sdb1'), + mock.call('multipath', '-ll', '/dev/sdb1'), + mock.call('multipath', '-c', '/dev/sdc'), + mock.call('multipath', '-c', '/dev/sdc1'), + mock.call('multipath', '-c', '/dev/dm-1'), + ] + self.assertEqual('/dev/sdc', self.hardware.get_os_install_device()) + mocked_execute.assert_has_calls(expected) + mock_cached_node.assert_called_once_with() + mocked_mpath.assert_called_once_with() + + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) @mock.patch.object(il_utils, 'execute', autospec=True) def test_get_os_install_device_raid(self, mocked_execute, mock_cached_node, mocked_listdir, - mocked_readlink): + mocked_readlink, mocked_mpath): # NOTE(TheJulia): The readlink and listdir mocks are just to satisfy # what is functionally an available path check and that information # is stored in the returned result for use by root device hints. mocked_readlink.side_effect = '../../sda' mocked_listdir.return_value = ['1:0:0:0'] mock_cached_node.return_value = None - mocked_execute.return_value = (hws.RAID_BLK_DEVICE_TEMPLATE, '') + mocked_mpath.return_value = False + mocked_execute.side_effect = [ + (hws.RAID_BLK_DEVICE_TEMPLATE, ''), + ] + # This should ideally select the smallest device and in theory raid # should always be smaller self.assertEqual('/dev/md0', self.hardware.get_os_install_device()) - mocked_execute.assert_called_once_with( - 'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID') + expected = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + ] + + mocked_execute.assert_has_calls(expected) mock_cached_node.assert_called_once_with() + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) @mock.patch.object(il_utils, 'execute', autospec=True) def test_get_os_install_device_fails(self, mocked_execute, mock_cached_node, - mocked_listdir, mocked_readlink): + mocked_listdir, mocked_readlink, + mocked_mpath): """Fail to find device >=4GB w/o root device hints""" mocked_readlink.return_value = '../../sda' mocked_listdir.return_value = ['1:0:0:0'] + mocked_mpath.return_value = False mock_cached_node.return_value = None mocked_execute.return_value = (hws.BLK_DEVICE_TEMPLATE_SMALL, '') ex = self.assertRaises(errors.DeviceNotFound, self.hardware.get_os_install_device) - mocked_execute.assert_called_once_with( - 'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID') + expected = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + ] + + mocked_execute.assert_has_calls(expected) self.assertIn(str(4 * units.Gi), ex.details) mock_cached_node.assert_called_once_with() + self.assertEqual(1, mocked_mpath.call_count) @mock.patch.object(hardware, 'list_all_block_devices', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) @@ -1215,6 +1393,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): ignore_raid=True)], list_mock.call_args_list) + @mock.patch.object(hardware, 'get_multipath_status', lambda *_: True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, '_get_device_info', autospec=True) @@ -1232,7 +1411,34 @@ class TestGenericHardwareManager(base.IronicAgentTest): mock_readlink.side_effect = lambda x, m=by_path_map: m[x] mock_listdir.return_value = [os.path.basename(x) for x in sorted(by_path_map)] - mocked_execute.return_value = (hws.BLK_DEVICE_TEMPLATE, '') + mocked_execute.side_effect = [ + (hws.BLK_DEVICE_TEMPLATE, ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdb'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdc'), + # Pretend sdd is a multipath device... because why not. + (hws.MULTIPATH_VALID_PATH % '/dev/sdd', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # loop0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # zram0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram1 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram2 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram3 + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdf'), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-0 + ] mocked_udev.side_effect = [pyudev.DeviceNotFoundByFileError(), pyudev.DeviceNotFoundByNumberError('block', 1234), @@ -1262,7 +1468,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): vendor='Super Vendor', hctl='1:0:0:0', by_path='/dev/disk/by-path/1:0:0:2'), - hardware.BlockDevice(name='/dev/sdd', + hardware.BlockDevice(name='/dev/dm-0', model='NWD-BLP4-1600', size=1765517033472, rotational=False, @@ -1278,21 +1484,42 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.assertEqual(getattr(expected, attr), getattr(device, attr)) expected_calls = [mock.call('/sys/block/%s/device/scsi_device' % dev) - for dev in ('sda', 'sdb', 'sdc', 'sdd')] + for dev in ('sda', 'sdb', 'sdc', 'dm-0')] mock_listdir.assert_has_calls(expected_calls) expected_calls = [mock.call('/dev/disk/by-path/1:0:0:%d' % dev) for dev in range(3)] mock_readlink.assert_has_calls(expected_calls) + expected_calls = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + mock.call('multipath', '-c', '/dev/sda'), + mock.call('multipath', '-c', '/dev/sdb'), + mock.call('multipath', '-c', '/dev/sdc'), + mock.call('multipath', '-c', '/dev/sdd'), + mock.call('multipath', '-ll', '/dev/sdd'), + mock.call('multipath', '-c', '/dev/loop0'), + mock.call('multipath', '-c', '/dev/zram0'), + mock.call('multipath', '-c', '/dev/ram0'), + mock.call('multipath', '-c', '/dev/ram1'), + mock.call('multipath', '-c', '/dev/ram2'), + mock.call('multipath', '-c', '/dev/ram3'), + mock.call('multipath', '-c', '/dev/sdf'), + mock.call('multipath', '-c', '/dev/dm-0') + ] + mocked_execute.assert_has_calls(expected_calls) + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, '_get_device_info', autospec=True) @mock.patch.object(pyudev.Devices, 'from_device_file', autospec=False) @mock.patch.object(il_utils, 'execute', autospec=True) def test_list_all_block_device_hctl_fail(self, mocked_execute, mocked_udev, mocked_dev_vendor, - mocked_listdir): + mocked_listdir, + mocked_mpath): mocked_listdir.side_effect = (OSError, OSError, IndexError) + mocked_mpath.return_value = False mocked_execute.return_value = (hws.BLK_DEVICE_TEMPLATE_SMALL, '') mocked_dev_vendor.return_value = 'Super Vendor' devices = hardware.list_all_block_devices() @@ -1304,6 +1531,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): ] self.assertEqual(expected_calls, mocked_listdir.call_args_list) + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, '_get_device_info', autospec=True) @@ -1311,15 +1539,62 @@ class TestGenericHardwareManager(base.IronicAgentTest): @mock.patch.object(il_utils, 'execute', autospec=True) def test_list_all_block_device_with_udev(self, mocked_execute, mocked_udev, mocked_dev_vendor, mocked_listdir, - mocked_readlink): + mocked_readlink, mocked_mpath): mocked_readlink.return_value = '../../sda' mocked_listdir.return_value = ['1:0:0:0'] - mocked_execute.return_value = (hws.BLK_DEVICE_TEMPLATE, '') + mocked_execute.side_effect = [ + (hws.BLK_DEVICE_TEMPLATE, ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdb'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdc'), + # Pretend sdd is a multipath device... because why not. + (hws.MULTIPATH_VALID_PATH % '/dev/sdd', ''), + (hws.MULTIPATH_LINKS_DM % 'dm-0', ''), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # loop0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # zram0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram1 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram2 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # ram3 + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdf'), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # dm-0 + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ] + + mocked_mpath.return_value = False mocked_udev.side_effect = iter([ {'ID_WWN': 'wwn%d' % i, 'ID_SERIAL_SHORT': 'serial%d' % i, 'ID_WWN_WITH_EXTENSION': 'wwn-ext%d' % i, 'ID_WWN_VENDOR_EXTENSION': 'wwn-vendor-ext%d' % i} - for i in range(4) + for i in range(5) ]) mocked_dev_vendor.return_value = 'Super Vendor' devices = hardware.list_all_block_devices() @@ -1377,6 +1652,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): expected_calls = [mock.call('/sys/block/%s/device/scsi_device' % dev) for dev in ('sda', 'sdb', 'sdc', 'sdd')] mocked_listdir.assert_has_calls(expected_calls) + mocked_mpath.assert_called_once_with() @mock.patch.object(hardware, 'ThreadPool', autospec=True) @mock.patch.object(hardware, 'dispatch_to_managers', autospec=True) @@ -4160,6 +4436,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.hardware._nvme_erase, block_device) +@mock.patch.object(hardware, '_enable_multipath', autospec=True) @mock.patch.object(hardware, '_load_ipmi_modules', autospec=True) @mock.patch.object(hardware.GenericHardwareManager, 'get_os_install_device', autospec=True) @@ -4174,7 +4451,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): def test_evaluate_hw_waits_for_disks( self, mocked_sleep, mocked_check_for_iscsi, mocked_md_assemble, mocked_get_inst_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, mocked_enable_mpath): mocked_get_inst_dev.side_effect = [ errors.DeviceNotFound('boom'), None @@ -4194,7 +4471,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): def test_evaluate_hw_no_wait_for_disks( self, mocked_log, mocked_sleep, mocked_check_for_iscsi, mocked_md_assemble, mocked_get_inst_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, mocked_enable_mpath): CONF.set_override('disk_wait_attempts', '0') result = self.hardware.evaluate_hardware_support() @@ -4209,7 +4486,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): def test_evaluate_hw_waits_for_disks_nonconfigured( self, mocked_log, mocked_sleep, mocked_check_for_iscsi, mocked_md_assemble, mocked_get_inst_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, mocked_enable_mpath): mocked_get_inst_dev.side_effect = [ errors.DeviceNotFound('boom'), errors.DeviceNotFound('boom'), @@ -4241,7 +4518,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): mocked_check_for_iscsi, mocked_md_assemble, mocked_get_inst_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, + mocked_enable_mpath): CONF.set_override('disk_wait_attempts', '1') mocked_get_inst_dev.side_effect = [ @@ -4262,7 +4540,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): mocked_check_for_iscsi, mocked_md_assemble, mocked_get_inst_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, + mocked_enable_mpath): mocked_get_inst_dev.side_effect = errors.DeviceNotFound('boom') self.hardware.evaluate_hardware_support() mocked_sleep.assert_called_with(3) @@ -4271,7 +4550,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): mocked_check_for_iscsi, mocked_md_assemble, mocked_root_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, + mocked_enable_mpath): CONF.set_override('disk_wait_delay', '5') mocked_root_dev.side_effect = errors.DeviceNotFound('boom') @@ -4281,7 +4561,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): def test_evaluate_hw_disks_timeout( self, mocked_sleep, mocked_check_for_iscsi, mocked_md_assemble, mocked_get_inst_dev, - mocked_load_ipmi_modules): + mocked_load_ipmi_modules, + mocked_enable_mpath): mocked_get_inst_dev.side_effect = errors.DeviceNotFound('boom') result = self.hardware.evaluate_hardware_support() self.assertEqual(hardware.HardwareSupport.GENERIC, result) @@ -4295,6 +4576,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest): @mock.patch.object(il_utils, 'execute', autospec=True) class TestModuleFunctions(base.IronicAgentTest): + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(hardware, '_get_device_info', lambda x, y, z: 'FooTastic') @@ -4303,16 +4585,31 @@ class TestModuleFunctions(base.IronicAgentTest): autospec=False) def test_list_all_block_devices_success(self, mocked_fromdevfile, mocked_udev, mocked_readlink, - mocked_execute): + mocked_mpath, mocked_execute): + mocked_mpath.return_value = True mocked_readlink.return_value = '../../sda' mocked_fromdevfile.return_value = {} - mocked_execute.return_value = (hws.BLK_DEVICE_TEMPLATE_SMALL, '') + mocked_execute.side_effect = [ + (hws.BLK_DEVICE_TEMPLATE_SMALL, ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdb'), + ] result = hardware.list_all_block_devices() - mocked_execute.assert_called_once_with( - 'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID') + expected_calls = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + mock.call('multipath', '-c', '/dev/sda'), + mock.call('multipath', '-c', '/dev/sdb') + ] + + mocked_execute.assert_has_calls(expected_calls) self.assertEqual(BLK_DEVICE_TEMPLATE_SMALL_DEVICES, result) mocked_udev.assert_called_once_with() + mocked_mpath.assert_called_once_with() + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(hardware, '_get_device_info', lambda x, y, z: 'FooTastic') @@ -4321,16 +4618,48 @@ class TestModuleFunctions(base.IronicAgentTest): autospec=False) def test_list_all_block_devices_success_raid(self, mocked_fromdevfile, mocked_udev, mocked_readlink, - mocked_execute): + mocked_mpath, mocked_execute): mocked_readlink.return_value = '../../sda' mocked_fromdevfile.return_value = {} - mocked_execute.return_value = (hws.RAID_BLK_DEVICE_TEMPLATE, '') + mocked_mpath.return_value = True + mocked_execute.side_effect = [ + (hws.RAID_BLK_DEVICE_TEMPLATE, ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda1'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdb'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdb1'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda'), + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # md0p1 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # md0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # md0 + processutils.ProcessExecutionError( + stderr='the -c option requires a path to check'), # md1 + ] + expected_calls = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + mock.call('multipath', '-c', '/dev/sda'), + mock.call('multipath', '-c', '/dev/sda1'), + mock.call('multipath', '-c', '/dev/sdb'), + mock.call('multipath', '-c', '/dev/sdb1'), + mock.call('multipath', '-c', '/dev/md0p1'), + mock.call('multipath', '-c', '/dev/md0'), + mock.call('multipath', '-c', '/dev/md1'), + ] result = hardware.list_all_block_devices(ignore_empty=False) - mocked_execute.assert_called_once_with( - 'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID') + mocked_execute.assert_has_calls(expected_calls) self.assertEqual(RAID_BLK_DEVICE_TEMPLATE_DEVICES, result) mocked_udev.assert_called_once_with() + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(hardware, '_get_device_info', lambda x, y, z: 'FooTastic') @@ -4340,21 +4669,37 @@ class TestModuleFunctions(base.IronicAgentTest): def test_list_all_block_devices_partuuid_success( self, mocked_fromdevfile, mocked_udev, mocked_readlink, - mocked_execute): + mocked_mpath, mocked_execute): mocked_readlink.return_value = '../../sda' mocked_fromdevfile.return_value = {} - mocked_execute.return_value = (hws.PARTUUID_DEVICE_TEMPLATE, '') + mocked_mpath.return_value = True + mocked_execute.side_effect = [ + (hws.PARTUUID_DEVICE_TEMPLATE, ''), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sda'), + processutils.ProcessExecutionError( + stderr=hws.MULTIPATH_INVALID_PATH % '/dev/sdb'), + ] result = hardware.list_all_block_devices(block_type='part') - mocked_execute.assert_called_once_with( - 'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID') + expected_calls = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + mock.call('multipath', '-c', '/dev/sda'), + mock.call('multipath', '-c', '/dev/sda1'), + ] + mocked_execute.assert_has_calls(expected_calls) self.assertEqual(BLK_DEVICE_TEMPLATE_PARTUUID_DEVICE, result) mocked_udev.assert_called_once_with() + mocked_mpath.assert_called_once_with() + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(hardware, '_get_device_info', lambda x, y: "FooTastic") @mock.patch.object(hardware, '_udev_settle', autospec=True) def test_list_all_block_devices_wrong_block_type(self, mocked_udev, + mock_mpath_enabled, mocked_execute): + mock_mpath_enabled.return_value = False mocked_execute.return_value = ('TYPE="foo" MODEL="model"', '') result = hardware.list_all_block_devices() mocked_execute.assert_called_once_with( @@ -4362,17 +4707,32 @@ class TestModuleFunctions(base.IronicAgentTest): self.assertEqual([], result) mocked_udev.assert_called_once_with() + @mock.patch.object(hardware, 'get_multipath_status', autospec=True) @mock.patch.object(hardware, '_udev_settle', autospec=True) def test_list_all_block_devices_missing(self, mocked_udev, + mocked_mpath, mocked_execute): """Test for missing values returned from lsblk""" - mocked_execute.return_value = ('TYPE="disk" MODEL="model"', '') + mocked_mpath.return_value = False + mocked_execute.side_effect = [ + ('TYPE="disk" MODEL="model"', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ] + expected_calls = [ + mock.call('lsblk', '-Pbia', + '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'), + ] + self.assertRaisesRegex( errors.BlockDeviceError, r'^Block device caused unknown error: KNAME, PARTUUID, ROTA, ' r'SIZE, UUID must be returned by lsblk.$', hardware.list_all_block_devices) mocked_udev.assert_called_once_with() + mocked_execute.assert_has_calls(expected_calls) def test__udev_settle(self, mocked_execute): hardware._udev_settle() @@ -4399,6 +4759,85 @@ class TestModuleFunctions(base.IronicAgentTest): mock.call('modprobe', 'ipmi_si')]) +@mock.patch.object(il_utils, 'execute', autospec=True) +class TestMultipathEnabled(base.IronicAgentTest): + + @mock.patch.object(os.path, 'isfile', autospec=True) + def test_enable_multipath_with_config(self, mock_isfile, mocked_execute): + mock_isfile.side_effect = [True, True] + mocked_execute.side_effect = [ + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ] + self.assertTrue(hardware._enable_multipath()) + mocked_execute.assert_has_calls([ + mock.call('modprobe', 'dm_multipath'), + mock.call('modprobe', 'multipath'), + mock.call('multipathd'), + mock.call('multipath', '-ll'), + ]) + + @mock.patch.object(os.path, 'isfile', autospec=True) + def test_enable_multipath_mpathconf(self, mock_isfile, mocked_execute): + mock_isfile.side_effect = [True, False] + mocked_execute.side_effect = [ + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ] + self.assertTrue(hardware._enable_multipath()) + mocked_execute.assert_has_calls([ + mock.call('/usr/sbin/mpathconf', '--enable', + '--find_multipaths', 'yes', + '--with_module', 'y', + '--with_multipathd', 'y'), + mock.call('multipathd'), + mock.call('multipath', '-ll'), + ]) + + @mock.patch.object(os.path, 'isfile', autospec=True) + def test_enable_multipath_no_multipath(self, mock_isfile, mocked_execute): + mock_isfile.return_value = False + mocked_execute.side_effect = [ + ('', ''), + ('', ''), + ('', ''), + ('', ''), + ] + self.assertTrue(hardware._enable_multipath()) + mocked_execute.assert_has_calls([ + mock.call('modprobe', 'dm_multipath'), + mock.call('modprobe', 'multipath'), + mock.call('multipathd'), + mock.call('multipath', '-ll'), + ]) + + @mock.patch.object(hardware, '_load_multipath_modules', autospec=True) + def test_enable_multipath_not_found_mpath_config(self, + mock_modules, + mocked_execute): + mocked_execute.side_effect = FileNotFoundError() + self.assertFalse(hardware._enable_multipath()) + self.assertEqual(1, mocked_execute.call_count) + self.assertEqual(1, mock_modules.call_count) + + @mock.patch.object(hardware, '_load_multipath_modules', autospec=True) + def test_enable_multipath_lacking_support(self, + mock_modules, + mocked_execute): + mocked_execute.side_effect = [ + ('', ''), # Help will of course work. + processutils.ProcessExecutionError('lacking kernel support') + ] + self.assertFalse(hardware._enable_multipath()) + self.assertEqual(2, mocked_execute.call_count) + self.assertEqual(1, mock_modules.call_count) + + def create_hdparm_info(supported=False, enabled=False, locked=False, frozen=False, enhanced_erase=False): diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py index bb6a6f79c..c99f7dcaa 100644 --- a/ironic_python_agent/tests/unit/test_utils.py +++ b/ironic_python_agent/tests/unit/test_utils.py @@ -428,8 +428,13 @@ class TestUtils(ironic_agent_base.IronicAgentTest): mock.call(['lshw', '-quiet', '-json'])] mock_outputs.assert_has_calls(calls, any_order=True) mock_gzip_b64.assert_called_once_with( - file_list=[], - io_dict=mock.ANY) + io_dict={'journal': mock.ANY, 'ps': mock.ANY, 'df': mock.ANY, + 'iptables': mock.ANY, 'ip_addr': mock.ANY, + 'lshw': mock.ANY, 'lsblk': mock.ANY, + 'lsblk-full': mock.ANY, 'mdstat': mock.ANY, + 'mount': mock.ANY, 'parted': mock.ANY, + 'multipath': mock.ANY}, + file_list=[]) @mock.patch.object(utils, 'gzip_and_b64encode', autospec=True) @mock.patch.object(utils, 'is_journalctl_present', autospec=True) @@ -453,8 +458,13 @@ class TestUtils(ironic_agent_base.IronicAgentTest): mock.call(['lshw', '-quiet', '-json'])] mock_outputs.assert_has_calls(calls, any_order=True) mock_gzip_b64.assert_called_once_with( - file_list=[tmp.name], - io_dict=mock.ANY) + io_dict={'journal': mock.ANY, 'ps': mock.ANY, 'df': mock.ANY, + 'iptables': mock.ANY, 'ip_addr': mock.ANY, + 'lshw': mock.ANY, 'lsblk': mock.ANY, + 'lsblk-full': mock.ANY, 'mdstat': mock.ANY, + 'mount': mock.ANY, 'parted': mock.ANY, + 'multipath': mock.ANY}, + file_list=[tmp.name]) @mock.patch.object(utils, 'gzip_and_b64encode', autospec=True) @mock.patch.object(utils, 'is_journalctl_present', autospec=True) @@ -473,8 +483,13 @@ class TestUtils(ironic_agent_base.IronicAgentTest): mock.call(['lshw', '-quiet', '-json'])] mock_outputs.assert_has_calls(calls, any_order=True) mock_gzip_b64.assert_called_once_with( - file_list=['/var/log'], - io_dict=mock.ANY) + io_dict={'dmesg': mock.ANY, 'ps': mock.ANY, 'df': mock.ANY, + 'iptables': mock.ANY, 'ip_addr': mock.ANY, + 'lshw': mock.ANY, 'lsblk': mock.ANY, + 'lsblk-full': mock.ANY, 'mdstat': mock.ANY, + 'mount': mock.ANY, 'parted': mock.ANY, + 'multipath': mock.ANY}, + file_list=['/var/log']) @mock.patch.object(utils, 'gzip_and_b64encode', autospec=True) @mock.patch.object(utils, 'is_journalctl_present', autospec=True) @@ -497,8 +512,13 @@ class TestUtils(ironic_agent_base.IronicAgentTest): mock.call(['lshw', '-quiet', '-json'])] mock_outputs.assert_has_calls(calls, any_order=True) mock_gzip_b64.assert_called_once_with( - file_list=['/var/log', tmp.name], - io_dict=mock.ANY) + io_dict={'dmesg': mock.ANY, 'ps': mock.ANY, 'df': mock.ANY, + 'iptables': mock.ANY, 'ip_addr': mock.ANY, + 'lshw': mock.ANY, 'lsblk': mock.ANY, + 'lsblk-full': mock.ANY, 'mdstat': mock.ANY, + 'mount': mock.ANY, 'parted': mock.ANY, + 'multipath': mock.ANY}, + file_list=['/var/log', tmp.name]) def test_get_ssl_client_options(self): # defaults diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py index 3f27cb77f..66f6819fd 100644 --- a/ironic_python_agent/utils.py +++ b/ironic_python_agent/utils.py @@ -72,6 +72,7 @@ COLLECT_LOGS_COMMANDS = { 'mdstat': ['cat', '/proc/mdstat'], 'mount': ['mount'], 'parted': ['parted', '-l'], + 'multipath': ['multipath', '-ll'], } diff --git a/releasenotes/notes/multipath-handling-00a5b412d2cf2e4e.yaml b/releasenotes/notes/multipath-handling-00a5b412d2cf2e4e.yaml new file mode 100644 index 000000000..19f299cab --- /dev/null +++ b/releasenotes/notes/multipath-handling-00a5b412d2cf2e4e.yaml @@ -0,0 +1,18 @@ +--- +fixes: + - | + Fixes failures with handling of Multipath IO devices where Active/Passive + storage arrays are in use. Previously, "standby" paths could result in + IO errors causing cleaning to terminate. The agent now explicitly attempts + to handle and account for multipaths based upon the MPIO data available. + This requires the ``multipath`` and ``multipathd`` utility to be present + in the ramdisk. These are supplied by the ``device-mapper-multipath`` or + ``multipath-tools`` packages, and are not requried for the agent's use. + - | + Fixes non-ideal behavior when performing cleaning where Active/Active + MPIO devices would ultimately be cleaned once per IO path, instead of + once per backend device. +other: + - | + The agent will now attempt to collect any multipath path information + and upload it to the agent ramdisk, if the tooling is present.