libvirt: Register defaults for undefined hw image properties

Currently, device bus and model types defined as image properties associated with an instance are always used when launching instances with the libvirt driver. When these types are not defined as image properties, their values either come from libosinfo or those directly hardcoded into the libvirt driver. This means that any changes to the defaults provided by libosinfo or the libvirt driver could result in unforeseen changes to existing instances. This has been encountered in the past as libosinfo assumes that libvirt domain definitions are static when OpenStack Nova specifically rewrites and redefines these domains during a hard reboot or migration allowing changes to possibly occur. This adds persistence of device bus and model type defaults to the instance's system metadata so that they will remain stable across reboots and migrations. Co-Authored-By: melanie witt <melwittt@gmail.com> Blueprint: libvirt-device-bus-model-update Change-Id: I44d41a134a7fab638e2ea88e7ae86d25070e8a43
2021-07-12 17:55:21 +01:00
parent b5029890c1
commit 7ecdfb61a9
4 changed files with 683 additions and 97 deletions
@@ -0,0 +1,234 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import datetime
+from unittest import mock
+
+from oslo_utils.fixture import uuidsentinel as uuids
+
+from nova import context as nova_context
+from nova import objects
+from nova import test
+from nova.tests.functional.libvirt import base
+from nova.virt.libvirt import driver as libvirt_driver
+
+
+class LibvirtDeviceBusMigration(base.ServersTestBase):
+
+    microversion = 'latest'
+    # needed for move operations
+    ADMIN_API = True
+
+    def setUp(self):
+        super().setUp()
+        self.context = nova_context.get_admin_context()
+        self.compute_hostname = self.start_compute()
+        self.compute = self.computes[self.compute_hostname]
+
+    def _unset_stashed_image_properties(self, server_id, properties):
+        instance = objects.Instance.get_by_uuid(self.context, server_id)
+        for p in properties:
+            instance.system_metadata.pop(f'image_{p}')
+        instance.save()
+
+    def _assert_stashed_image_properties(self, server_id, properties):
+        instance = objects.Instance.get_by_uuid(self.context, server_id)
+        for p, value in properties.items():
+            self.assertEqual(instance.system_metadata.get(f'image_{p}'), value)
+
+    def _assert_stashed_image_properties_persist(self, server, properties):
+        # Assert the stashed properties persist across a host reboot
+        self.restart_compute_service(self.compute)
+        self._assert_stashed_image_properties(server['id'], properties)
+
+        # Assert the stashed properties persist across a guest reboot
+        self._reboot_server(server, hard=True)
+        self._assert_stashed_image_properties(server['id'], properties)
+
+        # Assert the stashed properties persist across a migration
+        if 'other_compute' not in self.computes:
+            self.start_compute('other_compute')
+        # TODO(stephenfin): The mock of 'migrate_disk_and_power_off' should
+        # probably be less...dumb
+        with mock.patch(
+            'nova.virt.libvirt.driver.LibvirtDriver'
+            '.migrate_disk_and_power_off', return_value='{}',
+        ):
+            self._migrate_server(server)
+        self._confirm_resize(server)
+        self._assert_stashed_image_properties(server['id'], properties)
+
+    def test_default_image_property_registration(self):
+        """Assert that the defaults for various hw image properties don't
+        change over the lifecycle of an instance.
+        """
+        default_image_properties = {
+            'hw_machine_type': 'pc',
+            'hw_cdrom_bus': 'ide',
+            'hw_disk_bus': 'virtio',
+            'hw_input_bus': 'usb',
+            'hw_pointer_model': 'usbtablet',
+            'hw_video_model': 'virtio',
+            'hw_vif_model': 'virtio',
+        }
+
+        server = self._create_server(networks='none')
+        self._assert_stashed_image_properties(
+            server['id'], default_image_properties)
+
+        # Unset the defaults here to ensure that init_host resets them
+        # when the compute restarts the libvirt driver
+        self._unset_stashed_image_properties(
+            server['id'], libvirt_driver.REGISTER_IMAGE_PROPERTY_DEFAULTS)
+
+        # Assert the defaults persist across a host reboot, guest reboot, and
+        # guest migration
+        self._assert_stashed_image_properties_persist(
+            server, default_image_properties)
+
+    def test_non_default_image_property_registration(self):
+        """Assert that non-default values for various hw image properties
+        don't change over the lifecycle of an instance.
+        """
+        non_default_image_properties = {
+            'hw_machine_type': 'q35',
+            'hw_cdrom_bus': 'sata',
+            'hw_disk_bus': 'sata',
+            'hw_input_bus': 'virtio',
+            'hw_video_model': 'qxl',
+            'hw_vif_model': 'e1000',
+        }
+        self.glance.create(
+            None,
+            {
+                'id': uuids.hw_bus_model_image_uuid,
+                'name': 'hw_bus_model_image',
+                'created_at': datetime.datetime(2011, 1, 1, 1, 2, 3),
+                'updated_at': datetime.datetime(2011, 1, 1, 1, 2, 3),
+                'deleted_at': None,
+                'deleted': False,
+                'status': 'active',
+                'is_public': False,
+                'container_format': 'bare',
+                'disk_format': 'qcow2',
+                'size': '74185822',
+                'min_ram': 0,
+                'min_disk': 0,
+                'protected': False,
+                'visibility': 'public',
+                'tags': [],
+                'properties': non_default_image_properties,
+            }
+        )
+        server = self._create_server(
+            networks='none', image_uuid=uuids.hw_bus_model_image_uuid)
+        self._assert_stashed_image_properties(
+            server['id'], non_default_image_properties)
+
+        # Assert the non defaults persist across a host reboot, guest reboot,
+        # and guest migration
+        self._assert_stashed_image_properties_persist(
+            server, non_default_image_properties)
+
+    def test_default_image_property_persists_across_osinfo_changes(self):
+        # Create a server with default image properties
+        default_image_properties = {
+            'hw_vif_model': 'virtio',
+            'hw_disk_bus': 'virtio',
+        }
+        server = self._create_server(networks='none')
+        self._assert_stashed_image_properties(
+            server['id'], default_image_properties)
+
+        with test.nested(
+            mock.patch('nova.virt.osinfo.HardwareProperties.network_model',
+                new=mock.PropertyMock()),
+            mock.patch('nova.virt.osinfo.HardwareProperties.disk_model',
+                new=mock.PropertyMock())
+        ) as (mock_nw_model, mock_disk_model):
+            # osinfo returning new things
+            mock_nw_model.return_value = 'e1000'
+            mock_disk_model.return_value = 'sata'
+
+            # Assert the defaults persist across a host reboot, guest reboot,
+            # and guest migration
+            self._assert_stashed_image_properties_persist(
+                server, default_image_properties)
+
+    def test_default_image_property_persists_across_host_flag_changes(self):
+        # Set the default to ps2 via host flag
+        self.flags(pointer_model='ps2mouse')
+        # Restart compute to pick up ps2 setting, which means the guest will
+        # not get a prescribed pointer device
+        self.restart_compute_service(self.compute)
+
+        # Create a server with default image properties
+        default_image_properties1 = {
+            'hw_pointer_model': None,
+            'hw_input_bus': None,
+        }
+        server1 = self._create_server(networks='none')
+        self._assert_stashed_image_properties(
+            server1['id'], default_image_properties1)
+
+        # Assert the defaults persist across a host flag change
+        self.flags(pointer_model='usbtablet')
+        # Restart compute to pick up usb setting
+        self.restart_compute_service(self.compute)
+        self._assert_stashed_image_properties(
+            server1['id'], default_image_properties1)
+
+        # Assert the defaults persist across a host reboot, guest reboot, and
+        # guest migration
+        self._assert_stashed_image_properties_persist(
+            server1, default_image_properties1)
+
+        # Create a server with new default image properties since the host flag
+        # change
+        default_image_properties2 = {
+            'hw_pointer_model': 'usbtablet',
+            'hw_input_bus': 'usb',
+        }
+        server2 = self._create_server(networks='none')
+        self._assert_stashed_image_properties(
+            server2['id'], default_image_properties2)
+
+        # Assert the defaults persist across a host reboot, guest reboot, and
+        # guest migration
+        self._assert_stashed_image_properties_persist(
+            server2, default_image_properties2)
+
+        # Finally, try changing the host flag again to None. Note that it is
+        # not possible for a user to specify None for this option:
+        # https://bugs.launchpad.net/nova/+bug/1866106
+        self.flags(pointer_model=None)
+        # Restart compute to pick up None setting
+        self.restart_compute_service(self.compute)
+        self._assert_stashed_image_properties(
+            server1['id'], default_image_properties1)
+        self._assert_stashed_image_properties(
+            server2['id'], default_image_properties2)
+
+        # Create a server since the host flag change to None. The defaults
+        # should be the same as for ps2mouse
+        server3 = self._create_server(networks='none')
+        self._assert_stashed_image_properties(
+            server3['id'], default_image_properties1)
+
+        # Assert the defaults persist across a host reboot, guest reboot, and
+        # guest migration for server1, server2, and server3
+        self._assert_stashed_image_properties_persist(
+            server1, default_image_properties1)
+        self._assert_stashed_image_properties_persist(
+            server2, default_image_properties2)
+        self._assert_stashed_image_properties_persist(
+            server3, default_image_properties1)
@@ -870,9 +870,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
            "'swtpm_enabled=True'"
        )

