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 <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[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
