diff --git a/oci/remote/oci_accelerators.py b/oci/remote/oci_accelerators.py index 353356bf..d6f24b84 100644 --- a/oci/remote/oci_accelerators.py +++ b/oci/remote/oci_accelerators.py @@ -69,7 +69,7 @@ def check(config, deployment): filtered = [] for line in config.splitlines(keepends=True): if re.match(rb'dev[0-9]+: ', line): - fields = dict(part.split('=', 1) for part in line.decode().strip().split(': ', 1)[1].split(',')) + fields = drm.device_fields(line.decode().strip().split(': ', 1)[1]) if fields.get('path') in value['devices']: continue filtered.append(line) diff --git a/oci/remote/oci_gpu_devices.py b/oci/remote/oci_gpu_devices.py index e4502470..1155ff6e 100644 --- a/oci/remote/oci_gpu_devices.py +++ b/oci/remote/oci_gpu_devices.py @@ -98,11 +98,27 @@ def verify(expected): raise ValueError(translate('The GPU identity or permissions changed; the container is not modified')) +def device_fields(value): + """The options of a devN line. Proxmox writes its path either as `path=` + or, when the device was added from its own interface, as the first value + without a name: `dev0: /dev/dri/renderD128,gid=993`.""" + fields = {} + for index, part in enumerate(value.split(',')): + if '=' in part: + key, item = part.split('=', 1) + fields[key] = item + elif index == 0 and part: + fields['path'] = part + else: + raise ValueError(translate('Incomplete device configuration')) + return fields + + def actual_devices(config): result = {} for key, value in parse_config(config).items(): if re.fullmatch(r'dev[0-9]+', key): - fields = dict(part.split('=', 1) for part in value.split(',')) + fields = device_fields(value) path = fields.get('path') if path in result: raise ValueError(translate('Duplicated GPU device in the container')) diff --git a/oci/remote/oci_instance_reconcile.py b/oci/remote/oci_instance_reconcile.py index 9a03412d..6df08fee 100644 --- a/oci/remote/oci_instance_reconcile.py +++ b/oci/remote/oci_instance_reconcile.py @@ -58,9 +58,10 @@ def _mount(key, value, vmid): def _device(key, value): parts = value.split(',') - if any('=' not in part for part in parts): - raise ValueError(f'{key}: {translate("Incomplete device configuration")}') - fields = dict(part.split('=', 1) for part in parts) + try: + fields = gpu_devices.device_fields(value) + except ValueError: + raise ValueError(f'{key}: {translate("Incomplete device configuration")}') from None if len(fields) != len(parts) or set(fields) - {'path', 'mode', 'gid', 'uid', 'deny-write'}: raise ValueError(f'{key}: {translate("Unsupported device options")}') path = fields.get('path') diff --git a/oci/remote/oci_native_modify.py b/oci/remote/oci_native_modify.py index 44670a78..40fbc5f1 100644 --- a/oci/remote/oci_native_modify.py +++ b/oci/remote/oci_native_modify.py @@ -171,7 +171,7 @@ def plan(record, candidate, config): else: delete.append('env') if 'devices' in changed: inventory = devices.planned(new) - existing = {v['path']: k for k, v in ((k, dict(p.split('=', 1) for p in v.split(','))) + existing = {v['path']: k for k, v in ((k, devices.device_fields(v)) for k, v in cfg.items() if re.fullmatch(r'dev[0-9]+', k))} used = set(existing.values()) for path, key in existing.items(): diff --git a/oci/remote/oci_nvidia_runtime.py b/oci/remote/oci_nvidia_runtime.py index 3c150fb8..1ba4182b 100644 --- a/oci/remote/oci_nvidia_runtime.py +++ b/oci/remote/oci_nvidia_runtime.py @@ -7,7 +7,7 @@ from pathlib import Path, PurePosixPath import stat import subprocess -from oci_gpu_devices import actual_devices +from oci_gpu_devices import actual_devices, device_fields from oci_ui import translate KEY = '_nvidia_runtime' @@ -104,7 +104,7 @@ def refresh_plan(config, previous, current=None, same_gpu=True): if line.startswith('dev') and ': ' in line: key, properties = line.split(': ', 1) if key[3:].isdigit(): - fields = dict(part.split('=', 1) for part in properties.split(',')) + fields = device_fields(properties) if fields.get('path') in previous['devices']: continue kept.append(line) diff --git a/oci/remote/oci_ui.sh b/oci/remote/oci_ui.sh index dbab0cd7..3a9a2c42 100644 --- a/oci/remote/oci_ui.sh +++ b/oci/remote/oci_ui.sh @@ -208,7 +208,8 @@ oci_apply_extra_devices() { || die "$(translate "Invalid device path:") $path" [[ -c $path ]] || die "$(translate "The character device does not exist:") $path" [[ $mode =~ ^0?[0-7]{3}$ ]] || die "$(translate "Invalid device mode:") $mode" - pct config "$vmid" | grep -Eq "^dev[0-9]+: (.*,)?path=${path}(,|$)" && continue + # Proxmox may write the path without `path=` when it was added from its interface. + pct config "$vmid" | grep -Eq "^dev[0-9]+: ((.*,)?path=)?${path}(,|$)" && continue index=0 while pct config "$vmid" | grep -q "^dev${index}:"; do index=$((index + 1)); done gid=$(stat -c '%g' "$path") diff --git a/oci/tests/test_device_line_default_path.py b/oci/tests/test_device_line_default_path.py new file mode 100644 index 00000000..31aae2d7 --- /dev/null +++ b/oci/tests/test_device_line_default_path.py @@ -0,0 +1,77 @@ +"""A device added from the Proxmox interface is written with its path as the +first value, without `path=`. Every reader of the container configuration +accepts both forms.""" +from pathlib import Path +import re +import subprocess +import sys +import unittest + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT / 'remote')) + +import oci_gpu_devices as devices # noqa: E402 +import oci_instance_reconcile as reconcile # noqa: E402 + + +class DeviceFieldsTests(unittest.TestCase): + def test_both_forms_give_the_same_fields(self): + expected = {'path': '/dev/dri/renderD128', 'gid': '993'} + self.assertEqual(devices.device_fields('/dev/dri/renderD128,gid=993'), expected) + self.assertEqual(devices.device_fields('path=/dev/dri/renderD128,gid=993'), expected) + self.assertEqual(devices.device_fields('/dev/apex_0'), {'path': '/dev/apex_0'}) + + def test_a_value_without_name_after_the_first_is_refused(self): + for value in ('/dev/dri/renderD128,993', 'gid=993,/dev/dri/renderD128', ''): + with self.subTest(value=value), self.assertRaises(ValueError): + devices.device_fields(value) + + def test_actual_devices_reads_a_line_written_by_proxmox(self): + config = (b'arch: amd64\n' + b'dev0: /dev/dri/renderD128,gid=993\n' + b'dev1: path=/dev/apex_0,mode=0660\n') + self.assertEqual(devices.actual_devices(config), { + '/dev/dri/renderD128': {'path': '/dev/dri/renderD128', 'gid': '993'}, + '/dev/apex_0': {'path': '/dev/apex_0', 'mode': '0660'}, + }) + + def test_the_same_device_in_both_forms_is_a_duplicate(self): + config = b'dev0: /dev/dri/renderD128\ndev1: path=/dev/dri/renderD128\n' + with self.assertRaises(ValueError): + devices.actual_devices(config) + + def test_adoption_names_the_line_it_cannot_read(self): + with self.assertRaisesRegex(ValueError, '^dev2: '): + reconcile._device('dev2', '/dev/dri/renderD128,993') + + +class NoPairParsingLeftTests(unittest.TestCase): + def test_no_reader_builds_the_fields_from_pairs_alone(self): + pattern = re.compile(r"dict\(\s*\w+\.split\('=', 1\) for \w+ in .*split\(','\)") + for path in sorted((ROOT / 'remote').glob('*.py')): + source = path.read_text() + for line in source.splitlines(): + if 'dev' in line or 'properties' in line: + with self.subTest(file=path.name): + self.assertIsNone(pattern.search(line), line) + + +class ShellDuplicateCheckTests(unittest.TestCase): + def check(self, config, path): + script = ('pct() { printf "%s" "$CONFIG"; }\n' + f'pct config 1 | grep -Eq "^dev[0-9]+: ((.*,)?path=)?{path}(,|$)"') + return subprocess.run(['bash', '-c', script], env={'CONFIG': config, 'PATH': '/usr/bin:/bin'}).returncode + + def test_the_installer_sees_a_device_already_added_in_either_form(self): + self.assertEqual(self.check('dev0: /dev/ttyUSB0,gid=20\n', '/dev/ttyUSB0'), 0) + self.assertEqual(self.check('dev0: mode=0660,path=/dev/ttyUSB0\n', '/dev/ttyUSB0'), 0) + self.assertEqual(self.check('dev0: /dev/ttyUSB1\n', '/dev/ttyUSB0'), 1) + self.assertEqual(self.check('dev0: /dev/ttyUSB01\n', '/dev/ttyUSB0'), 1) + + def test_the_installer_uses_that_check(self): + source = (ROOT / 'remote/oci_ui.sh').read_text() + self.assertIn('grep -Eq "^dev[0-9]+: ((.*,)?path=)?${path}(,|$)"', source) + + +if __name__ == '__main__': + unittest.main()