From 3f63c68195d296ad44cff615a470cd11518a41df Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 6 Oct 2020 13:49:58 +0100 Subject: [PATCH] libvirt: Add support for virtio-based input devices The USB-based tablet is often the only USB device in an x86 instance, while the USB-based keyboard is often the only such device in an AArch64 instance (x86 have PS2 keyboards and mice). Replacing these with virtio-based devices can eliminate the need to have a USB host adapter in the instance. Enable just that possibility by adding a new value image metadata property, 'hw_input_bus'. This allows us to specify not only virtio-based pointer and keyboard input devices but also USB equivalents. Note that this also fixes one instance of a particular class of bugs, whereby we have checks for *guest* architecture-specific behavior that are being toggled based on the *host* architecture. In this instance, we were attempting to add a keyboard device on AArch64 guests since they don't have one by default, but we were determining the architecture by looking at the CPU architecture reported in the host capabilities. By replacing this check of the host capabilities with a call to the 'nova.virt.libvirt.utils.get_arch' helper, we correctly handle requests to create non-host architecture guests via the 'hw_architecture' image metadata property. There are many other instances of this bug and those can be resolved separately. Change-Id: If9f3ede3e8449f9a6c8d1da927974c0a73923d51 Signed-off-by: Stephen Finucane --- .../ImageMetaPropsPayload.json | 2 +- nova/conf/compute.py | 4 +- nova/notifications/objects/image.py | 3 +- nova/objects/fields.py | 14 ++- nova/objects/image_meta.py | 8 +- .../test_instance.py | 4 +- .../objects/test_notification.py | 2 +- nova/tests/unit/objects/test_image_meta.py | 16 ++++ nova/tests/unit/objects/test_objects.py | 2 +- nova/tests/unit/virt/libvirt/test_driver.py | 87 ++++++++++++++++--- nova/virt/libvirt/driver.py | 75 ++++++++++------ ...-image-metadata-prop-059bea459dec618e.yaml | 10 +++ 12 files changed, 180 insertions(+), 47 deletions(-) create mode 100644 releasenotes/notes/add-hw_input_bus-image-metadata-prop-059bea459dec618e.yaml diff --git a/doc/notification_samples/common_payloads/ImageMetaPropsPayload.json b/doc/notification_samples/common_payloads/ImageMetaPropsPayload.json index 3435730b1c10..c970a4c45df8 100644 --- a/doc/notification_samples/common_payloads/ImageMetaPropsPayload.json +++ b/doc/notification_samples/common_payloads/ImageMetaPropsPayload.json @@ -4,5 +4,5 @@ "hw_architecture": "x86_64" }, "nova_object.name": "ImageMetaPropsPayload", - "nova_object.version": "1.6" + "nova_object.version": "1.7" } diff --git a/nova/conf/compute.py b/nova/conf/compute.py index df68f0d82a55..e787424977ac 100644 --- a/nova/conf/compute.py +++ b/nova/conf/compute.py @@ -296,8 +296,8 @@ Generic property to specify the pointer type. Input devices allow interaction with a graphical framebuffer. For example to provide a graphic tablet for absolute cursor movement. -If set, the 'hw_pointer_model' image property takes precedence over -this configuration option. +If set, either the ``hw_input_bus`` or ``hw_pointer_model`` image metadata +properties will take precedence over this configuration option. Related options: diff --git a/nova/notifications/objects/image.py b/nova/notifications/objects/image.py index 4b673ab56652..d35627eb3d2b 100644 --- a/nova/notifications/objects/image.py +++ b/nova/notifications/objects/image.py @@ -123,7 +123,8 @@ class ImageMetaPropsPayload(base.NotificationPayloadBase): # Version 1.4: Added 'mixed' to hw_cpu_policy field # Version 1.5: Added 'hw_tpm_model' and 'hw_tpm_version' fields # Version 1.6: Added 'socket' to hw_pci_numa_affinity_policy - VERSION = '1.6' + # Version 1.7: Added 'hw_input_bus' field + VERSION = '1.7' SCHEMA = { k: ('image_meta_props', k) for k in image_meta.ImageMetaProps.fields} diff --git a/nova/objects/fields.py b/nova/objects/fields.py index 4d03456848d3..8c4fc299aab6 100644 --- a/nova/objects/fields.py +++ b/nova/objects/fields.py @@ -465,6 +465,14 @@ class ImageSignatureKeyType(BaseNovaEnum): ) +class InputBus(BaseNovaEnum): + + USB = 'usb' + VIRTIO = 'virtio' + + ALL = (USB, VIRTIO) + + class MigrationType(BaseNovaEnum): MIGRATION = 'migration' # cold migration @@ -793,7 +801,7 @@ class PointerModelType(BaseNovaEnum): USBTABLET = "usbtablet" - ALL = (USBTABLET) + ALL = (USBTABLET,) class NotificationPriority(BaseNovaEnum): @@ -1236,6 +1244,10 @@ class ImageSignatureKeyTypeField(BaseEnumField): AUTO_TYPE = ImageSignatureKeyType() +class InputBusField(BaseEnumField): + AUTO_TYPE = InputBus() + + class MigrationTypeField(BaseEnumField): AUTO_TYPE = MigrationType() diff --git a/nova/objects/image_meta.py b/nova/objects/image_meta.py index 184c37b862ac..c07b358647c4 100644 --- a/nova/objects/image_meta.py +++ b/nova/objects/image_meta.py @@ -177,14 +177,17 @@ class ImageMetaProps(base.NovaObject): # Version 1.26: Added 'mixed' to 'hw_cpu_policy' field # Version 1.27: Added 'hw_tpm_model' and 'hw_tpm_version' fields # Version 1.28: Added 'socket' to 'hw_pci_numa_affinity_policy' + # Version 1.29: Added 'hw_input_bus' field # NOTE(efried): When bumping this version, the version of # ImageMetaPropsPayload must also be bumped. See its docstring for details. - VERSION = '1.28' + VERSION = '1.29' def obj_make_compatible(self, primitive, target_version): super(ImageMetaProps, self).obj_make_compatible(primitive, target_version) target_version = versionutils.convert_version_to_tuple(target_version) + if target_version < (1, 29): + primitive.pop('hw_input_bus', None) if target_version < (1, 28): policy = primitive.get('hw_pci_numa_affinity_policy', None) if policy == fields.PCINUMAAffinityPolicy.SOCKET: @@ -330,6 +333,9 @@ class ImageMetaProps(base.NovaObject): # This indicates the guest needs UEFI firmware 'hw_firmware_type': fields.FirmwareTypeField(), + # name of the input bus type to use, e.g. usb, virtio + 'hw_input_bus': fields.InputBusField(), + # boolean - used to trigger code to inject networking when booting a CD # image with a network boot image 'hw_ipxe_boot': fields.FlexibleBooleanField(), diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index 3af9f8720567..21c922845427 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -1233,7 +1233,7 @@ class TestInstanceNotificationSample( 'nova_object.data': {}, 'nova_object.name': 'ImageMetaPropsPayload', 'nova_object.namespace': 'nova', - 'nova_object.version': u'1.6', + 'nova_object.version': '1.7', }, 'image.size': 58145823, 'image.tags': [], @@ -1329,7 +1329,7 @@ class TestInstanceNotificationSample( 'nova_object.data': {}, 'nova_object.name': 'ImageMetaPropsPayload', 'nova_object.namespace': 'nova', - 'nova_object.version': u'1.6', + 'nova_object.version': '1.7', }, 'image.size': 58145823, 'image.tags': [], diff --git a/nova/tests/unit/notifications/objects/test_notification.py b/nova/tests/unit/notifications/objects/test_notification.py index 2cb97f8a2d7a..a55bbc0b9eb1 100644 --- a/nova/tests/unit/notifications/objects/test_notification.py +++ b/nova/tests/unit/notifications/objects/test_notification.py @@ -387,7 +387,7 @@ notification_object_data = { # ImageMetaProps, so when you see a fail here for that reason, you must # *also* bump the version of ImageMetaPropsPayload. See its docstring for # more information. - 'ImageMetaPropsPayload': '1.6-2be4d0bdd1d19a541c46a0d69e244d3f', + 'ImageMetaPropsPayload': '1.7-652a6048036c0d0cb8740ea62521c459', 'InstanceActionNotification': '1.0-a73147b93b520ff0061865849d3dfa56', 'InstanceActionPayload': '1.8-4fa3da9cbf0761f1f700ae578f36dc2f', 'InstanceActionRebuildNotification': diff --git a/nova/tests/unit/objects/test_image_meta.py b/nova/tests/unit/objects/test_image_meta.py index 4238d056a785..a4b7a33fa826 100644 --- a/nova/tests/unit/objects/test_image_meta.py +++ b/nova/tests/unit/objects/test_image_meta.py @@ -349,6 +349,22 @@ class TestImageMetaProps(test.NoDBTestCase): self.assertRaises(exception.ObjectActionError, obj.obj_to_primitive, '1.0') + def test_obj_make_compatible_input_bus(self): + """Check 'hw_input_bus' compatibility.""" + # assert that 'hw_input_bus' is supported on a suitably new version + obj = objects.ImageMetaProps( + hw_input_bus=objects.fields.InputBus.VIRTIO, + ) + primitive = obj.obj_to_primitive('1.29') + self.assertIn('hw_input_bus', primitive['nova_object.data']) + self.assertEqual( + objects.fields.InputBus.VIRTIO, + primitive['nova_object.data']['hw_input_bus']) + + # and is absent on older versions + primitive = obj.obj_to_primitive('1.28') + self.assertNotIn('hw_input_bus', primitive['nova_object.data']) + def test_obj_make_compatible_video_model(self): # assert that older video models are preserved. obj = objects.ImageMetaProps( diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 8c80c64d817b..38761463f4dc 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1074,7 +1074,7 @@ object_data = { 'HyperVLiveMigrateData': '1.4-e265780e6acfa631476c8170e8d6fce0', 'IDEDeviceBus': '1.0-29d4c9f27ac44197f01b6ac1b7e16502', 'ImageMeta': '1.8-642d1b2eb3e880a367f37d72dd76162d', - 'ImageMetaProps': '1.28-a55674868f9e319a6b49d688e558d0aa', + 'ImageMetaProps': '1.29-5c02bd7b1e050ef39513d3fca728e543', 'Instance': '2.7-d187aec68cad2e4d8b8a03a68e4739ce', 'InstanceAction': '1.2-9a5abc87fdd3af46f45731960651efb5', 'InstanceActionEvent': '1.4-5b1f361bd81989f8bb2c20bb7e8a4cb4', diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index b84fabb960a2..553203b8734d 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -5796,25 +5796,38 @@ class LibvirtConnTestCase(test.NoDBTestCase, cfg = self._get_guest_config_with_graphics() - self.assertEqual(len(cfg.devices), 9) + if guestarch != fields.Architecture.AARCH64: + self.assertEqual(len(cfg.devices), 9) + else: + # AArch64 adds a keyboard by default + self.assertEqual(len(cfg.devices), 10) + + devices = iter(cfg.devices) + self.assertIsInstance( - cfg.devices[0], vconfig.LibvirtConfigGuestDisk) + next(devices), vconfig.LibvirtConfigGuestDisk) self.assertIsInstance( - cfg.devices[1], vconfig.LibvirtConfigGuestDisk) + next(devices), vconfig.LibvirtConfigGuestDisk) self.assertIsInstance( - cfg.devices[2], vconfig.LibvirtConfigGuestSerial) + next(devices), vconfig.LibvirtConfigGuestSerial) self.assertIsInstance( - cfg.devices[3], vconfig.LibvirtConfigGuestChannel) + next(devices), vconfig.LibvirtConfigGuestChannel) self.assertIsInstance( - cfg.devices[4], vconfig.LibvirtConfigGuestGraphics) + next(devices), vconfig.LibvirtConfigGuestGraphics) self.assertIsInstance( - cfg.devices[5], vconfig.LibvirtConfigGuestVideo) + next(devices), vconfig.LibvirtConfigGuestVideo) + + if guestarch == fields.Architecture.AARCH64: + # AArch64 adds a keyboard by default + self.assertIsInstance( + next(devices), vconfig.LibvirtConfigGuestInput) + self.assertIsInstance( - cfg.devices[6], vconfig.LibvirtConfigGuestRng) + next(devices), vconfig.LibvirtConfigGuestRng) self.assertIsInstance( - cfg.devices[7], vconfig.LibvirtConfigGuestUSBHostController) + next(devices), vconfig.LibvirtConfigGuestUSBHostController) self.assertIsInstance( - cfg.devices[8], vconfig.LibvirtConfigMemoryBalloon) + next(devices), vconfig.LibvirtConfigMemoryBalloon) self.assertEqual(cfg.devices[3].target_name, "com.redhat.spice.0") self.assertEqual(cfg.devices[3].type, 'spicevmc') @@ -6400,6 +6413,18 @@ class LibvirtConnTestCase(test.NoDBTestCase, dev = self._test_guest_add_pointer_device(image_meta) self.assertIsNotNone(dev) + image_meta = {'properties': {'hw_input_bus': 'usb'}} + + dev = self._test_guest_add_pointer_device(image_meta) + self.assertEqual('tablet', dev.type) + self.assertEqual('usb', dev.bus) + + image_meta = {'properties': {'hw_input_bus': 'virtio'}} + + dev = self._test_guest_add_pointer_device(image_meta) + self.assertEqual('tablet', dev.type) + self.assertEqual('virtio', dev.bus) + @mock.patch.object(libvirt_driver.LOG, 'warning') def test_guest_add_pointer_device__fail_with_spice_agent(self, mock_warn): self.flags(enabled=False, group='vnc') @@ -6437,6 +6462,48 @@ class LibvirtConnTestCase(test.NoDBTestCase, str(mock_warn.call_args[0]), ) + def _test_guest_add_keyboard_device(self, image_meta, arch=None): + self.mock_uname.return_value = fakelibvirt.os_uname( + 'Linux', '', '5.4.0-0-generic', '', + arch or fields.Architecture.X86_64) + + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) + guest = vconfig.LibvirtConfigGuest() + guest.os_type = fields.VMMode.HVM + image_meta = objects.ImageMeta.from_dict({'properties': image_meta}) + return drvr._guest_add_keyboard_device(guest, image_meta) + + def test_guest_add_keyboard_device(self): + props = {} + + dev = self._test_guest_add_keyboard_device(props) + self.assertIsNone(dev) + + props = {'hw_input_bus': 'usb'} + + dev = self._test_guest_add_keyboard_device(props) + self.assertEqual('usb', dev.bus) + self.assertEqual('keyboard', dev.type) + + props = {'hw_input_bus': 'virtio'} + + dev = self._test_guest_add_keyboard_device(props) + self.assertEqual('virtio', dev.bus) + self.assertEqual('keyboard', dev.type) + + props = {'hw_architecture': fields.Architecture.AARCH64} + + dev = self._test_guest_add_keyboard_device(props) + self.assertEqual('usb', dev.bus) + self.assertEqual('keyboard', dev.type) + + props = {} + + dev = self._test_guest_add_keyboard_device( + props, arch=fields.Architecture.AARCH64) + self.assertEqual('usb', dev.bus) + self.assertEqual('keyboard', dev.type) + def test_get_guest_config_with_watchdog_action_flavor(self): self.flags(virt_type='kvm', group='libvirt') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 8490687cc941..d48d0ecbad93 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -6197,17 +6197,6 @@ class LibvirtDriver(driver.ComputeDriver): return True return False - def _guest_add_usb_host_keyboard(self, guest): - """Add USB Host controller and keyboard for graphical console use. - - Add USB keyboard as PS/2 support may not be present on non-x86 - architectures. - """ - keyboard = vconfig.LibvirtConfigGuestInput() - keyboard.type = "keyboard" - keyboard.bus = "usb" - guest.add_device(keyboard) - def _get_guest_config(self, instance, network_info, image_meta, disk_info, rescue=None, block_device_info=None, context=None, mdevs=None, accel_info=None): @@ -6307,14 +6296,7 @@ class LibvirtDriver(driver.ComputeDriver): self._add_video_driver(guest, image_meta, flavor) self._guest_add_pointer_device(guest, image_meta) - - # We want video == we want graphical console. Some architectures - # do not have input devices attached in default configuration. - # Let then add USB Host controller and USB keyboard. - # x86(-64) and ppc64 have usb host controller and keyboard - # s390x does not support USB - if caps.host.cpu.arch == fields.Architecture.AARCH64: - self._guest_add_usb_host_keyboard(guest) + self._guest_add_keyboard_device(guest, image_meta) # Some features are only supported 'qemu' and 'kvm' hypervisor if virt_type in ('qemu', 'kvm'): @@ -6568,11 +6550,25 @@ 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. + """ + pointer_bus = image_meta.properties.get('hw_input_bus') pointer_model = image_meta.properties.get('hw_pointer_model') - # 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 - if pointer_model is None and CONF.pointer_model in (None, 'ps2mouse'): + if pointer_bus: + pointer_model = 'tablet' + pointer_bus = pointer_bus + elif pointer_model or CONF.pointer_model == 'usbtablet': + # Handle the legacy 'hw_pointer_model' image metadata property + pointer_model = 'tablet' + pointer_bus = 'usb' + 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 # For backward compatibility, we don't want to error out if the host @@ -6599,12 +6595,37 @@ class LibvirtDriver(driver.ComputeDriver): 'configuration.') return - tablet = vconfig.LibvirtConfigGuestInput() - tablet.type = 'tablet' - tablet.bus = 'usb' - guest.add_device(tablet) + pointer = vconfig.LibvirtConfigGuestInput() + pointer.type = pointer_model + pointer.bus = pointer_bus + guest.add_device(pointer) + # returned for unit testing purposes - return tablet + return pointer + + def _guest_add_keyboard_device(self, guest, image_meta): + """Add keyboard for graphical console use.""" + bus = image_meta.properties.get('hw_input_bus') + + if not bus: + # AArch64 doesn't provide a default keyboard so we explicitly add + # one; for everything else we rely on default (e.g. for x86, + # libvirt will automatically add a PS2 keyboard) + # TODO(stephenfin): We might want to do this for other non-x86 + # architectures + arch = libvirt_utils.get_arch(image_meta) + if arch != fields.Architecture.AARCH64: + return None + + bus = 'usb' + + keyboard = vconfig.LibvirtConfigGuestInput() + keyboard.type = 'keyboard' + keyboard.bus = bus + guest.add_device(keyboard) + + # returned for unit testing purposes + return keyboard def _get_guest_xml(self, context, instance, network_info, disk_info, image_meta, rescue=None, diff --git a/releasenotes/notes/add-hw_input_bus-image-metadata-prop-059bea459dec618e.yaml b/releasenotes/notes/add-hw_input_bus-image-metadata-prop-059bea459dec618e.yaml new file mode 100644 index 000000000000..d06499abb8c9 --- /dev/null +++ b/releasenotes/notes/add-hw_input_bus-image-metadata-prop-059bea459dec618e.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + A new image metadata property, ``hw_input_bus``, has been added. This + allows you to specify the bus used for input devices - a pointer and + keyboard - which are attached to the instance when graphics are enabled + on compute nodes using the libvirt virt driver. Two values are currently + accepted: ``usb`` and ``virtio``. This image metadata property effectively + replaced the ``hw_pointer_model`` image metadata property, which is + nontheless retained for backwards compatibility purposes.