Ayal Baron has posted comments on this change. Change subject: BZ#836161 - Refactored deleteImage. ......................................................................
Patch Set 1: I would prefer that you didn't submit this (17 inline comments) .................................................... Commit Message Line 3: AuthorDate: 2012-10-04 10:55:51 +0200 Line 4: Commit: Eduardo Warszawski <[email protected]> Line 5: CommitDate: 2012-10-11 19:28:56 +0200 Line 6: Line 7: BZ#836161 - Refactored deleteImage. why? Line 8: Line 9: Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 .................................................... File vdsm/storage/blockSD.py Line 162: for vName, vImg, vPar in vols.itervalues(): Line 163: res[vName]['parent'] = vPar Line 164: if vImg not in res[vName]['imgs']: Line 165: res[vName]['imgs'].insert(0, vImg) Line 166: if vPar != sd.BLANK_UUID \ vPar is an ugly name, while you're at it, change it to parentVol Line 167: and not vName.startswith(sd.REMOVED_IMAGE_PREFIX) \ Line 168: and not vName.startswith(sd.ZERO_ME_IMAGE_PREFIX) \ Line 169: and vImg not in res[vPar]['imgs']: Line 170: res[vPar]['imgs'].append(vImg) Line 163: res[vName]['parent'] = vPar Line 164: if vImg not in res[vName]['imgs']: Line 165: res[vName]['imgs'].insert(0, vImg) Line 166: if vPar != sd.BLANK_UUID \ Line 167: and not vName.startswith(sd.REMOVED_IMAGE_PREFIX) \ s/vName/volName/ Line 168: and not vName.startswith(sd.ZERO_ME_IMAGE_PREFIX) \ Line 169: and vImg not in res[vPar]['imgs']: Line 170: res[vPar]['imgs'].append(vImg) Line 171: Line 164: if vImg not in res[vName]['imgs']: Line 165: res[vName]['imgs'].insert(0, vImg) Line 166: if vPar != sd.BLANK_UUID \ Line 167: and not vName.startswith(sd.REMOVED_IMAGE_PREFIX) \ Line 168: and not vName.startswith(sd.ZERO_ME_IMAGE_PREFIX) \ why do we need this prefix? an lv to remove which doesn't require zeroing should not be renamed, it should be directly deleted Line 169: and vImg not in res[vPar]['imgs']: Line 170: res[vPar]['imgs'].append(vImg) Line 171: Line 172: return dict((k, sd.ImgsPar(tuple(v['imgs']), v['parent'])) Line 180: def _zeroVolume(sdUUID, volUUID): Line 181: """ Line 182: This function requires an active LV. Line 183: """ Line 184: # Change for zero 128 M chuncks and log. Add 'TODO' Line 185: # 128 M is the vdsm extent size default Line 186: cmd = tuple(constants.EXT_DD, "if=/dev/zero", Line 187: "of=%s" % os.path.join("/dev", sdUUID, volUUID)) Line 188: p = misc.execCmd(cmd, sudo=False, sync=False) Line 182: This function requires an active LV. Line 183: """ Line 184: # Change for zero 128 M chuncks and log. Line 185: # 128 M is the vdsm extent size default Line 186: cmd = tuple(constants.EXT_DD, "if=/dev/zero", this should use ddwatchcopy from misc. if it doesn't give you everything you need you should modify it to do so. every long running dd must run with low priority which is controlled in the function and there is no reason to reimplement it. Line 187: "of=%s" % os.path.join("/dev", sdUUID, volUUID)) Line 188: p = misc.execCmd(cmd, sudo=False, sync=False) Line 189: return p Line 190: .................................................... File vdsm/storage/fileSD.py Line 68: pattern = os.path.join(sd.mountBasePath, '*', sdUUID) Line 69: # Warning! You need a global proc pool big as the number of NFS domains. Line 70: domPaths = getProcPool.glob.glob(pattern) Line 71: if len(domPaths) != 1: Line 72: raise se.StorageDomainLayoutError("domain", sdUUID) Should be a storage domain not found error. Need to differentiate between could not find storage domain and found multiple storage domains with same uuid Line 73: return domPaths[0] Line 74: Line 75: Line 76: def getImagePath(sdUUID, imgUUID): Line 313: imgList.append(os.path.basename(i)) Line 314: return imgList Line 315: Line 316: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 317: oPath = getImagePath(sdUUID, imgUUID) what is 'oPath'? please rename to something legible and meaningful. currImageDir Line 318: head, tail = self.oop.os.path.split(oPath) Line 319: nPath = os.tempnam(head, sd.REMOVED_IMAGE_PREFIX + tail) Line 320: try: Line 321: self.oop.os.rename(oPath, nPath) Line 314: return imgList Line 315: Line 316: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 317: oPath = getImagePath(sdUUID, imgUUID) Line 318: head, tail = self.oop.os.path.split(oPath) head, tail? please rename as well. Line 319: nPath = os.tempnam(head, sd.REMOVED_IMAGE_PREFIX + tail) Line 320: try: Line 321: self.oop.os.rename(oPath, nPath) Line 322: except OSError as e: Line 315: Line 316: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 317: oPath = getImagePath(sdUUID, imgUUID) Line 318: head, tail = self.oop.os.path.split(oPath) Line 319: nPath = os.tempnam(head, sd.REMOVED_IMAGE_PREFIX + tail) nPath? toDelDir? Line 320: try: Line 321: self.oop.os.rename(oPath, nPath) Line 322: except OSError as e: Line 323: self.log.error("image: %s can't be moved", oPath) Line 319: nPath = os.tempnam(head, sd.REMOVED_IMAGE_PREFIX + tail) Line 320: try: Line 321: self.oop.os.rename(oPath, nPath) Line 322: except OSError as e: Line 323: self.log.error("image: %s can't be moved", oPath) s/.*/rename of image dir %s failed/ Line 324: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) Line 325: for volUUID in volsImgs: Line 326: volPath = os.path.join(nPath, volUUID) Line 327: try: Line 320: try: Line 321: self.oop.os.rename(oPath, nPath) Line 322: except OSError as e: Line 323: self.log.error("image: %s can't be moved", oPath) Line 324: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) not sure this exception will look good in the log Line 325: for volUUID in volsImgs: Line 326: volPath = os.path.join(nPath, volUUID) Line 327: try: Line 328: self.oop.os.remove(volPath) Line 325: for volUUID in volsImgs: Line 326: volPath = os.path.join(nPath, volUUID) Line 327: try: Line 328: self.oop.os.remove(volPath) Line 329: self.oop.os.remove(volPath + '.meta') why delete 1 by 1 and not the entire dir? shutil.rmtree? Line 330: except OSError as e: Line 331: self.log.error("vol: %s can't be removed.", volPath) Line 332: try: Line 333: self.oop.os.remove(nPath) Line 334: except OSError as e: Line 335: self.log.error("removed image dir: %s can't be removed", nPath) Line 336: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) Line 337: Line 338: def zeroImage(self, sdUUID, imgUUID, volsImgs): why do I have zeroImage here? Line 339: return self.deleteImage(sdUUID, imgUUID, volsImgs) Line 340: Line 341: def getAllVolumes(self): Line 342: """ .................................................... File vdsm/storage/hsm.py Line 1278: if misc.parseBool(postZero): Line 1279: self._spmSchedule(spUUID, "zeroImage_%s" % imgUUID, dom.zeroImage, Line 1280: sdUUID, imgUUID, imgsByVol.keys()) Line 1281: else: Line 1282: dom.deleteImage(sdUUID, imgUUID, imgsByVol) again, you need to go through sp for deleteImage Line 1283: # This is a hack to keep the interface consistent Line 1284: # We currently have race conditions in delete image, to quickly fix Line 1285: # this we delete images in the "synchronous" state. This only works Line 1286: # because rhev-m does not send two requests at a time. This hack is .................................................... File vdsm/storage/sd.py Line 134: ImgsPar = namedtuple("ImgsPar", "imgs,parent") Line 135: ISO_IMAGE_UUID = '11111111-1111-1111-1111-111111111111' Line 136: BLANK_UUID = '00000000-0000-0000-0000-000000000000' Line 137: REMOVED_IMAGE_PREFIX = "_remove_me_" Line 138: ZERO_ME_IMAGE_PREFIX = "_zero_me_" ZERO_ME is redundant Line 139: Line 140: # Blocks used for each lease (valid on all domain types) Line 141: LEASE_BLOCKS = 2048 Line 142: Line 141: LEASE_BLOCKS = 2048 Line 142: Line 143: UNICODE_MINIMAL_VERSION = 3 Line 144: Line 145: storage_repository = config.get('irs', 'repository') why are these not constants? i.e. all capitals Line 146: mountBasePath = os.path.join(storage_repository, DOMAIN_MNT_POINT) Line 147: Line 148: Line 149: def getVolsOfImage(allVols, imgUUID): -- To view, visit http://gerrit.ovirt.org/8506 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