-    @mock.patch.object(
-        libvirt_driver.LibvirtDriver, '_register_instance_machine_type',
-        new=mock.Mock())
+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(
        host.Host, 'supports_secure_boot', new_callable=mock.PropertyMock)
    def test_driver_capabilities_secure_boot(self, mock_supports):
@@ -886,7 +886,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        mock_supports.assert_called_once_with()

    @mock.patch.object(
-        libvirt_driver.LibvirtDriver, '_register_instance_machine_type',
+        libvirt_driver.LibvirtDriver,
+        '_register_all_undefined_instance_details',
        new=mock.Mock())
    @mock.patch.object(
        host.Host, 'supports_remote_managed_ports',
@@ -1041,7 +1042,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        any_order=True)

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(host.Host, "has_min_version")
    def test_min_version_start_ok(self, mock_version):
        mock_version.return_value = True
@@ -1057,7 +1059,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                          "dummyhost")

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(fakelibvirt.Connection, 'getLibVersion',
                       return_value=versionutils.convert_version_to_int(
                            libvirt_driver.NEXT_MIN_LIBVIRT_VERSION) - 1)
@@ -1087,7 +1090,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertTrue(version_arg_found)

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(fakelibvirt.Connection, 'getVersion',
                       return_value=versionutils.convert_version_to_int(
                            libvirt_driver.NEXT_MIN_QEMU_VERSION) - 1)
