Ayal Baron has posted comments on this change.
Change subject: pool: live storage migration implementation
......................................................................
Patch Set 13: (29 inline comments)
....................................................
File vdsm/libvirtvm.py
Line 126: if vmDrive.device == 'disk' and vmDrive.isVdsmImage():
Line 127: volSize =
self._vm.cif.irs.getVolumeSize(vmDrive.domainID,
Line 128: vmDrive.poolID, vmDrive.imageID,
vmDrive.volumeID)
Line 129:
Line 130: if volSize['status']['code'] == 0:
this is a bug
Line 131: continue
Line 132:
Line 133: vmDrive.truesize = int(volSize['truesize'])
Line 134: vmDrive.apparentsize = int(volSize['apparentsize'])
Line 1741:
Line 1742: def snapshot(self, snapDrives):
Line 1743: """Live snapshot command"""
Line 1744:
Line 1745: def _diskSnapshot(vmDev, newPath):
getting rid of the mirror implementation should be in a separate patch
Line 1746: """Libvirt snapshot XML"""
Line 1747:
Line 1748: disk = xml.dom.minidom.Element('disk')
Line 1749: disk.setAttribute('name', vmDev)
Line 1990:
Line 1991: self.saveState()
Line 1992:
Line 1993: def _setDiskReplica(self, srcDrive, dstDisk):
Line 1994: srcDrive.diskReplicate = dstDisk
missing docstring explaining why we need 2 objects with same property
Also, should probably only update this if you find the device.
Line 1995:
Line 1996: for device in self.conf["devices"]:
Line 1997: if (device['type'] == vm.DISK_DEVICES
Line 1998: and device.get("name") == srcDrive.name):
Line 1995:
Line 1996: for device in self.conf["devices"]:
Line 1997: if (device['type'] == vm.DISK_DEVICES
Line 1998: and device.get("name") == srcDrive.name):
Line 1999: device['diskReplicate'] = dstDisk
if device not found throw an error.
break
else:
raise...
Alternatively pass the device as parameter when you found it before in the flow
Line 2000:
Line 2001: self.saveState()
Line 2002:
Line 2003: def _hasDiskReplica(self, srcDrive):
Line 1999: device['diskReplicate'] = dstDisk
Line 2000:
Line 2001: self.saveState()
Line 2002:
Line 2003: def _hasDiskReplica(self, srcDrive):
s/hasDiskReplica/DiskReplicationInProgress/
Line 2004: return hasattr(srcDrive, 'diskReplicate')
Line 2005:
Line 2006: def _delDiskReplica(self, srcDrive):
Line 2007: del srcDrive.diskReplicate
Line 2013:
Line 2014: self.saveState()
Line 2015:
Line 2016: def diskReplicateStart(self, srcDisk, dstDisk):
Line 2017: # Finding the source disk name and the destination path
destination path?
Line 2018: try:
Line 2019: srcDrive = self._findDriveByUUIDs(srcDisk)
Line 2020: except LookupError:
Line 2021: return errCode['imageErr']
Line 2019: srcDrive = self._findDriveByUUIDs(srcDisk)
Line 2020: except LookupError:
Line 2021: return errCode['imageErr']
Line 2022:
Line 2023: # Another replication is ongoing
comment not needed once method name is changed
Line 2024: if self._hasDiskReplica(srcDrive):
Line 2025: return errCode['replicaErr']
Line 2026:
Line 2027: # Updating the destination disk device and name
Line 2023: # Another replication is ongoing
Line 2024: if self._hasDiskReplica(srcDrive):
Line 2025: return errCode['replicaErr']
Line 2026:
Line 2027: # Updating the destination disk device and name
comment is redundant, this is self evident from the code
Line 2028: dstDisk.update({'device': srcDrive.device, 'name':
srcDrive.name})
Line 2029:
Line 2030: self._setDiskReplica(srcDrive, dstDisk)
Line 2031: dstDrive = dstDisk.copy()
Line 2024: if self._hasDiskReplica(srcDrive):
Line 2025: return errCode['replicaErr']
Line 2026:
Line 2027: # Updating the destination disk device and name
Line 2028: dstDisk.update({'device': srcDrive.device, 'name':
srcDrive.name})
Disk is a PDIV object, it should not contain additional info imo.
You already pass this info to setDiskReplica in srcDrive, it should be set
directly from srcDrive to the device object
Line 2029:
Line 2030: self._setDiskReplica(srcDrive, dstDisk)
Line 2031: dstDrive = dstDisk.copy()
Line 2032:
Line 2027: # Updating the destination disk device and name
Line 2028: dstDisk.update({'device': srcDrive.device, 'name':
srcDrive.name})
Line 2029:
Line 2030: self._setDiskReplica(srcDrive, dstDisk)
Line 2031: dstDrive = dstDisk.copy()
Drive is already used for something else and it's confusing here.
Line 2032:
Line 2033: try:
Line 2034: dstDrive['path'] = self.cif.prepareVolumePath(dstDrive)
Line 2035: self._dom.blockRebase(srcDrive.name, dstDrive['path'],
0, (
Line 2044: self._delDiskReplica(srcDrive)
Line 2045: return errCode['replicaErr']
Line 2046:
Line 2047: # Request an initial extension if the disk is on a block
device
Line 2048: if srcDrive.blockDev:
this is already checked inside extendDriveVolume
Line 2049: try:
Line 2050: self.extendDriveVolume(srcDrive)
Line 2051: except:
Line 2052: self.log.error("Initial extension request failed for
%s",
Line 2053: srcDrive.name, exc_info=True)
Line 2054:
Line 2055: return {'status': doneCode}
Line 2056:
Line 2057: def diskReplicateStop(self, srcDisk, dstDisk):
s/Stop/Finish/
Line 2058: # Finding the source disk name and the destination path
Line 2059: try:
Line 2060: srcDrive = self._findDriveByUUIDs(srcDisk)
Line 2061: except LookupError:
Line 2067:
Line 2068: # Looking for the replication blockJob info (checking its
presence)
Line 2069: blkJobInfo = self._dom.blockJobInfo(srcDrive.name, 0)
Line 2070:
Line 2071: if not isinstance(blkJobInfo, dict):
while you're at it, what about checking for the existence of cur and end in the
dict?
Line 2072: self.log.error("Replication job not found for disk %s
(%s)",
Line 2073: srcDrive.name, srcDisk)
Line 2074:
Line 2075: # Making sure that we don't have any stale information
Line 2075: # Making sure that we don't have any stale information
Line 2076: self._delDiskReplica(srcDrive)
Line 2077: return errCode['replicaErr']
Line 2078:
Line 2079: # Checking if we reached the mirroring mode
s/mirroring/replication/
Line 2080: if blkJobInfo['cur'] != blkJobInfo['end']:
Line 2081: return errCode['unavail']
Line 2082:
Line 2083: dstDrive = dstDisk.copy()
Line 2077: return errCode['replicaErr']
Line 2078:
Line 2079: # Checking if we reached the mirroring mode
Line 2080: if blkJobInfo['cur'] != blkJobInfo['end']:
Line 2081: return errCode['unavail']
why 'unavail'?
Line 2082:
Line 2083: dstDrive = dstDisk.copy()
Line 2084:
Line 2085: # Updating the destination disk device and name
Line 2079: # Checking if we reached the mirroring mode
Line 2080: if blkJobInfo['cur'] != blkJobInfo['end']:
Line 2081: return errCode['unavail']
Line 2082:
Line 2083: dstDrive = dstDisk.copy()
again naming issue
Line 2084:
Line 2085: # Updating the destination disk device and name
Line 2086: dstDrive.update({'device': srcDrive.device, 'name':
srcDrive.name})
Line 2087: dstDrive['path'] = self.cif.prepareVolumePath(dstDrive)
Line 2082:
Line 2083: dstDrive = dstDisk.copy()
Line 2084:
Line 2085: # Updating the destination disk device and name
Line 2086: dstDrive.update({'device': srcDrive.device, 'name':
srcDrive.name})
again looks redundant
Line 2087: dstDrive['path'] = self.cif.prepareVolumePath(dstDrive)
Line 2088:
Line 2089: if srcDisk != dstDisk:
Line 2090: self.log.error("Stopping the disk replication switching
to the "
Line 2094: self.log.error("Stopping the disk replication remaining
on the "
Line 2095: "source drive: %s", dstDisk)
Line 2096: blockJobFlags = 0
Line 2097:
Line 2098: self._volumesPrepared = False
wrap with stopDisksStatsCollection
and add a comment why
Line 2099:
Line 2100: # Stopping the replication
Line 2101: try:
Line 2102: self._dom.blockJobAbort(srcDrive.name, blockJobFlags)
Line 2108: self.cif.teardownVolumePath(srcDrive.diskReplicate)
Line 2109:
Line 2110: return errCode['changeDisk']
Line 2111: finally:
Line 2112: self._volumesPrepared = True
too early
Line 2113:
Line 2114: # Tearing down the old drive only if we switched to the new
one
Line 2115: try:
Line 2116: if blockJobFlags:
Line 2112: self._volumesPrepared = True
Line 2113:
Line 2114: # Tearing down the old drive only if we switched to the new
one
Line 2115: try:
Line 2116: if blockJobFlags:
instead of this, set a variable under the previous if.
Line 2117: self.cif.teardownVolumePath(srcDrive)
Line 2118: else:
Line 2119: self.cif.teardownVolumePath(srcDrive.diskReplicate)
Line 2120: except:
Line 2122: self.log.error("Unable to teardown the replication
source ",
Line 2123: "disk", exc_info=True)
Line 2124:
Line 2125: # Updating the drive structure
Line 2126: self._updateDrive(dstDrive)
restart stats collection
Line 2127: self._delDiskReplica(srcDrive)
Line 2128:
Line 2129: return {'status': doneCode}
Line 2130:
....................................................
File vdsm/storage/hsm.py
Line 1345: misc.parseBool(force)
Line 1346: )
Line 1347:
Line 1348: @public
Line 1349: def syncImage(self, spUUID, sdUUID, imgUUID, dstSdUUID,
operations):
why is this one method and not 2?
cloneImageStructure
syncImageData
Line 1350: """
Line 1351: Synchronize images between storage domains within same
storage pool
Line 1352: """
Line 1353: pool = self.getPool(spUUID)
Line 1347:
Line 1348: @public
Line 1349: def syncImage(self, spUUID, sdUUID, imgUUID, dstSdUUID,
operations):
Line 1350: """
Line 1351: Synchronize images between storage domains within same
storage pool
s/same/the same/
Line 1352: """
Line 1353: pool = self.getPool(spUUID)
Line 1354:
Line 1355: srcDom = self.validateSdUUID(sdUUID)
Line 1353: pool = self.getPool(spUUID)
Line 1354:
Line 1355: srcDom = self.validateSdUUID(sdUUID)
Line 1356: dstDom = self.validateSdUUID(dstSdUUID)
Line 1357: self.validateImageMove(srcDom, dstDom, imgUUID)
remind me why we need this call here?
Line 1358:
Line 1359: for dom in sorted((sdUUID, dstSdUUID)):
Line 1360: vars.task.getSharedLock(STORAGE, dom)
Line 1361:
....................................................
File vdsm/storage/image.py
Line 609: self.log.info("%s task on image %s was successfully
finished", OP_TYPES[op], imgUUID)
Line 610: return True
Line 611:
Line 612: def _syncExecute(self, dstDom, sdUUID, imgUUID,
excludeLeaf=False):
Line 613: srcChain = self.getChain(sdUUID, imgUUID)
switch parameters order?
Line 614: dstChain = self.getChain(dstDom.sdUUID, imgUUID)
Line 615:
Line 616: # Removing the leaf from the volumes to copy
Line 617: if excludeLeaf:
....................................................
File vdsm/storage/sp.py
Line 1750: srcImgResNs = sd.getNamespace(sdUUID, IMAGE_NAMESPACE)
Line 1751: dstImgResNs = sd.getNamespace(dstSdUUID, IMAGE_NAMESPACE)
Line 1752:
Line 1753: # Preparing the ordered resource list to be acquired
Line 1754: resList = (rmanager.acquireResource(*x) for x in sorted((
isn't acquireResource acquiring?
Line 1755: (srcImgResNs, imgUUID, rm.LockType.shared),
Line 1756: (dstImgResNs, imgUUID, rm.LockType.exclusive),
Line 1757: )))
Line 1758:
....................................................
File vdsm/vm.py
Line 721: self.conf['timeOffset'] = timeOffset
Line 722:
Line 723: def extendDriveVolume(self, vmDrive, newSize=None):
Line 724: if not vmDrive.blockDev:
Line 725: self.log.error("The device %s is not a block device",
vmDrive.name)
remove log
Line 726: return
Line 727:
Line 728: if newSize is None:
Line 729: newSize = (config.getint('irs',
'volume_utilization_chunk_mb')
Line 735: 'imageID': vmDrive.diskReplicate['imageID'],
Line 736: 'volumeID': vmDrive.diskReplicate['volumeID'],
Line 737: 'name': vmDrive.name, 'newSize': newSize}
Line 738: self.log.debug("Requesting extension for the replica
volume: %s",
Line 739: volInfo)
just call __extendDriveVolume and pass it the callback
in fact you can:
if getattr...:
drive = vmDrive.diskReplicate
callback = __afterReplicaExtend
else:
drive = vmDrive
callback = __afterDriveExtend
self.__extendDriveVolume...
Line 740: self.cif.irs.sendExtendMsg(vmDrive.poolID, volInfo,
Line 741: newSize,
self.__afterReplicaExtend)
Line 742: else:
Line 743: self.__extendDriveVolume(vmDrive, newSize)
Line 758: self._guestCpuLock.release()
Line 759:
Line 760: def __afterReplicaExtend(self, volInfo):
Line 761: self.log.debug("Refreshing replica volume: %s", volInfo)
Line 762: self.__refreshDriveVolume(volInfo)
are you sure that the new device size is the newSize?
I think you need to check and not proceed if it's not extended.
Line 763:
Line 764: vmDrive = self._findDriveByName(volInfo['name'])
Line 765:
Line 766: self.log.debug("Requesting extension for the original drive:
%s",
--
To view, visit http://gerrit.ovirt.org/5252
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I91e641cb1d25feb8a406aa7ad07415587a7ac290
Gerrit-PatchSet: 13
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: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
