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 ea1f497e..40560c21 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/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 9411b0ac..d49452fc 100644 --- a/AppImage/scripts/flask_server.py +++ b/AppImage/scripts/flask_server.py @@ -4193,13 +4193,17 @@ 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', *_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: + 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(): parts = line.split() @@ -5009,42 +5013,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 f7dbe5ad..8b6168a9 100644 --- a/AppImage/scripts/health_monitor.py +++ b/AppImage/scripts/health_monitor.py @@ -154,36 +154,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: @@ -2305,12 +2279,17 @@ 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, 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'} @@ -2335,6 +2314,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 new file mode 100644 index 00000000..43cf1896 --- /dev/null +++ b/AppImage/scripts/lvm_device_scope.py @@ -0,0 +1,116 @@ +# ========================================================== +# 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; +# 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): + """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 + topology = _topology() + if topology is None: + return None + 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() + 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) + 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 [] + # 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(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 new file mode 100644 index 00000000..30495950 --- /dev/null +++ b/tests/test_lvm_disk_read_policy.py @@ -0,0 +1,151 @@ +"""Automatic LVM readers must not bypass disk exclusions/idle policy.""" +import ast +import json +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 + +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): + 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", "-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/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/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/nvme0n1p1,/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), + (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): + self.assertIsNone(scope.lvm_device_args(self.standby)) + self.policy.side_effect = RuntimeError("policy unavailable") + 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()) + 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()