Merge pull request #601 from jcastro/fix/oci-preserve-container-device-gid

fix(oci): preserve recorded container device GIDs

.github/scripts/tests/test_oci_storage_sk.py

@@ -202,7 +202,8 @@ class InputAndCancellationTests(StorageFixture):
 
     def test_real_menu_error_handler_has_slovak_heading_and_preserves_detail(self):
         ui=Mock();ui.choose.side_effect=['custom',None]
-        with patch.object(cli,'interactive_ui',return_value=ui),patch.object(custom,'explore',side_effect=ValueError('fiction-detail')), \
+        with patch.object(cli,'interactive_ui',return_value=ui),patch.object(cli,'_catalog_options',return_value=([],{}, {}, {}, [])), \
+                patch.object(custom,'explore',side_effect=ValueError('fiction-detail')), \
                 patch.object(updates,'UpdateCheck') as checker, \
                 patch.object(catalog_refresh,'CatalogRefresh') as catalog_checker:
             # Keep this wording/error-handler test offline and free of writes.

lang/de.json

@@ -9017,5 +9017,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ Festplattendaten werden NICHT gelöscht.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ Die Festplatte wird ausgehängt und aus /etc/fstab entfernt.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ Der /etc/fstab-Eintrag wird entfernt.",
-  "⚠ The disk will be unmounted.": "⚠ Die Festplatte wird ausgehängt."
+  "⚠ The disk will be unmounted.": "⚠ Die Festplatte wird ausgehängt.",
+  "Invalid container device GID": "Ungültige Geräte-GID im Container",
+  "The container device settings changed since the last recorded operation": "Die Geräteeinstellungen im Container haben sich seit dem letzten protokollierten Vorgang geändert",
+  "The recorded container device GID does not match its configuration": "Die protokollierte Geräte-GID im Container stimmt nicht mit der Konfiguration überein"
 }

lang/es.json

@@ -8987,5 +8987,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ Los datos del disco NO se borrarán.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ El disco se desmontará y se eliminará de /etc/fstab.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ Se eliminará la entrada /etc/fstab.",
-  "⚠ The disk will be unmounted.": "⚠ El disco se desmontará."
+  "⚠ The disk will be unmounted.": "⚠ El disco se desmontará.",
+  "Invalid container device GID": "El GID del dispositivo del contenedor no es válido",
+  "The container device settings changed since the last recorded operation": "La configuración del dispositivo del contenedor ha cambiado desde la última operación registrada",
+  "The recorded container device GID does not match its configuration": "El GID registrado del dispositivo del contenedor no coincide con su configuración"
 }

lang/fr.json

@@ -9016,5 +9016,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ Les données du disque ne seront PAS effacées.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ Le disque sera démonté et supprimé de /etc/fstab.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ L'entrée /etc/fstab sera supprimée.",
-  "⚠ The disk will be unmounted.": "⚠ Le disque sera démonté."
+  "⚠ The disk will be unmounted.": "⚠ Le disque sera démonté.",
+  "Invalid container device GID": "GID de périphérique du conteneur invalide",
+  "The container device settings changed since the last recorded operation": "La configuration du périphérique du conteneur a changé depuis la dernière opération enregistrée",
+  "The recorded container device GID does not match its configuration": "Le GID enregistré du périphérique du conteneur ne correspond pas à sa configuration"
 }

lang/it.json

@@ -9017,5 +9017,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ I dati del disco NON verranno cancellati.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ Il disco verrà smontato e rimosso da /etc/fstab.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ La voce /etc/fstab verrà rimossa.",
-  "⚠ The disk will be unmounted.": "⚠ Il disco verrà smontato."
+  "⚠ The disk will be unmounted.": "⚠ Il disco verrà smontato.",
+  "Invalid container device GID": "GID del dispositivo del container non valido",
+  "The container device settings changed since the last recorded operation": "Le impostazioni del dispositivo del container sono cambiate dall’ultima operazione registrata",
+  "The recorded container device GID does not match its configuration": "Il GID registrato del dispositivo del container non corrisponde alla sua configurazione"
 }

lang/pt.json

