From c2936d45877d1a2f84c0668abb226965b3604295 Mon Sep 17 00:00:00 2001 From: MacRimi Date: Fri, 9 Oct 2026 21:00:33 +0200 Subject: [PATCH] fix(oci): a shared host directory is recognised by its filesystem, not its device number --- oci/remote/oci_carried_record.py | 10 +- oci/remote/oci_host_mounts.py | 97 +++++++++++++- oci/remote/oci_instance_transaction.py | 8 +- oci/src/proxmenux_oci/management.py | 3 +- oci/tests/test_carried_record.py | 36 +++++ oci/tests/test_host_source_identity.py | 175 +++++++++++++++++++++++++ oci/tests/test_work_backup.py | 34 +++++ 7 files changed, 358 insertions(+), 5 deletions(-) create mode 100644 oci/tests/test_host_source_identity.py diff --git a/oci/remote/oci_carried_record.py b/oci/remote/oci_carried_record.py index 68106e6c..725640dd 100644 --- a/oci/remote/oci_carried_record.py +++ b/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: diff --git a/oci/remote/oci_host_mounts.py b/oci/remote/oci_host_mounts.py index 12fec030..fe534e1d 100644 --- a/oci/remote/oci_host_mounts.py +++ b/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(): diff --git a/oci/remote/oci_instance_transaction.py b/oci/remote/oci_instance_transaction.py index d9941473..f17b0d86 100644 --- a/oci/remote/oci_instance_transaction.py +++ b/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: diff --git a/oci/src/proxmenux_oci/management.py b/oci/src/proxmenux_oci/management.py index bfe21fab..6ead495f 100644 --- a/oci/src/proxmenux_oci/management.py +++ b/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 diff --git a/oci/tests/test_carried_record.py b/oci/tests/test_carried_record.py index 54e5ded9..a4bda13f 100644 --- a/oci/tests/test_carried_record.py +++ b/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.""" diff --git a/oci/tests/test_host_source_identity.py b/oci/tests/test_host_source_identity.py new file mode 100644 index 00000000..6589af4b --- /dev/null +++ b/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() diff --git a/oci/tests/test_work_backup.py b/oci/tests/test_work_backup.py index 0735300e..9d0c6090 100644 --- a/oci/tests/test_work_backup.py +++ b/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()