Milan Zamazal has uploaded a new change for review. Change subject: virt: Don't fail when existingConnAction is unset for a SPICE device ......................................................................
virt: Don't fail when existingConnAction is unset for a SPICE device When a ticket for a SPICE device is set, existingConnAction parameter is sent by Engine as well. This parameter is used to set `connected' attribute of <graphics> element. But setting this parameter when updating the ticket is basically useless. It may be actually even harmful, e.g. when a user sets `connected' attribute value to `keep' in a Vdsm hook and Engine overrides it to `disconnect' without any good reason. See the referenced bug. This should be fixed on the Engine side. One approach may be not to send the parameter at all when updating the SPICE ticket. However, Vdsm crashes in such a case. This patch fixes Vdsm problems when the parameter is not present, making future changes in Engine possible. Change-Id: I2fde83ceb994eaafd2aa956d1fa82ce6cb16094c Bug-Url: https://bugzilla.redhat.com/1060573 Signed-off-by: Milan Zamazal <mzama...@redhat.com> --- M tests/vmOperationsTests.py M vdsm/virt/vm.py 2 files changed, 25 insertions(+), 12 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/36/56836/1 diff --git a/tests/vmOperationsTests.py b/tests/vmOperationsTests.py index 7c65428..82ad7b0 100644 --- a/tests/vmOperationsTests.py +++ b/tests/vmOperationsTests.py @@ -129,17 +129,26 @@ self.assertEqual(testvm.getStats()['timeOffset'], str(self.BASE_OFFSET + self.UPDATE_OFFSETS[-1])) - def testUpdateSingleDeviceGraphics(self): + @permutations([['disconnect'], [None]]) + def testUpdateSingleDeviceGraphics(self, connected): + if connected: + graphics_params = _GRAPHICS_DEVICE_PARAMS + connected_attribute = ' connected="disconnect"' + else: + graphics_params = {k: v for k, v in _GRAPHICS_DEVICE_PARAMS.items() + if k != 'existingConnAction'} + connected_attribute = '' devXmls = ( - '<graphics connected="disconnect" passwd="12345678"' - ' port="5900" type="spice"/>', + '<graphics%s passwd="12345678"' + ' port="5900" type="spice"/>' % (connected_attribute,), '<graphics passwd="12345678" port="5900" type="vnc"/>') for device, devXml in zip(self.GRAPHIC_DEVICES, devXmls): domXml = ''' <devices> <graphics type="%s" port="5900" /> </devices>''' % device['device'] - self._verifyDeviceUpdate(device, device, domXml, devXml) + self._verifyDeviceUpdate(device, device, domXml, devXml, + graphics_params) def testUpdateMultipleDeviceGraphics(self): devXmls = ( @@ -153,23 +162,26 @@ </devices>''' for device, devXml in zip(self.GRAPHIC_DEVICES, devXmls): self._verifyDeviceUpdate( - device, self.GRAPHIC_DEVICES, domXml, devXml) + device, self.GRAPHIC_DEVICES, domXml, devXml, + _GRAPHICS_DEVICE_PARAMS) - def _updateGraphicsDevice(self, testvm, device_type): + def _updateGraphicsDevice(self, testvm, device_type, graphics_params): def _check_ticket_params(domXML, conf, params): self.assertEqual(params, _TICKET_PARAMS) with MonkeyPatchScope([(hooks, 'before_vm_set_ticket', _check_ticket_params)]): params = {'graphicsType': device_type} - params.update(_GRAPHICS_DEVICE_PARAMS) + params.update(graphics_params) return testvm.updateDevice(params) - def _verifyDeviceUpdate(self, device, allDevices, domXml, devXml): + def _verifyDeviceUpdate(self, device, allDevices, domXml, devXml, + graphics_params): with fake.VM(devices=allDevices) as testvm: testvm._dom = fake.Domain(domXml) - self._updateGraphicsDevice(testvm, device['device']) + self._updateGraphicsDevice(testvm, device['device'], + graphics_params) self.assertEquals(testvm._dom.devXml, devXml) @@ -312,7 +324,8 @@ domain.updateDeviceFlags = _fail testvm._dom = domain - res = self._updateGraphicsDevice(testvm, device) + res = self._updateGraphicsDevice(testvm, device, + _GRAPHICS_DEVICE_PARAMS) self.assertEqual(res, response.error('ticketErr', message)) diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index 8ac6242..1148d8e 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -2302,7 +2302,7 @@ if graphics: result = self._setTicketForGraphicDev( graphics, params['password'], params['ttl'], - params['existingConnAction'], params['params']) + params.get('existingConnAction'), params['params']) if result['status']['code'] == 0: result['vmList'] = self.status() return result @@ -3812,7 +3812,7 @@ validto = time.strftime('%Y-%m-%dT%H:%M:%S', time.gmtime(time.time() + float(seconds))) graphics.setAttribute('passwdValidTo', validto) - if graphics.getAttribute('type') == 'spice': + if graphics.getAttribute('type') == 'spice' and connAct is not None: graphics.setAttribute('connected', connAct) hooks.before_vm_set_ticket(self._domain.xml, self.conf, params) try: -- To view, visit https://gerrit.ovirt.org/56836 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2fde83ceb994eaafd2aa956d1fa82ce6cb16094c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