@@ -9017,5 +9017,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ Os dados do disco NÃO serão apagados.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ O disco será desmontado e removido de /etc/fstab.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ A entrada /etc/fstab será removida.",
-  "⚠ The disk will be unmounted.": "⚠ O disco será desmontado."
+  "⚠ The disk will be unmounted.": "⚠ O disco será desmontado.",
+  "Invalid container device GID": "GID do dispositivo do contentor inválido",
+  "The container device settings changed since the last recorded operation": "As definições do dispositivo do contentor mudaram desde a última operação registada",
+  "The recorded container device GID does not match its configuration": "O GID registado do dispositivo do contentor não corresponde à configuração"
 }

lang/sk.json

@@ -9017,5 +9017,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ Dáta na disku sa NEVYMAŽÚ.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ Disk sa odpojí a odstráni z /etc/fstab.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ Záznam v /etc/fstab bude odstránený.",
-  "⚠ The disk will be unmounted.": "⚠ Disk bude odpojený."
+  "⚠ The disk will be unmounted.": "⚠ Disk bude odpojený.",
+  "Invalid container device GID": "Neplatné GID zariadenia v kontajneri",
+  "The container device settings changed since the last recorded operation": "Nastavenia zariadenia v kontajneri sa od poslednej zaznamenanej operácie zmenili",
+  "The recorded container device GID does not match its configuration": "Zaznamenané GID zariadenia v kontajneri sa nezhoduje s jeho konfiguráciou"
 }

lang/sv.json

@@ -9017,5 +9017,8 @@
   "⚠ Disk data will NOT be erased.": "⚠ Diskdata kommer INTE att raderas.",
   "⚠ Disk will be unmounted and removed from /etc/fstab.": "⚠ Disken kommer att avmonteras och tas bort från /etc/fstab.",
   "⚠ The /etc/fstab entry will be removed.": "⚠ /etc/fstab-posten kommer att tas bort.",
-  "⚠ The disk will be unmounted.": "⚠ Disken kommer att avmonteras."
+  "⚠ The disk will be unmounted.": "⚠ Disken kommer att avmonteras.",
+  "Invalid container device GID": "Ogiltigt enhets-GID i containern",
+  "The container device settings changed since the last recorded operation": "Inställningarna för enheten i containern har ändrats sedan den senaste registrerade åtgärden",
+  "The recorded container device GID does not match its configuration": "Det registrerade enhets-GID:t i containern stämmer inte med konfigurationen"
 }

oci/remote/install_oci.sh

@@ -939,7 +939,7 @@ set_lxc_directive() {
 
 add_device() {
   local host_path=$1 expected_type=$2 mode_spec=$3 gid_strategy=$4 deny_write=$5
-  local device_uid=${6:-}
+  local device_uid=${6:-} container_gid=${7:-}
   local mode gid dev_value
   [[ $host_path == /dev/* && $host_path != *[[:space:]]* && $host_path != *","* ]] \
     || die "$(translate "Invalid device path:") $host_path"
@@ -963,7 +963,12 @@ add_device() {
       || die "$(translate "Invalid device UID:") $device_uid"
     dev_value="${dev_value},uid=${device_uid}"
   fi
-  if [[ $gid_strategy == host-device-gid ]]; then
+  if [[ -n $container_gid ]]; then
+    [[ $container_gid =~ ^[0-9]{1,10}$ ]] && (( container_gid < 4294967295 )) \
+      || die "$(translate "Invalid container device GID"): $container_gid"
+    gid=$container_gid
+    dev_value="${dev_value},gid=${gid}"
+  elif [[ $gid_strategy == host-device-gid ]]; then
     gid=$(stat -c '%g' "$host_path")
     dev_value="${dev_value},gid=${gid}"
   elif [[ $gid_strategy != none ]]; then
@@ -978,7 +983,7 @@ add_device() {
 }
 
 add_character_device() {
-  add_device "$1" character "$2" "$3" "$4" "${5:-}"
+  add_device "$1" character "$2" "$3" "$4" "${5:-}" "${6:-}"
 }
 
 prepare_nvidia_driver_links() {
@@ -2043,6 +2048,7 @@ while IFS= read -r DEVICE_ENCODED; do
   DEVICE=$(printf '%s' "$DEVICE_ENCODED" | base64 -d)
   DEVICE_KIND=$(jq -r '.kind // "character-device"' <<<"$DEVICE")
   DEVICE_GID_ENV=$(jq -r '.append_host_device_gid_to_environment // empty' <<<"$DEVICE")
+  CONTAINER_GID=$(jq -r '.container_gid // empty' <<<"$DEVICE")
   if [[ $DEVICE_KIND == nvidia-runtime ]]; then
     NVIDIA_GID_ENV=$DEVICE_GID_ENV
     if (( NVIDIA_RUNTIME_CONFIGURED == 0 )); then
@@ -2062,13 +2068,13 @@ while IFS= read -r DEVICE_ENCODED; do
   case "$DEVICE_KIND" in
     character-device)
       DEVICE_UID=$(jq -r '.uid // empty' <<<"$DEVICE")
-      add_character_device "$HOST_PATH" "$MODE" "$GID_STRATEGY" "$DENY_WRITE" "$DEVICE_UID"
+      add_character_device "$HOST_PATH" "$MODE" "$GID_STRATEGY" "$DENY_WRITE" "$DEVICE_UID" "$CONTAINER_GID"
       if [[ -n $DEVICE_GID_ENV ]]; then
         append_deployment_environment_csv "$DEVICE_GID_ENV" "$(stat -c '%g' "$HOST_PATH")"
       fi
       ;;
     block-device)
-      add_device "$HOST_PATH" block "$MODE" "$GID_STRATEGY" "$DENY_WRITE"
+      add_device "$HOST_PATH" block "$MODE" "$GID_STRATEGY" "$DENY_WRITE" "" "$CONTAINER_GID"
       if [[ -n $DEVICE_GID_ENV ]]; then
         append_deployment_environment_csv "$DEVICE_GID_ENV" "$(stat -c '%g' "$HOST_PATH")"
       fi
@@ -2078,7 +2084,7 @@ while IFS= read -r DEVICE_ENCODED; do
       FOUND_TREE_DEVICE=0
       while IFS= read -r TREE_DEVICE; do
         [[ -n $TREE_DEVICE ]] || continue
-        add_character_device "$TREE_DEVICE" "$MODE" "$GID_STRATEGY" "$DENY_WRITE"
+        add_character_device "$TREE_DEVICE" "$MODE" "$GID_STRATEGY" "$DENY_WRITE" "" "$CONTAINER_GID"
         FOUND_TREE_DEVICE=1
       done < <(find "$HOST_PATH" -print 2>/dev/null | while IFS= read -r path; do
         [[ -c $path ]] && printf '%s\n' "$path"

oci/remote/oci_accelerators.py

@@ -40,6 +40,14 @@ def drm_plan(deployment):
     return dict(deployment, devices=[d for d in deployment.get('devices', []) if d.get('kind') != 'nvidia-runtime'])
 
 
+def container_gid(device, host_value):
+    return drm.container_gid(device, host_value)
+
+
+def preserve_recorded_container_gids(record, candidate, config):
+    drm.preserve_recorded_container_gids(record, candidate, config)
+
+
 def planned(deployment):
     result = drm.planned(drm_plan(deployment))
     if nvidia.enabled(deployment):

oci/remote/oci_gpu_devices.py

@@ -88,10 +88,71 @@ def planned(deployment):
             raise ValueError(translate('Invalid device mode'))
         if device.get('gid_strategy') not in ('none', 'host-device-gid'):
             raise ValueError(translate('Unsupported device GID strategy'))
+        if 'container_gid' in device:
+            container_gid(device, value)
         result[path] = value
     return result
 
 
+def container_gid(device, host_value):
+    """The GID assigned to the device node in the LXC's user namespace.
+
+    A recorded container GID is distinct from the host device's GID. Proxmox
+    maps the configured LXC GID through the container's id map when it creates
+    its passthrough node; the numeric host GID is not automatically translated.
+    """
+    if 'container_gid' in device:
+        gid = device['container_gid']
+        if isinstance(gid, bool) or not isinstance(gid, int) or not 0 <= gid < 4294967295:
+            raise ValueError(translate('Invalid container device GID'))
+        return gid
+    return host_value['gid'] if device['gid_strategy'] == 'host-device-gid' else 0
+
+
+def _permission_fields(fields):
+    return (fields.get('path'), int(fields.get('mode', '0660'), 8),
+            int(fields.get('uid', 0)), int(fields.get('gid', 0)),
+            fields.get('deny-write', '0'))
+
+
+def preserve_recorded_container_gids(record, candidate, config):
+    """Carry forward only device GIDs already captured in the saved config.
+
+    This is not a host-to-container GID translation. The saved config is the
+    baseline for the GID Proxmox previously assigned inside the LXC. Any
+    unrecorded change is rejected, and the host device is still verified
+    independently by ``verify_baseline``.
+    """
+    saved_config = record.get('observed', {}).get('config')
+    if not isinstance(saved_config, str) or not saved_config:
+        return
+    saved = actual_devices(saved_config.encode())
+    current = actual_devices(config)
+    if set(saved) != set(current):
+        return  # check() below reports the device set mismatch.
+    if any(_permission_fields(saved[path]) != _permission_fields(current[path]) for path in saved):
+        raise ValueError(translate('The container device settings changed since the last recorded operation'))
+
+    original_devices = record.get('deployment', {}).get('devices', [])
+    old_snapshots = planned(record.get('deployment', {}))
+    for device in original_devices:
+        path = device.get('host_path')
+        if path not in saved or path not in old_snapshots:
+            continue
+        observed_gid = int(saved[path].get('gid', 0))
+        expected_gid = container_gid(device, old_snapshots[path])
+        if observed_gid != expected_gid and 'container_gid' in device:
+            raise ValueError(translate('The recorded container device GID does not match its configuration'))
+        if observed_gid != expected_gid:
+            device['container_gid'] = observed_gid
+
+        for desired in candidate.get('deployment', {}).get('devices', []):
+            if (desired.get('host_path') == path
+                    and desired.get('gid_strategy') == device.get('gid_strategy')
+                    and 'container_gid' not in desired):
+                desired['container_gid'] = observed_gid
+
+
 def verify(expected):
     for path, value in expected.items():
         if snapshot(path) != value:
@@ -137,7 +198,7 @@ def check(config, deployment):
         value = expected[path]
         requested_mode = device.get('mode', '0660')
         mode = value['mode'] if requested_mode == 'preserve-host' else int(requested_mode, 8)
-        gid = value['gid'] if device['gid_strategy'] == 'host-device-gid' else 0
+        gid = container_gid(device, value)
         if (set(fields) - {'path', 'mode', 'gid', 'uid', 'deny-write'}
                 or int(fields.get('mode', '0660'), 8) != mode
                 or int(fields.get('gid', 0)) != gid

oci/remote/oci_instance_transaction.py

@@ -745,6 +745,7 @@ def preflight(record, candidate, config, coordinated=None, in_place=False, basel
     if desired['rootfs'] != deployment['rootfs']:
         raise ValueError(translate('Changing the rootfs or its storage requires a separate migration'))
     actual = mounts(config)
+    gpu_devices.preserve_recorded_container_gids(record, candidate, config)
     gpu_devices.check(config, deployment)
     if baseline:
         gpu_devices.verify_baseline(record['observed'].get('gpu_devices', {}), deployment)

oci/remote/oci_native_modify.py

@@ -185,7 +185,7 @@ def plan(record, candidate, config):
             info = inventory[path]
             mode = info['mode'] if device.get('mode') == 'preserve-host' else int(device.get('mode', '0660'), 8)
             values[key] = (f"path={path},mode={mode:04o},uid={int(device.get('uid', 0))},"
-                           f"gid={info['gid'] if device['gid_strategy'] == 'host-device-gid' else 0},"
+                           f"gid={devices.container_gid(device, info)},"
                            f"deny-write={1 if device.get('deny_write') else 0}")
     if 'mounts' in changed:
         actual = transaction.mounts(config)

oci/remote/oci_stack_native.py

@@ -585,7 +585,7 @@ class NativeAdapter:
             if sha(config) != child.get('staged_config_sha256'):
                 raise ValueError(translate('The configuration of a new member changed after it was created'))
             member_tx.check_runtime_mounts(config, original['deployment'])
-            member_tx.gpu_devices.check(config, original['deployment'])
+            member_tx.gpu_devices.check(config, child['candidate_contract']['deployment'])
             child['candidate_host_sources'] = member_tx.candidate_host_sources(journal, child)
             child['validated_config_sha256'] = sha(config)
             instances.write(journal, child)

oci/tests/test_gpu_device_gid.py

@@ -0,0 +1,76 @@
+"""Keep recorded LXC device-node GIDs distinct from host device ownership."""
+from pathlib import Path
+import sys
+import unittest
+from unittest.mock import patch
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1] / 'remote'))
+
+import oci_gpu_devices as devices
+import oci_accelerators as accelerators
+
+
+HOST = {
+    'path': '/dev/dri/renderD128', 'major': 226, 'minor': 128,
+    'uid': 0, 'gid': 993, 'mode': 0o660,
+    'vendor': '0x8086', 'pci_path': '/sys/devices/pci0000:00/0000:00:02.0',
+}
+DEPLOYMENT = {'devices': [{
+    'kind': 'character-device', 'host_path': '/dev/dri/renderD128',
+    'container_path': '/dev/dri/renderD128', 'gid_strategy': 'host-device-gid',
+    'mode': '0660', 'drm_vendor_ids': ['0x8086', '0x1002'],
+}]}
+SAVED_CONFIG = 'dev0: path=/dev/dri/renderD128,mode=0660,deny-write=0,gid=44\n'
+CURRENT_CONFIG = SAVED_CONFIG.encode()
+
+
+class ContainerDeviceGidTests(unittest.TestCase):
+    @patch.object(devices, 'snapshot', return_value=HOST)
+    def test_recorded_guest_gid_is_carried_to_update_and_still_checks_host_identity(self, _snapshot):
+        record = {'deployment': {'devices': [dict(DEPLOYMENT['devices'][0])]},
+                  'observed': {'config': SAVED_CONFIG}}
+        candidate = {'deployment': {'devices': [dict(DEPLOYMENT['devices'][0])]}}
+
+        devices.preserve_recorded_container_gids(record, candidate, CURRENT_CONFIG)
+
+        self.assertEqual(record['deployment']['devices'][0]['container_gid'], 44)
+        self.assertEqual(candidate['deployment']['devices'][0]['container_gid'], 44)
+        self.assertEqual(devices.check(CURRENT_CONFIG, record['deployment'])['/dev/dri/renderD128'], HOST)
+        self.assertEqual(devices.check(CURRENT_CONFIG, candidate['deployment'])['/dev/dri/renderD128'], HOST)
+
+    @patch.object(devices, 'snapshot', return_value=HOST)
+    def test_unrecorded_guest_gid_change_is_rejected(self, _snapshot):
+        record = {'deployment': {'devices': [dict(DEPLOYMENT['devices'][0])]},
+                  'observed': {'config': SAVED_CONFIG}}
+        candidate = {'deployment': {'devices': [dict(DEPLOYMENT['devices'][0])]}}
+        changed = CURRENT_CONFIG.replace(b'gid=44', b'gid=45')
+
+        with self.assertRaisesRegex(ValueError, 'device settings changed'):
+            devices.preserve_recorded_container_gids(record, candidate, changed)
+
+    @patch.object(devices, 'snapshot', return_value=HOST)
+    def test_host_gid_is_not_automatically_mapped_to_guest_gid_44(self, _snapshot):
+        with self.assertRaisesRegex(ValueError, 'native GPU permissions were not kept'):
+            devices.check(CURRENT_CONFIG, DEPLOYMENT)
+
+        # Under Proxmox's default unprivileged map, guest GID 44 maps to host
+        # GID 100044; host GID 993 is outside that map.
+        self.assertEqual(100000 + 44, 100044)
+        self.assertFalse(100000 <= 993 < 165536)
+
+    @patch.object(devices, 'snapshot', return_value=HOST)
+    def test_changed_host_device_identity_or_permissions_still_fail_baseline(self, snapshot):
+        baseline = {'/dev/dri/renderD128': HOST}
+        current = dict(HOST, gid=992)
+        snapshot.return_value = current
+        with self.assertRaisesRegex(ValueError, 'GPU identity or permissions changed'):
+            accelerators.verify_baseline(baseline, DEPLOYMENT)
+
+    def test_container_gid_must_be_a_bounded_integer(self):
+        for invalid in (True, -1, 4294967295, '44'):
+            with self.subTest(invalid=invalid), self.assertRaisesRegex(ValueError, 'Invalid container device GID'):
+                devices.container_gid({'gid_strategy': 'host-device-gid', 'container_gid': invalid}, HOST)
+
+
+if __name__ == '__main__':
+    unittest.main()