Dan Kenigsberg has posted comments on this change. Change subject: vm: handle missing domains on recovery ......................................................................
Patch Set 4: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/25275/4//COMMIT_MSG Commit Message: Line 21: Id495f6047ba658c2b04da19bd7bf76425b3b9659 Line 22: Line 23: In this case, the reconnection attempt to libvirt from VDSM Line 24: will cause a domain lookup fail, but we still want Line 25: to create an hollow VM object, granted it is in status Down. Please consider replacing with a paragraph like: In this case, the attempt to acquire the underlying libvirt domain would fail,but we still want to create an hollow VM object, granted it is in status Down. Line 26: Line 27: This patch add an explicit check in the recovery path to Line 28: make sure that a VM is either created with a valid libvirt Line 29: domain handle, or it is reported as Down so the engine Line 23: In this case, the reconnection attempt to libvirt from VDSM Line 24: will cause a domain lookup fail, but we still want Line 25: to create an hollow VM object, granted it is in status Down. Line 26: Line 27: This patch add an explicit check in the recovery path to add->adds Line 28: make sure that a VM is either created with a valid libvirt Line 29: domain handle, or it is reported as Down so the engine Line 30: can collect its state and explicitely destroy it. Line 31: http://gerrit.ovirt.org/#/c/25275/4/vdsm/vm.py File vdsm/vm.py: Line 2348: # we cannot continue without a libvirt connection handle Line 2349: # to avoid state desync or worse split-brain scenarios. Line 2350: if isinstance(e, libvirt.libvirtError) and \ Line 2351: e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 2352: raise LibvirtConnectionError() I am ashamed to admit for how long did the exception name confused me... The error condition here has nothing to do with libvirtconnection. Would you please rename it to something like NoUnderlyingVm() (or a better name) ? Line 2353: elif not self.recovering: Line 2354: raise Line 2355: else: Line 2356: self.log.info("Skipping errors on recovery", exc_info=True) Line 2378: self.saveState() Line 2379: except LibvirtConnectionError: Line 2380: self.recovering = False Line 2381: # we cannot ever deal with this, not even on recovery. Line 2382: self.setDownStatus(ERROR, vmexitreason.LIBVIRT_CONNECT_FAILED) here, too, please find a name that would not throw the poor reviewer to the realm of libvirtconnection.get() somehow failing. Line 2383: except Exception as e: Line 2384: if self.recovering: Line 2385: self.log.info("Skipping errors on recovery", exc_info=True) Line 2386: else: -- To view, visit http://gerrit.ovirt.org/25275 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I00ef12883c8035209de0f273925eb8603d6b6da8 Gerrit-PatchSet: 4 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: 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
