ceph-volume: fix inventory without /dev/vg/lv (slashed paths)

Ths makes ceph-volume use UdevData.preferred_block_path() in
get_devices() so it keeps /dev/vg/lv (slashed path form) when
it exists, else /dev/mapper/<name> (dashed path form).

This is needed for thin-pool LVs and environments where udev
does not create slashed paths.

Fixes: https://tracker.ceph.com/issues/75615
Signed-off-by: Guillaume Abrioux <gabrioux@ibm.com>

diff --git a/src/ceph-volume/ceph_volume/tests/util/test_disk.py b/src/ceph-volume/ceph_volume/tests/util/test_disk.py
index 68eb944a34c68be24eeed8407b22673f7ebaca9f..7164088a367ddfbe3cfc84dd608d60b811b81396 100644 (file)
--- a/src/ceph-volume/ceph_volume/tests/util/test_disk.py
+++ b/src/ceph-volume/ceph_volume/tests/util/test_disk.py
@@ -1,5 +1,7 @@
 import pytest
 import stat
+from typing import Any, Callable, ClassVar, Optional
+
 from ceph_volume.util import disk
 from unittest.mock import patch, Mock, MagicMock, mock_open
 from pyfakefs.fake_filesystem_unittest import TestCase
@@ -358,7 +360,7 @@ class TestGetDevices(object):
         fake_filesystem.create_file('/sys/block/dm-0/queue/hw_sector_size', contents='512')
         with patch("ceph_volume.util.disk.UdevData") as MockUdevData:
             mock_instance = MagicMock()
-            mock_instance.slashed_path = lv_path
+            mock_instance.preferred_block_path = lv_path
             mock_instance.environment = {}
             MockUdevData.return_value = mock_instance
             result = disk.get_devices()
@@ -676,7 +678,29 @@ class TestBlockSysFs(TestCase):
         assert b.active_mappers()['dm-1']['uuid'] == 'abcdef'
 
 
+class _StatWithStRdev:
+    __slots__ = ('_st', 'st_rdev')
+
+    def __init__(self, st: Any, rdev: int = 0) -> None:
+        object.__setattr__(self, '_st', st)
+        object.__setattr__(self, 'st_rdev', rdev)
+
+    def __getattr__(self, name: str) -> Any:
+        return getattr(self._st, name)
+
+
+def _udev_data_patched_os_stat(path: str, *args: Any, **kwargs: Any) -> Any:
+    if TestUdevData._pyfakefs_os_stat is None:
+        raise RuntimeError('TestUdevData.setUp must assign _pyfakefs_os_stat')
+    st = TestUdevData._pyfakefs_os_stat(path, *args, **kwargs)
+    if not hasattr(st, 'st_rdev'):
+        return _StatWithStRdev(st, 0)
+    return st
+
+
 class TestUdevData(TestCase):
+    _pyfakefs_os_stat: ClassVar[Optional[Callable[..., Any]]] = None
+
     def setUp(self) -> None:
         udev_data_lv_device: str = """
 S:disk/by-id/dm-uuid-LVM-1f1RaxWlzQ61Sbc7oCIHRMdh0M8zRTSnU03ekuStqWuiA6eEDmwoGg3cWfFtE2li
@@ -719,13 +743,20 @@ V:1"""
         self.fs.create_file('/run/udev/data/b999:0', create_missing_dirs=True, contents=udev_data_bare_device)
         self.fs.create_file('/run/udev/data/b998:1', create_missing_dirs=True, contents=udev_data_lv_device)
         self.fs.create_file('/run/udev/data/b997:2', create_missing_dirs=True, contents="")
+        TestUdevData._pyfakefs_os_stat = disk.os.stat
+
+    def tearDown(self) -> None:
+        try:
+            super().tearDown()
+        finally:
+            TestUdevData._pyfakefs_os_stat = None
 
     def test_device_not_found(self) -> None:
         self.fs.remove(self.fake_device)
         with pytest.raises(RuntimeError):
             disk.UdevData(self.fake_device)
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=0))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=999))
     def test_no_data(self) -> None:
