Change in vdsm[master]: libvirtvm: avoid concurrent VM changes during saveState

abaron at redhat.com abaron at redhat.com
Thu May 2 19:31:05 UTC 2013


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 <fsimonce at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dafna Ron <dron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list