@@ -1117,7 +1121,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertTrue(version_arg_found)

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(fakelibvirt.Connection, 'getLibVersion',
                       return_value=versionutils.convert_version_to_int(
                            libvirt_driver.NEXT_MIN_LIBVIRT_VERSION))
@@ -1147,7 +1152,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertFalse(version_arg_found)

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(fakelibvirt.Connection, 'getVersion',
                       return_value=versionutils.convert_version_to_int(
                            libvirt_driver.NEXT_MIN_QEMU_VERSION))
@@ -1177,7 +1183,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertFalse(version_arg_found)

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test_min_version_ppc_ok(self):
        self.mock_uname.return_value = fakelibvirt.os_uname(
            'Linux', '', '5.4.0-0-generic', '', fields.Architecture.PPC64)
@@ -1185,7 +1192,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        drvr.init_host("dummyhost")

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test_min_version_s390_ok(self):
        self.mock_uname.return_value = fakelibvirt.os_uname(
            'Linux', '', '5.4.0-0-generic', '', fields.Architecture.S390X)
@@ -1193,7 +1201,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        drvr.init_host("dummyhost")

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test_file_backed_memory_support_called(self):
        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
        with mock.patch.object(drvr,
@@ -1248,7 +1257,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        )

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test__prepare_cpu_flag(self):
        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)

@@ -1278,7 +1288,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertXmlEqual(expected_xml, cpu.to_xml())

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test__check_cpu_compatibility_start_ok(self):
        self.flags(cpu_mode="custom",
                   cpu_models=["Penryn"],
@@ -1311,7 +1322,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                          drvr.init_host, "dummyhost")

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test__check_cpu_compatibility_with_flag(self):
        self.flags(cpu_mode="custom",
                   cpu_models=["Penryn"],
@@ -1370,7 +1382,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertRaises(exception.Invalid, drvr.init_host, "dummyhost")

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test__check_cpu_compatibility_aarch64_qemu_custom_start_OK(self):
        """Test getting CPU traits when using a virt_type that doesn't support
        the feature, only kvm and qemu supports reporting CPU traits.
@@ -1388,6 +1401,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
            drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
            drvr.init_host("dummyhost")

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test__check_vtpm_support_non_qemu(self):
        """Test checking for vTPM support when we're not using QEMU or KVM."""
        self.flags(swtpm_enabled=True, virt_type='lxc', group='libvirt')
@@ -1475,7 +1491,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        mock_getgrnam.assert_called_with('admins')

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch('shutil.which')
    @mock.patch('pwd.getpwnam')
    @mock.patch('grp.getgrnam')
@@ -2450,7 +2467,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertEqual(storage_ip, result['ip'])

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test_lifecycle_event_registration(self):
        calls = []

@@ -14582,6 +14600,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        mock_stat.assert_called_once_with(path)
        mock_get_size.assert_called_once_with(path)

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    def test_spawn_with_network_info(self, power_on=True):
        def fake_getLibVersion():
            return fakelibvirt.FAKE_LIBVIRT_VERSION
@@ -14742,6 +14763,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                   accel_info=accel_info, power_on=False)
        return instance

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_guest_xml')
    def test_spawn_accels_no_accel_info(self, mock_get_guest_xml):
        # accel_info should be passed to get_guest_xml even if it is []
@@ -14752,6 +14776,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
            block_device_info=None, mdevs=mock.ANY,
            accel_info=[])

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_guest_xml')
    def test_spawn_accels_with_accel_info(self, mock_get_guest_xml):
        # accel_info should be passed to get_guest_xml if it is not []
@@ -14762,6 +14789,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
            block_device_info=None, mdevs=mock.ANY,
            accel_info=accel_info)

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    # Methods called directly by spawn()
    @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_guest_xml')
    @mock.patch.object(libvirt_driver.LibvirtDriver,
@@ -14809,6 +14839,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        config_disk.import_file.assert_called_once_with(instance, mock.ANY,
                                                        'disk.config')

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    def test_spawn_without_image_meta(self):
        instance_ref = self.test_instance
        instance_ref['image_ref'] = uuids.image_ref
@@ -14833,6 +14866,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        self.assertEqual(['disk', 'disk.local'],
                         sorted(backend.created_disks.keys()))

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    def _test_spawn_disks(self, image_ref, block_device_info):
        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)

@@ -14893,6 +14929,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
        # We should have created the root and ephemeral disks
        self.assertEqual(['disk', 'disk.local'], disks_created)

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    def test_spawn_lxc_from_volume(self):
        self.flags(virt_type="lxc",
                   group='libvirt')
