diff --git a/crmsh/cluster_fs.py b/crmsh/cluster_fs.py index 87de396a73..e8f2303603 100644 --- a/crmsh/cluster_fs.py +++ b/crmsh/cluster_fs.py @@ -129,7 +129,7 @@ def _verify_devices(self): raise Error(f"{dev} is a Logical Volume, cannot be used with the -C option") if storage_utils.has_disk_mounted(dev): raise Error(f"{dev} is already mounted") - storage_utils.MultipathInspector.check_device_under_multipath(dev) + storage_utils.MultipathInspector.check_device_under_multipath(dev, node_list) def _check_if_already_configured(self): """ diff --git a/crmsh/sbd.py b/crmsh/sbd.py index e52d74f79f..30d87632fc 100644 --- a/crmsh/sbd.py +++ b/crmsh/sbd.py @@ -96,7 +96,7 @@ def verify_sbd_device(dev_list, node_list=None, compare_uuid=False): if failed_nodes: raise ValueError(f"{dev} is not a block device on {', '.join(failed_nodes)}") - storage_utils.MultipathInspector.check_device_under_multipath(dev) + storage_utils.MultipathInspector.check_device_under_multipath(dev, node_list) if compare_uuid: SBDUtils.compare_device_uuid(dev, node_list) diff --git a/crmsh/storage_utils.py b/crmsh/storage_utils.py index 79808f265e..d235d51166 100644 --- a/crmsh/storage_utils.py +++ b/crmsh/storage_utils.py @@ -203,19 +203,31 @@ class DeviceInfo: class MultipathInspector: - def __init__(self, dev): + def __init__(self, dev, peer=None): self._shell = sh.cluster_shell() + self._peer = peer self._device_info = self._inspect(dev) def _get_parent_device(self, dev) -> str: - resolved = Path(dev).resolve() - cmd = f"lsblk -dn -o PKNAME {shlex.quote(str(resolved))}" - _, out, _ = self._shell.get_rc_stdout_stderr_without_input(None, cmd) - return out or resolved.name + cmd = f"lsblk -dnP -o PKNAME,KNAME {shlex.quote(dev)}" + rc, out, _ = self._shell.get_rc_stdout_stderr_without_input(self._peer, cmd) + node = self._peer or utils.this_node() + if rc != 0 or not out: + raise ValueError(f"Cannot determine kernel device name for {dev} on {node}") + + values = dict( + item.split("=", 1) + for item in shlex.split(out.splitlines()[0]) + if "=" in item + ) + device_name = values.get("PKNAME") or values.get("KNAME") + if not device_name: + raise ValueError(f"Cannot determine kernel device name for {dev} on {node}") + return device_name def _get_multipath_mapping(self) -> dict[str, str]: cmd = "multipathd show paths format \"%d %m\"" - rc, out, _ = self._shell.get_rc_stdout_stderr_without_input(None, cmd) + rc, out, _ = self._shell.get_rc_stdout_stderr_without_input(self._peer, cmd) mapping = dict() if rc != 0: return mapping @@ -242,8 +254,8 @@ def _is_under_multipath(self) -> bool: return self._device_info.under_multipath @classmethod - def check_device_under_multipath(cls, dev): - inspector = cls(dev) - if inspector._is_under_multipath(): - error_msg = f"Device {dev} is under multipath, please provide the multipath device instead" - raise ValueError(error_msg) + def check_device_under_multipath(cls, dev, node_list=None): + for node in node_list or [utils.this_node()]: + inspector = cls(dev, node) + if inspector._is_under_multipath(): + raise ValueError(f"Device {dev} is under multipath on {node}") diff --git a/test/unittests/test_sbd.py b/test/unittests/test_sbd.py index fe7d010380..ed38e47ba3 100644 --- a/test/unittests/test_sbd.py +++ b/test/unittests/test_sbd.py @@ -76,9 +76,13 @@ def test_verify_sbd_device_non_block(self, mock_compare_device_uuid, mock_get_no @patch('crmsh.sbd.storage_utils.get_non_block_device_nodes') @patch('crmsh.sbd.SBDUtils.compare_device_uuid') - def test_verify_sbd_device_valid(self, mock_compare_device_uuid, mock_get_non_block_device_nodes): + @patch('crmsh.sbd.storage_utils.MultipathInspector.check_device_under_multipath') + def test_verify_sbd_device_valid(self, mock_check_multipath, mock_compare_device_uuid, mock_get_non_block_device_nodes): + node_list = ["node1", "node2"] mock_get_non_block_device_nodes.return_value = [] - SBDUtils.verify_sbd_device(["/dev/sbd_device"], ["node1", "node2"]) + SBDUtils.verify_sbd_device(["/dev/sbd_device"], node_list) + + mock_check_multipath.assert_called_once_with("/dev/sbd_device", node_list) @patch('crmsh.utils.parse_sysconfig') def test_get_sbd_value_from_config(self, mock_parse_sysconfig): diff --git a/test/unittests/test_storage_utils.py b/test/unittests/test_storage_utils.py index f72c4ff1be..d295a670e1 100644 --- a/test/unittests/test_storage_utils.py +++ b/test/unittests/test_storage_utils.py @@ -295,26 +295,45 @@ def test_init(self, mock_cluster_shell): mock_shell_inst = mock.Mock() mock_cluster_shell.return_value = mock_shell_inst mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ - (0, "sda", ""), # lsblk output for parent device + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), # lsblk output for parent device (0, "dev multipath\nsda mpatha", "") # multipathd show paths output ] inspector = storage_utils.MultipathInspector("/dev/sda1") assert inspector._shell == mock_shell_inst + assert inspector._peer is None assert inspector._device_info.device == "/dev/sda1" assert inspector._device_info.parent_device == "sda" assert inspector._device_info.under_multipath is True + @mock.patch('crmsh.sh.cluster_shell') + def test_init_with_peer(self, mock_cluster_shell): + """Test MultipathInspector initialization with peer""" + mock_shell_inst = mock.Mock() + mock_cluster_shell.return_value = mock_shell_inst + mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), + (0, "dev multipath\nsda mpatha", "") + ] + + inspector = storage_utils.MultipathInspector("/dev/sda1", "node1") + + assert inspector._peer == "node1" + mock_shell_inst.get_rc_stdout_stderr_without_input.assert_has_calls([ + mock.call("node1", "lsblk -dnP -o PKNAME,KNAME /dev/sda1"), + mock.call("node1", "multipathd show paths format \"%d %m\"") + ]) + @mock.patch('crmsh.sh.cluster_shell') def test_get_parent_device(self, mock_cluster_shell): """Test _get_parent_device method""" mock_shell_inst = mock.Mock() mock_cluster_shell.return_value = mock_shell_inst mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ - (0, "sda", ""), # lsblk output for __init__ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), # lsblk output for __init__ (0, "", ""), # multipathd show paths output for __init__ - (0, "sda", "") # lsblk output for test call + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", "") # lsblk output for test call ] inspector = storage_utils.MultipathInspector("/dev/sda1") @@ -322,6 +341,79 @@ def test_get_parent_device(self, mock_cluster_shell): assert parent == "sda" + @mock.patch('crmsh.sh.cluster_shell') + def test_get_parent_device_for_whole_disk(self, mock_cluster_shell): + """Test _get_parent_device falls back to KNAME for a whole disk""" + mock_shell_inst = mock.Mock() + mock_cluster_shell.return_value = mock_shell_inst + mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ + (0, "PKNAME=\"\" KNAME=\"sda\"", ""), + (0, "", ""), + (0, "PKNAME=\"\" KNAME=\"sda\"", "") + ] + + inspector = storage_utils.MultipathInspector("/dev/sda") + parent = inspector._get_parent_device("/dev/sda") + + assert parent == "sda" + + @mock.patch('crmsh.sh.cluster_shell') + def test_get_parent_device_uses_original_path(self, mock_cluster_shell): + """Test _get_parent_device does not resolve symlinks locally""" + mock_shell_inst = mock.Mock() + mock_cluster_shell.return_value = mock_shell_inst + mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), + (0, "", ""), + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", "") + ] + + inspector = storage_utils.MultipathInspector("/dev/disk/by-id/scsi-test-part1") + parent = inspector._get_parent_device("/dev/disk/by-id/scsi-test-part1") + + assert parent == "sda" + mock_shell_inst.get_rc_stdout_stderr_without_input.assert_has_calls([ + mock.call(None, "lsblk -dnP -o PKNAME,KNAME /dev/disk/by-id/scsi-test-part1"), + mock.call(None, "multipathd show paths format \"%d %m\""), + mock.call(None, "lsblk -dnP -o PKNAME,KNAME /dev/disk/by-id/scsi-test-part1") + ]) + + @mock.patch('crmsh.sh.cluster_shell') + def test_get_parent_device_raises_when_lsblk_empty(self, mock_cluster_shell): + """Test _get_parent_device raises when lsblk cannot identify the device""" + mock_shell_inst = mock.Mock() + mock_cluster_shell.return_value = mock_shell_inst + mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), + (0, "", ""), + (0, "", "") + ] + + inspector = storage_utils.MultipathInspector("/dev/sda1") + + with pytest.raises(ValueError) as exc_info: + inspector._get_parent_device("/dev/sdb1") + + assert str(exc_info.value) == "Cannot determine kernel device name for /dev/sdb1 on {}".format(storage_utils.utils.this_node()) + + @mock.patch('crmsh.sh.cluster_shell') + def test_get_parent_device_raises_when_lsblk_has_no_names(self, mock_cluster_shell): + """Test _get_parent_device raises when lsblk output has no usable names""" + mock_shell_inst = mock.Mock() + mock_cluster_shell.return_value = mock_shell_inst + mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), + (0, "", ""), + (0, "PKNAME=\"\" KNAME=\"\"", "") + ] + + inspector = storage_utils.MultipathInspector("/dev/sda1") + + with pytest.raises(ValueError) as exc_info: + inspector._get_parent_device("/dev/sdb1") + + assert str(exc_info.value) == "Cannot determine kernel device name for /dev/sdb1 on {}".format(storage_utils.utils.this_node()) + @mock.patch('crmsh.sh.cluster_shell') def test_get_multipath_mapping(self, mock_cluster_shell): """Test _get_multipath_mapping method with valid output""" @@ -332,7 +424,7 @@ def test_get_multipath_mapping(self, mock_cluster_shell): sdb mpatha sdc mpathb""" mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ - (0, "sda", ""), # lsblk for __init__ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), # lsblk for __init__ (0, multipathd_output, ""), # multipathd for __init__ (0, multipathd_output, "") # multipathd for test call ] @@ -348,7 +440,7 @@ def test_inspect_device_under_multipath(self, mock_cluster_shell): mock_shell_inst = mock.Mock() mock_cluster_shell.return_value = mock_shell_inst mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ - (0, "sda", ""), + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), (0, "dev multipath\nsda mpatha", "") ] @@ -365,7 +457,7 @@ def test_is_under_multipath_true(self, mock_cluster_shell): mock_shell_inst = mock.Mock() mock_cluster_shell.return_value = mock_shell_inst mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ - (0, "sda", ""), + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), (0, "dev multipath\nsda mpatha", "") ] @@ -379,11 +471,40 @@ def test_check_device_under_multipath_raises_error(self, mock_cluster_shell): mock_shell_inst = mock.Mock() mock_cluster_shell.return_value = mock_shell_inst mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ - (0, "sda", ""), + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), (0, "dev multipath\nsda mpatha", "") ] + error_msg = "Device /dev/sda1 is under multipath on {}".format(storage_utils.utils.this_node()) with pytest.raises(ValueError) as exc_info: storage_utils.MultipathInspector.check_device_under_multipath("/dev/sda1") - assert str(exc_info.value) == "Device /dev/sda1 is under multipath, please provide the multipath device instead" + assert str(exc_info.value) == error_msg + mock_shell_inst.get_rc_stdout_stderr_without_input.assert_has_calls([ + mock.call(storage_utils.utils.this_node(), "lsblk -dnP -o PKNAME,KNAME /dev/sda1"), + mock.call(storage_utils.utils.this_node(), "multipathd show paths format \"%d %m\"") + ]) + + @mock.patch('crmsh.sh.cluster_shell') + def test_check_device_under_multipath_with_node_list(self, mock_cluster_shell): + """Test check_device_under_multipath checks nodes and reports the failed node""" + mock_shell_inst = mock.Mock() + mock_cluster_shell.return_value = mock_shell_inst + mock_shell_inst.get_rc_stdout_stderr_without_input.side_effect = [ + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), + (0, "dev multipath\nsdb mpatha", ""), + (0, "PKNAME=\"sda\" KNAME=\"sda1\"", ""), + (0, "dev multipath\nsda mpatha", "") + ] + + with pytest.raises(ValueError) as exc_info: + storage_utils.MultipathInspector.check_device_under_multipath("/dev/sda1", ["node1", "node2"]) + + error_msg = "Device /dev/sda1 is under multipath on node2" + assert str(exc_info.value) == error_msg + mock_shell_inst.get_rc_stdout_stderr_without_input.assert_has_calls([ + mock.call("node1", "lsblk -dnP -o PKNAME,KNAME /dev/sda1"), + mock.call("node1", "multipathd show paths format \"%d %m\""), + mock.call("node2", "lsblk -dnP -o PKNAME,KNAME /dev/sda1"), + mock.call("node2", "multipathd show paths format \"%d %m\"") + ])