Change in vdsm[master]: Live Merge: work around racy libvirt pivot

nsoffer at redhat.com nsoffer at redhat.com
Mon Mar 30 14:54:16 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: Live Merge: work around racy libvirt pivot
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/39303/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 5090
Line 5091
Line 5092
Line 5093
Line 5094
> I'd prefer not to since we could then be out of sync with libvirt for awhil
OK


Line 5060:         self.drive = drive
Line 5061:         self.doPivot = doPivot
Line 5062:         self.success = False
Line 5063: 
Line 5064:     def _waitForBuggyLibvirt(self):
> Yes to moving it and I can create a tamer name for the function.  I do want
I think the "TODO: remove once ..." is good enough. Anway this is not critical.
Line 5065:         # Libvirt version 1.2.8-16.el7_1.2 introduced a bug where the
Line 5066:         # synchronous call to blockJobAbort will return before the domain XML
Line 5067:         # has been updated.  This makes it look like the pivot failed when it
Line 5068:         # actually succeeded.  This means that vdsm state will not be properly


Line 5072:         #
Line 5073:         # TODO: Remove once we depend on a libvirt with this bug fixed.
Line 5074:         origVols = [x['volumeID'] for x in self.drive.volumeChain]
Line 5075:         alias = self.drive['alias']
Line 5076:         while True:
> The pivot has already been reported as successful by libvirt.  I am not sur
We agreed offline that this is correct. A verbose comment should be added explaining the whole flow, and why continuing this forever is the right solution.
Line 5077:             chains = self.vm._driveGetActualVolumeChain([self.drive])
Line 5078:             try:
Line 5079:                 curVols = chains[alias]
Line 5080:             except KeyError:


Line 5073:         # TODO: Remove once we depend on a libvirt with this bug fixed.
Line 5074:         origVols = [x['volumeID'] for x in self.drive.volumeChain]
Line 5075:         alias = self.drive['alias']
Line 5076:         while True:
Line 5077:             chains = self.vm._driveGetActualVolumeChain([self.drive])
> An array with one element is the positive case.  The plural signifies it's 
Strange api. Why the positive case should have more then one chain?

Should we check if there are multiple chains here?
Line 5078:             try:
Line 5079:                 curVols = chains[alias]
Line 5080:             except KeyError:
Line 5081:                 raise RuntimeError("Failed to retrieve volume chain for "


-- 
To view, visit https://gerrit.ovirt.org/39303
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e794622baf66c75cbe583be03a7b9a4a7e4883d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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