Federico Simoncelli has uploaded a new change for review. Change subject: libvirtvm: avoid concurrent saveState during diskReplica ......................................................................
libvirtvm: avoid concurrent saveState during diskReplica When multiple diskReplica requests come in for the same VM there a chance that the VM configuration is modified during a deepcopy in the saveState method resulting in an exception (e.g. RuntimeError: dictionary changed size during iteration). To avoid this problem the saveState requests are now serialized. Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=923194 Change-Id: Ic08b4073f5e3f5184baa5f1c7dd3ec5a148ff60b Signed-off-by: Federico Simoncelli <[email protected]> --- M vdsm/libvirtvm.py 1 file changed, 34 insertions(+), 24 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/24/13624/1 diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py index 607dedb..9c9fd9c 100644 --- a/vdsm/libvirtvm.py +++ b/vdsm/libvirtvm.py @@ -1030,6 +1030,9 @@ self._customize() + def isDiskReplicationInProgress(self): + return hasattr(self, "diskReplicate"): + @property def volExtensionChunk(self): """ @@ -1038,7 +1041,7 @@ can also dynamically change according to the VM needs (e.g. increase during a live storage migration). """ - if hasattr(self, "diskReplicate"): + if self.isDiskReplicationInProgress(): return self.VOLWM_CHUNK_MB * self.VOLWM_CHUNK_REPLICATE_MULT return self.VOLWM_CHUNK_MB @@ -1276,6 +1279,7 @@ self._devXmlHash = '0' self._released = False self._releaseLock = threading.Lock() + self._diskReplicaLock = threading.Lock() self.saveState() self._watchdogEvent = {} @@ -2252,35 +2256,38 @@ dictionary that is stored on disk (so that the information is not lost across restarts). """ - for device in self.conf["devices"]: - if (device['type'] == vm.DISK_DEVICES - and device.get("name") == srcDrive.name): - device['diskReplicate'] = dstDisk - break - else: - raise LookupError("No such drive: '%s'" % srcDrive.name) + with self._diskReplicaLock: + if srcDrive.isDiskReplicationInProgress(): + raise RuntimeError("Disk '%s' already has an ongoing " + "replication" % srcDrive.name) - srcDrive.diskReplicate = dstDisk - self.saveState() + for device in self.conf["devices"]: + if (device['type'] == vm.DISK_DEVICES + and device.get("name") == srcDrive.name): + device['diskReplicate'] = dstDisk + break + else: + raise LookupError("No such drive: '%s'" % srcDrive.name) - def isDiskReplicationInProgress(self, srcDrive): - return hasattr(srcDrive, 'diskReplicate') + srcDrive.diskReplicate = dstDisk + self.saveState() def _delDiskReplica(self, srcDrive): """ This utility method is the inverse of _setDiskReplica, look at the _setDiskReplica description for more information. """ - for device in self.conf["devices"]: - if (device['type'] == vm.DISK_DEVICES - and device.get("name") == srcDrive.name): - del device['diskReplicate'] - break - else: - raise LookupError("No such drive: '%s'" % srcDrive.name) + with self._diskReplicaLock: + for device in self.conf["devices"]: + if (device['type'] == vm.DISK_DEVICES + and device.get("name") == srcDrive.name): + del device['diskReplicate'] + break + else: + raise LookupError("No such drive: '%s'" % srcDrive.name) - del srcDrive.diskReplicate - self.saveState() + del srcDrive.diskReplicate + self.saveState() def diskReplicateStart(self, srcDisk, dstDisk): try: @@ -2288,10 +2295,13 @@ except LookupError: return errCode['imageErr'] - if self.isDiskReplicationInProgress(srcDrive): + try: + self._setDiskReplica(srcDrive, dstDisk) + except: + self.log.error("Unable to set the replication for disk %s" % + srcDrive.name, exc_info=True) return errCode['replicaErr'] - self._setDiskReplica(srcDrive, dstDisk) dstDiskCopy = dstDisk.copy() # The device entry is enforced because stricly required by @@ -2332,7 +2342,7 @@ except LookupError: return errCode['imageErr'] - if not self.isDiskReplicationInProgress(srcDrive): + if not srcDrive.isDiskReplicationInProgress(): return errCode['replicaErr'] # Looking for the replication blockJob info (checking its presence) -- To view, visit http://gerrit.ovirt.org/13624 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic08b4073f5e3f5184baa5f1c7dd3ec5a148ff60b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
