fix(oci): a shared host directory is recognised by its filesystem, not its device number

oci/remote/oci_carried_record.py

@@ -58,6 +58,14 @@ def running_pid(vmid):
     return int(pid) if result.returncode == 0 and pid.isdigit() else None
 
 
+def _host_directory_missing(vmid):
+    """Whether the container mounts a directory of the host that is not
+    there. It is not mounted then: Proxmox would leave it locked."""
+    from oci_host_mounts import missing_sources
+    result = _run('pct', 'config', str(int(vmid)))
+    return result.returncode != 0 or bool(missing_sources(result.stdout.encode()))
+
+
 @contextlib.contextmanager
 def container_root(vmid, mount_stopped=True):
     """The root filesystem of the container as the host sees it, or None. A
@@ -67,7 +75,7 @@ def container_root(vmid, mount_stopped=True):
     if pid:
         yield Path(f'/proc/{pid}/root')
         return
-    if not mount_stopped or _run('pct', 'mount', str(int(vmid))).returncode != 0:
+    if not mount_stopped or _host_directory_missing(vmid) or _run('pct', 'mount', str(int(vmid))).returncode != 0:
         yield None
         return
     try:

oci/remote/oci_host_mounts.py

@@ -1,9 +1,11 @@
 """Read-only identity checks for directory bind mounts; never manage their data."""
 from __future__ import annotations
 
+import os
 from pathlib import Path, PurePosixPath
 import re
 import stat
+import subprocess
 
 from oci_installation_state import parse_config
 from oci_ui import translate
@@ -18,6 +20,74 @@ def valid_path(value):
     return value
 
 
+MOUNTINFO = Path('/proc/self/mountinfo')
+BY_UUID = Path('/dev/disk/by-uuid')
+# Filesystems that number their folders again every time they are mounted.
+RENUMBERED = ('vfat', 'exfat', 'msdos')
+
+
+def _field(value):
+    return re.sub(r'\\([0-7]{3})', lambda match: chr(int(match.group(1), 8)), value)
+
+
+def _mounted_at(resolved):
+    """Type and source of the filesystem a path is on: the mount with the
+    longest mount point above it, the last one when several share it."""
+    found = None
+    try:
+        lines = MOUNTINFO.read_text().splitlines()
+    except OSError:
+        return None
+    for line in lines:
+        before, _, after = line.partition(' - ')
+        fields, origin = before.split(), after.split()
+        if len(fields) < 5 or len(origin) < 2:
+            continue
+        point = _field(fields[4])
+        if resolved == point or resolved.startswith(point.rstrip('/') + '/'):
+            if found is None or len(point) >= len(found[0]):
+                found = (point, origin[0], _field(origin[1]))
+    return found and found[1:]
+
+
+def _uuid(device):
+    """UUID of the filesystem on a block device, as the host publishes it."""
+    try:
+        number = os.stat(device).st_rdev
+        for link in BY_UUID.iterdir():
+            if os.stat(link).st_rdev == number:
+                return link.name
+    except OSError:
+        pass
+    try:
+        result = subprocess.run(['blkid', '-o', 'value', '-s', 'UUID', device],
+                                capture_output=True, text=True, timeout=10, check=False)
+    except (OSError, subprocess.SubprocessError):
+        return None
+    return result.stdout.strip() or None
+
+
+def filesystem(resolved):
+    """What tells the filesystem of a directory apart across restarts, and its
+    type. The device number does not: the host gives it again on every boot,
+    and to a disk every time it is connected. A filesystem on a block device
+    is told by its UUID; one without, as a ZFS dataset or a network share, by
+    what is mounted. The identity is None when the host does not tell."""
+    mounted = _mounted_at(resolved)
+    if mounted is None:
+        return None, None
+    kind, origin = mounted
+    if origin.startswith('/'):
+        try:
+            block = stat.S_ISBLK(os.stat(origin).st_mode)
+        except OSError:
+            block = False
+        if block:
+            uuid = _uuid(origin)
+            return (f'uuid:{uuid}' if uuid else None), kind
+    return f'{kind}:{origin}', kind
+
+
 def snapshot(source, allow_missing=False):
     path = Path(source)
     resolved = str(path.resolve())
@@ -29,9 +99,10 @@ def snapshot(source, allow_missing=False):
         return {'resolved_path': resolved, 'exists': False}
     if not stat.S_ISDIR(info.st_mode):
         raise ValueError(translate('This profile only supports directory bind mounts'))
+    identity, kind = filesystem(resolved)
     return {'resolved_path': resolved, 'exists': True, 'device': info.st_dev,
             'inode': info.st_ino, 'uid': info.st_uid, 'gid': info.st_gid,
-            'mode': stat.S_IMODE(info.st_mode)}
+            'mode': stat.S_IMODE(info.st_mode), 'filesystem': identity, 'fstype': kind}
 
 
 def validate_source(source, allow_missing=False, declared=False):
@@ -51,7 +122,20 @@ def validate_source(source, allow_missing=False, declared=False):
 
 def same_source(a, b):
     # Native application init may legitimately change permissions, not identity.
-    return all(a.get(k) == b.get(k) for k in ('resolved_path', 'exists', 'device', 'inode'))
+    if any(a.get(k) != b.get(k) for k in ('resolved_path', 'exists')):
+        return False
+    known = bool(a.get('filesystem') and b.get('filesystem'))
+    if known and a['filesystem'] != b['filesystem']:
+        return False
+    if a.get('inode') != b.get('inode') and not (known and a.get('fstype') in RENUMBERED):
+        return False
+    if known:
+        return True
+    # A record written before the filesystem was looked at has only the device
+    # number, which does not survive a restart: there is nothing to compare.
+    if 'filesystem' not in a or 'filesystem' not in b:
+        return True
+    return a.get('device') == b.get('device')
 
 
 def verify_sources(expected):
@@ -69,6 +153,15 @@ def verify_observation(expected, observed):
         raise ValueError(translate('The mount evidence does not match the verified directories'))
 
 
+def missing_sources(config):
+    """Host directories a container mounts that are not there, as when their
+    disk is not connected. Proxmox cannot mount the container without them,
+    and leaves it locked when it tries."""
+    return [value.split(',', 1)[0] for key, value in parse_config(config).items()
+            if re.fullmatch(r'mp[0-9]+', key) and value.startswith('/')
+            and not Path(value.split(',', 1)[0]).exists()]
+
+
 def capture_sources(config):
     result = {}
     for key, value in parse_config(config).items():

oci/remote/oci_instance_transaction.py

@@ -590,6 +590,8 @@ def freeze_host_sources(record, candidate, acknowledge_external_data, files=()):
         log('host directories: not included in the backup and not reverted by a recovery')
     if old - set(baseline):
         log('host directories without a recorded identity: their current identity is pinned')
+    if any('filesystem' not in previous for source, previous in baseline.items() if source in original):
+        log('host directories recorded without the identity of their filesystem: it is pinned now')
     return original, desired
 
 
@@ -914,11 +916,15 @@ def gib(size):
 def backup_size(vmid):
     """Bytes in use on the volumes that vzdump includes: the rootfs and the
     mount points with backup enabled."""
-    cfg = parse_config(run('pct', 'config', str(vmid)))
+    config = run('pct', 'config', str(vmid))
+    cfg = parse_config(config)
     included = {'rootfs'} | {key for key, value in cfg.items()
                              if re.fullmatch(r'mp[0-9]+', key) and 'backup=1' in value.split(',')}
     units = {'': 1, 'K': 1024, 'M': 1024**2, 'G': 1024**3, 'T': 1024**4, 'P': 1024**5}
     total = 0
+    for source in host_mounts.missing_sources(config):
+        # A stopped container is mounted to be measured.
+        raise ValueError(f"{translate('The container is not modified because a host directory is not available:')} {source}")
     for line in run('pct', 'df', str(vmid)).decode().splitlines()[1:]:
         fields = line.split()
         if not fields or fields[0] not in included:

oci/src/proxmenux_oci/management.py

@@ -305,7 +305,8 @@ def _work_backup_ready(project, ui, vmid):
     import oci_work_backup
     try:
         state = oci_work_backup.status(instances.ROOT, vmid)
-    except (OSError, ValueError, KeyError, subprocess.SubprocessError):
+    except (OSError, ValueError, KeyError, RuntimeError, subprocess.SubprocessError):
+        # What cannot be measured here is told by the engine when it stops.
         return True
     if state['fits'] or state['saved_fits']:
         return True

oci/tests/test_carried_record.py

@@ -78,6 +78,42 @@ class CarriedRecordTests(unittest.TestCase):
 
 
 
+
+class StoppedContainerTests(unittest.TestCase):
+    """A stopped container is mounted to reach its copy of the record, unless
+    a directory of the host it mounts is missing: Proxmox would fail to mount
+    it and leave it locked."""
+
+    def reach(self, config):
+        from types import SimpleNamespace
+        from unittest.mock import patch
+        calls = []
+
+        def run(*args):
+            calls.append(args[:2])
+            if args[0] == "lxc-info":
+                return SimpleNamespace(returncode=1, stdout="")
+            if args[1] == "config":
+                return SimpleNamespace(returncode=0, stdout=config)
+            return SimpleNamespace(returncode=0, stdout="")
+        with patch.object(carried, "_run", side_effect=run):
+            with carried.container_root(120) as rootfs:
+                pass
+        return rootfs, calls
+
+    def test_it_is_mounted_when_its_host_directories_are_there(self):
+        with tempfile.TemporaryDirectory() as directory:
+            rootfs, calls = self.reach(f"rootfs: local:120/disk.raw\nmp0: {directory},mp=/media,backup=0\n")
+        self.assertEqual(rootfs, Path("/var/lib/lxc/120/rootfs"))
+        self.assertEqual(calls, [("lxc-info", "-n"), ("pct", "config"), ("pct", "mount"), ("pct", "unmount")])
+
+    def test_it_is_left_alone_when_one_is_missing(self):
+        with tempfile.TemporaryDirectory() as directory:
+            rootfs, calls = self.reach(f"rootfs: local:120/disk.raw\nmp0: {directory}/gone,mp=/media,backup=0\n")
+        self.assertIsNone(rootfs)
+        self.assertNotIn(("pct", "mount"), calls)
+
+
 class ShownSyncTests(unittest.TestCase):
     """Every installed application is looked at after an operation, which
     takes a while with many of them: the screen says what is being done."""

oci/tests/test_host_source_identity.py

@@ -0,0 +1,175 @@
+"""A shared host directory is recognised by its path, its folder and the
+filesystem it is on, not by the device number the host gives that filesystem:
+the number changes on every boot and every time a disk is connected."""
+
+from pathlib import Path
+import stat
+import sys
+import tempfile
+from types import SimpleNamespace
+import unittest
+from unittest.mock import patch
+
+ROOT = Path(__file__).resolve().parents[1]
+sys.path.insert(0, str(ROOT / "remote"))
+
+import oci_host_mounts as host_mounts
+
+
+def source(**changes):
+    value = {"resolved_path": "/mnt/pve/usb/media", "exists": True, "device": 2081, "inode": 13,
+             "uid": 1000, "gid": 1000, "mode": 0o775,
+             "filesystem": "uuid:58a41de6-b98f-481a-9698-aee647fccba3", "fstype": "ext4"}
+    value.update(changes)
+    return value
+
+
+def recorded_before(**changes):
+    value = source(**changes)
+    del value["filesystem"], value["fstype"]
+    return value
+
+
+class SameSourceTests(unittest.TestCase):
+    def test_a_disk_that_comes_back_under_another_device_is_the_same_source(self):
+        self.assertTrue(host_mounts.same_source(source(), source(device=2097)))
+
+    def test_another_filesystem_in_the_same_place_is_not(self):
+        other = source(filesystem="uuid:0c1f6a51-2d3e-4b7a-9f10-3a5b8f2d7c44")
+        self.assertFalse(host_mounts.same_source(source(), other))
+        # Not even when the host gives it the device number the first one had.
+        self.assertFalse(host_mounts.same_source(source(), dict(other, device=2081)))
+
+    def test_another_folder_or_another_path_is_not(self):
+        self.assertFalse(host_mounts.same_source(source(), source(inode=14)))
+        self.assertFalse(host_mounts.same_source(source(), source(resolved_path="/mnt/pve/usb/other")))
+        self.assertFalse(host_mounts.same_source(source(), {"resolved_path": "/mnt/pve/usb/media", "exists": False}))
+
+    def test_permissions_are_not_part_of_the_identity(self):
+        self.assertTrue(host_mounts.same_source(source(), source(uid=101000, gid=101000, mode=0o2775)))
+
+    def test_a_filesystem_that_renumbers_its_folders_is_told_by_its_identity(self):
+        fat = source(filesystem="uuid:094E-2E73", fstype="exfat", inode=403)
+        self.assertTrue(host_mounts.same_source(fat, dict(fat, inode=405)))
+        self.assertFalse(host_mounts.same_source(fat, dict(fat, inode=405, filesystem="uuid:7EFB-F094")))
+
+    def test_a_record_made_before_the_filesystem_was_looked_at_has_no_device_to_compare(self):
+        self.assertTrue(host_mounts.same_source(recorded_before(), source(device=2097)))
+        self.assertFalse(host_mounts.same_source(recorded_before(), source(inode=14)))
+
+    def test_without_an_identity_the_device_number_is_what_is_left(self):
+        unknown = source(filesystem=None)
+        self.assertTrue(host_mounts.same_source(unknown, source(filesystem=None)))
+        self.assertFalse(host_mounts.same_source(unknown, source(filesystem=None, device=2097)))
+
+    def test_two_missing_directories_at_the_same_path_are_the_same(self):
+        missing = {"resolved_path": "/mnt/new", "exists": False}
+        self.assertTrue(host_mounts.same_source(missing, dict(missing)))
+
+
+class FilesystemTests(unittest.TestCase):
+    MOUNTS = (
+        "34 2 252:1 / / rw,relatime shared:1 - ext4 /dev/mapper/pve-root rw\n"
+        "314 34 0:68 / /mnt/pve/Public rw,relatime shared:310 - nfs4 192.168.0.15:/volume3/Public rw,vers=4.1\n"
+        "90 34 0:44 / /tank/media rw,relatime - zfs tank/media rw,xattr\n"
+        "95 34 8:33 / /mnt/pve/usb rw,relatime - ext4 /dev/sdc1 rw\n"
+        "96 34 8:49 / /mnt/my\\040disk rw,relatime - exfat /dev/sdd1 rw\n"
+    )
+
+    def setUp(self):
+        directory = tempfile.TemporaryDirectory()
+        self.addCleanup(directory.cleanup)
+        self.mounts = Path(directory.name) / "mountinfo"
+        self.mounts.write_text(self.MOUNTS)
+        patcher = patch.object(host_mounts, "MOUNTINFO", self.mounts)
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def blocks(self, uuids):
+        """The host as seen by the module: these devices exist and have these UUIDs."""
+        real = host_mounts.os.stat
+
+        def fake(path, *args, **kwargs):
+            if str(path) in uuids:
+                return SimpleNamespace(st_mode=stat.S_IFBLK | 0o660, st_rdev=1)
+            return real(path, *args, **kwargs)
+        return (patch.object(host_mounts.os, "stat", side_effect=fake),
+                patch.object(host_mounts, "_uuid", side_effect=lambda device: uuids.get(device)))
+
+    def test_a_network_share_or_a_dataset_is_told_by_what_is_mounted(self):
+        self.assertEqual(host_mounts.filesystem("/mnt/pve/Public/films"),
+                         ("nfs4:192.168.0.15:/volume3/Public", "nfs4"))
+        self.assertEqual(host_mounts.filesystem("/tank/media"), ("zfs:tank/media", "zfs"))
+
+    def test_a_disk_is_told_by_the_uuid_of_its_filesystem(self):
+        stat_patch, uuid_patch = self.blocks({"/dev/sdc1": "58a41de6", "/dev/sdd1": "094E-2E73",
+                                              "/dev/mapper/pve-root": "93310121"})
+        with stat_patch, uuid_patch:
+            self.assertEqual(host_mounts.filesystem("/mnt/pve/usb/media"), ("uuid:58a41de6", "ext4"))
+            # The longest mount point above the path, with its spaces.
+            self.assertEqual(host_mounts.filesystem("/mnt/my disk/photos"), ("uuid:094E-2E73", "exfat"))
+            self.assertEqual(host_mounts.filesystem("/mnt/pve/usbstick"), ("uuid:93310121", "ext4"))
+
+    def test_a_disk_whose_uuid_the_host_does_not_tell_has_no_identity(self):
+        stat_patch, uuid_patch = self.blocks({"/dev/sdc1": None})
+        with stat_patch, uuid_patch:
+            self.assertEqual(host_mounts.filesystem("/mnt/pve/usb"), (None, "ext4"))
+
+    def test_the_last_filesystem_mounted_on_a_place_is_the_one_there(self):
+        self.mounts.write_text(self.MOUNTS + "97 34 0:70 / /tank/media rw - nfs4 nas:/media rw\n")
+        self.assertEqual(host_mounts.filesystem("/tank/media/x"), ("nfs4:nas:/media", "nfs4"))
+
+    def test_a_host_that_does_not_list_its_mounts_gives_no_identity(self):
+        self.mounts.unlink()
+        self.assertEqual(host_mounts.filesystem("/mnt/pve/usb"), (None, None))
+
+    def test_the_snapshot_of_a_directory_carries_the_identity(self):
+        with tempfile.TemporaryDirectory() as directory, \
+                patch.object(host_mounts, "filesystem", return_value=("zfs:tank/media", "zfs")):
+            value = host_mounts.snapshot(directory)
+        self.assertEqual((value["filesystem"], value["fstype"], value["exists"]), ("zfs:tank/media", "zfs", True))
+        self.assertIn("device", value)
+
+
+
+class UpdateOfAnApplicationWithASharedDirectoryTests(unittest.TestCase):
+    """The first thing an update or a modification does with the directories
+    the application shares with the host."""
+
+    def freeze(self, directory, recorded):
+        import oci_instance_transaction as transaction
+        mount = {"type": "host-bind", "source": directory, "container_path": "/media"}
+        record = {"deployment": {"mounts": [mount]}, "observed": {"host_bind_sources": {directory: recorded}}}
+        with patch.object(transaction, "log") as log:
+            original, desired = transaction.freeze_host_sources(record, {"deployment": {"mounts": [mount]}}, True)
+        return original, desired, [call.args[0] for call in log.call_args_list]
+
+    def test_the_disk_came_back_under_another_device(self):
+        with tempfile.TemporaryDirectory() as directory:
+            now = host_mounts.snapshot(directory)
+            # As the release before this one recorded it, and with the device it had then.
+            recorded = {k: v for k, v in now.items() if k not in ("filesystem", "fstype")}
+            recorded["device"] += 16
+            original, desired, logged = self.freeze(directory, recorded)
+            self.assertEqual(original[directory]["inode"], now["inode"])
+            self.assertEqual(desired, original)
+            self.assertTrue(any("identity of their filesystem" in line for line in logged))
+
+    def test_another_folder_in_its_place_stops_the_operation(self):
+        with tempfile.TemporaryDirectory() as directory:
+            recorded = dict(host_mounts.snapshot(directory))
+            recorded["inode"] += 1
+            with self.assertRaisesRegex(ValueError, "does not match its recorded identity"):
+                self.freeze(directory, recorded)
+
+    def test_another_filesystem_in_its_place_stops_the_operation(self):
+        with tempfile.TemporaryDirectory() as directory:
+            with patch.object(host_mounts, "filesystem", return_value=("uuid:first", "ext4")):
+                recorded = host_mounts.snapshot(directory)
+            with patch.object(host_mounts, "filesystem", return_value=("uuid:second", "ext4")), \
+                    self.assertRaisesRegex(ValueError, "does not match its recorded identity"):
+                self.freeze(directory, recorded)
+
+
+if __name__ == "__main__":
+    unittest.main()

oci/tests/test_work_backup.py

@@ -247,6 +247,15 @@ class Menu(unittest.TestCase):
         self.assertEqual(len(shown), 1)
         self.assertIn("No other storage", shown[0])
 
+    def test_a_space_that_cannot_be_measured_leaves_the_answer_to_the_engine(self):
+        sys.path.insert(0, str(ROOT / "src"))
+        from proxmenux_oci import management
+        ui = type("UI", (), {"message": lambda *a, **k: self.fail("nothing is shown"),
+                              "choose": lambda *a, **k: self.fail("nothing is asked")})()
+        for failure in (RuntimeError("pct failed with exit code 255"), ValueError("a host directory is not available")):
+            with patch.object(work, "status", side_effect=failure):
+                self.assertTrue(management._work_backup_ready(ROOT, ui, 109))
+
     def test_a_scheduled_run_asks_nothing_and_lets_the_engine_decide(self):
         result, shown, status, _ = self.ready(self.STATE, unattended=True)
         self.assertTrue(result)
@@ -254,5 +263,30 @@ class Menu(unittest.TestCase):
         status.assert_not_called()
 
 
+
+class MissingHostDirectory(unittest.TestCase):
+    """The disk a shared directory is on is not connected: the container is
+    not mounted to be measured, which would leave it locked."""
+
+    def test_the_size_is_not_measured_and_the_directory_is_named(self):
+        import oci_instance_transaction as transaction
+        with tempfile.TemporaryDirectory() as directory:
+            present, missing = Path(directory) / "here", Path(directory) / "gone"
+            present.mkdir()
+            config = (f"rootfs: local-lvm:vm-111-disk-0,size=4G\nmp0: local-lvm:vm-111-disk-1,mp=/config,backup=1,size=1G\n"
+                      f"mp1: {present},mp=/shares/a,backup=0\nmp2: {missing},mp=/shares/b,backup=0\n").encode()
+            calls = []
+
+            def run(*args):
+                calls.append(args[:2])
+                return config if args[1] == "config" else b"MP VOLUME SIZE USED AVAIL USE% PATH\nrootfs x 4G 1G 3G 25% /\n"
+            with patch.object(transaction, "run", side_effect=run):
+                with self.assertRaisesRegex(ValueError, f"host directory is not available: {missing}"):
+                    transaction.backup_size(111)
+                self.assertEqual(calls, [("pct", "config")])
+                missing.mkdir()
+                self.assertEqual(transaction.backup_size(111), 1024 ** 3)
+
+
 if __name__ == "__main__":
     unittest.main()