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(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkobzik(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes