libvirt: Use SATA bus for cdrom devices when using Q35 machine type

The Q35 machine type no longer provides an IDE bus and will need to use
a SATA bus to attach legacy devices such as cdroms. More details can be
found in the following related bug:

Don't assume the guest machine type to be of 'pc'
https://bugs.launchpad.net/nova/+bug/1780138

This change now ensures the blockinfo.get_disk_bus_for_device_type
method will now return "sata" as the bus type when the Q35 machine type
is used for cdrom devices on QEMU or KVM hosts that are not PPC, S390 or
AArch64 based.

To enable this the _get_machine_type method has been extracted from the
Libvirt driver into the Libvirt utils module. This method has also been
simplified through the removal of the caps parameter, replaced with
calls to the get_arch utility method and additional extraction of
architecture specific defaults into the existing
get_default_machine_type utility method.

Related-bug: 1780138
Closes-bug: 1831538
Change-Id: Id97f4baddcf2caff91599773d9b5de5181b7fdf6

Author: lyarwood

nova/tests/functional/libvirt/base.py

@@ -17,6 +17,8 @@
 import fixtures
 import mock
 
+from nova.objects import fields as obj_fields
+
 from nova.tests import fixtures as nova_fixtures
 from nova.tests.functional import fixtures as func_fixtures
 from nova.tests.functional import test_servers as base
@@ -54,6 +56,13 @@ def setUp(self):
         _p = mock.patch('nova.virt.libvirt.host.Host.get_connection')
         self.mock_conn = _p.start()
         self.addCleanup(_p.stop)
+        # As above, mock the 'get_arch' function as we may need to provide
+        # different host architectures during some tests.
+        _a = mock.patch('nova.virt.libvirt.utils.get_arch')
+        self.mock_arch = _a.start()
+        # Default to X86_64
+        self.mock_arch.return_value = obj_fields.Architecture.X86_64
+        self.addCleanup(_a.stop)
 
     def _setup_compute_service(self):
         # NOTE(stephenfin): We don't start the compute service here as we wish

nova/tests/unit/virt/libvirt/fake_libvirt_utils.py

@@ -179,6 +179,10 @@ def cpu_features_to_traits(features):
     return libvirt_utils.cpu_features_to_traits(features)
 
 
+def get_machine_type(image_meta):
+    return libvirt_utils.get_machine_type(image_meta)
+
+
 def get_default_machine_type(arch):
     return libvirt_utils.get_default_machine_type(arch)
 

nova/tests/unit/virt/libvirt/test_blockinfo.py

@@ -815,6 +815,29 @@ def test_fail_get_disk_bus_for_disk_dev(self):
         self.assertRaises(exception.NovaException,
                 blockinfo.get_disk_bus_for_disk_dev, 'inv', 'val')
 
+    @mock.patch('nova.virt.libvirt.utils.get_machine_type')
+    @mock.patch('nova.virt.libvirt.utils.get_arch')
+    def test_get_disk_bus_for_device_type_cdrom_with_q35_get_arch(self,
+            mock_get_arch, mock_get_machine_type):
+        instance = objects.Instance(**self.test_instance)
+        mock_get_machine_type.return_value = 'pc-q35-rhel8.0.0'
+        mock_get_arch.return_value = obj_fields.Architecture.X86_64
+        image_meta = {'properties': {}}
+        image_meta = objects.ImageMeta.from_dict(image_meta)
+        bus = blockinfo.get_disk_bus_for_device_type(instance, 'kvm',
+                                                     image_meta,
+                                                     device_type='cdrom')
+        self.assertEqual('sata', bus)
+
+    def test_get_disk_bus_for_device_type_cdrom_with_q35_image_meta(self):
+        instance = objects.Instance(**self.test_instance)
+        image_meta = {'properties': {'hw_machine_type': 'pc-q35-rhel8.0.0'}}
+        image_meta = objects.ImageMeta.from_dict(image_meta)
+        bus = blockinfo.get_disk_bus_for_device_type(instance, 'kvm',
+                                                     image_meta,
+                                                     device_type='cdrom')
+        self.assertEqual('sata', bus)
+
     def test_get_config_drive_type_default(self):
         config_drive_type = blockinfo.get_config_drive_type()
         self.assertEqual('cdrom', config_drive_type)

nova/tests/unit/virt/libvirt/test_driver.py

@@ -6231,10 +6231,13 @@ def test_get_guest_config_os_command_empty(self):
                                      image_meta, disk_info)
         self.assertNotEqual(cfg.os_cmdline, "")
 
