Nir Soffer has posted comments on this change. Change subject: vm: Libvirt quering after disk detach operation addition. ......................................................................
Patch Set 6: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/45138/6/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 26: parameters = [ Line 27: # Section: [vars] Line 28: ('vars', [ Line 29: Line 30: ('wait_for_detach', '20', This name is named like a boolean option; lets rename it to hotunplug_timeout. Also, not sure if "vars" is the correct section, and it should not be the first configuration value. Please check if we have a better place for this, maybe create a new section, and add this option after the relevant existing options, or at the end. Line 31: 'Time amount to wait for a VM to detach its disk'), Line 32: Line 33: ('core_dump_enable', 'true', Line 34: 'Enable core dump.'), Line 27: # Section: [vars] Line 28: ('vars', [ Line 29: Line 30: ('wait_for_detach', '20', Line 31: 'Time amount to wait for a VM to detach its disk'), Time amount is not clear - should be "Number of seconds to wait..." Line 32: Line 33: ('core_dump_enable', 'true', Line 34: 'Enable core dump.'), Line 35: https://gerrit.ovirt.org/#/c/45138/6/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2620 Line 2621 Line 2622 Line 2623 Line 2624 Log info message before detaching the disk (in a separate patch). Line 2622 Line 2623 Line 2624 Line 2625 Line 2626 Please extract the wait logic into a private helper function - wait_for_detach() Line 2625: params=drive.custom) Line 2626: try: Line 2627: self._dom.detachDevice(driveXml) Line 2628: deadline = utils.monotonic_time() + \ Line 2629: float(config.get('vars', 'wait_for_detach')) Do not use \ for long lines, it is very fragile syntax. Instead put the expression in (): foo = (long expression that does not fit in one line) Use config.getint - we do not need to support fractions of a seconds. Line 2630: while drive.serial in self._dom.XMLDesc(0): Line 2631: self.log.debug("Waiting for hotunplug to finish.") Line 2632: time.sleep(1) Line 2633: if utils.monotonic_time() > deadline: Line 2626: try: Line 2627: self._dom.detachDevice(driveXml) Line 2628: deadline = utils.monotonic_time() + \ Line 2629: float(config.get('vars', 'wait_for_detach')) Line 2630: while drive.serial in self._dom.XMLDesc(0): This is too dirty, please find the disk using the apis in this module. Line 2631: self.log.debug("Waiting for hotunplug to finish.") Line 2632: time.sleep(1) Line 2633: if utils.monotonic_time() > deadline: Line 2634: return response.error('hotunplugDisk', Line 2631: self.log.debug("Waiting for hotunplug to finish.") Line 2632: time.sleep(1) Line 2633: if utils.monotonic_time() > deadline: Line 2634: return response.error('hotunplugDisk', Line 2635: "libvirt could not detach the disk.") Log info message when disk was finally detached (in a separate patch). Line 2636: except libvirt.libvirtError as e: Line 2637: self.log.exception("Hotunplug failed") Line 2638: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 2639: return response.error('noVM') -- 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: 6 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
