Change in vdsm[master]: caps: vm: check abort on EIO in libvirt

danken at redhat.com danken at redhat.com
Thu Jan 16 17:54:37 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: caps: vm: check abort on EIO in libvirt
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

http://gerrit.ovirt.org/#/c/23273/2/vdsm/caps.py
File vdsm/caps.py:

Line 388:     caps['rngSources'] = _getRngSources()
Line 389: 
Line 390:     return caps
Line 391: 
Line 392: 
this could be @memoized
Line 393: def _getVersionInfo():
Line 394:     # commit bbeb165e42673cddc87495c3d12c4a7f7572013c
Line 395:     # added default abort of the VM migration on EIO.
Line 396:     # libvirt 1.0.5.8 found in Fedora 19 does not export


Line 401:     # Workaround: we drop the cluster 3.4+
Line 402:     # compatibility when we run on top of
Line 403:     # a libvirt without this flag.
Line 404:     info = dsaversion.version_info
Line 405:     # this to stay in 80 columns
you can start a list here = [
 and, end, it, here]
Line 406:     levels = [ver for ver in info['clusterLevels'] if ver < '3.4']
Line 407:     info['clusterLevels'] = levels
Line 408:     return info
Line 409: 


Line 402:     # compatibility when we run on top of
Line 403:     # a libvirt without this flag.
Line 404:     info = dsaversion.version_info
Line 405:     # this to stay in 80 columns
Line 406:     levels = [ver for ver in info['clusterLevels'] if ver < '3.4']
Version comparison is safer with distutils.version.StrictVersion. If you don't care about the general case ('3.10' is lesser than '3.4'!) you can just drop 3.4.
Line 407:     info['clusterLevels'] = levels
Line 408:     return info
Line 409: 
Line 410: 


http://gerrit.ovirt.org/#/c/23273/2/vdsm/vm.py
File vdsm/vm.py:

Line 133:                 not hasattr(libvirt, 'VIR_MIGRATE_ABORT_ON_ERROR'):
Line 134:             self.log.warning('libvirt does not support'
Line 135:                              ' VIR_MIGRATE_ABORT_ON_ERROR, but requested;'
Line 136:                              ' forced to disabled')
Line 137:             self._abortOnError = False
> AFAIK, yes. I added this check as safety net, but it can be dropped if it i
Yeah, now that we report the correct clusterLevel, we can trust the client to know what it's doing. IMO, this snippet can go (as well as its reference in the commit message).
Line 138:         self._dstqemu = dstqemu
Line 139:         self._downtime = kwargs.get('downtime') or \
Line 140:             config.get('vars', 'migration_downtime')
Line 141:         self.status = {


-- 
To view, visit http://gerrit.ovirt.org/23273
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7dd857ac3c00c48d4de3a50931cb259f45ff4f5d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland 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: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list