NAS-134791 / 25.10 / Add CDROM device type (#16483)

## Problem

Currently users need to repack windows iso because it is not supported
out of the box by incus as there are missing drivers.

## Solution

While this gets fixed upstream, we have decided to use `raw.qemu` and
specify relevant CDROM directly so it works as expected with windows.
However while doing this, we see boot priority does not take effect for
CDROM entry as incus first initiates qemu process with qemu cmd where
this CDROM entry is specified and then using QMP, it dynamically handles
devices for the virt instance which overrides boot entry. So to get that
to work, we still need to specify the CDROM as a disk device in incus
and everything else falls into place nicely. It has been confirmed that
this works as expected with windows and that it boots nicely.

Author: Qubad786

src/middlewared/middlewared/api/v25_10_0/virt_device.py

@@ -126,8 +126,14 @@ class PCI(Device):
     address: NonEmptyString
 
 
+class CDROM(Device):
+    dev_type: Literal['CDROM']
+    source: NonEmptyString
+    boot_priority: int | None = Field(default=None, ge=0)
+
+
 DeviceType: TypeAlias = Annotated[
-    Disk | GPU | Proxy | TPM | USB | NIC | PCI,
+    Disk | GPU | Proxy | TPM | USB | NIC | PCI | CDROM,
     Field(discriminator='dev_type')
 ]
 

src/middlewared/middlewared/plugins/virt/instance.py

@@ -25,8 +25,9 @@
 
 from .utils import (
     create_vnc_password_file, get_max_boot_priority_device, get_root_device_dict, get_vnc_info_from_config,
-    get_vnc_password_file_path, incus_call, incus_call_and_wait, incus_pool_to_storage_pool,
-    root_device_pool_from_raw, VirtGlobalStatus, storage_pool_to_incus_pool, VNC_BASE_PORT,
+    VirtGlobalStatus, incus_call, incus_call_and_wait, incus_pool_to_storage_pool, root_device_pool_from_raw,
+    storage_pool_to_incus_pool, validate_device_name, generate_qemu_cmd, generate_qemu_cdrom_metadata,
+    INCUS_METADATA_CDROM_KEY,
 )
 
 
@@ -255,7 +256,9 @@ async def validate(self, new, schema_name, verrors, old=None):
                     if any(new['vnc_port'] in v for v in port_mapping.values()):
                         verrors.add(f'{schema_name}.vnc_port', 'VNC port is already in use by another virt instance')
 
-    def __data_to_config(self, instance_name: str, data: dict, raw: dict = None, instance_type=None):
+    def __data_to_config(
+        self, instance_name: str, data: dict, devices: dict, raw: dict = None, instance_type=None
+    ):
         config = {}
         if 'environment' in data:
             # If we are updating environment we need to remove current values
@@ -291,17 +294,10 @@ def __data_to_config(self, instance_name: str, data: dict, raw: dict = None, ins
                     'vnc_port': data['vnc_port'],
                     'vnc_password': data['vnc_password'],
                 }),
+                INCUS_METADATA_CDROM_KEY: generate_qemu_cdrom_metadata(devices),
             })
 
-            if data.get('enable_vnc') and data.get('vnc_port'):
-                vnc_config = f'-vnc :{data["vnc_port"] - VNC_BASE_PORT}'
-                if data.get('vnc_password'):
-                    vnc_config = (f'-object secret,id=vnc0,file={get_vnc_password_file_path(instance_name)} '
-                                  f'{vnc_config},password-secret=vnc0')
-
-                config['raw.qemu'] = vnc_config
-            if data.get('enable_vnc') is False:
-                config['raw.qemu'] = ''
+            config['raw.qemu'] = generate_qemu_cmd(config, instance_name)
         else:
             config.update({
                 'security.privileged': 'true' if data.get('privileged_mode') else 'false',
@@ -424,6 +420,7 @@ async def do_create(self, job, data):
             }
         }
         for i in data_devices:
+            validate_device_name(i, verrors)
             await self.middleware.call(
                 'virt.instance.validate_device', i, 'virt_instance_create', verrors, data['name'], data['instance_type']
             )
