Change in vdsm[master]: hooks: Add disk hot plug/unplug hooks
danken at redhat.com
danken at redhat.com
Fri Mar 1 09:57:13 UTC 2013
Dan Kenigsberg has posted comments on this change.
Change subject: hooks: Add disk hot plug/unplug hooks
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/libvirtvm.py
Line 1801:
Line 1802: self.saveState()
Line 1803:
Line 1804: try:
Line 1805: hooks.before_disk_hotunplug(driveXml, self.conf)
try-except block should be kept as short as possible. the new code is not expected to raise alibvirtError, and should be kept outside the block.
Line 1806: self._dom.detachDevice(driveXml)
Line 1807: hooks.after_disk_hotunplug(driveXml, self.conf)
Line 1808: except libvirt.libvirtError as e:
Line 1809: self.log.error("Hotunplug failed", exc_info=True)
Line 1818: return {
Line 1819: 'status': {'code': errCode['hotunplugDisk']['status']['code'],
Line 1820: 'message': e.message}}
Line 1821: else:
Line 1822: self._cleanup()
that's unrelated to your patch, but what does this call doing here? Why on earth do we call LibvirtVm._cleanup on hotunplug success? It seems VERY wrong.
Line 1823:
Line 1824: return {'status': doneCode, 'vmList': self.status()}
Line 1825:
Line 1826: def _readPauseCode(self, timeout):
....................................................
File vdsm/vdsmd.8.in
Line 66: representation of the relevant virtual machine.
Line 67: The uuid of the virtual machine may be deduced from that xml, but it is also
Line 68: available as the environment variable
Line 69: .B vmId.
Line 70: Hooks that handle NIC hotplug and hotunplug have the _hook_domxml variable but it
nit: line length should be below 80 chars (easier to review).
Line 71: contains the representation of the NIC rather than the VM.
Line 72: Hotplug/hotunplug disk hooks also have the _hook_dom_xml variable, but it contains
Line 73: the drive definition and not the VM.
Line 74:
Line 68: available as the environment variable
Line 69: .B vmId.
Line 70: Hooks that handle NIC hotplug and hotunplug have the _hook_domxml variable but it
Line 71: contains the representation of the NIC rather than the VM.
Line 72: Hotplug/hotunplug disk hooks also have the _hook_dom_xml variable, but it contains
the "but" here is misleading: I'd use "and".
Line 73: the drive definition and not the VM.
Line 74:
Line 75: On top of these, @ENGINENAME@ allows to set a collection of "custom parameters" for
Line 76: each virtual machine. Each of these parameters is provided to hooks as an
--
To view, visit http://gerrit.ovirt.org/12479
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10b689964fad3a1118974f7ba7b91e795cbffd1f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list