Dan Kenigsberg has posted comments on this change.
Change subject: Adding hooks support for NIC hotplug
......................................................................
Patch Set 1: (4 inline comments)
Thanks, Izik, for this patch. I think it would be cool to add more hook
locations to vdsm.
I have a couple of questions inside. Please update the man page in this commit,
too.
....................................................
File vdsm/hooks.py
Line 173: return _runHooksDir(domxml, 'after_vm_set_ticket', vmconf=vmconf,
Line 174: raiseError=False, params=params)
Line 175:
Line 176:
Line 177: def before_nic_hotplug(domxml, vmconf={}):
it's not domxml, but nicxml, or something.
Line 178: return _runHooksDir(domxml, 'before_vm_hotplug', vmconf=vmconf)
Line 179:
Line 180:
Line 181: def after_nic_unplug(domxml, vmconf={}):
Line 178: return _runHooksDir(domxml, 'before_vm_hotplug', vmconf=vmconf)
Line 179:
Line 180:
Line 181: def after_nic_unplug(domxml, vmconf={}):
Line 182: return _runHooksDir(domxml, 'after_vm_unplug', vmconf=vmconf)
I think you don't want this hook to raiseError. if the hook script fails,
nothing is to be canceled.
Line 183:
Line 184:
Line 185: def before_vdsm_start():
Line 186: return _runHooksDir(None, 'before_vdsm_start', raiseError=False)
....................................................
File vdsm/libvirtvm.py
Line 1401: try:
Line 1402: self._dom.attachDevice(nicXml)
Line 1403: except libvirt.libvirtError, e:
Line 1404: self.log.error("Hotplug failed", exc_info=True)
Line 1405: nicXml = hooks.after_nic_unplug(nicXml, self.conf)
we've never had a similar hook, placed in a fail condition. I suppose that it
is here in order to release resources that where allocated in
before_nic_hotplug.
I find the naming quite misleading - there was *no* unplug in this case. Maybe
another name (after_nic_hotplug_fail??) would make it clearer, maybe something
completely different.
Line 1406: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 1407: return errCode['noVM']
Line 1408: return {'status' : {'code':
errCode['hotplugNic']['status']['code'],
Line 1409: 'message': e.message}}
Line 1455: self.saveState()
Line 1456:
Line 1457: try:
Line 1458: self._dom.detachDevice(nicXml)
Line 1459: hooks.after_nic_unplug(nicXml, self.conf)
does this really need to sit in the try-block?
Line 1460: except libvirt.libvirtError, e:
Line 1461: self.log.error("Hotunplug failed", exc_info=True)
Line 1462: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 1463: return errCode['noVM']
--
To view, visit http://gerrit.ovirt.org/7224
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I253104ee9831a881e2fb06f0a658631662611d77
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Itzik Brown <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