@@ -469,7 +466,7 @@ async def running_cb(data):
             await incus_call_and_wait('1.0/instances', 'post', {'json': {
                 'name': data['name'],
                 'ephemeral': False,
-                'config': self.__data_to_config(data['name'], data, instance_type=data['instance_type']),
+                'config': self.__data_to_config(data['name'], data, devices, instance_type=data['instance_type']),
                 'devices': devices,
                 'source': source,
                 'profiles': ['default'],
@@ -527,7 +524,9 @@ async def do_update(self, job, oid, data):
 
         verrors.check()
 
-        instance['raw']['config'].update(self.__data_to_config(oid, data, instance['raw']['config'], instance['type']))
+        instance['raw']['config'].update(
+            self.__data_to_config(oid, data, instance['raw']['devices'], instance['raw']['config'], instance['type'])
+        )
         if data.get('root_disk_size') or data.get('root_disk_io_bus'):
             if (pool := root_device_pool_from_raw(instance['raw'])) is None:
                 raise CallError(f'{oid}: instance does not have a configured pool')

src/middlewared/middlewared/plugins/virt/instance_device.py

@@ -15,7 +15,10 @@
     VirtInstanceBootableDiskArgs, VirtInstanceBootableDiskResult,
 )
 from middlewared.async_validators import check_path_resides_within_volume
-from .utils import get_max_boot_priority_device, incus_call_and_wait, incus_pool_to_storage_pool, storage_pool_to_incus_pool
+from .utils import (
+    get_max_boot_priority_device, incus_call_and_wait, incus_pool_to_storage_pool, storage_pool_to_incus_pool,
+    validate_device_name, CDROM_PREFIX, update_instance_metadata_and_qemu_cmd_on_device_change,
+)
 
 
 class VirtInstanceDeviceService(Service):
@@ -62,15 +65,23 @@ async def incus_to_device(self, name: str, incus: dict[str, Any], context: dict)
 
         match incus['type']:
             case 'disk':
-                device.update({
-                    'dev_type': 'DISK',
-                    'source': incus.get('source'),
-                    'storage_pool': incus_pool_to_storage_pool(incus.get('pool')),
-                    'destination': incus.get('path'),
-                    'description': f'{incus.get("source")} -> {incus.get("path")}',
-                    'boot_priority': int(incus['boot.priority']) if incus.get('boot.priority') else None,
-                    'io_bus': incus['io.bus'].upper() if incus.get('io.bus') else None,
-                })
+                if name.startswith(CDROM_PREFIX):
+                    device.update({
+                        'dev_type': 'CDROM',
+                        'source': incus.get('source'),
+                        'description': f'{incus.get("source")!r} CDROM device source',
+                        'boot_priority': int(incus['boot.priority']) if incus.get('boot.priority') else None,
+                    })
+                else:
+                    device.update({
+                        'dev_type': 'DISK',
+                        'source': incus.get('source'),
+                        'storage_pool': incus_pool_to_storage_pool(incus.get('pool')),
+                        'destination': incus.get('path'),
+                        'description': f'{incus.get("source")} -> {incus.get("path")}',
+                        'boot_priority': int(incus['boot.priority']) if incus.get('boot.priority') else None,
+                        'io_bus': incus['io.bus'].upper() if incus.get('io.bus') else None,
+                    })
             case 'nic':
                 device.update({
                     'dev_type': 'NIC',
@@ -195,7 +206,14 @@ async def device_to_incus(self, instance_type: str, device: dict[str, Any]) -> d
                         new['pool'] = None
                 if device.get('io_bus'):
                     new['io.bus'] = device['io_bus'].lower()
-
+            case 'CDROM':
+                new |= {
+                    'type': 'disk',
+                    'source': device['source'],
+                    'path': None,
+                }
+                if device['boot_priority'] is not None:
+                    new['boot.priority'] = str(device['boot_priority'])
             case 'NIC':
                 new.update({
                     'type': 'nic',
@@ -266,6 +284,9 @@ async def generate_device_name(self, device_names: list[str], device_type: str)
         name = device_type.lower()
         if name == 'nic':
             name = 'eth'
+        elif name == 'cdrom':
+            name = CDROM_PREFIX
+
         i = 0
         while True:
             new_name = f'{name}{i}'
@@ -323,6 +344,16 @@ async def validate_device(
                         )
                         verrors.extend(verror)
                         break
+            case 'CDROM':
+                source = device['source']
+                if os.path.isabs(source) is False:
+                    verrors.add(schema, 'Source must be an absolute path')
+                if await self.middleware.run_in_thread(os.path.exists, source) is False:
+                    verrors.add(schema, 'Specified source path does not exist')
+                elif await self.middleware.run_in_thread(os.path.isfile, source) is False:
+                    verrors.add(schema, 'Specified source path is not a file')
+                if instance_type == 'CONTAINER':
+                    verrors.add(schema, 'Container instance type is not supported')
             case 'DISK':
                 source = device['source'] or ''
                 if source == '' and device['name'] != 'root':
@@ -464,71 +495,93 @@ async def get_all_disk_sources_of_instance(self, instance_name):
         VirtInstanceDeviceAddArgs,
         VirtInstanceDeviceAddResult,
         audit='Virt: Adding device',
-        audit_extended=lambda id, device: f'{device["dev_type"]!r} to {id!r} instance',
+        audit_extended=lambda i, device: f'{device["dev_type"]!r} to {i!r} instance',
         roles=['VIRT_INSTANCE_WRITE']
     )
-    async def device_add(self, id, device):
+    async def device_add(self, oid, device):
         """
         Add a device to an instance.
         """
-        instance = await self.middleware.call('virt.instance.get_instance', id, {'extra': {'raw': True}})
+        instance = await self.middleware.call('virt.instance.get_instance', oid, {'extra': {'raw': True}})
         data = instance['raw']
+        verrors = ValidationErrors()
+        validate_device_name(device, verrors)
         if device['name'] is None:
             device['name'] = await self.generate_device_name(data['devices'].keys(), device['dev_type'])
 
-        verrors = ValidationErrors()
-        await self.validate_device(device, 'virt_device_add', verrors, id, instance['type'], instance_config=instance)
+        await self.validate_device(device, 'virt_device_add', verrors, oid, instance['type'], instance_config=instance)
         verrors.check()
 
         data['devices'][device['name']] = await self.device_to_incus(instance['type'], device)
-        await incus_call_and_wait(f'1.0/instances/{id}', 'put', {'json': data})
+        if device['dev_type'] == 'CDROM':
+            # We want to update qemu config here and make sure we keep track of which
+            # devices we have added as cdroms here
+            data['config'].update(update_instance_metadata_and_qemu_cmd_on_device_change(
+                oid, data['config'], data['devices']
+            ))
+
+        await incus_call_and_wait(f'1.0/instances/{oid}', 'put', {'json': data})
         return True
 
     @api_method(
         VirtInstanceDeviceUpdateArgs,
         VirtInstanceDeviceUpdateResult,
         audit='Virt: Updating device',
-        audit_extended=lambda id, device: f'{device["name"]!r} of {id!r} instance',
+        audit_extended=lambda i, device: f'{device["name"]!r} of {i!r} instance',
         roles=['VIRT_INSTANCE_WRITE']
     )
-    async def device_update(self, id, device):
+    async def device_update(self, oid, device):
         """
         Update a device in an instance.
         """
-        instance = await self.middleware.call('virt.instance.get_instance', id, {'extra': {'raw': True}})
+        instance = await self.middleware.call('virt.instance.get_instance', oid, {'extra': {'raw': True}})
         data = instance['raw']
 
-        for old in await self.device_list(id):
+        for old in await self.device_list(oid):
             if old['name'] == device['name']:
                 break
         else:
             raise CallError('Device does not exist.', errno.ENOENT)
 
         verrors = ValidationErrors()
-        await self.validate_device(device, 'virt_device_update', verrors, id, instance['type'], old, instance)
+        await self.validate_device(device, 'virt_device_update', verrors, oid, instance['type'], old, instance)
         verrors.check()
 
         data['devices'][device['name']] = await self.device_to_incus(instance['type'], device)
-        await incus_call_and_wait(f'1.0/instances/{id}', 'put', {'json': data})
+        if device['dev_type'] == 'CDROM':
+            # We want to update qemu config here and make sure we keep track of which
+            # devices we have added as cdroms here
+            data['config'].update(update_instance_metadata_and_qemu_cmd_on_device_change(
+                oid, data['config'], data['devices']
+            ))
+
+        await incus_call_and_wait(f'1.0/instances/{oid}', 'put', {'json': data})
         return True
 
     @api_method(
         VirtInstanceDeviceDeleteArgs,
         VirtInstanceDeviceDeleteResult,
         audit='Virt: Deleting device',
-        audit_extended=lambda id, device: f'{device!r} from {id!r} instance',
+        audit_extended=lambda i, device: f'{device!r} from {i!r} instance',
         roles=['VIRT_INSTANCE_DELETE']
     )
-    async def device_delete(self, id, device):
+    async def device_delete(self, oid, device):
         """
         Delete a device from an instance.
         """
-        instance = await self.middleware.call('virt.instance.get_instance', id, {'extra': {'raw': True}})
+        instance = await self.middleware.call('virt.instance.get_instance', oid, {'extra': {'raw': True}})
         data = instance['raw']
         if device not in data['devices']:
             raise CallError('Device not found.', errno.ENOENT)
         data['devices'].pop(device)
-        await incus_call_and_wait(f'1.0/instances/{id}', 'put', {'json': data})
+        if device.startswith(CDROM_PREFIX):
+            # We want to update qemu config here and make sure we keep track of which
+            # devices we have added as cdroms here
+            data['config'].update(update_instance_metadata_and_qemu_cmd_on_device_change(
+                oid, data['config'], data['devices']
+            ))
+
+        await incus_call_and_wait(f'1.0/instances/{oid}', 'put', {'json': data})
         return True
 
     @api_method(
@@ -559,16 +612,25 @@ async def set_bootable_disk(self, id, disk):
         if desired_disk is None:
             raise CallError(f'{disk!r} device does not exist.', errno.ENOENT)
 
-        if desired_disk['dev_type'] != 'DISK':
+        if desired_disk['dev_type'] not in ('CDROM', 'DISK'):
             raise CallError(f'{disk!r} device type is not DISK.')
 
         if max_boot_priority_device and max_boot_priority_device['name'] == disk:
             return True
 
-        return await self.device_update(id, {
-            'dev_type': 'DISK',
+        data = {
             'name': disk,
             'source': desired_disk.get('source'),
-            'io_bus': desired_disk.get('io_bus'),
             'boot_priority': max_boot_priority_device['boot_priority'] + 1 if max_boot_priority_device else 1,
-        } | ({'destination': desired_disk['destination']} if disk != 'root' else {}))
+        }
+        if desired_disk['dev_type'] == 'CDROM':
+            data |= {
+                'dev_type': 'CDROM',
+            }
+        else:
+            data |= {
+                'dev_type': 'DISK',
+                'io_bus': desired_disk.get('io_bus'),
+            } | ({'destination': desired_disk['destination']} if disk != 'root' else {})
+
+        return await self.device_update(id, data)

src/middlewared/middlewared/plugins/virt/utils.py

@@ -9,14 +9,16 @@
 from collections.abc import Callable
 
 from middlewared.plugins.zfs_.utils import TNUserProp
-from middlewared.service import CallError
+from middlewared.service import CallError, ValidationErrors
 from middlewared.utils import MIDDLEWARE_RUN_DIR
 
 from .websocket import IncusWS
 
 
-SOCKET = '/var/lib/incus/unix.socket'
+CDROM_PREFIX = 'ix_cdrom'
 HTTP_URI = 'http://unix.socket'
+INCUS_METADATA_CDROM_KEY = 'user.ix_cdrom_devices'
+SOCKET = '/var/lib/incus/unix.socket'
 VNC_BASE_PORT = 5900
 VNC_PASSWORD_DIR = os.path.join(MIDDLEWARE_RUN_DIR, 'incus/passwords')
 TRUENAS_STORAGE_PROP_STR = TNUserProp.INCUS_POOL.value
@@ -226,3 +228,44 @@ class PciEntry:
     reset_mechanism_defined: bool
     description: str
     error: str | None
+
+
+def generate_qemu_cmd(instance_config: dict, instance_name: str) -> str:
+    vnc_config = json.loads(instance_config.get('user.ix_vnc_config', '{}'))
+    cdrom_config = json.loads(instance_config.get(INCUS_METADATA_CDROM_KEY, '[]'))
+    cmd = ''
+    if vnc_config['vnc_enabled'] and vnc_config['vnc_port']:
+        cmd = f'-vnc :{vnc_config["vnc_port"] - VNC_BASE_PORT}'
+        if vnc_config.get('vnc_password'):
+            cmd = (f'-object secret,id=vnc0,file={get_vnc_password_file_path(instance_name)} '
+                   f'{cmd},password-secret=vnc0')
+
+    for cdrom_file in cdrom_config:
+        cmd += f'{" " if cmd else ""}-drive media=cdrom,if=ide,file={cdrom_file},file.locking=off'
+
+    return cmd
+
+
+def generate_qemu_cdrom_metadata(devices: dict) -> str:
+    return json.dumps([
+        d['source'] for name, d in devices.items() if name.startswith(CDROM_PREFIX)
+    ])
+
+
+def validate_device_name(device: dict, verrors: ValidationErrors):
+    if device['dev_type'] == 'CDROM':
+        if device['name'] and device['name'].startswith(CDROM_PREFIX) is False:
+            verrors.add('virt_device_add.name', f'CDROM device name must start with {CDROM_PREFIX!r} prefix')
+    elif device['name'] and device['name'].startswith(CDROM_PREFIX):
+        verrors.add('virt_device_add.name', f'Device name must not start with {CDROM_PREFIX!r} prefix')
+
+
+def update_instance_metadata_and_qemu_cmd_on_device_change(
+    instance_name: str, instance_config: dict, devices: dict
+) -> dict:
+    data = {
+        INCUS_METADATA_CDROM_KEY: generate_qemu_cdrom_metadata(devices)
+    }
+    data['raw.qemu'] = generate_qemu_cmd(instance_config | data, instance_name)
+    data['user.ix_old_raw_qemu_config'] = instance_config.get('raw.qemu', '')
+    return data