+    @mock.patch('nova.virt.libvirt.utils.get_arch',
+                return_value=fields.Architecture.ARMV7)
     @mock.patch.object(libvirt_driver.LibvirtDriver,
                        "_get_guest_storage_config")
     @mock.patch.object(libvirt_driver.LibvirtDriver, "_has_numa_support")
-    def test_get_guest_config_armv7(self, mock_numa, mock_storage):
+    def test_get_guest_config_armv7(self, mock_numa, mock_storage,
+                                    mock_get_arch):
         def get_host_capabilities_stub(self):
             cpu = vconfig.LibvirtConfigGuestCPU()
             cpu.arch = fields.Architecture.ARMV7
@@ -6263,12 +6266,14 @@ def get_host_capabilities_stub(self):
                                      image_meta, disk_info)
         self.assertEqual(cfg.os_mach_type, "virt")
 
+    @mock.patch('nova.virt.libvirt.utils.get_arch',
+                return_value=fields.Architecture.AARCH64)
     @mock.patch.object(libvirt_driver.LibvirtDriver,
                        "_get_guest_storage_config")
     @mock.patch.object(libvirt_driver.LibvirtDriver, "_has_numa_support")
     @mock.patch('os.path.exists', return_value=True)
     def test_get_guest_config_aarch64(self, mock_path_exists,
-                                      mock_numa, mock_storage):
+                                      mock_numa, mock_storage, mock_get_arch):
         def get_host_capabilities_stub(self):
             cpu = vconfig.LibvirtConfigGuestCPU()
             cpu.arch = fields.Architecture.AARCH64
@@ -6315,12 +6320,15 @@ def get_host_capabilities_stub(self):
 
         self.assertEqual(TEST_AMOUNT_OF_PCIE_SLOTS, num_ports)
 
+    @mock.patch('nova.virt.libvirt.utils.get_arch',
+                return_value=fields.Architecture.AARCH64)
     @mock.patch.object(libvirt_driver.LibvirtDriver,
                        "_get_guest_storage_config")
     @mock.patch.object(libvirt_driver.LibvirtDriver, "_has_numa_support")
     @mock.patch('os.path.exists', return_value=True)
     def test_get_guest_config_aarch64_with_graphics(self, mock_path_exists,
-                                                    mock_numa, mock_storage):
+                                                    mock_numa, mock_storage,
+                                                    mock_get_arch):
         def get_host_capabilities_stub(self):
             cpu = vconfig.LibvirtConfigGuestCPU()
             cpu.arch = fields.Architecture.AARCH64
@@ -6356,18 +6364,14 @@ def get_host_capabilities_stub(self):
         self.assertTrue(usbhost_exists)
         self.assertTrue(keyboard_exists)
 
-    def test_get_guest_config_machine_type_s390(self):
-        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
-
-        caps = vconfig.LibvirtConfigCaps()
-        caps.host = vconfig.LibvirtConfigCapsHost()
-        caps.host.cpu = vconfig.LibvirtConfigGuestCPU()
+    @mock.patch('nova.virt.libvirt.utils.get_arch')
+    def test_get_guest_config_machine_type_s390(self, mock_get_arch):
 
         image_meta = objects.ImageMeta.from_dict(self.test_image_meta)
         host_cpu_archs = (fields.Architecture.S390, fields.Architecture.S390X)
         for host_cpu_arch in host_cpu_archs:
-            caps.host.cpu.arch = host_cpu_arch
-            os_mach_type = drvr._get_machine_type(image_meta, caps)
+            mock_get_arch.return_value = host_cpu_arch
+            os_mach_type = libvirt_utils.get_machine_type(image_meta)
             self.assertEqual('s390-ccw-virtio', os_mach_type)
 
     def test_get_guest_config_machine_type_through_image_meta(self):

nova/virt/libvirt/blockinfo.py

@@ -222,14 +222,14 @@ def get_disk_bus_for_device_type(instance,
                                  device_type="disk"):
     """Determine the best disk bus to use for a device type.
 
-       Considering the currently configured virtualization
-       type, return the optimal disk_bus to use for a given
-       device type. For example, for a disk on KVM it will
-       return 'virtio', while for a CDROM it will return 'ide'
-       on x86_64 and 'scsi' on ppc64.
-
-       Returns the disk_bus, or returns None if the device
-       type is not supported for this virtualization
+    Considering the currently configured virtualization type, return the
+    optimal disk_bus to use for a given device type. For example, for a disk
+    on KVM it will return 'virtio', while for a CDROM, it will return 'ide'
+    for the 'pc' machine type on x86_64, 'sata' for the 'q35' machine type on
+    x86_64 and 'scsi' on ppc64.
+
+    Returns the disk_bus, or returns None if the device type is not supported
+    for this virtualization
     """
 
     # Prefer a disk bus set against the image first of all
