Change in vdsm[master]: vdsm: [wip] modeling graphics as a device
fromani at redhat.com
fromani at redhat.com
Wed Jan 29 10:34:35 UTC 2014
Francesco Romani has posted comments on this change.
Change subject: vdsm: [wip] modeling graphics as a device
......................................................................
Patch Set 3:
(13 comments)
preliminary review; not voting since it is clearly and explicitely marked as draft
http://gerrit.ovirt.org/#/c/23555/3//COMMIT_MSG
Commit Message:
Line 13:
Line 14: to be done:
Line 15: - spice vmc channel
Line 16: - the test data should be probably shared between legacy test and the
Line 17: new one
Yes, if the XML test data is identical, I think it is better as it was done with the PPC patch. see tests/vmTestData.py
Line 18:
Line 19: Signed-off-by: Frantisek Kobzik <fkobzik at redhat.com>
http://gerrit.ovirt.org/#/c/23555/3/tests/vmTests.py
File tests/vmTests.py:
Line 333: """
Line 334: <graphics autoport="yes" keymap="en-us" passwd="*****"
Line 335: passwdValidTo="1970-01-01T00:00:01" port="-1" type="vnc">
Line 336: <listen network="vdsm-vmDisplay" type="network"/>
Line 337: </graphics>""",
Indeed the XML looks identical (well at least this snippet), if it is not please highlight the differences
Line 338:
Line 339: """
Line 340: <graphics autoport="yes" listen="0" passwd="*****"
Line 341: passwdValidTo="1970-01-01T00:00:01" port="-1"
Line 359: 'spiceSecureChannels':
Line 360: "smain,sinputs,scursor,splayback,sdisplay"
Line 361: }}]
Line 362: for dev, xml in zip(devs, expectedXMLs):
Line 363: genxml = vm.GraphicsDevice(self.conf, self.log, **dev)
nit: genxml is misleading here
Line 364: self.assertXML(genxml.getXML(), xml)
Line 365:
Line 366: def testBalloonXML(self):
Line 367: balloonXML = '<memballoon model="virtio"/>'
http://gerrit.ovirt.org/#/c/23555/3/vdsm/API.py
File vdsm/API.py:
Line 266: # legacy
Line 267: vmParams['displayIp'] = self._getNetworkIp(vmParams.get(
Line 268: 'displayNetwork'))
Line 269: vmParams['displayPort'] = '-1' # selected by libvirt
Line 270: vmParams['displaySecurePort'] = '-1'
since those three lines above seems to be used verbatim even in the new approach, better to factor them out in a common method
Line 271:
Line 272: # new approach - graphics is a device
Line 273: for dev in vmParams.get('devices', []):
Line 274: if dev.get('type') == 'graphics':
Line 268: 'displayNetwork'))
Line 269: vmParams['displayPort'] = '-1' # selected by libvirt
Line 270: vmParams['displaySecurePort'] = '-1'
Line 271:
Line 272: # new approach - graphics is a device
I don't get yet two things: are the two approaches mutually exclusive? there is some fallback to put in place (e.g try the old way first, overwrite with the new one, or vice versa)
worth a comment, I think.
Line 273: for dev in vmParams.get('devices', []):
Line 274: if dev.get('type') == 'graphics':
Line 275: # adjust my stuff
Line 276: dev['displayIp'] = self._getNetworkIp(dev.get(
http://gerrit.ovirt.org/#/c/23555/3/vdsm/vm.py
File vdsm/vm.py:
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20:
Line 21: from pprint import pprint
ok since it is a draft :)
Line 22:
Line 23: # stdlib imports
Line 24: from contextlib import contextmanager
Line 25: from copy import deepcopy
Line 1190: </channel>
Line 1191: """
Line 1192: for dev in self.conf.get('devices', []):
Line 1193: if dev['type'] == GRAPHICS_DEVICES:
Line 1194: return # probably could be written more sane
I think this should be handled in the caller, not here.
Line 1195:
Line 1196: graphicsAttrs = {'port': self.conf['displayPort'], 'autoport': 'yes'}
Line 1197: if self.conf['display'] == 'vnc':
Line 1198: graphicsAttrs['type'] = 'vnc'
Line 1346: self.log.error("=========== Using new GraphicsDevice")
Line 1347: pprint(self.conf)
Line 1348: self.log.error("*********** ")
Line 1349: pprint(self.specParams)
Line 1350: self.log.error("=========== ")
To avoid this I think you just need more unit tests; VmDevices are reasonnably easy to test and exercise in isolation in the unit tests.
Line 1351:
Line 1352: specParams = self.specParams
Line 1353: graphicsAttrs = {'type': specParams['type'],
Line 1354: 'port': specParams['port'],
Line 1352: specParams = self.specParams
Line 1353: graphicsAttrs = {'type': specParams['type'],
Line 1354: 'port': specParams['port'],
Line 1355: 'autoport': 'yes'
Line 1356: }
nit: shouldn't pep8 complain here?
Line 1357:
Line 1358: for key in ('tlsPort', 'keymap'):
Line 1359: if key in specParams:
Line 1360: graphicsAttrs[key] = specParams[key]
Line 1358: for key in ('tlsPort', 'keymap'):
Line 1359: if key in specParams:
Line 1360: graphicsAttrs[key] = specParams[key]
Line 1361:
Line 1362: if specParams.get('disableTicketing', '').lower() != 'true':
maybe utils.tobool?
Line 1363: graphicsAttrs['passwd'] = '*****'
Line 1364: graphicsAttrs['passwdValidTo'] = '1970-01-01T00:00:01'
Line 1365:
Line 1366: graphics = XMLElement('graphics', **graphicsAttrs)
Line 1372: for channel in specParams['spiceSecureChannels'].split(','):
Line 1373: graphics.appendChildWithArgs('channel', name=channel[1:],
Line 1374: mode='secure')
Line 1375:
Line 1376: # this MUST be certainly handled as a separate device
Agreed, just not sure it is in scope here.
Line 1377: #vmc = XMLElement('channel', type='spicevmc')
Line 1378: #vmc.appendChildWithArgs('target', type='virtio',
Line 1379: # name='com.redhat.spice.0')
Line 1380: #self._devices.appendChild(vmc)
Line 1380: #self._devices.appendChild(vmc)
Line 1381:
Line 1382: if specParams.get('displayNetwork'):
Line 1383: graphics.appendChildWithArgs('listen', type='network',
Line 1384: network=netinfo.LIBVIRT_NET_PREFIX +
Is this a network address?
Line 1385: specParams.get('displayNetwork'))
Line 1386: else:
Line 1387: graphics.setAttrs(listen='0')
Line 1388:
Line 5107: graphicsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \
Line 5108: getElementsByTagName('devices')[0].getElementsByTagName('graphics')
Line 5109:
Line 5110: for gxml in graphicsxml:
Line 5111: graphics_type = gxml.getAttribute('type')
nit: note that VDSM coding style require a different
namingRuleForIdentifiers
Line 5112:
Line 5113: for dev in self.conf['devices']:
Line 5114: specParams = dev.get('specParams', {})
Line 5115: if specParams.get('type', '') == graphics_type:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <fkobzik at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list