diff --git a/nova/tests/functional/libvirt/test_device_bus_migration.py b/nova/tests/functional/libvirt/test_device_bus_migration.py new file mode 100644 index 0000000000..eccb0738a9 --- /dev/null +++ b/nova/tests/functional/libvirt/test_device_bus_migration.py @@ -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) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 293f36bf8a..feb63daf99 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -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.""" diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 486830ca81..83be383abc 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -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.""" diff --git a/releasenotes/notes/register-defaults-for-undefined-hw-image-properties-d86bcf99f4610239.yaml b/releasenotes/notes/register-defaults-for-undefined-hw-image-properties-d86bcf99f4610239.yaml new file mode 100644 index 0000000000..40ac1e4d85 --- /dev/null +++ b/releasenotes/notes/register-defaults-for-undefined-hw-image-properties-d86bcf99f4610239.yaml @@ -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.