Merge pull request ceph#64833 from exkson/wip-exkson-ignore-invalid-loop-devices

ceph-volume: avoid RuntimeError on ceph-volume raw list with non-existent loop devices

Author: guits

src/ceph-volume/ceph_volume/devices/raw/list.py

@@ -1,13 +1,15 @@
 from __future__ import print_function
+from typing import Any, Dict, Optional, List as _List
+import os
 import argparse
 import json
 import logging
 from textwrap import dedent
+from concurrent.futures import ThreadPoolExecutor
+
 from ceph_volume import decorators, process
 from ceph_volume.util import disk
 from ceph_volume.util.device import Device
-from typing import Any, Dict, Optional, List as _List
-from concurrent.futures import ThreadPoolExecutor
 
 logger = logging.getLogger(__name__)
 
@@ -57,6 +59,13 @@ def __init__(self, argv: _List[str]) -> None:
         self.info_devices: _List[Dict[str, str]] = []
         self.devices_to_scan: _List[str] = []
 
+    def exclude_invalid_devices(self, devices: _List[Dict[str, str]]) -> _List[Dict[str, str]]:
+        return [
+            dev
+            for dev in devices
+            if (dev_name := dev["NAME"]) and os.path.exists(dev_name)
+        ]
+
     def exclude_atari_partitions(self) -> None:
         result: _List[str] = []
         for info_device in self.info_devices:
@@ -92,6 +101,8 @@ def generate(self, devices: Optional[_List[str]] = None) -> Dict[str, Any]:
             # the parent disk has a bluestore header, but children may be the most appropriate
             # devices to return if the parent disk does not have a bluestore header.
             self.info_devices = disk.lsblk_all(abspath=True)
+            self.info_devices = self.exclude_invalid_devices(self.info_devices)
+            
             # Linux kernels built with CONFIG_ATARI_PARTITION enabled can falsely interpret
             # bluestore's on-disk format as an Atari partition table. These false Atari partitions
             # can be interpreted as real OSDs if a bluestore OSD was previously created on the false

src/ceph-volume/ceph_volume/tests/devices/lvm/test_zap.py

@@ -26,6 +26,11 @@ def process_call(command, **kw):
         result = [], [], 0
     return result
 
+@pytest.fixture()
+def prevent_exclude_invalid_devices():
+    with patch('ceph_volume.devices.raw.list.List.exclude_invalid_devices') as mock:
+        mock.side_effect = lambda x: x
+        yield
 
 class TestZap:
     def test_invalid_osd_id_passed(self) -> None:
@@ -128,6 +133,7 @@ def test_lv_is_matched_id(self, mock_zap, monkeypatch, is_root):
         mock_zap.assert_called_once()
 
     # @patch('ceph_volume.devices.lvm.zap.disk.has_bluestore_label', Mock(return_value=True))
+    @pytest.mark.usefixtures("prevent_exclude_invalid_devices")
     @patch('ceph_volume.devices.lvm.zap.Zap.zap')
     @patch('ceph_volume.devices.raw.list.List.filter_lvm_osd_devices', Mock(return_value='/dev/sdb'))
     @patch('ceph_volume.process.call', Mock(side_effect=process_call))
@@ -156,6 +162,7 @@ def test_lv_is_matched_fsid(self, mock_zap, monkeypatch, is_root):
         assert z.args.devices[0].path == '/dev/VolGroup/lv'
         mock_zap.assert_called_once
 
+    @pytest.mark.usefixtures("prevent_exclude_invalid_devices")
     @patch('ceph_volume.devices.lvm.zap.Zap.zap')
     @patch('ceph_volume.devices.raw.list.List.filter_lvm_osd_devices', Mock(return_value='/dev/sdb'))
     @patch('ceph_volume.process.call', Mock(side_effect=process_call))
@@ -186,6 +193,7 @@ def test_lv_is_matched_id_fsid(self, mock_zap, monkeypatch, is_root):
         assert z.args.devices[0].path == '/dev/VolGroup/lv'
         mock_zap.assert_called_once
 
+    @pytest.mark.usefixtures("prevent_exclude_invalid_devices")
     @patch('ceph_volume.devices.lvm.zap.Zap.zap')
     @patch('ceph_volume.devices.raw.list.List.filter_lvm_osd_devices', Mock(return_value='/dev/sdb'))
     @patch('ceph_volume.process.call', Mock(side_effect=process_call))
@@ -199,6 +207,7 @@ def test_raw_is_matched_id_fsid(self, mock_zap, monkeypatch, is_root):
         assert z.args.devices[0].path == '/dev/sdb'
         mock_zap.assert_called_once
 
+    @pytest.mark.usefixtures("prevent_exclude_invalid_devices")
     @patch('ceph_volume.devices.lvm.zap.Zap.zap')
     @patch('ceph_volume.devices.raw.list.List.filter_lvm_osd_devices', Mock(side_effect=['/dev/vdx', '/dev/vdy', '/dev/vdz', None]))
     @patch('ceph_volume.process.call', Mock(side_effect=process_call))

