Deepak C Shetty has posted comments on this change. Change subject: WIP: storage: Introduce prepareBackupDisk vdsm verb ......................................................................
Patch Set 4: (15 inline comments) .................................................... File vdsm/API.py Line 744: def tearDown(self): Line 745: return self._irs.teardownVolume(self._sdUUID, self._spUUID, Line 746: self._imgUUID, self._UUID) Line 747: Line 748: def prepareForBackup(self, bkupType): exportAsBlockDevice sounds good ? Line 749: return self._irs.prepareBackupDisk(self._sdUUID, self._spUUID, Line 750: self._imgUUID, self._UUID, bkupType) Line 751: Line 752: Line 744: def tearDown(self): Line 745: return self._irs.teardownVolume(self._sdUUID, self._spUUID, Line 746: self._imgUUID, self._UUID) Line 747: Line 748: def prepareForBackup(self, bkupType): I am not very clear on Image Vs Volume in the context here. Are you suggesting image based verb because snapshot being exported is part of a Image ? I see it as volume verb since snapshot is a volume, which is part of a Image and we are only working/exporting with this snapshot/Volume here.. not very clear yet why this should be image bsaed ? I can activate the volume ( by caling vol.refresh()) if thats the only reason why you think this should be a image verb ? Line 749: return self._irs.prepareBackupDisk(self._sdUUID, self._spUUID, Line 750: self._imgUUID, self._UUID, bkupType) Line 751: Line 752: .................................................... File vdsm/storage/hsm.py Line 3130: Line 3131: @public Line 3132: def prepareBackupDisk(self, sdUUID, spUUID, imgUUID, volUUID, bkupType): Line 3133: Line 3134: if bkupType not in volume.BACKUP_APP_TYPES: If API.py verb name is exportAsBlockDevice then connType can make sense here. How about exportAs (local|remote) instead of bkupType... ? Line 3135: raise se.prepareBackupDiskError() Line 3136: Line 3137: dom = sdCache.produce(sdUUID=sdUUID) Line 3138: snapVol = dom.produceVolume(imgUUID=imgUUID, volUUID=volUUID) Line 3141: sockType = 'unix' Line 3142: if bkupType == volume.BACKUP_HOST: Line 3143: sockType = 'tcp' Line 3144: Line 3145: nbdPath = snapVol.exportAsNbd(sockType) If by activating you mean calling vol.refresh.. then its applicable only for block based domains.. and IIUC it refreshed the LV metadata. Is that the only reason for using Image based verb ? what if i do snalvol.refresh() before exportAsNbd ? I didn't understnad the last 2 lines of your comment.. can u explain what you mean by "trips over the activation/deactivation of lvs and it will make a difference when the vm is running on the same node (spurious text-only errors)" ? Line 3146: Line 3147: cbtInfo = snapVol.queryCBTInfo() Line 3148: return dict(protocol=sockType, path=nbdPath, cbtInfo=cbtInfo) Line 3149: Line 3141: sockType = 'unix' Line 3142: if bkupType == volume.BACKUP_HOST: Line 3143: sockType = 'tcp' Line 3144: Line 3145: nbdPath = snapVol.exportAsNbd(sockType) That is exactly something thats not clear to me. We are exporting "a snapshot" (there could be multiple, but we are only exporting the leafmost ) hence its looks like a volume operation.... how does this pertain to the vdisk is not clear to me. Line 3146: Line 3147: cbtInfo = snapVol.queryCBTInfo() Line 3148: return dict(protocol=sockType, path=nbdPath, cbtInfo=cbtInfo) Line 3149: .................................................... File vdsm/storage/storage_exception.py Line 341: code = 232 Line 342: message = "Error exporting Volume as NBD" Line 343: Line 344: Line 345: class prepareBackupDiskError(StorageException): Sorry! that was my overlook.. will fix it in next patch Line 346: code = 233 Line 347: message = "Error preparing disk for Backup" Line 348: Line 349: .................................................... File vdsm/storage/volume.py Line 57: INTERNAL_VOL = 7 Line 58: LEAF_VOL = 8 Line 59: Line 60: # Backup Application Types Line 61: BACKUP_VA = 9 Looking at ayal's comment.. we shouldn't make this feel backup/restore specific. So i think this will chnage to something liek local & remote exports Line 62: BACKUP_HOST = 10 Line 63: Line 64: VOL_TYPE = [PREALLOCATED_VOL, SPARSE_VOL] Line 65: VOL_FORMAT = [COW_FORMAT, RAW_FORMAT] Line 675: def getNbdPid(self): Line 676: return int(self.getMetaParam(NBDPID)) Line 677: Line 678: def setNbdPid(self, pid): Line 679: self.setMetaParam(NBDPID, pid) I need the qemu-nbd PID saved, so that in teardownDisk i can kill it. I had initially stored the pid only in the Volume class.. but I dont' get it, since Volume instance gets created afresh via sdcache when needed, hence it won't have the PID. I thought using MD was the only way to persist. What is the way to persist Volume instance to a disk... such that it works even when used via sdCache.. i don't see sdcache creating Volume from disk. Line 680: Line 681: def isShared(self): Line 682: return self.getVolType() == type2name(SHARED_VOL) Line 683: Line 939: def exportAsNbd(self, sockType): Line 940: volPath = self.getVolumePath() Line 941: Line 942: if sockType == 'unix': Line 943: rc, ret1, ret2 = qemuExportNbdOverUnix(volPath) I used ret1 and ret2 bcos based on success/failure qemuExport....() function returns diff values for ret1 and ret2 Line 944: else: Line 945: # TODO: Implement this later Line 946: return None Line 947: Line 941: Line 942: if sockType == 'unix': Line 943: rc, ret1, ret2 = qemuExportNbdOverUnix(volPath) Line 944: else: Line 945: # TODO: Implement this later Done Line 946: return None Line 947: Line 948: if rc: Line 949: self.log.error("Error exporting volume as NBD, out=%s, err=%s", Line 947: Line 948: if rc: Line 949: self.log.error("Error exporting volume as NBD, out=%s, err=%s", Line 950: (ret1, ret2)) Line 951: raise se.VolumeExportAsNbdError() Done Line 952: Line 953: self.setNbdPid(ret1) Line 954: self.nbdSockPath = ret2 Line 955: Line 959: # For now, return the entire volume as part of changed info Line 960: size = int(self.getMetaParam(SIZE)) Line 961: changedArea = [dict(startOffset=0, length=size)] Line 962: Line 963: changes = {'startOffset': 0, 'length': size, It isn't. it looks like that bcos we are returning the whole disk as changed This is similar to other well known solution. changes.startOffset and .length gives you idea of how much disk space has changed. changes['changedArea'] is one of the changed areas.. there could be more than one.. typically there will be multiple changedArea's representing each contiguous chunk of the disk space that has changed... Per my understandig, this is done so that queryDiskChangeArea (when implemented) can be called in a loop to get a part of diskChanged area... and you do this until you hit changes.length.. in whcih case you know the entire disk change area's are processed. This helps when describing many changes to a large virtual disk which otherwise could end up occupyign too much memory Line 964: 'changedArea': changedArea} Line 965: return changes Line 966: Line 967: Line 1107: # qemu-nbd starts as a user space nbd server, doesn't return, until killed. Line 1108: proc = misc.execCmd(cmd, sync=False) Line 1109: rc = proc.returncode Line 1110: Line 1111: if proc.returncode: Done Line 1112: # qemu-nbd bad happened Line 1113: log.error('(qemuExportNbdOverUnix): QEMU-NBD BAD HAPPENED !!! ') Line 1114: os.unlink(sockPath) Line 1115: Line 1109: rc = proc.returncode Line 1110: Line 1111: if proc.returncode: Line 1112: # qemu-nbd bad happened Line 1113: log.error('(qemuExportNbdOverUnix): QEMU-NBD BAD HAPPENED !!! ') Done Line 1114: os.unlink(sockPath) Line 1115: Line 1116: rc = proc.returncode Line 1117: out = misc.stripNewLines(proc.stdout) Line 1112: # qemu-nbd bad happened Line 1113: log.error('(qemuExportNbdOverUnix): QEMU-NBD BAD HAPPENED !!! ') Line 1114: os.unlink(sockPath) Line 1115: Line 1116: rc = proc.returncode Done Line 1117: out = misc.stripNewLines(proc.stdout) Line 1118: err = misc.stripNewLines(proc.stderr) Line 1119: Line 1120: return (rc, out, err) -- To view, visit http://gerrit.ovirt.org/14403 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ccacda2b09810a71f445ed65de6a09336ea3fb1 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Deepak C Shetty <deepa...@linux.vnet.ibm.com> Gerrit-Reviewer: Adam Litke <a...@us.ibm.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Deepak C Shetty <deepa...@linux.vnet.ibm.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: Sharad Mishra <snmis...@linux.vnet.ibm.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches