Change in vdsm[master]: vm: Libvirt quering after disk detach operation addition.

nsoffer at redhat.com nsoffer at redhat.com
Mon Aug 31 11:06:50 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: vm: Libvirt quering after disk detach operation addition.
......................................................................


Patch Set 10: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/45138/10/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2624:         hooks.before_disk_hotunplug(driveXml, self.conf,
Line 2625:                                     params=drive.custom)
Line 2626:         try:
Line 2627:             self._dom.detachDevice(driveXml)
Line 2628:             _verifyDriveRemoval(drive)
You are ignoring the return value, this will never fail. Also, you don't really verify that the drive is removed, you just wait for it, so the name should reflect that.

The best way to handle this is to raise a Timeout from the waiting function.

The code here should be:

    try:
        detach device...
        wait until device is detached...
    except Timeout:
        log that device cannot be detached
        return proper error code
    except libvirt.libvirtError:
        ...
Line 2629:         except libvirt.libvirtError as e:
Line 2630:             self.log.exception("Hotunplug failed")
Line 2631:             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 2632:                 return response.error('noVM')


Line 2657:             self.log.debug("Waiting for hotunplug to finish.")
Line 2658:             time.sleep(1)
Line 2659:             if utils.monotonic_time() > deadline:
Line 2660:                 return response.error('hotunplugDisk',
Line 2661:                                       "libvirt could not detach the disk.")
- Raise Timeout here (you may need to add this exception class) and let the caller return the response.
- Log one line before the wait, one line after
- Use utils.stopwatch to log the operation duration
Line 2662: 
Line 2663:     def _isDriveAttached(self, drive):
Line 2664:         domtree = ET.fromstring(self._dom.XMLDesc(0))
Line 2665:         for disk in domtree.findall("./devices/disk[serial='%s']" % drive.serial):


Line 2661:                                       "libvirt could not detach the disk.")
Line 2662: 
Line 2663:     def _isDriveAttached(self, drive):
Line 2664:         domtree = ET.fromstring(self._dom.XMLDesc(0))
Line 2665:         for disk in domtree.findall("./devices/disk[serial='%s']" % drive.serial):
This line is too long, making pep8 unhappy. This line would make it happy:

    for disk in root.findall("./devices/disk[serial='%s']" % drive.serial):

And is also more idiomatic etree code.
Line 2666:             return True
Line 2667:         return False
Line 2668: 
Line 2669:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45138
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I393ce55dd761ac825cb96bd499976fd74c366b09
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list