Dan Kenigsberg has posted comments on this change. Change subject: vm: simplify the releaseVm flow ......................................................................
Patch Set 2: (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 the same. The second text is WRONG. It's probably wrong to go on as if libvirt's destroy() succeeded, while it actually failed. The outer try-block is WAY too wide. it should cover ONLY self._dom.destroy() - nothing else can raise a libvirtError. 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']) > Isn't this the purpose of this method? It seems that when this method retur It's fine if it's intentional; given the commit message, it was surprising, and deserved a comment. But you are right, Nir. The original code is very problematic. It requires more than this facelift. Francesco, please suggest code that logs 100% of the errors, and does not report success if the second destroy() failed. 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
