Merge pull request #464 from jcastro/fix/lvm-disk-read-policy

Apply disk exclusions and idle policy to automatic LVM scans

.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

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"

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)

AppImage/scripts/flask_server.py

@@ -4188,13 +4188,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()
@@ -5004,42 +5008,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):

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,12 +2277,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'}
@@ -2333,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:

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

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