Nir Soffer has uploaded a new change for review. Change subject: vm: Unify checks for vdsm image ......................................................................
vm: Unify checks for vdsm image When testing if dict or vm.Drive object are vdsm image, we have to do ugly type check, and then invoke either vm.Drive.isVdsmImage, or vm.isVdsmImage. These functions does not have the same logic. Additionally, when working with dicts, we also check if device key is 'disk' before invoking vm.isVdsmImage. Having Drive.isVdsmImage looks first as an improvement, but since we handle both dicts and drive objects, this only complicates the code, The different logic for testing drive keys creates confusion and lead to pointless discussions if one key is needed or not in certain context. There is one call site which did not check if device is 'disk'. I think this was an error and not the intended behavior. This patch unify this check; both vm.Drive and dict have now similar interface so isVdsmImage can handle both, and we use the same logic. Change-Id: Iaa109a19aeec56f09ed2e6d64dee636c7779d426 Signed-off-by: Nir Soffer <[email protected]> --- M vdsm/clientIF.py M vdsm/vm.py 2 files changed, 21 insertions(+), 12 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/70/22370/1 diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py index c083991..124f8e5 100644 --- a/vdsm/clientIF.py +++ b/vdsm/clientIF.py @@ -244,7 +244,7 @@ def prepareVolumePath(self, drive, vmId=None): if type(drive) is dict: # PDIV drive format - if drive['device'] == 'disk' and vm.isVdsmImage(drive): + if vm.isVdsmImage(drive): res = self.irs.prepareImage( drive['domainID'], drive['poolID'], drive['imageID'], drive['volumeID']) diff --git a/vdsm/vm.py b/vdsm/vm.py index 1358b09..4c6cba2 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -81,8 +81,17 @@ def isVdsmImage(drive): - return all(k in drive.keys() for k in ('volumeID', 'domainID', 'imageID', - 'poolID')) + """ + Tell if drive looks like a vdsm image + + :param drive: drive to check + :type drive: dict or vm.Drive + :return: bool + """ + if drive['device'] != 'disk': + return False + required = ('volumeID', 'domainID', 'imageID', 'poolID') + return all(k in drive for k in required) def _filterSnappableDiskDevices(diskDeviceXmlElements): @@ -620,7 +629,7 @@ try: dStats = {'truesize': str(vmDrive.truesize), 'apparentsize': str(vmDrive.apparentsize)} - if vmDrive.isVdsmImage(): + if isVdsmImage(vmDrive): dStats['imageID'] = vmDrive.imageID dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) / sampleInterval) @@ -1403,6 +1412,9 @@ else: return value + def __contains__(self, attr): + return hasattr(self, attr) + def isDiskReplicationInProgress(self): return hasattr(self, "diskReplicate") @@ -1487,9 +1499,6 @@ i /= 26 return devname.get(self.iface, 'hd') + (devindex or 'a') - - def isVdsmImage(self): - return getattr(self, 'poolID', False) def _checkIoTuneCategories(self): categories = ("bytes", "iops") @@ -2184,7 +2193,7 @@ # A destroy request has been issued, exit early break drive['path'] = self.cif.prepareVolumePath(drive, self.id) - if drive['device'] == 'disk' and isVdsmImage(drive): + if isVdsmImage(drive): domains.append(drive['domainID']) else: self.sdIds.extend(domains) @@ -2249,7 +2258,7 @@ del toSave['floppy'] for drive in toSave.get('drives', []): for d in self._devices[DISK_DEVICES]: - if d.isVdsmImage() and drive.get('volumeID') == d.volumeID: + if isVdsmImage(d) and drive.get('volumeID') == d.volumeID: drive['truesize'] = str(d.truesize) drive['apparentsize'] = str(d.apparentsize) @@ -2991,7 +3000,7 @@ self.saveState() else: for drive in devices[DISK_DEVICES]: - if drive['device'] == 'disk' and isVdsmImage(drive): + if isVdsmImage(drive): self.sdIds.append(drive['domainID']) for devType, devClass in self.DeviceMapping: @@ -3471,7 +3480,7 @@ driveXml = drive.getXML().toprettyxml(encoding='utf-8') self.log.debug("Hotunplug disk xml: %s", driveXml) # Remove found disk from vm's drives list - if drive.isVdsmImage(): + if isVdsmImage(drive): self.sdIds.remove(drive.domainID) self._devices[DISK_DEVICES].remove(drive) # Find and remove disk device from vm's conf @@ -3612,7 +3621,7 @@ raise LookupError("No such drive: '%s'" % drive) def updateDriveVolume(self, vmDrive): - if not vmDrive.device == 'disk' or not vmDrive.isVdsmImage(): + if not isVdsmImage(vmDrive): return volSize = self.cif.irs.getVolumeSize( -- To view, visit http://gerrit.ovirt.org/22370 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iaa109a19aeec56f09ed2e6d64dee636c7779d426 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