@@ -14983,6 +15022,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                             inst_obj.system_metadata.get(
                             'rootfs_device_name'))

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    def test_spawn_with_pci_devices(self):
        class FakeLibvirtPciDevice(object):
            def dettach(self):
@@ -15027,6 +15069,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                               return_value=mock_connection):
            drvr.spawn(self.context, instance, image_meta, [], None, {})

+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_register_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch('nova.crypto.ensure_vtpm_secret')
    @mock.patch.object(hardware, 'get_vtpm_constraint')
    @mock.patch(
@@ -15821,7 +15866,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                                          'my_ip': mock.ANY})

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test_init_host_checks_ip(self):
        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
        with mock.patch.object(drvr, '_check_my_ip') as mock_check:
@@ -15877,7 +15923,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
            self.assertTrue(service_mock.disabled)

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    def test_service_resume_after_broken_connection(self):
        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
        service_mock = mock.MagicMock()
@@ -20015,7 +20062,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                              driver.init_host, 'wibble')

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch.object(fakelibvirt.Connection, 'getVersion',
                       return_value=versionutils.convert_version_to_int(
                           libvirt_driver.MIN_VIRTUOZZO_VERSION))
@@ -25992,7 +26040,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
                         drvr._get_existing_mdevs_not_assigned(parent=None))

    @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_register_instance_machine_type', new=mock.Mock())
+                       '_register_all_undefined_instance_details',
+                       new=mock.Mock())
    @mock.patch('nova.compute.utils.get_machine_ips',
                new=mock.Mock(return_value=[]))
    @mock.patch.object(nova.privsep.libvirt, 'create_mdev')
@@ -26708,12 +26757,13 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
        mock_execute.assert_called_once_with('qemu-img', 'rebase',
                                             '-b', '', 'disk')

+    @mock.patch('nova.objects.instance.Instance.save')
    @mock.patch('nova.objects.instance.InstanceList.get_by_host')
    @mock.patch('nova.virt.libvirt.host.Host.get_hostname',
        new=mock.Mock(return_value=mock.sentinel.hostname))
    @mock.patch('nova.context.get_admin_context', new=mock.Mock())
    def test_register_machine_type_already_registered_image_metadata(
-        self, mock_get_by_host
+        self, mock_get_by_host, mock_instance_save,
    ):
        instance = self._create_instance(
            params={
@@ -26723,7 +26773,14 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
            }
        )
        mock_get_by_host.return_value = [instance]
-        self.drvr._register_instance_machine_type()
+
+        # We only care about hw_machine_type for this test
+        with mock.patch(
+            'nova.virt.libvirt.driver.REGISTER_IMAGE_PROPERTY_DEFAULTS',
+            ['hw_machine_type']
+        ):
+            self.drvr._register_all_undefined_instance_details()
+
        # Assert that we don't overwrite the existing type
        self.assertEqual(
            'existing_type',
@@ -26733,6 +26790,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
            'existing_type',
            instance.system_metadata.get('image_hw_machine_type')
        )
+        mock_instance_save.assert_not_called()

    @mock.patch('nova.objects.instance.Instance.save')
    @mock.patch('nova.objects.instance.InstanceList.get_by_host')
@@ -26745,7 +26803,14 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
    ):
        instance = self._create_instance()
        mock_get_by_host.return_value = [instance]
-        self.drvr._register_instance_machine_type()
+
+        # We only care about hw_machine_type for this test
+        with mock.patch(
+            'nova.virt.libvirt.driver.REGISTER_IMAGE_PROPERTY_DEFAULTS',
+            ['hw_machine_type']
+        ):
+            self.drvr._register_all_undefined_instance_details()
+
        mock_instance_save.assert_called_once()
        self.assertEqual(
            'conf_type',
@@ -26756,6 +26821,143 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
            instance.system_metadata.get('image_hw_machine_type')
        )

