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