Ayal Baron has posted comments on this change.
Change subject: libvirtvm: avoid concurrent saveState during diskReplica
......................................................................
Patch Set 3: (3 inline comments)
....................................................
File vdsm/libvirtvm.py
Line 2255: both in the live object used by vdsm and the vm configuration
Line 2256: dictionary that is stored on disk (so that the information
is not
Line 2257: lost across restarts).
Line 2258: """
Line 2259: with self._diskReplicaLock:
this is a very specific lock while saveState is called elsewhere as well?
why not a stateChangeLock or something?
Line 2260: if srcDrive.isDiskReplicationInProgress():
Line 2261: raise RuntimeError("Disk '%s' already has an ongoing
"
Line 2262: "replication" % srcDrive.name)
Line 2263:
Line 2292: def diskReplicateStart(self, srcDisk, dstDisk):
Line 2293: try:
Line 2294: srcDrive = self._findDriveByUUIDs(srcDisk)
Line 2295: except LookupError:
Line 2296: return errCode['imageErr']
as long as you're adding logging, why not log the findDrive error?
Line 2297:
Line 2298: try:
Line 2299: self._setDiskReplica(srcDrive, dstDisk)
Line 2300: except:
Line 2298: try:
Line 2299: self._setDiskReplica(srcDrive, dstDisk)
Line 2300: except:
Line 2301: self.log.error("Unable to set the replication for disk
%s" %
Line 2302: srcDrive.name, exc_info=True)
the exc_info=True here would create a stack trace also for exceptions that we
threw and don't need it (every error before the saveState basically).
I'd rather print only relevant info when possible.
e.g. if diskReplication is in progress, no need for the stack trace.
Same for the LookupError
Line 2303: return errCode['replicaErr']
Line 2304:
Line 2305: dstDiskCopy = dstDisk.copy()
Line 2306:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
