Change in vdsm[master]: vm: handle missing domains on recovery

danken at redhat.com danken at redhat.com
Thu Apr 3 21:59:41 UTC 2014


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 <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: 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