From d983234288728427235ef2c1f355ec135119b865 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 23 Aug 2017 11:15:17 +0100 Subject: [PATCH] conf: Allow users to unset 'keymap' options Defining the 'keymap' option in libvirt results in the '-k' option being passed through to QEMU [1][2]. This QEMU option has some uses, primarily for users interacting with QEMU via stdin on the text console. However, for users interacting with QEMU via VNC or Spice, like nova users do, it is strongly recommended to never add the "-k" option. Doing so will force QEMU to do keymap conversions which are known to be lossy. This disproportionately affects users with non-US keyboard layouts, who would be better served by relying on the guest OS to manage this. In the long term, we would like to deprecate these options. However, we must do this in three parts. This part allows users to unset the options and warns users who have them set about the side effects. This change is intended to be backported. A future change will fully deprecate the options. Finally, after the deprecation cycle has passed, we can remove these options in their entirety. [1] https://github.com/libvirt/libvirt/blob/v1.2.9-maint/src/qemu/qemu_command.c#L6985-L6986 [2] https://github.com/libvirt/libvirt/blob/v1.2.9-maint/src/qemu/qemu_command.c#L7215-L7216 Change-Id: I6b1d719db0537b0f53768dbb00a5b4d01c85ba3a Related-Bug: #1682020 --- nova/tests/unit/virt/libvirt/test_driver.py | 53 +++++++++++++++++-- nova/virt/libvirt/driver.py | 12 ++++- ...able-keymap-settings-fa831c02e4158507.yaml | 6 +++ 3 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/unsettable-keymap-settings-fa831c02e4158507.yaml diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index c29b39693280..83a46aa21064 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -3566,7 +3566,10 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_save.assert_called_with() def test_get_guest_config_with_vnc(self): - self.flags(enabled=True, group='vnc') + self.flags(enabled=True, + vncserver_listen='10.0.0.1', + keymap='en-ie', + group='vnc') self.flags(virt_type='kvm', group='libvirt') self.flags(pointer_model='ps2mouse') self.flags(enabled=False, group='spice') @@ -3596,7 +3599,9 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertIsInstance(cfg.devices[6], vconfig.LibvirtConfigMemoryBalloon) - self.assertEqual(cfg.devices[4].type, "vnc") + self.assertEqual(cfg.devices[4].type, 'vnc') + self.assertEqual(cfg.devices[4].keymap, 'en-ie') + self.assertEqual(cfg.devices[4].listen, '10.0.0.1') def test_get_guest_config_with_vnc_and_tablet(self): self.flags(enabled=True, group='vnc') @@ -3642,6 +3647,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, group='libvirt') self.flags(enabled=True, agent_enabled=False, + server_listen='10.0.0.1', + keymap='en-ie', group='spice') drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) @@ -3671,8 +3678,10 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertIsInstance(cfg.devices[7], vconfig.LibvirtConfigMemoryBalloon) - self.assertEqual(cfg.devices[4].type, "tablet") - self.assertEqual(cfg.devices[5].type, "spice") + self.assertEqual(cfg.devices[4].type, 'tablet') + self.assertEqual(cfg.devices[5].type, 'spice') + self.assertEqual(cfg.devices[5].keymap, 'en-ie') + self.assertEqual(cfg.devices[5].listen, '10.0.0.1') def test_get_guest_config_with_spice_and_agent(self): self.flags(enabled=False, group='vnc') @@ -3715,6 +3724,42 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(cfg.devices[5].type, "spice") self.assertEqual(cfg.devices[6].type, "qxl") + def _test_get_guest_config_with_graphics(self): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) + instance_ref = objects.Instance(**self.test_instance) + image_meta = objects.ImageMeta.from_dict(self.test_image_meta) + + disk_info = blockinfo.get_disk_info(CONF.libvirt.virt_type, + instance_ref, + image_meta) + cfg = drvr._get_guest_config(instance_ref, [], + image_meta, disk_info) + return cfg.devices + + def test_get_guest_config_with_vnc_no_keymap(self): + self.flags(virt_type='kvm', group='libvirt') + self.flags(enabled=True, keymap=None, group='vnc') + self.flags(enabled=False, group='spice') + devices = self._test_get_guest_config_with_graphics() + for device in devices: + if device.root_name == 'graphics': + self.assertIsInstance(device, + vconfig.LibvirtConfigGuestGraphics) + self.assertEqual('vnc', device.type) + self.assertIsNone(device.keymap) + + def test_get_guest_config_with_spice_no_keymap(self): + self.flags(virt_type='kvm', group='libvirt') + self.flags(enabled=True, keymap=None, group='spice') + self.flags(enabled=False, group='vnc') + devices = self._test_get_guest_config_with_graphics() + for device in devices: + if device.root_name == 'graphics': + self.assertIsInstance(device, + vconfig.LibvirtConfigGuestGraphics) + self.assertEqual('spice', device.type) + self.assertIsNone(device.keymap) + @mock.patch.object(host.Host, 'get_guest') @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_serial_ports_from_guest') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index c124c9c4c7e0..a46b6a69d9b1 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -4950,14 +4950,22 @@ class LibvirtDriver(driver.ComputeDriver): if CONF.vnc.enabled and guest.virt_type not in ('lxc', 'uml'): graphics = vconfig.LibvirtConfigGuestGraphics() graphics.type = "vnc" - graphics.keymap = CONF.vnc.keymap + if CONF.vnc.keymap: + # TODO(stephenfin): There are some issues here that may + # necessitate deprecating this option entirely in the future. + # Refer to bug #1682020 for more information. + graphics.keymap = CONF.vnc.keymap graphics.listen = CONF.vnc.vncserver_listen guest.add_device(graphics) add_video_driver = True if CONF.spice.enabled and guest.virt_type not in ('lxc', 'uml', 'xen'): graphics = vconfig.LibvirtConfigGuestGraphics() graphics.type = "spice" - graphics.keymap = CONF.spice.keymap + if CONF.spice.keymap: + # TODO(stephenfin): There are some issues here that may + # necessitate deprecating this option entirely in the future. + # Refer to bug #1682020 for more information. + graphics.keymap = CONF.spice.keymap graphics.listen = CONF.spice.server_listen guest.add_device(graphics) add_video_driver = True diff --git a/releasenotes/notes/unsettable-keymap-settings-fa831c02e4158507.yaml b/releasenotes/notes/unsettable-keymap-settings-fa831c02e4158507.yaml new file mode 100644 index 000000000000..300684655de9 --- /dev/null +++ b/releasenotes/notes/unsettable-keymap-settings-fa831c02e4158507.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + It is now possible to unset the ``[vnc]keymap`` and ``[spice]keymap`` + configuration options. These were known to cause issues for some users + with non-US keyboards and may be deprecated in the future.