Change in vdsm[master]: vm: simplify the releaseVm flow
danken at redhat.com
danken at redhat.com
Tue May 20 21:01:24 UTC 2014
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 <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list