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