Change in vdsm[master]: virt: graphdev: add the GraphicsDevice class

fromani at redhat.com fromani at redhat.com
Wed May 7 06:49:27 UTC 2014


Francesco Romani has posted comments on this change.

Change subject: virt: graphdev: add the GraphicsDevice class
......................................................................


Patch Set 19:

(4 comments)

http://gerrit.ovirt.org/#/c/26895/19//COMMIT_MSG
Commit Message:

Line 14: The GraphicDevice is built internally automatically
Line 15: if no graphic device is passed into the device list,
Line 16: but if the display{,Type,Network,...} parameters are passed.
Line 17: 
Line 18: Backward compatibility with old unaware engines is preserved
> We must maintain backward compat with old unaware Vdsms, too.
Understood. Then I'll change te code to do migrations using the 'old' way (display* params, only one graphic device).
Line 19: both in the creation and reporting; however, in all VDSM<=>VDSM
Line 20: communications, e.g. migrations and state saving for recovery,
Line 21: the new graphic device representation is used.
Line 22: 


http://gerrit.ovirt.org/#/c/26895/19/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 937:         if linkDict.get('device') == iface:
Line 938:             yield linkDict['name']
Line 939: 
Line 940: 
Line 941: def getNetworkIp(network):
> I wish the movement of this function had a dedicated commit message. This f
Sorry, I didn't know this function was so evil :)
I'll move into vm.py as private.
Line 942:     try:
Line 943:         nets = networks()
Line 944:         device = nets[network].get('iface', network)
Line 945:         ip = getaddr(device)


http://gerrit.ovirt.org/#/c/26895/19/vdsm/API.py
File vdsm/API.py:

Line 206:                     return {'status': {'code': errCode['MissParam']
Line 207:                                                       ['status']['code'],
Line 208:                                        'message': 'Missing required '
Line 209:                                        'parameter %s' % (param)}}
Line 210:             if 'display' not in vmParams and 'migrationDest' not in vmParams:
> I see the motivation for migrationDest to handle incoming "old" configs, bu
With this patch VDSM will report two graphic devices in the stats, and thus as remoteMachineParams for migrations:

- one expressed through display* params, for backward compatibility with old client (engine being the first)
- one expressed as proper graphic device, which is the "official" one and from which the above display* params are reconstructed

however, this break migrations because the destination VM will see two graphic device with the same type (one being reconstructed for bc, one real).

So here the solution implemented was to NOT require the legacy way of expressing a graphic device only for internal VDSM<=>VDSM communications, so relaxing the requiredParams for the  migrationDest create flow.

But as Dan and Michal pointed out commenting the changes to migration.py, this will break the clusterLevel
so I need to turn this concept the way around and to pass only the display* params.

Will also check the scenario Michal described here to avoid more subtle breakage.
Line 211:                 return {'status': {'code': errCode['MissParam']
Line 212:                                                   ['status']['code'],
Line 213:                                    'message': 'Missing required '
Line 214:                                    'parameter display'}}


http://gerrit.ovirt.org/#/c/26895/19/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 191:             self._vm.setDownStatus(NORMAL, vmexitreason.SAVE_STATE_SUCCEEDED)
Line 192:             self.status['status']['message'] = 'SaveState done'
Line 193: 
Line 194:     def _patchConfigForGraphics(self):
Line 195:         if 'display' in self._machineParams:
> I'd think so… Only the spice+vnc configs do not need to maintain BC as olde
Same rationale as explained for the change in API.py (please see it).

I need to re-check but I think if we sent a Graphic Device (aka the new way) to an unaware VDSM it will just silently discard it.

Other than that, I'll amend the code to pass the display* configuration the 'old way' during migrations.
Line 196:             del self._machineParams['display']
Line 197:         if 'displayIp' in self._machineParams:
Line 198:             del self._machineParams['displayIp']
Line 199: 


-- 
To view, visit http://gerrit.ovirt.org/26895
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I82e4414aa3efe7c756ba8ef9b7b47e591613a717
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkobzik at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list