Nir Soffer has posted comments on this change. Change subject: vm: Libvirt quering after disk detach operation addition. ......................................................................
Patch Set 7: Code-Review-1 (9 comments) https://gerrit.ovirt.org/#/c/45138/7/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 343: 'Max number of tasks which can be queued on workers.' Line 344: ' This is for internal usage and may change without warning'), Line 345: Line 346: ('hotunplug_timeout', '20', Line 347: 'Number of seconds to wait for a VM to detach its disk'), I don't think this belongs here, this is not about sampling. This fits better after the xxx_timeout options in vars. Also lets use timeout of 30 seconds, similar to other other timeouts. Line 348: ]), Line 349: Line 350: # Section: [devel] Line 351: ('devel', [ https://gerrit.ovirt.org/#/c/45138/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 28: import tempfile Line 29: import threading Line 30: import time Line 31: import uuid Line 32: import datetime We dont' need this import now - this probably cause the CI failures. You can run make check locally to detect such errors. Line 33: Line 34: # 3rd party libs imports Line 35: import libvirt Line 36: import xml.etree.ElementTree as ET Line 32: import datetime Line 33: Line 34: # 3rd party libs imports Line 35: import libvirt Line 36: import xml.etree.ElementTree as ET This belongs to stdlib imports above, this is part of standard library. Line 37: Line 38: # vdsm imports Line 39: from vdsm import constants Line 40: from vdsm import libvirtconnection Line 2626: params=drive.custom) Line 2627: try: Line 2628: self._dom.detachDevice(driveXml) Line 2629: deadline = (utils.monotonic_time() + Line 2630: config.getint('sampling', 'hotunplug_timeout')) Should be indented under utils. I don't think pep8 likes your indentation. You can run make check to detect such issues. Line 2631: while self.isDriveInDom(drive): Line 2632: self.log.debug("Waiting for hotunplug to finish.") Line 2633: time.sleep(1) Line 2634: if utils.monotonic_time() > deadline: Line 2628: self._dom.detachDevice(driveXml) Line 2629: deadline = (utils.monotonic_time() + Line 2630: config.getint('sampling', 'hotunplug_timeout')) Line 2631: while self.isDriveInDom(drive): Line 2632: self.log.debug("Waiting for hotunplug to finish.") We may see 30 logs like this if a drive fail to detach, and I don't think it is very useful. Lets move this log before the while, so we get one message that we are waiting for hotunplug. Line 2633: time.sleep(1) Line 2634: if utils.monotonic_time() > deadline: Line 2635: return response.error('hotunplugDisk', Line 2636: "libvirt could not detach the disk.") Line 2632: self.log.debug("Waiting for hotunplug to finish.") Line 2633: time.sleep(1) Line 2634: if utils.monotonic_time() > deadline: Line 2635: return response.error('hotunplugDisk', Line 2636: "libvirt could not detach the disk.") Same indentation issue, should be aligned with 'hotunplugDisk' Line 2637: except libvirt.libvirtError as e: Line 2638: self.log.exception("Hotunplug failed") Line 2639: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 2640: return response.error('noVM') Line 2657: self._cleanupDrives(drive) Line 2658: Line 2659: return {'status': doneCode, 'vmList': self.status()} Line 2660: Line 2661: def isDriveInDom(self, drive): Should be private, add a _ prefix. Since we are trying to check if drive is attached, I think renaming it to _isDriveAttached() would be little nicer: while self._isDriveAttached(drive): ... Line 2662: domXmlTree = ET.fromstring(self._dom.XMLDesc(0)) Line 2663: for serialElmt in domXmlTree.findall("./devices/disk/serial"): Line 2664: if drive.serial == serialElmt.text: Line 2665: return True Line 2658: Line 2659: return {'status': doneCode, 'vmList': self.status()} Line 2660: Line 2661: def isDriveInDom(self, drive): Line 2662: domXmlTree = ET.fromstring(self._dom.XMLDesc(0)) Please use shorter lowercase names, we don't need such names in the context of a 3 lines method. Line 2663: for serialElmt in domXmlTree.findall("./devices/disk/serial"): Line 2664: if drive.serial == serialElmt.text: Line 2665: return True Line 2666: return False Line 2662: domXmlTree = ET.fromstring(self._dom.XMLDesc(0)) Line 2663: for serialElmt in domXmlTree.findall("./devices/disk/serial"): Line 2664: if drive.serial == serialElmt.text: Line 2665: return True Line 2666: return False This can be simplified using xpath: for disk in root.findall("./devices/disk[serial='%s']" % drive.serial): return True return False see https://docs.python.org/2/library/xml.etree.elementtree.html#supported-xpath-syntax Line 2667: Line 2668: Line 2669: def _readPauseCode(self): Line 2670: state, reason = self._dom.state(0) -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
