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