src/ceph-volume/ceph_volume/tests/devices/raw/test_list.py

@@ -3,6 +3,7 @@
 from .data_list import ceph_bluestore_tool_show_label_output
 from unittest.mock import patch, Mock
 from ceph_volume.devices import raw
+from ceph_volume.devices.raw import list as list_command
 
 # Sample lsblk output is below that overviews the test scenario. (--json output for reader clarity)
 #  - sda and all its children are used for the OS
@@ -127,12 +128,14 @@ class TestList(object):
     @patch('ceph_volume.util.disk.has_bluestore_label')
     @patch('ceph_volume.process.call')
     @patch('ceph_volume.util.disk.lsblk_all')
-    def test_raw_list(self, patched_disk_lsblk, patched_call, patched_bluestore_label, patched_get_devices):
+    @patch('ceph_volume.devices.raw.list.os.path.exists')
+    def test_raw_list(self, patched_path_exists, patched_disk_lsblk, patched_call, patched_bluestore_label, patched_get_devices):
         raw.list.logger.setLevel("DEBUG")
         patched_call.side_effect = _process_call_side_effect
         patched_disk_lsblk.side_effect = _lsblk_all_devices
         patched_bluestore_label.side_effect = _has_bluestore_label_side_effect
         patched_get_devices.side_effect = _devices_side_effect
+        patched_path_exists.return_value = True
 
         result = raw.list.List([]).generate()
         assert len(result) == 3
@@ -161,13 +164,15 @@ def test_raw_list(self, patched_disk_lsblk, patched_call, patched_bluestore_labe
     @patch('ceph_volume.util.disk.has_bluestore_label')
     @patch('ceph_volume.process.call')
     @patch('ceph_volume.util.disk.lsblk_all')
-    def test_raw_list_with_OSError(self, patched_disk_lsblk, patched_call, patched_bluestore_label, patched_get_devices):
+    @patch('ceph_volume.devices.raw.list.os.path.exists')
+    def test_raw_list_with_OSError(self, patched_path_exists, patched_disk_lsblk, patched_call, patched_bluestore_label, patched_get_devices):
         def _has_bluestore_label_side_effect_with_OSError(device_path):
             if device_path == "/dev/sdd":
                 raise OSError('fake OSError')
             return _has_bluestore_label_side_effect(device_path)
 
         raw.list.logger.setLevel("DEBUG")
+        patched_path_exists.return_value = True
         patched_disk_lsblk.side_effect = _lsblk_all_devices
         patched_call.side_effect = _process_call_side_effect
         patched_bluestore_label.side_effect = _has_bluestore_label_side_effect_with_OSError
@@ -176,3 +181,44 @@ def _has_bluestore_label_side_effect_with_OSError(device_path):
         result = raw.list.List([]).generate()
         assert len(result) == 2
         assert {'sdb-uuid', 'sde1-uuid'} == set(result.keys())
+
+    @patch('ceph_volume.devices.raw.list.os.path.exists')
+    def test_raw_list_exclude_non_existent_loop_devices(self, path_exists_patch):
+        def path_exists_side_effect(path):
+            return path in ["/dev/sda"]
+        path_exists_patch.side_effect = path_exists_side_effect
+
+        devices = [
+            {"NAME": "/dev/loop0", "KNAME": "/dev/loop0", "PKNAME": "", "TYPE": "loop"},
+            {"NAME": "/dev/nvme1n1", "KNAME": "/dev/nvme1n1", "PKNAME": "", "TYPE": "disk"},
+            {"NAME": "/dev/sda", "KNAME": "/dev/sda", "PKNAME": "", "TYPE": "disk"},
+        ]
+        cmd = list_command.List([])
+
+        assert cmd.exclude_invalid_devices(devices) == [
+            {"NAME": "/dev/sda", "KNAME": "/dev/sda", "PKNAME": "", "TYPE": "disk"}
+        ]
+
+    @patch("ceph_volume.devices.raw.list.List.exclude_lvm_osd_devices", Mock())
+    @patch("ceph_volume.util.device.disk.get_devices")
+    @patch("ceph_volume.util.disk.has_bluestore_label")
+    @patch("ceph_volume.process.call")
+    @patch("ceph_volume.util.disk.lsblk_all")
+    def test_exclude_invalid_devices_is_called(
+        self,
+        patched_disk_lsblk,
+        patched_call,
+        patched_bluestore_label,
+        patched_get_devices,
+    ):
+        patched_disk_lsblk.side_effect = _lsblk_all_devices
+        patched_call.side_effect = _process_call_side_effect
+        patched_bluestore_label.side_effect = _has_bluestore_label_side_effect
+        patched_get_devices.side_effect = _devices_side_effect
+
+        with patch(
+            "ceph_volume.devices.raw.list.List.exclude_invalid_devices"
+        ) as mock:
+            list_command.List([]).generate()
+            mock.assert_called_once()
+