@@ -733,56 +764,110 @@ V:1"""
         with pytest.raises(RuntimeError):
             disk.UdevData(self.fake_device)
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=2))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=997))
     def test_empty_data(self) -> None:
         # no exception should be raised when a /run/udev/data/* file is empty
         _ = disk.UdevData(self.fake_device)
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=0))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=999))
     def test_is_dm_false(self) -> None:
         assert not disk.UdevData(self.fake_device).is_dm
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
     def test_is_dm_true(self) -> None:
         assert disk.UdevData(self.fake_device).is_dm
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
     def test_is_lvm_true(self) -> None:
-        assert disk.UdevData(self.fake_device).is_dm
+        assert disk.UdevData(self.fake_device).is_lvm
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=0))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=999))
     def test_is_lvm_false(self) -> None:
-        assert not disk.UdevData(self.fake_device).is_dm
+        assert not disk.UdevData(self.fake_device).is_lvm
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
     def test_slashed_path_with_lvm(self) -> None:
         assert disk.UdevData(self.fake_device).slashed_path == '/dev/fake_vg1/fake-lv1'
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
     def test_dashed_path_with_lvm(self) -> None:
         assert disk.UdevData(self.fake_device).dashed_path == '/dev/mapper/fake_vg1-fake-lv1'
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
+    @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
+    @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
+    def test_preferred_block_path_lvm_falls_back_to_mapper(self) -> None:
+        """When /dev/vg/lv is missing, use /dev/mapper/vg-lv (container-style LVM)."""
+        mapper: str = '/dev/mapper/fake_vg1-fake-lv1'
+        self.fs.create_file(mapper, st_mode=(stat.S_IFBLK | 0o600))
+        assert disk.UdevData(self.fake_device).preferred_block_path == mapper
+
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
+    @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
+    @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
+    def test_preferred_block_path_lvm_skips_slashed_when_not_block_device(self) -> None:
+        """If /dev/vg/lv exists but is not a block device (for example, a directory), try mapper."""
+        slashed: str = '/dev/fake_vg1/fake-lv1'
+        mapper: str = '/dev/mapper/fake_vg1-fake-lv1'
+        self.fs.create_dir(slashed)
+        self.fs.create_file(mapper, st_mode=(stat.S_IFBLK | 0o600))
+        assert disk.UdevData(self.fake_device).preferred_block_path == mapper
+
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
+    @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
+    @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
+    def test_preferred_block_path_lvm_falls_back_when_candidates_are_not_block_devices(
+        self,
+    ) -> None:
+        """Neither candidate may be used if they are only directories (for example, empty udev fields)."""
+        slashed: str = '/dev/fake_vg1/fake-lv1'
+        mapper: str = '/dev/mapper/fake_vg1-fake-lv1'
+        self.fs.create_dir(slashed)
+        self.fs.create_dir(mapper)
+        assert disk.UdevData(self.fake_device).preferred_block_path == self.fake_device
+
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
+    @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
+    @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
+    def test_preferred_block_path_lvm_prefers_slashed_when_present(self) -> None:
+        slashed: str = '/dev/fake_vg1/fake-lv1'
+        self.fs.create_file(slashed, st_mode=(stat.S_IFBLK | 0o600))
+        assert disk.UdevData(self.fake_device).preferred_block_path == slashed
+
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
+    @patch('ceph_volume.util.disk.os.minor', Mock(return_value=1))
+    @patch('ceph_volume.util.disk.os.major', Mock(return_value=998))
+    def test_preferred_block_path_lvm_falls_back_to_opening_path_when_no_symlinks(self) -> None:
+        """Neither /dev/vg/lv nor /dev/mapper/name exist; keep the original device node."""
+        assert disk.UdevData(self.fake_device).preferred_block_path == self.fake_device
+
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
+    @patch('ceph_volume.util.disk.os.minor', Mock(return_value=0))
+    @patch('ceph_volume.util.disk.os.major', Mock(return_value=999))
+    def test_preferred_block_path_bare_device(self) -> None:
+        assert disk.UdevData(self.fake_device).preferred_block_path == '/dev/cephtest'
+
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=0))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=999))
     def test_slashed_path_with_bare_device(self) -> None:
         assert disk.UdevData(self.fake_device).slashed_path == '/dev/cephtest'
 
-    @patch('ceph_volume.util.disk.os.stat', MagicMock())
+    @patch('ceph_volume.util.disk.os.stat', _udev_data_patched_os_stat)
     @patch('ceph_volume.util.disk.os.minor', Mock(return_value=0))
     @patch('ceph_volume.util.disk.os.major', Mock(return_value=999))
     def test_dashed_path_with_bare_device(self) -> None:

diff --git a/src/ceph-volume/ceph_volume/util/disk.py b/src/ceph-volume/ceph_volume/util/disk.py
index d8871a17437936502ed403aadb5041b6db63c86a..3a73500702355ce0243d1c17eedd64a25b97603c 100644 (file)
--- a/src/ceph-volume/ceph_volume/util/disk.py
+++ b/src/ceph-volume/ceph_volume/util/disk.py
@@ -787,7 +787,7 @@ def get_devices(_sys_block_path='/sys/block', device=''):
     for block in block_devs:
         metadata: Dict[str, Any] = {}
         if block[2] == 'lvm':
-            block[1] = UdevData(block[1]).slashed_path
+            block[1] = UdevData(block[1]).preferred_block_path
         devname = os.path.basename(block[0])
         diskname = block[1]
         if block[2] not in block_types:
@@ -1430,3 +1430,33 @@ class UdevData:
             name: str = self.environment.get('DM_NAME', '')
             result = f'/dev/mapper/{name}'
         return result
+
+    @staticmethod
+    def _path_is_block_device(path: str) -> bool:
+        """True if ``path`` exists and is a block device (follows symlinks)."""
+        try:
+            return _stat_is_device(os.stat(path).st_mode)
+        except OSError:
+            return False
+
+    @property
+    def preferred_block_path(self) -> str:
+        """Return a device path that exists for typical open(2) / blkid usage.
+
+        `slashed_path` (/dev/vg/lv) is only present when udev/LVM created those
+        nodes; many environments (e.g. containers) only provide
+        `dashed_path` (/dev/mapper/name).
+
+        Returns:
+            str: For non-LVM, `path`. For LVM, `slashed_path` if it is a block
+                 device, else `dashed_path` if it is a block device, else `path`.
+        """
+        if not self.is_lvm:
+            return self.path
+        slashed: str = self.slashed_path
+        if self._path_is_block_device(slashed):
+            return slashed
+        dashed: str = self.dashed_path
+        if self._path_is_block_device(dashed):
+            return dashed
+        return self.path