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