Francesco Romani has posted comments on this change. Change subject: vm: simplify the releaseVm flow ......................................................................
Patch Set 2: -Verified (2 comments) http://gerrit.ovirt.org/#/c/27175/2/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 4259: if self._dom: Line 4260: self._destroyVm() Line 4261: except libvirt.libvirtError as e: Line 4262: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 4263: self.log.warning("libvirt domain not found", exc_info=True) > one branch has warn(), the other - warning(), both text says practically th Will fix in follow-up patches. Line 4264: else: Line 4265: self.log.warn("VM %s is not running", self.conf['vmId']) Line 4266: Line 4267: if not self.cif.mom: Line 4294: self._dom.destroyFlags(libvirt.VIR_DOMAIN_DESTROY_GRACEFUL) Line 4295: except libvirt.libvirtError as e: Line 4296: if e.get_error_code() == libvirt.VIR_ERR_OPERATION_FAILED: Line 4297: self.log.warn("Failed to destroy VM '%s' " Line 4298: "gracefully", self.conf['vmId']) > all other errors are silently ignored?! (yeah, I see that the original code The indentantion change was unintentional as it slipped into when I extracted the method. Will fix. Everything else was intentional, since this patch wants to\ - extract the destroy* calls into a method - add a docstring - strip away the wasteful time.sleep() However, given the comments, I'll re-arrange this patch into: - a trivial one-line patch which just strips away the time.sleep() - a follow-up patchset to cleanup this path starting from the comments received here Line 4299: self._dom.destroy() Line 4300: Line 4301: def deleteVm(self): Line 4302: """ -- To view, visit http://gerrit.ovirt.org/27175 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia25794217e63dd4c755d9c141a2e941e4baa6fd3 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