@@ -268,6 +268,14 @@ def get_disk_bus_for_device_type(instance,
                     obj_fields.Architecture.S390X,
                     obj_fields.Architecture.AARCH64):
                 return "scsi"
+            machine_type = libvirt_utils.get_machine_type(image_meta)
+            # NOTE(lyarwood): We can't be any more explicit here as QEMU
+            # provides a version of the Q35 machine type per release.
+            # Additionally downstream distributions can also provide their own.
+            if machine_type and 'q35' in machine_type:
+                # NOTE(lyarwood): The Q35 machine type does not provide an IDE
+                # bus and as such we must use a SATA bus for cdroms.
+                return "sata"
             else:
                 return "ide"
         elif device_type == "disk":

nova/virt/libvirt/driver.py

@@ -4324,36 +4324,6 @@ def _get_guest_config_meta(self, instance):
 
         return meta
 
-    def _get_machine_type(self, image_meta, caps):
-        # The guest machine type can be set as an image metadata
-        # property, or otherwise based on architecture-specific
-        # defaults.
-        mach_type = None
-
-        if image_meta.properties.get('hw_machine_type') is not None:
-            mach_type = image_meta.properties.hw_machine_type
-        else:
-            # NOTE(kchamart): For ARMv7 and AArch64, use the 'virt'
-            # board as the default machine type.  It is the recommended
-            # board, which is designed to be used with virtual machines.
-            # The 'virt' board is more flexible, supports PCI, 'virtio',
-            # has decent RAM limits, etc.
-            if caps.host.cpu.arch in (fields.Architecture.ARMV7,
-                                      fields.Architecture.AARCH64):
-                mach_type = "virt"
-
-            if caps.host.cpu.arch in (fields.Architecture.S390,
-                                      fields.Architecture.S390X):
-                mach_type = 's390-ccw-virtio'
-
-            # If set in the config, use that as the default.
-            mach_type = (
-                libvirt_utils.get_default_machine_type(caps.host.cpu.arch)
-                or mach_type
-            )
-
-        return mach_type
-
     @staticmethod
     def _create_idmaps(klass, map_strings):
         idmaps = []
@@ -5041,7 +5011,7 @@ def _configure_guest_by_virt_type(self, guest, virt_type, caps, instance,
                     guest.os_loader_type = "pflash"
                 else:
                     raise exception.UEFINotSupported()
-            guest.os_mach_type = self._get_machine_type(image_meta, caps)
+            guest.os_mach_type = libvirt_utils.get_machine_type(image_meta)
             if image_meta.properties.get('hw_boot_menu') is None:
                 guest.os_bootmenu = strutils.bool_from_string(
                     flavor.extra_specs.get('hw:boot_menu', 'no'))

nova/virt/libvirt/utils.py

@@ -541,6 +541,19 @@ def get_cpu_model_from_arch(arch):
     return mode
 
 
+def get_machine_type(image_meta):
+    """The guest machine type can be set as an image metadata property, or
+    otherwise based on architecture-specific defaults. If no defaults are
+    found then None will be returned. This will ultimately lead to QEMU using
+    its own default which is currently the 'pc' machine type.
+    """
+    if image_meta.properties.get('hw_machine_type') is not None:
+        return image_meta.properties.hw_machine_type
+
+    # If set in the config, use that as the default.
+    return get_default_machine_type(get_arch(image_meta))
+
+
 def machine_type_mappings():
     mappings = {}
     for mapping in CONF.libvirt.hw_machine_type or {}:
@@ -553,7 +566,23 @@ def machine_type_mappings():
 
 
 def get_default_machine_type(arch):
-    return machine_type_mappings().get(arch)
+    # NOTE(lyarwood): Values defined in [libvirt]/hw_machine_type take
+    # precedence here if available for the provided arch.
+    machine_type = machine_type_mappings().get(arch)
+    if machine_type:
+        return machine_type
+    # NOTE(kchamart): For ARMv7 and AArch64, use the 'virt' board as the
+    # default machine type.  It is the recommended board, which is designed
+    # to be used with virtual machines.  The 'virt' board is more flexible,
+    # supports PCI, 'virtio', has decent RAM limits, etc.
+    if arch in (obj_fields.Architecture.ARMV7,
+                obj_fields.Architecture.AARCH64):
+        return "virt"
+
+    if arch in (obj_fields.Architecture.S390,
+                obj_fields.Architecture.S390X):
+        return "s390-ccw-virtio"
+    return None
 
 
 def mdev_name2uuid(mdev_name):