+    @mock.patch('nova.virt.libvirt.driver.LOG.exception')
+    @mock.patch('nova.objects.instance.InstanceList.get_by_host')
+    @mock.patch('nova.virt.libvirt.host.Host.get_hostname', new=mock.Mock())
+    def test_register_all_undefined_details_unknown_failure(
+        self, mock_get_by_host, mock_log_exc
+    ):
+        instance = self._create_instance()
+        mock_get_by_host.return_value = [instance]
+
+        # Assert that we swallow anything raised below us
+        with mock.patch.object(
+            self.drvr,
+            '_register_undefined_instance_details',
+            side_effect=test.TestingException()
+        ):
+            self.drvr._register_all_undefined_instance_details()
+
+        # Assert that we logged the failure
+        self.assertEqual(1, mock_log_exc.call_count)
+        self.assertIn('Ignoring unknown failure while attempting '
+                      'to save the defaults for unregistered image properties',
+                      mock_log_exc.call_args.args[0])
+
+    @mock.patch('nova.virt.libvirt.driver.LOG.exception')
+    @mock.patch('nova.objects.instance.Instance.save')
+    @mock.patch('nova.virt.libvirt.host.Host.get_guest', new=mock.Mock())
+    @mock.patch('nova.objects.block_device.BlockDeviceMappingList.'
+                'get_by_instance_uuid')
+    @mock.patch('nova.objects.instance.InstanceList.get_by_host')
+    @mock.patch('nova.virt.libvirt.host.Host.get_hostname', new=mock.Mock())
+    def test_register_all_undefined_details_unknown_failure_finding_default(
+        self, mock_get_by_host, mock_get_bdms, mock_save, mock_log_exc
+    ):
+        instance = self._create_instance()
+        mock_get_by_host.return_value = [instance]
+        mock_get_bdms.return_value = []
+
+        # Assert that we swallow anything raised below us
+        with mock.patch.object(
+            self.drvr,
+            '_find_default_for_image_property',
+            side_effect=test.TestingException()
+        ):
+            self.drvr._register_all_undefined_instance_details()
+
+        # Assert that we logged the failures (once for each unregistered
+        # image property)
+        self.assertEqual(len(libvirt_driver.REGISTER_IMAGE_PROPERTY_DEFAULTS),
+                         mock_log_exc.call_count)
+        self.assertIn('Ignoring unknown failure while attempting '
+                      'to find the default of',
+                      mock_log_exc.call_args.args[0])
+
+        # Assert that we updated the instance
+        mock_save.assert_called_once_with()
+
+    @mock.patch('nova.objects.instance.Instance.save',
+                new=mock.NonCallableMock())
+    @mock.patch('nova.virt.libvirt.host.Host.get_guest',
+                new=mock.NonCallableMock())
+    @mock.patch('nova.objects.block_device.BlockDeviceMappingList.'
+                'get_by_instance_uuid', new=mock.NonCallableMock())
+    def test_register_undefined_instance_details_nothing_to_register(self):
+        instance = self._create_instance()
+
+        # Set a value for all REGISTER_IMAGE_PROPERTY_DEFAULTS
+        for p in libvirt_driver.REGISTER_IMAGE_PROPERTY_DEFAULTS:
+            instance.system_metadata[f"image_{p}"] = 'foo'
+
+        # We should not have pulled bdms or updated the instance
+        self.drvr._register_undefined_instance_details(self.context, instance)
+
+    @mock.patch('nova.objects.instance.Instance.save')
+    @mock.patch('nova.virt.libvirt.host.Host.get_guest')
+    @mock.patch('nova.objects.block_device.BlockDeviceMappingList.'
+                'get_by_instance_uuid')
+    def test_register_undefined_instance_details_disk_info_and_guest_config(
+        self, mock_get_bdms, mock_get_guest, mock_save
+    ):
+        instance = self._create_instance()
+        mock_get_bdms.return_value = []
+
+        # Test all props unregistered
+        with mock.patch.object(
+            self.drvr,
+            '_find_default_for_image_property'
+        ) as mock_find:
+            self.drvr._register_undefined_instance_details(self.context,
+                                                           instance)
+        # We should have pulled bdms
+        mock_get_bdms.assert_called_once_with(self.context, instance.uuid)
+        # We should have pulled disk_info
+        self.assertIsNotNone(mock_find.call_args.args[2])
+        # We should have pulled guest config
+        mock_get_guest.return_value.get_config.assert_called_once_with()
+        self.assertIsNotNone(mock_find.call_args.args[3])
+
+        # Set one of ['hw_disk_bus', 'hw_cdrom_bus']
+        # Set one of ['hw_pointer_model', 'hw_input_bus']
+        mock_get_bdms.reset_mock()
+        mock_get_guest.reset_mock()
+        instance.system_metadata['image_hw_disk_bus'] = 'scsi'
+        instance.system_metadata['image_hw_pointer_model'] = None
+        with mock.patch.object(
+            self.drvr,
+            '_find_default_for_image_property'
+        ) as mock_find:
+            self.drvr._register_undefined_instance_details(self.context,
+                                                           instance)
+        # We should have pulled bdms
+        mock_get_bdms.assert_called_once_with(self.context, instance.uuid)
+        # We should have pulled disk_info
+        self.assertIsNotNone(mock_find.call_args.args[2])
+        # We should have pulled guest config
+        mock_get_guest.return_value.get_config.assert_called_once_with()
+        self.assertIsNotNone(mock_find.call_args.args[3])
+
+        # Set the other, now we have both ['hw_disk_bus', 'hw_cdrom_bus']
+        # Set the other, now we have both ['hw_pointer_model', 'hw_input_bus']
+        mock_get_bdms.reset_mock()
+        mock_get_guest.reset_mock()
+        instance.system_metadata['image_hw_cdrom_bus'] = 'scsi'
+        instance.system_metadata['image_hw_input_bus'] = None
+        with mock.patch.object(
+            self.drvr,
+            '_find_default_for_image_property'
+        ) as mock_find:
+            self.drvr._register_undefined_instance_details(self.context,
+                                                           instance)
+        # We should not have pulled bdms at all
+        mock_get_bdms.assert_not_called()
+        # And disk_info should not have been pulled
+        self.assertIsNone(mock_find.call_args.args[2])
+        # We should not have pulled guest config
+        mock_get_guest.return_value.assert_not_called()
+        self.assertIsNone(mock_find.call_args.args[3])
+

 class LibvirtVolumeUsageTestCase(test.NoDBTestCase):
    """Test for LibvirtDriver.get_all_volume_usage."""
