Francesco Romani has posted comments on this change. Change subject: vdsm: [wip] modeling graphics as a device ......................................................................
Patch Set 8: (19 comments) http://gerrit.ovirt.org/#/c/23555/8/tests/vmTests.py File tests/vmTests.py: Line 646: @MonkeyPatch(libvirtconnection, 'get', lambda x: ConnectionMock()) Line 647: @MonkeyPatch(utils, 'getHostUUID', Line 648: lambda: "fc25cbbe-5520-4f83-b82e-1541914753d9") Line 649: def testBuildCmdLineX86_64(self): Line 650: self.assertBuildCmdLine(vmTestsData.CONF_TO_DOMXML_X86_64) > nice, but unrelated Done Line 651: Line 652: @MonkeyPatch(caps, 'getTargetArch', lambda: caps.Architecture.PPC64) Line 653: @MonkeyPatch(caps, 'osversion', lambda: { Line 654: 'release': '1', 'version': '18', 'name': 'Fedora'}) Line 655: @MonkeyPatch(libvirtconnection, 'get', lambda x: ConnectionMock()) Line 656: @MonkeyPatch(utils, 'getHostUUID', Line 657: lambda: "fc25cbbe-5520-4f83-b82e-1541914753d9") Line 658: def testBuildCmdLinePPC64(self): Line 659: self.assertBuildCmdLine(vmTestsData.CONF_TO_DOMXML_PPC64) > ditto Done Line 660: Line 661: Line 662: @contextmanager Line 663: def FakeVM(params=None): http://gerrit.ovirt.org/#/c/23555/8/tests/vmTestsData.py File tests/vmTestsData.py: Line 56 Line 57 Line 58 Line 59 Line 60 > Why this was removed from the generic XML? Pending Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: # list of tripples (json, legacy json, xml) > typo :) Done Line 22: CONF_TO_DOMXML_GRAPHICS = [({ Line 23: 'type': 'graphics', Line 24: 'device': 'spice', Line 25: 'specParams': {'port': '-1', http://gerrit.ovirt.org/#/c/23555/8/vdsm/API.py File vdsm/API.py: Line 253: Line 254: if 'nicModel' not in vmParams: Line 255: vmParams['nicModel'] = config.get('vars', 'nic_model') Line 256: Line 257: self._adjustDisplayParams(vmParams) > This code better fits into vm.py's conversion of old-style graphics args to Will move as parte of the planned reorganization in the next revisions. Line 258: Line 259: return self._cif.createVm(vmParams) Line 260: Line 261: except OSError as e: Line 272: Adjusts parameters of graphics on vm start: Line 273: - resets ports to be autoselected by libvirt, Line 274: - sets displayNetwork value according to vm display network. Line 275: """ Line 276: def adjustDisplayParamsInternal(obj): > "obj" is a very vague name, please avoid it. Done Line 277: LIBVIRT_PORT_AUTOSELECT = '-1' Line 278: obj['displayIp'] = \ Line 279: self._getNetworkIp(vmParams.get('displayNetwork')) Line 280: for i in ('port', 'tlsPort', 'displayPort', 'displaySecurePort'): Line 275: """ Line 276: def adjustDisplayParamsInternal(obj): Line 277: LIBVIRT_PORT_AUTOSELECT = '-1' Line 278: obj['displayIp'] = \ Line 279: self._getNetworkIp(vmParams.get('displayNetwork')) > (disclaimer: same potential issue in the old code) Still to be verified Line 280: for i in ('port', 'tlsPort', 'displayPort', 'displaySecurePort'): Line 281: if i in obj: Line 282: obj[i] = LIBVIRT_PORT_AUTOSELECT Line 283: Line 286: Line 287: # new approach - graphics is a device Line 288: for dev in vmParams.get('devices', []): Line 289: if dev.get('type') == 'graphics': Line 290: specPars = dev.get('specParams', []) > and even more importantly, specParams is a dictionary, not a list. Both fixed Line 291: adjustDisplayParamsInternal(specPars) Line 292: Line 293: def desktopLock(self): Line 294: """ http://gerrit.ovirt.org/#/c/23555/8/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 373: startTime) Line 374: self._monitorThread.start() Line 375: Line 376: try: Line 377: # todo take graphics from conf['devices'] into account > Indeed. It would be nice to have a very simple patch defining a function na Will do as part of upcoming patch split/reorganization Line 378: if ('qxl' in self._vm.conf['display'] and Line 379: self._vm.conf.get('clientIp')): Line 380: SPICE_MIGRATION_HANDOVER_TIME = 120 Line 381: self._vm._reviveTicket(SPICE_MIGRATION_HANDOVER_TIME) Line 977: Line 978: <clock offset="variable" adjustment="-3600"> Line 979: <timer name="rtc" tickpolicy="catchup"> Line 980: </clock> Line 981: """ > please avoid needless whitespace noise. Done Line 982: m = XMLElement('clock', offset='variable', Line 983: adjustment=str(self.conf.get('timeOffset', 0))) Line 984: m.appendChildWithArgs('timer', name='rtc', tickpolicy='catchup') Line 985: m.appendChildWithArgs('timer', name='pit', tickpolicy='delay') Line 1228: vmc = XMLElement('channel', type='spicevmc') Line 1229: vmc.appendChildWithArgs('target', type='virtio', Line 1230: name='com.redhat.spice.0') Line 1231: # self.dom.appendChild(vmc) Line 1232: self._devices.appendChild(vmc) > ? Not sure I get it, will figure it out. Line 1233: Line 1234: def toxml(self): Line 1235: return self.doc.toprettyxml(encoding='utf-8') Line 1236: Line 1358: <listen type='address' address='0'/> Line 1359: </graphics> Line 1360: Line 1361: """ Line 1362: specParams = self.specParams > Do we really need this? Usually shortcuts aren't much used in VDSM Removed Line 1363: graphicsAttrs = {'type': self.device, Line 1364: 'port': specParams.get('port', '-1'), Line 1365: 'autoport': 'yes'} Line 1366: Line 2161: 'index': 0, Line 2162: 'truesize': 0}) Line 2163: return removables Line 2164: Line 2165: def _getGraphicsDeviceFromConf(self): > it would make sense to follow the __legacyDrives naming convension for this Fixed (IIUC :) ) Line 2166: """ Line 2167: Derives graphics device (if it's missing in vm.devices) from vm.conf. Line 2168: Used for legacy engines (that don't use graphics framebuffer as a Line 2169: device). Line 2200: if self.conf.get('devices') is None: Line 2201: devices[DISK_DEVICES] = self.getConfDrives() Line 2202: devices[NIC_DEVICES] = self.getConfNetworkInterfaces() Line 2203: devices[SOUND_DEVICES] = self.getConfSound() Line 2204: devices[VIDEO_DEVICES] = self.getConfVideo() > This is wrong and I know it. Still pending Line 2205: devices[GRAPHICS_DEVICES] = [self._getGraphicsDeviceFromConf()] Line 2206: devices[CONTROLLER_DEVICES] = self.getConfController() Line 2207: else: Line 2208: derivedGraphics = self._getGraphicsDeviceFromConf() Line 2882: Line 2883: def _getGraphicsDevices(self): Line 2884: """ Line 2885: Returns graphics devices of a VM. If the VM has multiple devices, then Line 2886: SPICE dev is returned as a first one. > Why? And would there never be more than two? Still pending Line 2887: """ Line 2888: graphDevs = [d for d in self.conf.get('devices', []) Line 2889: if d['type'] == GRAPHICS_DEVICES] Line 2890: Line 2888: graphDevs = [d for d in self.conf.get('devices', []) Line 2889: if d['type'] == GRAPHICS_DEVICES] Line 2890: Line 2891: if len(graphDevs) > 1 and graphDevs[1]['device'] == 'spice': Line 2892: graphDevs[0], graphDevs[1] = graphDevs[1], graphDevs[0] > Not sure I understand the code here. What we want to achieve? Ditto Line 2893: Line 2894: return graphDevs Line 2895: Line 2896: def _getStatsInternal(self): Line 2926: 'network': {}, 'disks': {}, Line 2927: 'monitorResponse': str(self._monitorResponse), Line 2928: 'elapsedTime': str(int(time.time() - self._startTime)), } Line 2929: Line 2930: graphics = self._getGraphicsDevices() > Please encapsulate this in a separate properly-named function, maybe _getLe Done (but used different name) Line 2931: if len(graphics) > 0: Line 2932: fstGraphics = graphics[0] Line 2933: confToSpecParsMapping = GraphicsDevice.confToSpecParsMapping Line 2934: Line 2940: Line 2941: displayType = fstGraphics['device'] Line 2942: if displayType == 'spice': Line 2943: displayType = 'qxl' Line 2944: stats.update({'displayType': displayType}) > this is a very complex way of writing Done Line 2945: Line 2946: if 'cdrom' in self.conf: Line 2947: stats['cdrom'] = self.conf['cdrom'] Line 2948: if 'boot' in self.conf: Line 3119: _QEMU_GA_DEVICE_NAME) Line 3120: domxml.appendInput() Line 3121: Line 3122: # todo (this is just workaround for creating spicevmc channel) Line 3123: graphics = self._getGraphicsDevices() > if [for dev in self._getGraphicsDevices() if dev['device'] == 'spice']: Fixed an a slightly different way Line 3124: if len(graphics) > 0 and graphics[0]['device'] == 'spice': Line 3125: domxml.appendSpiceVmcChannel() Line 3126: Line 3127: if self.arch == caps.Architecture.PPC64: -- To view, visit http://gerrit.ovirt.org/23555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia59b933f4cd1e3ab562ad2ec1c237007c83f214c Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
