Ayal Baron has posted comments on this change.
Change subject: libvirtvm: avoid concurrent VM changes during saveState
......................................................................
Patch Set 6: (4 inline comments)
....................................................
File vdsm/libvirtvm.py
Line 1865: self.conf['devices'].remove(dev)
Line 1866: diskDev = dev
Line 1867: break
Line 1868:
Line 1869: self.saveState()
doesn't moving the lock to be at the granularity of every change and leaving
saveState out of it mean that now some other thread can call saveState while
not all devices have been removed (in this case) and if vdsm crashes after
that, the persistent state is weird? (same applies to hotunplug nic and
possibly other places).
Line 1870:
Line 1871: hooks.before_disk_hotunplug(driveXml, self.conf,
Line 1872: params=params.get('custom', {}))
Line 1873: try:
Line 1881: with self._confLock:
Line 1882: self.conf['devices'].append(diskDev)
Line 1883: if drive:
Line 1884: self._devices[vm.DISK_DEVICES].append(drive)
Line 1885: self.saveState()
I think it would be clearer if the 'if drive' clause would move up to group the
conf change and saveState
Line 1886: return {
Line 1887: 'status': {'code':
errCode['hotunplugDisk']['status']['code'],
Line 1888: 'message': e.message}}
Line 1889: else:
Line 2310: else:
Line 2311: raise LookupError("No such drive: '%s'" % srcDrive.name)
Line 2312:
Line 2313: srcDrive.diskReplicate = dstDisk
Line 2314: self.saveState()
here too I would move the saveState right before the 'break' above as they are
logically joined.
Line 2315:
Line 2316: def _delDiskReplica(self, srcDrive):
Line 2317: """
Line 2318: This utility method is the inverse of _setDiskReplica, look
at the
Line 2327: else:
Line 2328: raise LookupError("No such drive: '%s'" % srcDrive.name)
Line 2329:
Line 2330: del srcDrive.diskReplicate
Line 2331: self.saveState()
same
Line 2332:
Line 2333: def diskReplicateStart(self, srcDisk, dstDisk):
Line 2334: try:
Line 2335: srcDrive = self._findDriveByUUIDs(srcDisk)
--
To view, visit http://gerrit.ovirt.org/13624
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08b4073f5e3f5184baa5f1c7dd3ec5a148ff60b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dafna Ron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
