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

fsimonce at redhat.com fsimonce at redhat.com
Thu May 23 19:06:49 UTC 2013


Federico Simoncelli has posted comments on this change.

Change subject: WIP: storage: Introduce prepareBackupDisk vdsm verb
......................................................................


Patch Set 4: I would prefer that you didn't submit this

(6 inline comments)

Marked -1 for visibility.

....................................................
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):
I have the feeling (not sure yet) that this is an image verb rather than a volume verb. See the comments in the other files to understand why.
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 3141:             sockType = 'unix'
Line 3142:         if bkupType == volume.BACKUP_HOST:
Line 3143:             sockType = 'tcp'
Line 3144: 
Line 3145:         nbdPath = snapVol.exportAsNbd(sockType)
Don't you have to activate the volume and its chain first? That's why you might need to move this to the image module... to use the getChain method.

I feel that this will be another flow that 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: 


....................................................
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):
I don't like the fact that we (already) have some classes with a lowercase at the beginning. Could you fix that at least in the new code you're adding. Thanks.
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
I'm fine with VA... but if you have a string more meaningful to use it would be better (also in the API file).
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)
In the metadata, why? It's relevant for the local host only. It should be per-host not per-volume entity.
Line 680: 
Line 681:     def isShared(self):
Line 682:         return self.getVolType() == type2name(SHARED_VOL)
Line 683: 


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 !!! ')
we might wanna use an error message that is more consistent with the other ones
Line 1114:         os.unlink(sockPath)
Line 1115: 
Line 1116:         rc = proc.returncode
Line 1117:         out = misc.stripNewLines(proc.stdout)


--
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