fix(oci): read device lines Proxmox writes without path=

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)

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

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):
+    try:
-        raise ValueError(f'{key}: {translate("Incomplete device configuration")}')
+        fields = gpu_devices.device_fields(value)
-    fields = dict(part.split('=', 1) for part in parts)
+    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')

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

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)

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

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