Change in vdsm[master]: WIP: storage: Introduce prepareBackupDisk vdsm verb

deepakcs at linux.vnet.ibm.com deepakcs at linux.vnet.ibm.com
Tue May 28 11:50:42 UTC 2013


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 <deepakcs at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Deepak C Shetty <deepakcs at linux.vnet.ibm.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Sharad Mishra <snmishra at linux.vnet.ibm.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list