Fix: storage_utils: Check multipath devices on all nodes

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

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)

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}")

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

test/unittests/test_storage_utils.py

@@ -295,33 +295,125 @@ 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")
         parent = inspector._get_parent_device("/dev/sda1")
 
         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\"")
+        ])