@@ -243,6 +243,16 @@ LIBVIRT_PERF_EVENT_PREFIX = 'VIR_PERF_PARAM_'
 MIN_LIBVIRT_VDPA = (6, 9, 0)
 MIN_QEMU_VDPA = (5, 1, 0)

+REGISTER_IMAGE_PROPERTY_DEFAULTS = [
+    'hw_machine_type',
+    'hw_cdrom_bus',
+    'hw_disk_bus',
+    'hw_input_bus',
+    'hw_pointer_model',
+    'hw_video_model',
+    'hw_vif_model',
+]
+

 class AsyncDeviceEventsHandler:
    """A synchornization point between libvirt events an clients waiting for
@@ -804,7 +814,9 @@ class LibvirtDriver(driver.ComputeDriver):

        self._check_vtpm_support()

-        self._register_instance_machine_type()
+        # Set REGISTER_IMAGE_PROPERTY_DEFAULTS in the instance system_metadata
+        # to default values for properties that have not already been set.
+        self._register_all_undefined_instance_details()

    def _update_host_specific_capabilities(self) -> None:
        """Update driver capabilities based on capabilities of the host."""
@@ -816,34 +828,114 @@ class LibvirtDriver(driver.ComputeDriver):
            self._host.supports_remote_managed_ports
        })

-    def _register_instance_machine_type(self):
-        """Register the machine type of instances on this host
+    def _register_all_undefined_instance_details(self) -> None:
+        """Register the default image properties of instances on this host

        For each instance found on this host by InstanceList.get_by_host ensure
-        a machine type is registered within the system metadata of the instance
+        REGISTER_IMAGE_PROPERTY_DEFAULTS are registered within the system
+        metadata of the instance
        """
        context = nova_context.get_admin_context()
        hostname = self._host.get_hostname()
+        for instance in objects.InstanceList.get_by_host(
+            context, hostname, expected_attrs=['flavor', 'system_metadata']
+        ):
+            try:
+                self._register_undefined_instance_details(context, instance)
+            except Exception:
+                LOG.exception('Ignoring unknown failure while attempting '
+                              'to save the defaults for unregistered image '
+                              'properties', instance=instance)

-        for instance in objects.InstanceList.get_by_host(context, hostname):
-            # NOTE(lyarwood): Skip if hw_machine_type is set already in the
-            # image_meta of the instance. Note that this value comes from the
-            # system metadata of the instance where it is stored under the
-            # image_hw_machine_type key.
-            if instance.image_meta.properties.get('hw_machine_type'):
-                continue
+    def _register_undefined_instance_details(
+        self,
+        context: nova_context.RequestContext,
+        instance: 'objects.Instance',
+    ) -> None:
+        # Find any unregistered image properties against this instance
+        unregistered_image_props = [
+            p for p in REGISTER_IMAGE_PROPERTY_DEFAULTS
+            if f"image_{p}" not in instance.system_metadata
+        ]

-            # Fetch and record the machine type from the config
-            hw_machine_type = libvirt_utils.get_machine_type(
-                instance.image_meta)
-            # NOTE(lyarwood): As above this updates
-            # image_meta.properties.hw_machine_type within the instance and
-            # will be returned the next time libvirt_utils.get_machine_type is
-            # called for the instance image meta.
-            instance.system_metadata['image_hw_machine_type'] = hw_machine_type
-            instance.save()
-            LOG.debug("Instance machine_type updated to %s", hw_machine_type,
-                      instance=instance)
+        # Return if there's nothing left to register for this instance
+        if not unregistered_image_props:
+            return
+
+        LOG.debug(f'Attempting to register defaults for the following '
+                  f'image properties: {unregistered_image_props}',
+                  instance=instance)
+
+        # NOTE(lyarwood): Only build disk_info once per instance if we need it
+        # for hw_{disk,cdrom}_bus to avoid pulling bdms from the db etc.
+        requires_disk_info = ['hw_disk_bus', 'hw_cdrom_bus']
+        disk_info = None
+        if set(requires_disk_info) & set(unregistered_image_props):
+            bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
+                context, instance.uuid)
+            block_device_info = driver.get_block_device_info(instance, bdms)
+            disk_info = blockinfo.get_disk_info(
+                CONF.libvirt.virt_type, instance, instance.image_meta,
+                block_device_info)
+
+        # Only pull the guest config once per instance if we need it for
+        # hw_pointer_model or hw_input_bus.
+        requires_guest_config = ['hw_pointer_model', 'hw_input_bus']
+        guest_config = None
+        if set(requires_guest_config) & set(unregistered_image_props):
+            guest_config = self._host.get_guest(instance).get_config()
+
+        for image_prop in unregistered_image_props:
+            try:
+                default_value = self._find_default_for_image_property(
+                    instance, image_prop, disk_info, guest_config)
+                instance.system_metadata[f"image_{image_prop}"] = default_value
+
+                LOG.debug(f'Found default for {image_prop} of {default_value}',
+                          instance=instance)
+            except Exception:
+                LOG.exception(f'Ignoring unknown failure while attempting '
+                              f'to find the default of {image_prop}',
+                              instance=instance)
+        instance.save()
+
+    def _find_default_for_image_property(
+        self,
+        instance: 'objects.Instance',
+        image_property: str,
+        disk_info: ty.Optional[ty.Dict[str, ty.Any]],
+        guest_config: ty.Optional[vconfig.LibvirtConfigGuest],
+    ) -> ty.Optional[str]:
+        if image_property == 'hw_machine_type':
+            return libvirt_utils.get_machine_type(instance.image_meta)
+
+        if image_property == 'hw_disk_bus' and disk_info:
+            return disk_info.get('disk_bus')
+
+        if image_property == 'hw_cdrom_bus' and disk_info:
+            return disk_info.get('cdrom_bus')
+
+        if image_property == 'hw_input_bus' and guest_config:
+            _, default_input_bus = self._get_pointer_bus_and_model(
+                guest_config, instance.image_meta)
+            return default_input_bus
+
+        if image_property == 'hw_pointer_model' and guest_config:
+            default_pointer_model, _ = self._get_pointer_bus_and_model(
+                guest_config, instance.image_meta)
+            # hw_pointer_model is of type PointerModelType ('usbtablet' instead
+            # of 'tablet')
+            if default_pointer_model == 'tablet':
+                default_pointer_model = 'usbtablet'
+            return default_pointer_model
+
+        if image_property == 'hw_video_model':
+            return self._get_video_type(instance.image_meta)
+
+        if image_property == 'hw_vif_model':
+            return self.vif_driver.get_vif_model(instance.image_meta)
+
+        return None

    def _prepare_cpu_flag(self, flag):
        # NOTE(kchamart) This helper method will be used while computing
