From 4ffc883c32261056eff2c175f7889bdc21636ca1 Mon Sep 17 00:00:00 2001 From: Jonatan <190036+jcastro@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:45:17 +0200 Subject: [PATCH 1/2] Apply disk exclusions and idle policy to automatic LVM scans --- .github/workflows/test-lvm-disk-policy.yml | 55 ++++++++++ AppImage/scripts/build_appimage.sh | 1 + AppImage/scripts/flask_server.py | 47 ++------- AppImage/scripts/health_monitor.py | 37 ++----- AppImage/scripts/lvm_device_scope.py | 63 +++++++++++ tests/test_lvm_disk_read_policy.py | 116 +++++++++++++++++++++ 6 files changed, 250 insertions(+), 69 deletions(-) create mode 100644 .github/workflows/test-lvm-disk-policy.yml create mode 100644 AppImage/scripts/lvm_device_scope.py create mode 100644 tests/test_lvm_disk_read_policy.py diff --git a/.github/workflows/test-lvm-disk-policy.yml b/.github/workflows/test-lvm-disk-policy.yml new file mode 100644 index 00000000..c7e9ea92 --- /dev/null +++ b/.github/workflows/test-lvm-disk-policy.yml @@ -0,0 +1,55 @@ +name: Disk policy and host monitor regressions + +on: + pull_request: + paths: + - '.github/workflows/test-lvm-disk-policy.yml' + - 'AppImage/scripts/lvm_device_scope.py' + - 'AppImage/scripts/disk_temperature_history.py' + - 'AppImage/scripts/flask_server.py' + - 'AppImage/scripts/health_monitor.py' + - 'AppImage/scripts/flask_notification_routes.py' + - 'AppImage/scripts/proxmox_storage_monitor.py' + - 'AppImage/scripts/build_appimage.sh' + - 'AppImage/scripts/tests/test_host_backup_node.py' + - 'AppImage/scripts/tests/test_proxmox_node_name.py' + - 'AppImage/scripts/tests/test_pve_notifications_cfg_writes.py' + - 'tests/test_lvm_disk_read_policy.py' + push: + branches: [main, develop] + paths: + - 'AppImage/scripts/lvm_device_scope.py' + - 'AppImage/scripts/disk_temperature_history.py' + - 'AppImage/scripts/flask_server.py' + - 'AppImage/scripts/health_monitor.py' + - 'AppImage/scripts/flask_notification_routes.py' + - 'AppImage/scripts/proxmox_storage_monitor.py' + - 'AppImage/scripts/build_appimage.sh' + - 'AppImage/scripts/tests/test_host_backup_node.py' + - 'AppImage/scripts/tests/test_proxmox_node_name.py' + - 'AppImage/scripts/tests/test_pve_notifications_cfg_writes.py' + - 'tests/test_lvm_disk_read_policy.py' + workflow_dispatch: + +permissions: + contents: read + +jobs: + disk-policy: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + - name: Test automatic LVM readers with mocked inventory and policy + run: python -I -B -m unittest discover -s tests -p test_lvm_disk_read_policy.py -v + - name: Test backup node ownership and former jobs + run: python -I -B -m unittest discover -s AppImage/scripts/tests -p test_host_backup_node.py -v + - name: Test local node lookup without Proxmox API commands + run: python -I -B -m unittest discover -s AppImage/scripts/tests -p test_proxmox_node_name.py -v + - name: Test notification configuration writes against a cluster filesystem fixture + run: python -I -B -m unittest discover -s AppImage/scripts/tests -p test_pve_notifications_cfg_writes.py -v diff --git a/AppImage/scripts/build_appimage.sh b/AppImage/scripts/build_appimage.sh index b0df1302..f2defed3 100755 --- a/AppImage/scripts/build_appimage.sh +++ b/AppImage/scripts/build_appimage.sh @@ -141,6 +141,7 @@ cp "$SCRIPT_DIR/oci_console_logs.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo " cp "$SCRIPT_DIR/oci_instance_info.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo "⚠️ oci_instance_info.py not found" cp "$SCRIPT_DIR/oci_operations.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo "⚠️ oci_operations.py not found" cp "$SCRIPT_DIR/disk_temperature_history.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo "⚠️ disk_temperature_history.py not found" +cp "$SCRIPT_DIR/lvm_device_scope.py" "$APP_DIR/usr/bin/" cp "$SCRIPT_DIR/smartctl_resolver.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo "⚠️ smartctl_resolver.py not found" cp "$SCRIPT_DIR/disk_identity.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo "⚠️ disk_identity.py not found" cp "$SCRIPT_DIR/health_thresholds.py" "$APP_DIR/usr/bin/" 2>/dev/null || echo "⚠️ health_thresholds.py not found" diff --git a/AppImage/scripts/flask_server.py b/AppImage/scripts/flask_server.py index 13da0971..0989f352 100644 --- a/AppImage/scripts/flask_server.py +++ b/AppImage/scripts/flask_server.py @@ -4188,11 +4188,12 @@ def get_system_disks(): try: # 4. Check LVM physical volumes. `--devices` (added when a disk # is parked) keeps pvs from scanning — and waking — standby HDDs. + dev_args = _lvm_device_args() result = subprocess.run( - ['pvs', *_lvm_device_args(), '--noheadings', '-o', 'pv_name,vg_name'], + ['pvs', *dev_args, '--noheadings', '-o', 'pv_name,vg_name'], capture_output=True, text=True, timeout=5 - ) - if result.returncode == 0: + ) if dev_args is not None else None + if result is not None and result.returncode == 0: for line in result.stdout.strip().split('\n'): if line.strip(): parts = line.split() @@ -5002,42 +5003,10 @@ def _hdd_in_standby(disk_name: str) -> bool: return parked -def _lvm_device_args() -> list: - """Build the `--devices` argument that keeps an LVM tool (pvs/lvs/ - vgs) off parked disks. - - LVM tools scan every block device by default, which spins up a - standby HDD (issue #232). When any rotational disk is asleep, hand - LVM an explicit list of every block node EXCEPT those belonging to a - parked disk, so it never opens the sleeping drives. Each partition - must be listed too — `--devices /dev/sda` alone does not cover a PV - on /dev/sda3. Returns [] when nothing is parked, so hosts without - standby disks keep the plain pvs/lvs/vgs call unchanged. - """ - try: - r = subprocess.run( - ['lsblk', '-rno', 'NAME,TYPE,PKNAME'], - capture_output=True, text=True, timeout=5) - if r.returncode != 0: - return [] - nodes = [] - for line in r.stdout.strip().split('\n'): - parts = line.split() - if len(parts) < 2 or parts[1] not in ('disk', 'part'): - continue - name = parts[0] - base = name if parts[1] == 'disk' else (parts[2] if len(parts) > 2 else name) - nodes.append((name, base)) - standby_bases = { - name for name, base in nodes - if name == base and _hdd_in_standby(name) - } - if not standby_bases: - return [] - devices = [f'/dev/{name}' for name, base in nodes if base not in standby_bases] - return ['--devices', ','.join(devices)] if devices else [] - except (subprocess.SubprocessError, OSError): - return [] +def _lvm_device_args(): + """None means no automatic scan; [] permits LVM's normal scan.""" + from lvm_device_scope import lvm_device_args + return lvm_device_args(_hdd_in_standby) def get_smart_data(disk_name): diff --git a/AppImage/scripts/health_monitor.py b/AppImage/scripts/health_monitor.py index 23b6d7f1..81037aec 100644 --- a/AppImage/scripts/health_monitor.py +++ b/AppImage/scripts/health_monitor.py @@ -153,36 +153,10 @@ def _hdd_in_standby(disk_name: str) -> bool: return parked -def _lvm_device_args() -> list: - """`--devices` list scoping pvs/lvs/vgs off parked disks so they - aren't spun up by the LVM scan (issue #232). Empty when nothing is - parked, so the plain command runs unchanged. Each partition is - listed too — a whole-disk path alone doesn't cover a PV on a - partition.""" - try: - r = subprocess.run( - ['lsblk', '-rno', 'NAME,TYPE,PKNAME'], - capture_output=True, text=True, timeout=5) - if r.returncode != 0: - return [] - nodes = [] - for line in r.stdout.strip().split('\n'): - parts = line.split() - if len(parts) < 2 or parts[1] not in ('disk', 'part'): - continue - name = parts[0] - base = name if parts[1] == 'disk' else (parts[2] if len(parts) > 2 else name) - nodes.append((name, base)) - standby_bases = { - name for name, base in nodes - if name == base and _hdd_in_standby(name) - } - if not standby_bases: - return [] - devices = [f'/dev/{name}' for name, base in nodes if base not in standby_bases] - return ['--devices', ','.join(devices)] if devices else [] - except Exception: - return [] +def _lvm_device_args(): + """None means no automatic scan; [] permits LVM's normal scan.""" + from lvm_device_scope import lvm_device_args + return lvm_device_args(_hdd_in_standby) def _is_disk_removable(disk_name: str) -> bool: @@ -2303,6 +2277,9 @@ class HealthMonitor: # `--devices` keeps lvs from scanning parked HDDs (issue #232). dev_args = _lvm_device_args() + if dev_args is None: + return {'status': 'INFO', 'skipped': True, + 'reason': 'Automatic LVM scan skipped by disk read policy or unavailable inventory'} result = subprocess.run( ['lvs', *dev_args, '--noheadings', '--options', 'lv_name,vg_name,lv_attr'], capture_output=True, diff --git a/AppImage/scripts/lvm_device_scope.py b/AppImage/scripts/lvm_device_scope.py new file mode 100644 index 00000000..710957ae --- /dev/null +++ b/AppImage/scripts/lvm_device_scope.py @@ -0,0 +1,63 @@ +# ========================================================== +# ProxMenux Monitor - LVM disk read scope +# Author : Jonatan +# GitHub : github.com/jcastro +# Maintainer : MacRimi +# Copyright : (c) 2026 MacRimi & contributors +# License : (GPL-3.0) (https://github.com/MacRimi/ProxMenux/blob/main/LICENSE) +# Version : 1.0 +# Last Updated: 16/10/2026 +# ========================================================== +# Description: +# Applies the automatic disk-read policy to LVM metadata scans in both +# Monitor readers. Excluded and idle disks receive no power-mode probe; +# their partitions are excluded too. No eligible devices means no scan. +# ========================================================== + +import re +import subprocess + + +def lvm_device_args(standby_check): + """Return [] for an unrestricted scan, --devices args, or None to skip. + + A failed inventory must never fall back to an unrestricted scan. None + is distinct from []: if every disk is excluded/idle/asleep, callers + must not start pvs/lvs/vgs at all. + """ + try: + from disk_temperature_history import disk_read_policy, excluded_disk_names + result = subprocess.run(["lsblk", "-rno", "NAME,TYPE,PKNAME"], + capture_output=True, text=True, timeout=5) + if result.returncode: + return None + nodes = [] + for line in result.stdout.splitlines(): + parts = line.split() + if len(parts) < 2 or parts[1] not in ("disk", "part"): + continue + name = parts[0] + base = name if parts[1] == "disk" else parts[2] if len(parts) > 2 else "" + if not re.fullmatch(r"[A-Za-z0-9._+-]+", name) or not re.fullmatch(r"[A-Za-z0-9._+-]+", base): + return None + nodes.append((name, base)) + disks = {name for name, base in nodes if name == base} + if not disks or any(base not in disks for _, base in nodes): + return None + excluded = excluded_disk_names() + blocked = set() + for disk in sorted(disks): + # Test policy first: even CHECK POWER MODE is forbidden for an + # excluded/idle disk. Some USB bridges do not honour -n standby. + if disk_read_policy(disk, excluded) != "read" or standby_check(disk): + blocked.add(disk) + if not blocked: + return [] + devices = [f"/dev/{name}" for name, base in nodes if base not in blocked] + if not devices: + return None + return ["--devices", ",".join(dict.fromkeys(devices))] + except Exception: + # Unavailable exclusions, topology or policy are not permission to + # open devices that may have been deliberately excluded. + return None diff --git a/tests/test_lvm_disk_read_policy.py b/tests/test_lvm_disk_read_policy.py new file mode 100644 index 00000000..03e23c61 --- /dev/null +++ b/tests/test_lvm_disk_read_policy.py @@ -0,0 +1,116 @@ +"""Automatic LVM readers must not bypass disk exclusions/idle policy.""" +import ast +from pathlib import Path +import subprocess +import sys +import types +import unittest +from unittest.mock import Mock, patch + +SCRIPTS = Path(__file__).resolve().parents[1] / "AppImage/scripts" +sys.path.insert(0, str(SCRIPTS)) +import lvm_device_scope as scope + +INVENTORY = "nvme0n1 disk\nnvme0n1p1 part nvme0n1\nsda disk\nsda1 part sda\nsdb disk\nsdb1 part sdb\n" + + +class LvmScopeTests(unittest.TestCase): + def setUp(self): + self.policy = Mock(side_effect=lambda disk, excluded: "excluded" if disk in excluded else "read") + self.standby = Mock(return_value=False) + self.excluded = {"sda"} + self.disk_module = types.ModuleType("disk_temperature_history") + self.disk_module.disk_read_policy = self.policy + self.disk_module.excluded_disk_names = lambda: self.excluded + self.modules = patch.dict(sys.modules, disk_temperature_history=self.disk_module) + self.modules.start() + self.addCleanup(self.modules.stop) + + def args(self, inventory=INVENTORY, code=0): + with patch.object(scope.subprocess, "run", return_value=types.SimpleNamespace(returncode=code, stdout=inventory)) as run: + args = scope.lvm_device_args(self.standby) + self.assertEqual(run.call_args.args[0], ["lsblk", "-rno", "NAME,TYPE,PKNAME"]) + return args + + def test_excluded_disks_and_their_partitions_are_not_in_the_scan(self): + self.assertEqual(self.args(), ["--devices", "/dev/nvme0n1,/dev/nvme0n1p1,/dev/sdb,/dev/sdb1"]) + self.assertNotIn((("sda",), {}), self.standby.call_args_list) + self.assertEqual({call.args[0] for call in self.standby.call_args_list}, {"nvme0n1", "sdb"}) + + def test_idle_disks_receive_no_power_query(self): + self.excluded = set() + self.policy.side_effect = lambda disk, excluded: "idle" if disk == "sda" else "read" + self.assertNotIn("/dev/sda", self.args()[1]) + self.assertEqual({call.args[0] for call in self.standby.call_args_list}, {"nvme0n1", "sdb"}) + + def test_a_standby_disk_is_excluded_even_when_its_io_changed(self): + self.excluded = set() + self.standby.side_effect = lambda disk: disk == "sdb" + self.assertEqual(self.args(), ["--devices", "/dev/nvme0n1,/dev/nvme0n1p1,/dev/sda,/dev/sda1"]) + + def test_all_allowed_disks_keep_existing_scan_behaviour(self): + self.excluded = set() + self.assertEqual(self.args(), []) + + def test_all_blocked_disks_skip_instead_of_falling_back_to_full_scan(self): + self.excluded = {"nvme0n1", "sda", "sdb"} + self.assertIsNone(self.args()) + self.standby.assert_not_called() + + def test_all_standby_disks_skip(self): + self.excluded = set() + self.standby.return_value = True + self.assertIsNone(self.args()) + + def test_bad_inventory_or_policy_never_authorizes_an_unrestricted_scan(self): + for inventory, code in (("", 0), (INVENTORY, 1), ("sda1 part sda", 0), ("../sda disk", 0)): + self.assertIsNone(self.args(inventory, code)) + for error in (FileNotFoundError(), subprocess.TimeoutExpired("lsblk", 5)): + with patch.object(scope.subprocess, "run", side_effect=error): + self.assertIsNone(scope.lvm_device_args(self.standby)) + self.policy.side_effect = RuntimeError("policy unavailable") + self.assertIsNone(self.args()) + + +class LvmReaderTests(unittest.TestCase): + def extract(self, file, name, owner=None, namespace=None): + tree = ast.parse((SCRIPTS / file).read_text()) + body = tree.body if owner is None else next(n.body for n in tree.body if isinstance(n, ast.ClassDef) and n.name == owner) + node = next(n for n in body if isinstance(n, ast.FunctionDef) and n.name == name) + namespace = dict(namespace or {}) + exec(compile(ast.Module(body=[node], type_ignores=[]), file, "exec"), namespace) + return namespace[name] + + def test_both_helpers_delegate_to_the_same_policy(self): + for file in ("flask_server.py", "health_monitor.py"): + standby = Mock() + function = self.extract(file, "_lvm_device_args", namespace={"_hdd_in_standby": standby}) + with patch.object(scope, "lvm_device_args", return_value=None) as shared: + self.assertIsNone(function()) + shared.assert_called_once_with(standby) + + def test_health_skips_lvs_and_vgs_when_no_scan_is_permitted(self): + run = Mock(return_value=types.SimpleNamespace(returncode=0)) + function = self.extract("health_monitor.py", "_check_lvm", "HealthMonitor", { + "Dict": dict, "Any": object, "subprocess": types.SimpleNamespace(run=run), "_lvm_device_args": lambda: None, + }) + result = function(None) + self.assertEqual(result["status"], "INFO") + self.assertTrue(result["skipped"]) + self.assertEqual([call.args[0] for call in run.call_args_list], [["which", "lvs"]]) + + def test_storage_skips_pvs_when_no_scan_is_permitted(self): + tree = ast.parse((SCRIPTS / "flask_server.py").read_text()) + function = next(n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "get_system_disks") + block = next(node for node in function.body if isinstance(node, ast.Try) and "'pvs'" in ast.unparse(node)) + run = Mock(side_effect=AssertionError("pvs must not run")) + namespace = {"subprocess": types.SimpleNamespace(run=run), "_lvm_device_args": lambda: None} + exec(compile(ast.Module(body=[block], type_ignores=[]), "flask_server.py", "exec"), namespace) + run.assert_not_called() + + def test_module_is_shipped(self): + self.assertIn('cp "$SCRIPT_DIR/lvm_device_scope.py" "$APP_DIR/usr/bin/"', (SCRIPTS / "build_appimage.sh").read_text()) + + +if __name__ == "__main__": + unittest.main() From 628dbe3aa3eecabd42a080b8ec95c85e358af4db Mon Sep 17 00:00:00 2001 From: MacRimi <123239993+MacRimi@users.noreply.github.com> Date: Sat, 17 Oct 2026 00:03:39 +0200 Subject: [PATCH 2/2] Name only physical volumes in a restricted LVM scan and keep its own reads from counting as activity - An explicit --devices list makes LVM open every device in it, while a plain scan opens only known volumes; the list now names only LVM physical volumes, read from udev by lsblk, so no other disk is touched - RAID, multipath and encrypted volumes follow the disks they are built on, instead of turning the whole check off - After each pvs, lvs or vgs run, the disks it read take their counters as the new baseline, so an unused disk can become idle --- AppImage/scripts/disk_temperature_history.py | 10 +++ AppImage/scripts/flask_server.py | 7 +- AppImage/scripts/health_monitor.py | 3 + AppImage/scripts/lvm_device_scope.py | 91 ++++++++++++++++---- tests/test_lvm_disk_read_policy.py | 47 ++++++++-- 5 files changed, 131 insertions(+), 27 deletions(-) diff --git a/AppImage/scripts/disk_temperature_history.py b/AppImage/scripts/disk_temperature_history.py index baecafbd..2100c355 100644 --- a/AppImage/scripts/disk_temperature_history.py +++ b/AppImage/scripts/disk_temperature_history.py @@ -526,6 +526,16 @@ def disk_read_policy(disk_name: str, excluded: Optional[set] = None) -> str: return IDLE +def settle_own_reads(disk_names) -> None: + """A reader that has just read these disks itself takes their counters as + the new baseline, so its own I/O never counts as activity.""" + stats = _read_diskstats() + with _cache_lock: + for name in disk_names: + if name in _last_io and name in stats: + _last_io[name] = stats[name] + + def is_disk_idle(disk_name: str) -> bool: """True while the disk is being left alone for having no I/O.""" ts = _idle_state.get(disk_name) diff --git a/AppImage/scripts/flask_server.py b/AppImage/scripts/flask_server.py index 3e96e874..d8be04d6 100644 --- a/AppImage/scripts/flask_server.py +++ b/AppImage/scripts/flask_server.py @@ -4188,13 +4188,16 @@ def get_system_disks(): pass try: - # 4. Check LVM physical volumes. `--devices` (added when a disk - # is parked) keeps pvs from scanning — and waking — standby HDDs. + # 4. Check LVM physical volumes. `--devices` keeps pvs off the disks + # that are excluded, idle or asleep. dev_args = _lvm_device_args() result = subprocess.run( ['pvs', *dev_args, '--noheadings', '-o', 'pv_name,vg_name'], capture_output=True, text=True, timeout=5 ) if dev_args is not None else None + if result is not None: + from lvm_device_scope import settle_scan_reads + settle_scan_reads() if result is not None and result.returncode == 0: for line in result.stdout.strip().split('\n'): if line.strip(): diff --git a/AppImage/scripts/health_monitor.py b/AppImage/scripts/health_monitor.py index 81037aec..076a9cda 100644 --- a/AppImage/scripts/health_monitor.py +++ b/AppImage/scripts/health_monitor.py @@ -2286,6 +2286,8 @@ class HealthMonitor: text=True, timeout=3 ) + from lvm_device_scope import settle_scan_reads + settle_scan_reads() if result.returncode != 0: return {'status': 'WARNING', 'reason': 'lvs command failed'} @@ -2310,6 +2312,7 @@ class HealthMonitor: text=True, timeout=3 ) + settle_scan_reads() if vg_result.returncode == 0 and vg_result.stdout.strip(): return {'status': 'WARNING', 'reason': 'No active LVM volumes detected'} else: diff --git a/AppImage/scripts/lvm_device_scope.py b/AppImage/scripts/lvm_device_scope.py index 710957ae..43cf1896 100644 --- a/AppImage/scripts/lvm_device_scope.py +++ b/AppImage/scripts/lvm_device_scope.py @@ -11,11 +11,56 @@ # Description: # Applies the automatic disk-read policy to LVM metadata scans in both # Monitor readers. Excluded and idle disks receive no power-mode probe; -# their partitions are excluded too. No eligible devices means no scan. +# every device built on them is excluded too, and the scan's own reads +# are not taken as activity. No eligible devices means no scan. # ========================================================== +import json import re import subprocess +import threading + +_NAME = re.compile(r"[A-Za-z0-9._+-]+") +_scan_lock = threading.Lock() +_scanned_disks = set() + + +def _topology(): + """Every block device by kernel name: its type, its parents and whether + it is an LVM physical volume. lsblk reads this from udev, not the disks. + A RAID or multipath device appears once per member.""" + result = subprocess.run(["lsblk", "-J", "-l", "-o", "KNAME,TYPE,PKNAME,FSTYPE"], + capture_output=True, text=True, timeout=5) + if result.returncode: + return None + types, parents, volumes = {}, {}, set() + for device in json.loads(result.stdout).get("blockdevices") or []: + name, parent = device.get("kname"), device.get("pkname") + if not isinstance(name, str) or not _NAME.fullmatch(name): + return None + types[name] = device.get("type") + parents.setdefault(name, set()) + if parent is not None: + if not isinstance(parent, str) or not _NAME.fullmatch(parent): + return None + parents[name].add(parent) + if device.get("fstype") == "LVM2_member" and device.get("type") != "lvm": + volumes.add(name) + return types, parents, volumes + + +def _disks_under(name, parents, seen=None): + """The disks a device is built on: itself, or its parents' disks.""" + seen = seen if seen is not None else set() + if name in seen: + return set() + seen.add(name) + if not parents.get(name): + return {name} + found = set() + for parent in parents[name]: + found |= _disks_under(parent, parents, seen) + return found def lvm_device_args(standby_check): @@ -27,22 +72,12 @@ def lvm_device_args(standby_check): """ try: from disk_temperature_history import disk_read_policy, excluded_disk_names - result = subprocess.run(["lsblk", "-rno", "NAME,TYPE,PKNAME"], - capture_output=True, text=True, timeout=5) - if result.returncode: + topology = _topology() + if topology is None: return None - nodes = [] - for line in result.stdout.splitlines(): - parts = line.split() - if len(parts) < 2 or parts[1] not in ("disk", "part"): - continue - name = parts[0] - base = name if parts[1] == "disk" else parts[2] if len(parts) > 2 else "" - if not re.fullmatch(r"[A-Za-z0-9._+-]+", name) or not re.fullmatch(r"[A-Za-z0-9._+-]+", base): - return None - nodes.append((name, base)) - disks = {name for name, base in nodes if name == base} - if not disks or any(base not in disks for _, base in nodes): + types, parents, volumes = topology + disks = {name for name, kind in types.items() if kind == "disk"} + if not disks: return None excluded = excluded_disk_names() blocked = set() @@ -51,13 +86,31 @@ def lvm_device_args(standby_check): # excluded/idle disk. Some USB bridges do not honour -n standby. if disk_read_policy(disk, excluded) != "read" or standby_check(disk): blocked.add(disk) + allowed = sorted(f"/dev/{name}" for name in volumes if not (_disks_under(name, parents) & blocked)) + with _scan_lock: + _scanned_disks.clear() + _scanned_disks.update(disks - blocked) if not blocked: return [] - devices = [f"/dev/{name}" for name, base in nodes if base not in blocked] - if not devices: + # An explicit list makes LVM open every device in it, so only the + # physical volumes are named: a RAID, multipath or encrypted volume + # built on a blocked disk would read that disk too and stays out. + if not allowed: return None - return ["--devices", ",".join(dict.fromkeys(devices))] + return ["--devices", ",".join(allowed)] except Exception: # Unavailable exclusions, topology or policy are not permission to # open devices that may have been deliberately excluded. return None + + +def settle_scan_reads(): + """The LVM scan reads the disks it is allowed to. Those reads are its + own, so they must not make an unused disk look busy at the next check.""" + try: + from disk_temperature_history import settle_own_reads + with _scan_lock: + disks = set(_scanned_disks) + settle_own_reads(disks) + except Exception: + pass diff --git a/tests/test_lvm_disk_read_policy.py b/tests/test_lvm_disk_read_policy.py index 03e23c61..30495950 100644 --- a/tests/test_lvm_disk_read_policy.py +++ b/tests/test_lvm_disk_read_policy.py @@ -1,5 +1,6 @@ """Automatic LVM readers must not bypass disk exclusions/idle policy.""" import ast +import json from pathlib import Path import subprocess import sys @@ -11,7 +12,14 @@ SCRIPTS = Path(__file__).resolve().parents[1] / "AppImage/scripts" sys.path.insert(0, str(SCRIPTS)) import lvm_device_scope as scope -INVENTORY = "nvme0n1 disk\nnvme0n1p1 part nvme0n1\nsda disk\nsda1 part sda\nsdb disk\nsdb1 part sdb\n" +def lsblk(*devices): + """lsblk -J -l output from (kname, type, pkname, fstype) rows.""" + return json.dumps({"blockdevices": [dict(zip(("kname", "type", "pkname", "fstype"), row)) for row in devices]}) + + +INVENTORY = lsblk(("nvme0n1", "disk", None, None), ("nvme0n1p1", "part", "nvme0n1", "LVM2_member"), + ("sda", "disk", None, None), ("sda1", "part", "sda", "LVM2_member"), + ("sdb", "disk", None, None), ("sdb1", "part", "sdb", "LVM2_member")) class LvmScopeTests(unittest.TestCase): @@ -29,24 +37,24 @@ class LvmScopeTests(unittest.TestCase): def args(self, inventory=INVENTORY, code=0): with patch.object(scope.subprocess, "run", return_value=types.SimpleNamespace(returncode=code, stdout=inventory)) as run: args = scope.lvm_device_args(self.standby) - self.assertEqual(run.call_args.args[0], ["lsblk", "-rno", "NAME,TYPE,PKNAME"]) + self.assertEqual(run.call_args.args[0], ["lsblk", "-J", "-l", "-o", "KNAME,TYPE,PKNAME,FSTYPE"]) return args def test_excluded_disks_and_their_partitions_are_not_in_the_scan(self): - self.assertEqual(self.args(), ["--devices", "/dev/nvme0n1,/dev/nvme0n1p1,/dev/sdb,/dev/sdb1"]) + self.assertEqual(self.args(), ["--devices", "/dev/nvme0n1p1,/dev/sdb1"]) self.assertNotIn((("sda",), {}), self.standby.call_args_list) self.assertEqual({call.args[0] for call in self.standby.call_args_list}, {"nvme0n1", "sdb"}) def test_idle_disks_receive_no_power_query(self): self.excluded = set() self.policy.side_effect = lambda disk, excluded: "idle" if disk == "sda" else "read" - self.assertNotIn("/dev/sda", self.args()[1]) + self.assertNotIn("/dev/sda1", self.args()[1]) self.assertEqual({call.args[0] for call in self.standby.call_args_list}, {"nvme0n1", "sdb"}) def test_a_standby_disk_is_excluded_even_when_its_io_changed(self): self.excluded = set() self.standby.side_effect = lambda disk: disk == "sdb" - self.assertEqual(self.args(), ["--devices", "/dev/nvme0n1,/dev/nvme0n1p1,/dev/sda,/dev/sda1"]) + self.assertEqual(self.args(), ["--devices", "/dev/nvme0n1p1,/dev/sda1"]) def test_all_allowed_disks_keep_existing_scan_behaviour(self): self.excluded = set() @@ -63,7 +71,8 @@ class LvmScopeTests(unittest.TestCase): self.assertIsNone(self.args()) def test_bad_inventory_or_policy_never_authorizes_an_unrestricted_scan(self): - for inventory, code in (("", 0), (INVENTORY, 1), ("sda1 part sda", 0), ("../sda disk", 0)): + for inventory, code in (("", 0), (INVENTORY, 1), ("sda1 part sda", 0), + (lsblk(("sda1", "part", "sda", None)), 0), (lsblk(("../sda", "disk", None, None)), 0)): self.assertIsNone(self.args(inventory, code)) for error in (FileNotFoundError(), subprocess.TimeoutExpired("lsblk", 5)): with patch.object(scope.subprocess, "run", side_effect=error): @@ -72,6 +81,32 @@ class LvmScopeTests(unittest.TestCase): self.assertIsNone(self.args()) + def test_raid_multipath_and_encrypted_devices_follow_their_disks(self): + inventory = lsblk( + ("sda", "disk", None, None), ("sda1", "part", "sda", "linux_raid_member"), + ("sdb", "disk", None, None), ("sdb1", "part", "sdb", "linux_raid_member"), + ("md0", "raid1", "sda1", "LVM2_member"), ("md0", "raid1", "sdb1", "LVM2_member"), + ("sdc", "disk", None, None), ("sdc1", "part", "sdc", "crypto_LUKS"), + ("dm-0", "crypt", "sdc1", "LVM2_member"), ("dm-1", "lvm", "dm-0", "ext4"), + ("sdd", "disk", None, None), ("sde", "disk", None, None), + ("dm-2", "mpath", "sdd", None), ("dm-2", "mpath", "sde", None), ("dm-3", "part", "dm-2", "LVM2_member")) + self.excluded = {"sdb"} + # md0 reads sdb as well, so it stays out; the LUKS volume and the + # multipath partition are built only on readable disks. Disks that + # hold no physical volume are never named, so LVM does not open them. + self.assertEqual(self.args(inventory), ["--devices", "/dev/dm-0,/dev/dm-3"]) + self.excluded = set() + self.assertEqual(self.args(inventory), []) + + def test_the_scan_settles_its_own_reads_on_the_disks_it_read(self): + self.excluded = {"sda"} + settled = Mock() + self.disk_module.settle_own_reads = settled + self.args() + scope.settle_scan_reads() + settled.assert_called_once_with({"nvme0n1", "sdb"}) + + class LvmReaderTests(unittest.TestCase): def extract(self, file, name, owner=None, namespace=None): tree = ast.parse((SCRIPTS / file).read_text())