@@ -4246,6 +4338,11 @@ class LibvirtDriver(driver.ComputeDriver):
        else:
            LOG.info("Instance spawned successfully.", instance=instance)

+        # Finally register defaults for any undefined image properties so that
+        # future changes by QEMU, libvirt or within this driver don't change
+        # the ABI of the instance.
+        self._register_undefined_instance_details(context, instance)
+
    def _get_console_output_file(self, instance, console_log):
        bytes_to_read = MAX_CONSOLE_BYTES
        log_data = b""  # The last N read bytes
@@ -5953,40 +6050,7 @@ class LibvirtDriver(driver.ComputeDriver):

    def _add_video_driver(self, guest, image_meta, flavor):
        video = vconfig.LibvirtConfigGuestVideo()
-        # NOTE(ldbragst): The following logic sets the video.type
-        # depending on supported defaults given the architecture,
-        # virtualization type, and features. The video.type attribute can
-        # be overridden by the user with image_meta.properties, which
-        # is carried out in the next if statement below this one.
-        guestarch = libvirt_utils.get_arch(image_meta)
-        if CONF.libvirt.virt_type == 'parallels':
-            video.type = 'vga'
-        # NOTE(kchamart): 'virtio' is a sensible default whether or not
-        # the guest has the native kernel driver (called "virtio-gpu" in
-        # Linux) -- i.e. if the guest has the VirtIO GPU driver, it'll
-        # be used; otherwise, the 'virtio' model will gracefully
-        # fallback to VGA compatibiliy mode.
-        elif (guestarch in (fields.Architecture.I686,
-                            fields.Architecture.X86_64) and not
-                            CONF.spice.enabled):
-            video.type = 'virtio'
-        elif guestarch in (fields.Architecture.PPC,
-                           fields.Architecture.PPC64,
-                           fields.Architecture.PPC64LE):
-            # NOTE(ldbragst): PowerKVM doesn't support 'cirrus' be default
-            # so use 'vga' instead when running on Power hardware.
-            video.type = 'vga'
-        elif guestarch == fields.Architecture.AARCH64:
-            # NOTE(kevinz): Only virtio device type is supported by AARCH64
-            # so use 'virtio' instead when running on AArch64 hardware.
-            video.type = 'virtio'
-        elif CONF.spice.enabled:
-            video.type = 'qxl'
-        if image_meta.properties.get('hw_video_model'):
-            video.type = image_meta.properties.hw_video_model
-            if not self._video_model_supported(video.type):
-                raise exception.InvalidVideoMode(model=video.type)
-
+        video.type = self._get_video_type(image_meta) or video.type
        # Set video memory, only if the flavor's limit is set
        video_ram = image_meta.properties.get('hw_video_ram', 0)
        max_vram = int(flavor.extra_specs.get('hw_video:ram_max_mb', 0))
@@ -6001,6 +6065,62 @@ class LibvirtDriver(driver.ComputeDriver):
        # for simpler testing.
        return video

+    def _get_video_type(
+        self,
+        image_meta: objects.ImageMeta,
+    ) -> ty.Optional[str]:
+        # NOTE(ldbragst): The following logic returns the video type
+        # depending on supported defaults given the architecture,
+        # virtualization type, and features. The video type can
+        # be overridden by the user with image_meta.properties, which
+        # is carried out first.
+        if image_meta.properties.get('hw_video_model'):
+            video_type = image_meta.properties.hw_video_model
+            if not self._video_model_supported(video_type):
+                raise exception.InvalidVideoMode(model=video_type)
+            return video_type
+
+        guestarch = libvirt_utils.get_arch(image_meta)
+
+        if CONF.libvirt.virt_type == 'parallels':
+            return 'vga'
+
+        # NOTE(kchamart): 'virtio' is a sensible default whether or not
+        # the guest has the native kernel driver (called "virtio-gpu" in
+        # Linux) -- i.e. if the guest has the VirtIO GPU driver, it'll
+        # be used; otherwise, the 'virtio' model will gracefully
+        # fallback to VGA compatibiliy mode.
+        if (
+            guestarch in (
+                fields.Architecture.I686,
+                fields.Architecture.X86_64
+            ) and not CONF.spice.enabled
+        ):
+            return 'virtio'
+
+        if (
+            guestarch in (
+                fields.Architecture.PPC,
+                fields.Architecture.PPC64,
+                fields.Architecture.PPC64LE
+            )
+        ):
+            # NOTE(ldbragst): PowerKVM doesn't support 'cirrus' be default
+            # so use 'vga' instead when running on Power hardware.
+            return 'vga'
+
+        if guestarch == fields.Architecture.AARCH64:
+            # NOTE(kevinz): Only virtio device type is supported by AARCH64
+            # so use 'virtio' instead when running on AArch64 hardware.
+            return 'virtio'
+
+        if CONF.spice.enabled:
+            return 'qxl'
+
+        # NOTE(lyarwood): Return None and default to the default of
+        # LibvirtConfigGuestVideo.type that is currently virtio
+        return None
+
    def _add_qga_device(self, guest, instance):
        qga = vconfig.LibvirtConfigGuestChannel()
        qga.type = "unix"
@@ -6968,13 +7088,11 @@ class LibvirtDriver(driver.ComputeDriver):

        return add_video_driver

-    def _guest_add_pointer_device(self, guest, image_meta):
-        """Build the pointer device to add to the instance.
-
-        The configuration is determined by examining the 'hw_input_bus' image
-        metadata property, the 'hw_pointer_model' image metadata property, and
-        the '[DEFAULT] pointer_model' config option in that order.
-        """
+    def _get_pointer_bus_and_model(
+        self,
+        guest: vconfig.LibvirtConfigGuest,
+        image_meta: objects.ImageMeta,
+    ) -> ty.Tuple[ty.Optional[str], ty.Optional[str]]:
        pointer_bus = image_meta.properties.get('hw_input_bus')
        pointer_model = image_meta.properties.get('hw_pointer_model')

@@ -6988,7 +7106,7 @@ class LibvirtDriver(driver.ComputeDriver):
        else:
            # If the user hasn't requested anything and the host config says to
            # use something other than a USB tablet, there's nothing to do
-            return
+            return None, None

        # For backward compatibility, we don't want to error out if the host
        # configuration requests a USB tablet but the virtual machine mode is
@@ -6998,7 +7116,7 @@ class LibvirtDriver(driver.ComputeDriver):
                'USB tablet requested for guests on non-HVM host; '
                'in order to accept this request the machine mode should '
                'be configured as HVM.')
-            return
+            return None, None

        # Ditto for using a USB tablet when the SPICE agent is enabled, since
        # that has a paravirt mouse builtin which drastically reduces overhead;
@@ -7012,15 +7130,32 @@ class LibvirtDriver(driver.ComputeDriver):
                'USB tablet requested for guests but the SPICE agent is '
                'enabled; ignoring request in favour of default '
                'configuration.')
-            return
+            return None, None

-        pointer = vconfig.LibvirtConfigGuestInput()
-        pointer.type = pointer_model
-        pointer.bus = pointer_bus
-        guest.add_device(pointer)
+        return pointer_model, pointer_bus

-        # returned for unit testing purposes
-        return pointer
+    def _guest_add_pointer_device(
+        self,
+        guest: vconfig.LibvirtConfigGuest,
+        image_meta: objects.ImageMeta
+    ) -> None:
+        """Build the pointer device to add to the instance.
+
+        The configuration is determined by examining the 'hw_input_bus' image
+        metadata property, the 'hw_pointer_model' image metadata property, and
+        the '[DEFAULT] pointer_model' config option in that order.
+        """
+        pointer_model, pointer_bus = self._get_pointer_bus_and_model(
+            guest, image_meta)
+
+        if pointer_model and pointer_bus:
+            pointer = vconfig.LibvirtConfigGuestInput()
+            pointer.type = pointer_model
+            pointer.bus = pointer_bus
+            guest.add_device(pointer)
+
+            # returned for unit testing purposes
+            return pointer

    def _guest_add_keyboard_device(self, guest, image_meta):
        """Add keyboard for graphical console use."""
@@ -0,0 +1,15 @@
+other:
+  - |
+    Default image properties for device buses and models are now persisted in
+    the instance system metadata for the following image properties:
+
+    * ``hw_cdrom_bus``
+    * ``hw_disk_bus``
+    * ``hw_input_bus``
+    * ``hw_pointer_model``
+    * ``hw_video_model``
+    * ``hw_vif_model``
+
+    Instance device buses and models will now remain stable across reboots and
+    will not be changed by new defaults in libosinfo or the OpenStack Nova
+    libvirt driver.