Eduardo has uploaded a new change for review.
Change subject: BZ#836161 - Refactored deleteImage. ......................................................................
BZ#836161 - Refactored deleteImage.
Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 Signed-off-by: Eduardo ewarszaw@redhat.com --- M vdsm/storage/blockSD.py M vdsm/storage/fileSD.py M vdsm/storage/hsm.py M vdsm/storage/sd.py 4 files changed, 142 insertions(+), 14 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/06/8506/1
diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py index e596fb6..d26e53c 100644 --- a/vdsm/storage/blockSD.py +++ b/vdsm/storage/blockSD.py @@ -163,13 +163,88 @@ res[vName]['parent'] = vPar if vImg not in res[vName]['imgs']: res[vName]['imgs'].insert(0, vImg) - if vPar != sd.BLANK_UUID and \ - not vName.startswith(blockVolume.image.REMOVED_IMAGE_PREFIX) \ + if vPar != sd.BLANK_UUID \ + and not vName.startswith(sd.REMOVED_IMAGE_PREFIX) \ + and not vName.startswith(sd.ZERO_ME_IMAGE_PREFIX) \ and vImg not in res[vPar]['imgs']: res[vPar]['imgs'].append(vImg)
return dict((k, sd.ImgsPar(tuple(v['imgs']), v['parent'])) for k, v in res.iteritems()) + + +def deleteVolumes(sdUUID, vols): + lvm.removeLVs(sdUUID, vols) + + +def _zeroVolume(sdUUID, volUUID): + """ + This function requires an active LV. + """ + # Change for zero 128 M chuncks and log. + # 128 M is the vdsm extent size default + cmd = tuple(constants.EXT_DD, "if=/dev/zero", + "of=%s" % os.path.join("/dev", sdUUID, volUUID)) + p = misc.execCmd(cmd, sudo=False, sync=False) + return p + + +def _getZerosFinished(zeroPs): + for volUUID, zeroProc in zeroPs.items(): + finished = {} + if not zeroProc.wait(0): + continue + else: + zeroPs.pop(volUUID) + finished[volUUID] = zeroProc.returncode + if zeroProc.returncode != 0: + log.warning("zeroing %s failed. %s", volUUID, + zeroProc.stderr) + else: + log.debug("zeroing %s finished", volUUID) + return finished + + +def zeroImgVolumes(sdUUID, imgUUID, volUUIDs): + # Put a sensible value for dd zeroing a 128 M or 1 G chunk and lvremove + # spent time. + ZERO_SLEEP = 60 # [seconds] + # Prepare for zeroing + try: + lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), + ("--addtag", sd.ZERO_ME_IMAGE_PREFIX + imgUUID))) + except se.StorageException as e: + log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, + volUUIDs) + try: + lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) + except se.StorageException as e: + # Hope this only means that some volumes were already writable. + log.debug("IGNORED: %s", e) + # blank the volumes. + zeroPs = {} + for volUUID in volUUIDs: + zeroPs[volUUID] = _zeroVolume(sdUUID, volUUID) + + # Yes, this is a potentially infinite loop. Kill the vdsm task. + last = time.time() + while len(zeroPs) > 0: + time.sleep(ZERO_SLEEP) + toDelete = [] + for volUUID, rc in _getZerosFinished(zeroPs): + if rc != 0: + log.warning("zero leftover: %s/%s rc = %s", sdUUID, volUUID, rc) + else: + toDelete.append(volUUID) + if toDelete: + deleteVolumes(sdUUID, toDelete) + + now = time.time() + if now - (last + 10 * ZERO_SLEEP) > 0: + log.warning("Still zeroing VG:%s LVs: %s", + sdUUID, zeroPs.iterkeys()) + + log.debug("All rm_zeroed VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID)
class VGTagMetadataRW(object): @@ -827,6 +902,12 @@ if i.startswith(blockVolume.TAG_PREFIX_IMAGE) ] return images
+ def deleteImage(self, sdUUID, imgUUID, volsImgs): + return deleteVolumes(sdUUID, volsImgs) + + def zeroImage(self, sdUUID, imgUUID, volsImgs): + return zeroImgVolumes(sdUUID, imgUUID, volsImgs) + def getAllVolumes(self): """ Return all the images that depend on a volume. diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index 054fadb..d5b298f 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -62,6 +62,21 @@ return True
+def getDomPath(sdUUID): + # /rhev/data-center/mnt/export-path/sdUUID/dom_md/metadata + # /rhev/data-center/mnt/blockSD/sdUUID/dom_md/metadata + pattern = os.path.join(sd.mountBasePath, '*', sdUUID) + # Warning! You need a global proc pool big as the number of NFS domains. + domPaths = getProcPool.glob.glob(pattern) + if len(domPaths) != 1: + raise se.StorageDomainLayoutError("domain", sdUUID) + return domPaths[0] + + +def getImagePath(sdUUID, imgUUID): + return os.path.join(getDomPath(sdUUID), 'images', imgUUID) + + def getDomUuidFromMetafilePath(metafile): # Metafile path has pattern: # /rhev/data-center/mnt/export-path/sdUUID/dom_md/metadata @@ -298,6 +313,31 @@ imgList.append(os.path.basename(i)) return imgList
+ def deleteImage(self, sdUUID, imgUUID, volsImgs): + oPath = getImagePath(sdUUID, imgUUID) + head, tail = self.oop.os.path.split(oPath) + nPath = os.tempnam(head, sd.REMOVED_IMAGE_PREFIX + tail) + try: + self.oop.os.rename(oPath, nPath) + except OSError as e: + self.log.error("image: %s can't be moved", oPath) + raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) + for volUUID in volsImgs: + volPath = os.path.join(nPath, volUUID) + try: + self.oop.os.remove(volPath) + self.oop.os.remove(volPath + '.meta') + except OSError as e: + self.log.error("vol: %s can't be removed.", volPath) + try: + self.oop.os.remove(nPath) + except OSError as e: + self.log.error("removed image dir: %s can't be removed", nPath) + raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) + + def zeroImage(self, sdUUID, imgUUID, volsImgs): + return self.deleteImage(sdUUID, imgUUID, volsImgs) + def getAllVolumes(self): """ Return dict {volUUID: ((imgUUIDs,), parentUUID)} of the domain. diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py index 78eef77..2a8bc59 100644 --- a/vdsm/storage/hsm.py +++ b/vdsm/storage/hsm.py @@ -1260,24 +1260,26 @@ Delete Image folder with all volumes """ #vars.task.setDefaultException(se.ChangeMeError("%s" % args)) - pool = self.getPool(spUUID) #Validates that the pool is connected. WHY? - self.validateSdUUID(sdUUID) + self.getPool(spUUID) #Validates that the pool is connected. WHY? + dom = self.validateSdUUID(sdUUID)
vars.task.getExclusiveLock(STORAGE, imgUUID) vars.task.getSharedLock(STORAGE, sdUUID) + allVols = dom.getAllVolumes() + imgsByVol = sd.getVolsOfImage(allVols, imgUUID) # Do not validate if forced. - if not misc.parseBool(force): - pool.validateDelete(sdUUID, imgUUID) - # Rename image if postZero and perform delete as async operation - # else delete image in sync. stage + if not misc.parseBool(force) and not dom.isBackup() \ + and not all(len(v.imgs) == 1 for v in imgsByVol.itervalues()): + msg = "Cannot delete shared image %s. volImgs: %s" \ + % (imgUUID, imgsByVol) + raise se.CannotDeleteSharedVolume(msg) + + # zeroImage will delete zeroed volumes at the end. if misc.parseBool(postZero): - newImgUUID = pool.preDeleteRename(sdUUID, imgUUID) - self._spmSchedule(spUUID, "deleteImage", pool.deleteImage, sdUUID, newImgUUID, - misc.parseBool(postZero), misc.parseBool(force) - ) + self._spmSchedule(spUUID, "zeroImage_%s" % imgUUID, dom.zeroImage, + sdUUID, imgUUID, imgsByVol.keys()) else: - pool.deleteImage(sdUUID, imgUUID, - misc.parseBool(postZero), misc.parseBool(force)) + dom.deleteImage(sdUUID, imgUUID, imgsByVol) # This is a hack to keep the interface consistent # We currently have race conditions in delete image, to quickly fix # this we delete images in the "synchronous" state. This only works diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py index 55c5b12..fcb796c 100644 --- a/vdsm/storage/sd.py +++ b/vdsm/storage/sd.py @@ -134,12 +134,17 @@ ImgsPar = namedtuple("ImgsPar", "imgs,parent") ISO_IMAGE_UUID = '11111111-1111-1111-1111-111111111111' BLANK_UUID = '00000000-0000-0000-0000-000000000000' +REMOVED_IMAGE_PREFIX = "_remove_me_" +ZERO_ME_IMAGE_PREFIX = "_zero_me_"
# Blocks used for each lease (valid on all domain types) LEASE_BLOCKS = 2048
UNICODE_MINIMAL_VERSION = 3
+storage_repository = config.get('irs', 'repository') +mountBasePath = os.path.join(storage_repository, DOMAIN_MNT_POINT) +
def getVolsOfImage(allVols, imgUUID): """ Filter allVols dict for volumes related to imgUUID.
-- To view, visit http://gerrit.ovirt.org/8506 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com
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 ewarszaw@redhat.com 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 ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Refactored deleteImage. ......................................................................
Patch Set 1: (4 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 216: except se.StorageException as e: Line 217: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 218: volUUIDs) Line 219: try: Line 220: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) please open an lvm bug on this. refresh the one you have if it's relevant. Line 221: except se.StorageException as e: Line 222: # Hope this only means that some volumes were already writable. Line 223: log.debug("IGNORED: %s", e) Line 224: # blank the volumes.
Line 219: try: Line 220: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) Line 221: except se.StorageException as e: Line 222: # Hope this only means that some volumes were already writable. Line 223: log.debug("IGNORED: %s", e) this log line is really bad. Line 224: # blank the volumes. Line 225: zeroPs = {} Line 226: for volUUID in volUUIDs: Line 227: zeroPs[volUUID] = _zeroVolume(sdUUID, volUUID)
Line 228: Line 229: # Yes, this is a potentially infinite loop. Kill the vdsm task. Line 230: last = time.time() Line 231: while len(zeroPs) > 0: Line 232: time.sleep(ZERO_SLEEP) why sleep? poll? Line 233: toDelete = [] Line 234: for volUUID, rc in _getZerosFinished(zeroPs): Line 235: if rc != 0: Line 236: log.warning("zero leftover: %s/%s rc = %s", sdUUID, volUUID, rc)
.................................................... 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) s/deleteImage/deleveVolumes/ 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
-- 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 ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Refactored deleteImage. ......................................................................
Patch Set 1: (2 inline comments)
.................................................... File vdsm/storage/fileSD.py 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) I think you needn't have getImagePath() function. You only need: tail = imgUUID head = os.path.join(getDompath(sdUUID), 'images') 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:
.................................................... File vdsm/storage/hsm.py Line 1265: Line 1266: vars.task.getExclusiveLock(STORAGE, imgUUID) Line 1267: vars.task.getSharedLock(STORAGE, sdUUID) Line 1268: allVols = dom.getAllVolumes() Line 1269: imgsByVol = sd.getVolsOfImage(allVols, imgUUID) Is that not "volsByImg" instead of "imgsByVol"? Line 1270: # Do not validate if forced. Line 1271: if not misc.parseBool(force) and not dom.isBackup() \ Line 1272: and not all(len(v.imgs) == 1 for v in imgsByVol.itervalues()): Line 1273: msg = "Cannot delete shared image %s. volImgs: %s" \
-- 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 ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Refactored deleteImage. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
See my comments in the above.
-- 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 ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Rewrited deleteImage(). ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/storage/fileSD.py Line 317: return imgList Line 318: Line 319: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 320: currImgDir = getImagePath(sdUUID, imgUUID) Line 321: dirName, baseName = self.oop.os.path.split(currImgDir) The same comments as before, Does these make things simple? like: "baseName = imgUUID dirName = os.path.join(getDompath(sdUUID), 'images')"
You can save one function call at least, also "os.path.join()" is faster then split(). Line 322: toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName) Line 323: try: Line 324: self.oop.os.rename(currImgDir, toDelDir) Line 325: except OSError as e:
.................................................... File vdsm/storage/hsm.py Line 1265: Line 1266: vars.task.getExclusiveLock(STORAGE, imgUUID) Line 1267: vars.task.getSharedLock(STORAGE, sdUUID) Line 1268: allVols = dom.getAllVolumes() Line 1269: imgsByVol = sd.getVolsOfImage(allVols, imgUUID) The same comment as the comment in patch I. I think "imgsByVol" should be "volsByImg". Line 1270: # Do not validate if forced. Line 1271: if not misc.parseBool(force) and not dom.isBackup() \ Line 1272: and not all(len(v.imgs) == 1 for v in imgsByVol.itervalues()): Line 1273: msg = "Cannot delete shared image %s. volImgs: %s" \
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrited deleteImage(). ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(7 inline comments)
.................................................... Commit Message Line 3: AuthorDate: 2012-10-04 10:55:51 +0200 Line 4: Commit: Eduardo Warszawski ewarszaw@redhat.com Line 5: CommitDate: 2012-10-21 23:59:27 +0200 Line 6: Line 7: BZ#836161 - Rewrited deleteImage(). s/Rewrited/Rewrite of/ Line 8: Line 9: Volume operations should be done at the SD level to avoid Line 10: redundant metadata operations of information that is Line 11: already present in the SD layout.
Line 6: Line 7: BZ#836161 - Rewrited deleteImage(). Line 8: Line 9: Volume operations should be done at the SD level to avoid Line 10: redundant metadata operations of information that is to avoid retrieving static data multiple times from disk. Line 11: already present in the SD layout. Line 12: Line 13: Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695
.................................................... File vdsm/storage/blockSD.py Line 182: """ Line 183: # TODO: Change for zero 128 M chuncks and log. Line 184: # 128 M is the vdsm extent size default Line 185: cmd = tuple(constants.EXT_DD, "if=/dev/zero", Line 186: "of=%s" % os.path.join("/dev", sdUUID, volUUID)) This isn't running with the correct priority! Line 187: p = misc.execCmd(cmd, sudo=False, sync=False) Line 188: return p Line 189: Line 190:
Line 215: except se.StorageException as e: Line 216: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 217: volUUIDs) Line 218: try: Line 219: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) need a comment with lvm bug number which explains why you did this separation once it's fixed, we'd know we can get rid of it. Line 220: except se.StorageException as e: Line 221: # Hope this only means that some volumes were already writable. Line 222: log.debug("IGNORED: %s", e) Line 223: # blank the volumes.
Line 218: try: Line 219: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) Line 220: except se.StorageException as e: Line 221: # Hope this only means that some volumes were already writable. Line 222: log.debug("IGNORED: %s", e) this is still really bad Line 223: # blank the volumes. Line 224: zeroPs = {} Line 225: for volUUID in volUUIDs: Line 226: zeroPs[volUUID] = _zeroVolume(sdUUID, volUUID)
Line 227: Line 228: # Yes, this is a potentially infinite loop. Kill the vdsm task. Line 229: last = time.time() Line 230: while len(zeroPs) > 0: Line 231: time.sleep(ZERO_SLEEP) need to change to poll Line 232: toDelete = [] Line 233: for volUUID, rc in _getZerosFinished(zeroPs): Line 234: if rc != 0: Line 235: log.warning("zero leftover: %s/%s rc = %s", sdUUID, volUUID, rc)
Line 234: if rc != 0: Line 235: log.warning("zero leftover: %s/%s rc = %s", sdUUID, volUUID, rc) Line 236: else: Line 237: toDelete.append(volUUID) Line 238: log.debug("zeroed %s/%s will be deleted", sdUUID, volUUID) s/.*/%s/%s was zeroed and will be deleted / Line 239: if toDelete: Line 240: deleteVolumes(sdUUID, toDelete) Line 241: Line 242: now = time.time()
-- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(4 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 215: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 216: except se.StorageException as e: Line 217: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 218: volUUIDs) Line 219: raise I think raise is wrong here.
From the moment you sent the command the volume is dead. From this point on we would like to clean as much as possible. Postzero means that this disk contains sensitive data that we'd like to get rid of, so let's try to get rid of it...
Line 220: # Will fail for already writable volumes. Then it is a separate lvm call. Line 221: try: Line 222: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) Line 223: except se.StorageException as e:
Line 216: except se.StorageException as e: Line 217: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 218: volUUIDs) Line 219: raise Line 220: # Will fail for already writable volumes. Then it is a separate lvm call. /# Following call to changelv is separate since setting rw permission on an LV fails if the LV is already set to the same value, hence we would not be able to differentiate between a real failure of deltag/addtag and one we would like to ignore (permission is the same) Line 221: try: Line 222: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) Line 223: except se.StorageException as e: Line 224: # Hope this only means that some volumes were already writable.
Line 230: Line 231: # Yes, this is a potentially infinite loop. Kill the vdsm task. Line 232: last = time.time() Line 233: while len(zeroPs) > 0: Line 234: time.sleep(ZERO_SLEEP) still need to change to poll Line 235: toDelete = [] Line 236: for volUUID, rc in _getZerosFinished(zeroPs): Line 237: if rc != 0: Line 238: log.warning("zero leftover: %s/%s rc = %s", sdUUID, volUUID, rc)
.................................................... File vdsm/storage/fileSD.py Line 317: return imgList Line 318: Line 319: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 320: currImgDir = getImagePath(sdUUID, imgUUID) Line 321: dirName, baseName = self.oop.os.path.split(currImgDir) Shu Ming is probably right that 2 joins look clearer and more efficient than join and split. so should probably have a getImagesDir method which is: def getImagesDir(sdUUID): return getDompath(sdUUID), 'images') Line 322: toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName) Line 323: try: Line 324: self.oop.os.rename(currImgDir, toDelDir) Line 325: except OSError as e:
-- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 5: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 184: size: size to be blanked in bytes. Line 185: """ Line 186: # TODO: Change for zero 128 M chuncks and log. Line 187: # 128 M is the vdsm extent size default Line 188: BS = 512 issuing 512b writes will flood the storage with IOPS. At a minimum this should be in 1M blocks.
ddWatchCopy already does all these things, in the least you should reuse similar logic.
Also, I'm not sure whether this should be ODIRECT (when possible) or flood the pagecache with zeros (seems redundant, but faster as kernel can aggregate IOs) Line 189: count = size / BS Line 190: cmd = tuple(constants.CMD_LOWPRIO) Line 191: cmd += tuple(constants.EXT_DD, "if=/dev/zero", Line 192: "of=%s" % os.path.join("/dev", sdUUID, volUUID),
-- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 5: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 220: zerofds = {} Line 221: poller = select.poll() Line 222: for volUUID in volUUIDs: Line 223: dm = lvm.lvDmDev(sdUUID, volUUID) Line 224: size = multipath.getDeviceSize(dm) # Bytes this should be inside _zeroVolume Line 225: Line 226: proc = _zeroVolume(sdUUID, volUUID, size) Line 227: fd = proc.stdout.fileno() Line 228: zerofds[fd] = ProcVol(proc, volUUID)
-- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Haim Ateya has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 5: Verified
verified executing the following scenarios: - delete disk w\ post zero - delete disk w\o post zero - delete disk w\ post zero and 1 snapshot - delete disk w\o post zero and 1 snapshot - delete disk w\ post zero and 3 snapshot - delete disk w\o post zero and 3 snapshot - delete 3 disk w\ post zero and 1 snapshot - delete 3 disk w\o post zero and 1 snapshot
Tested block storage domains.
-- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
ShaoHe Feng has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 6: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/fileSD.py Line 339: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) Line 340: Line 341: def zeroImage(self, sdUUID, imgUUID, volsImgs): Line 342: raise se.SourceImageActionError(imgUUID, sdUUID, Line 343: "image %s on a fileSD %s should not be zeroed.", imgUUID, sdUUID) SourceImageActionError only need 4 parameters. you have passed six.
Here is the SourceImageActionError define:
class SourceImageActionError(StorageException):
def __init__(self, imgUUID, sdUUID, msg=""):
self.value = "image=%s, source domain=%s: %s" % (imgUUID, sdUUID, msg) Line 344: Line 345: def getAllVolumes(self): Line 346: """ Line 347: Return dict {volUUID: ((imgUUIDs,), parentUUID)} of the domain.
-- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 7: (2 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) sorry for suggesting this only now, but how about using sync=True, but passing this function to misc.itmap? Line 196: return p Line 197: Line 198: Line 199: def zeroImgVolumes(sdUUID, imgUUID, volUUIDs):
.................................................... File vdsm/storage/fileSD.py Line 339: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) Line 340: Line 341: def zeroImage(self, sdUUID, imgUUID, volsImgs): Line 342: raise se.SourceImageActionError(imgUUID, sdUUID, "image %s on a " Line 343: "fileSD %s should not be zeroed." % (imgUUID, sdUUID)) but why would you want to pass (imgUUID, sdUUID) both as values and as strings? Line 344: Line 345: def getAllVolumes(self): Line 346: """ Line 347: Return dict {volUUID: ((imgUUIDs,), parentUUID)} of the domain.
-- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com
Yeela Kaplan has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 7: (1 inline comment)
.................................................... File vdsm/storage/lvm.py Line 1175: def lvPath(vgName, lvName): Line 1176: return os.path.join("/dev", vgName, lvName) Line 1177: Line 1178: Line 1179: def lvDmDev(vgName, lvName): Need to change in the commit msg from: 'lvPhysDev' to: 'lvDmDev' Line 1180: """Return the LV dm device. Line 1181: Line 1182: returns: dm-X Line 1183: If the LV is inactive there is no dm device
-- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: I would prefer that you didn't submit this
(22 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 173: for k, v in res.iteritems()) Line 174: Line 175: Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) Just out there, and public who needs encapsulation anyway? Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): Line 181: """Fill a block volume.
Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): I don't even know where to start here Line 181: """Fill a block volume. Line 182: Line 183: This function requires an active LV. Line 184: """
Line 202: # spent time. Line 203: ZERO_TIMEOUT = 60000 # [miliseconds] Line 204: # Prepare for zeroing Line 205: try: Line 206: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), You don't deactivate one you are done. You trust that no one else uses it because you don't take locks. Line 207: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 208: except se.StorageException as e: Line 209: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 210: volUUIDs)
Line 212: # LV fails if the LV is already set to the same value, hence we would not Line 213: # be able to differentiate between a real failure of deltag/addtag and one Line 214: # we would like to ignore (permission is the same) Line 215: try: Line 216: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) Seems like there is a verb for that too. Line 217: except se.StorageException as e: Line 218: # Hope this only means that some volumes were already writable. Line 219: log.debug("Ignoring failed permission change: %s", e) Line 220: # blank the volumes.
Line 221: zerofds = {} Line 222: poller = select.poll() Line 223: for volUUID in volUUIDs: Line 224: proc = _zeroVolume(sdUUID, volUUID) Line 225: fd = proc.stdout.fileno() This is redundant as pool already checks for fileno() Line 226: zerofds[fd] = ProcVol(proc, volUUID) Line 227: poller.register(fd, select.EPOLLHUP) Line 228: Line 229: # Wait until all the asyncs procs return
Line 228: Line 229: # Wait until all the asyncs procs return Line 230: # Yes, this is a potentially infinite loop.. Kill the vdsm task. Line 231: while zerofds: Line 232: fdevents = poller.poll(ZERO_TIMEOUT) # [(fd, event)] ZERO_TIMEOUT is confusing (ie. I got confused thinking it's 0 timeout) instead use ZERO_OPERATION_TIMEOUT or something similar. Line 233: toDelete = [] Line 234: for fd, event in fdevents: Line 235: proc, vol = zerofds[fd] Line 236: if not proc.wait(0):
Line 232: fdevents = poller.poll(ZERO_TIMEOUT) # [(fd, event)] Line 233: toDelete = [] Line 234: for fd, event in fdevents: Line 235: proc, vol = zerofds[fd] Line 236: if not proc.wait(0): If you got an HUP but the proc is still running for some reason you will never get another event. This is potentially very problematic (if you actually choose to handle it). Line 237: continue Line 238: else: Line 239: poller.unregister(fd) Line 240: zerofds.pop(fd)
Line 236: if not proc.wait(0): Line 237: continue Line 238: else: Line 239: poller.unregister(fd) Line 240: zerofds.pop(fd) Both are redundant. Line 241: if proc.returncode != 0: Line 242: log.error("zeroing %s/%s failed. Zero and remove this " Line 243: "volume manually. rc=%s %s", Line 244: sdUUID, vol, proc.rc, proc.stderr)
Line 250: try: Line 251: deleteVolumes(sdUUID, toDelete) Line 252: except se.CannotRemoveLogicalVolume as e: Line 253: # TODO: Add the list of removed fail volumes to the exception. Line 254: log.error("Remove failed for zeroed volumes: %s", e) stack trace Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 258: return
Line 253: # TODO: Add the list of removed fail volumes to the exception. Line 254: log.error("Remove failed for zeroed volumes: %s", e) Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) What are we logging exactly? Line 258: return Line 259: Line 260: Line 261: class VGTagMetadataRW(object):
Line 254: log.error("Remove failed for zeroed volumes: %s", e) Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 258: return ? Line 259: Line 260: Line 261: class VGTagMetadataRW(object): Line 262: log = logging.getLogger("storage.Metadata.VGTagMetadataRW")
.................................................... File vdsm/storage/fileSD.py Line 61: Line 62: return True Line 63: Line 64: Line 65: def getDomPath(sdUUID): Bypassing sdc? How is this different from produce without being cacheless. Line 66: # /rhev/data-center/mnt/export-path/sdUUID/dom_md/metadata Line 67: # /rhev/data-center/mnt/blockSD/sdUUID/dom_md/metadata 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 75: else: Line 76: return domPaths[0] Line 77: Line 78: Line 79: def getImagePath(sdUUID, imgUUID): This whole bit seems to be just a way to put something in an unintiuitive place Line 80: return os.path.join(getDomPath(sdUUID), 'images', imgUUID) Line 81: Line 82: Line 83: def getDomUuidFromMetafilePath(metafile):
Line 316: imgList.append(os.path.basename(i)) Line 317: return imgList Line 318: Line 319: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 320: currImgDir = getImagePath(sdUUID, imgUUID) reproducing yourself? The domain path is available as a field in the instance. Line 321: dirName, baseName = self.oop.os.path.split(currImgDir) Line 322: toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName) Line 323: try: Line 324: self.oop.os.rename(currImgDir, toDelDir)
Line 317: return imgList Line 318: Line 319: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 320: currImgDir = getImagePath(sdUUID, imgUUID) Line 321: dirName, baseName = self.oop.os.path.split(currImgDir) no need for oop path.split doesn't touch the disk it even works with files that don't exist
In [2]: os.path.split("dasdsad/dsadsad") Out[2]: ('dasdsad', 'dsadsad') Line 322: toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName) Line 323: try: Line 324: self.oop.os.rename(currImgDir, toDelDir) Line 325: except OSError as e:
Line 330: try: Line 331: self.oop.os.remove(volPath) Line 332: self.oop.os.remove(volPath + '.meta') Line 333: self.oop.os.remove(volPath + '.lease') Line 334: except OSError as e: Since it could be any of these files that failed. And they all throw the same exceptions. I'd put exc_info=True Line 335: self.log.error("vol: %s can't be removed.", volPath) Line 336: try: Line 337: self.oop.os.rmdir(toDelDir) Line 338: except OSError as e:
.................................................... File vdsm/storage/hsm.py Line 1267: vars.task.getSharedLock(STORAGE, sdUUID) Line 1268: allVols = dom.getAllVolumes() Line 1269: volsByImg = sd.getVolsOfImage(allVols, imgUUID) Line 1270: # Do not validate if forced. Line 1271: if not misc.parseBool(force) and not dom.isBackup() \ put force = parseBool(force) at the start of the method so no one needs to worry about parsing it. Line 1272: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): Line 1273: msg = "Cannot delete shared image %s. volImgs: %s" \ Line 1274: % (imgUUID, volsByImg) Line 1275: raise se.CannotDeleteSharedVolume(msg)
Line 1268: allVols = dom.getAllVolumes() Line 1269: volsByImg = sd.getVolsOfImage(allVols, imgUUID) Line 1270: # Do not validate if forced. Line 1271: if not misc.parseBool(force) and not dom.isBackup() \ Line 1272: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): move this part to before the if and have: hasSharedVolume = all(len(v.imgs) == 1 for v in volsByImg.itervalues())
So someone can figure out what the hell that expression means.
If you are worrying that this means it will always get calculated even if force is True just move it to another if. This condition is already too long in my opinion Line 1273: msg = "Cannot delete shared image %s. volImgs: %s" \ Line 1274: % (imgUUID, volsByImg) Line 1275: raise se.CannotDeleteSharedVolume(msg) Line 1276:
Line 1271: if not misc.parseBool(force) and not dom.isBackup() \ Line 1272: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): Line 1273: msg = "Cannot delete shared image %s. volImgs: %s" \ Line 1274: % (imgUUID, volsByImg) Line 1275: raise se.CannotDeleteSharedVolume(msg) No need to separate the message and the invocation. Line 1276: Line 1277: # zeroImage will delete zeroed volumes at the end. Line 1278: if misc.parseBool(postZero): Line 1279: self._spmSchedule(spUUID, "zeroImage_%s" % imgUUID, dom.zeroImage,
Line 1278: if misc.parseBool(postZero): Line 1279: self._spmSchedule(spUUID, "zeroImage_%s" % imgUUID, dom.zeroImage, Line 1280: sdUUID, imgUUID, volsByImg.keys()) Line 1281: else: Line 1282: dom.deleteImage(sdUUID, imgUUID, volsByImg) I assume we can remove the verb from sp then 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/lvm.py Line 1175: def lvPath(vgName, lvName): Line 1176: return os.path.join("/dev", vgName, lvName) Line 1177: Line 1178: Line 1179: def lvDmDev(vgName, lvName): devicemapper has a routine for that. You should give it the full path to the lv dev Line 1180: """Return the LV dm device. Line 1181: Line 1182: returns: dm-X Line 1183: If the LV is inactive there is no dm device
.................................................... File vdsm/storage/sd.py Line 140: LEASE_BLOCKS = 2048 Line 141: Line 142: UNICODE_MINIMAL_VERSION = 3 Line 143: Line 144: storage_repository = config.get('irs', 'repository') No accessing config outside of hsm Line 145: mountBasePath = os.path.join(storage_repository, DOMAIN_MNT_POINT) Line 146: Line 147: Line 148: 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Eduardo has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: (16 inline comments)
By danken request.
.................................................... File vdsm/storage/blockSD.py Line 173: for k, v in res.iteritems()) Line 174: Line 175: Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) Volumes are entities that belong to the domain. What is "public" here? This is an internal interface. Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): Line 181: """Fill a block volume.
Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): Then don't even start. Line 181: """Fill a block volume. Line 182: Line 183: This function requires an active LV. Line 184: """
Line 202: # spent time. Line 203: ZERO_TIMEOUT = 60000 # [miliseconds] Line 204: # Prepare for zeroing Line 205: try: Line 206: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), The image is locked. We are removing the image (all the associates volumes) then if "one else" uses it, you have a problem that for sure should not be resolved here. The only reason for activate these volumes is because we need to zero them before deleting. What is the point of deactivating volumes that will be removed immediately? (*). Waste more lvm operations?
(*) Be quiet, we use -f. Line 207: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 208: except se.StorageException as e: Line 209: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 210: volUUIDs)
Line 212: # LV fails if the LV is already set to the same value, hence we would not Line 213: # be able to differentiate between a real failure of deltag/addtag and one Line 214: # we would like to ignore (permission is the same) Line 215: try: Line 216: lvm.changelv(sdUUID, volUUIDs, ("--permission", "rw")) lvm.setrwLV operates in a unique LV and we want to change a bunch at once. If it fails it checks if the volume is already writable, with another lvm operation and reload contention. Here the leaf is probably already writable (and will fail) when the internal volumes not, and we want make all of them writable at once without extra operations. Therefore setrwLV is not pluralized. Line 217: except se.StorageException as e: Line 218: # Hope this only means that some volumes were already writable. Line 219: log.debug("Ignoring failed permission change: %s", e) Line 220: # blank the volumes.
Line 228: Line 229: # Wait until all the asyncs procs return Line 230: # Yes, this is a potentially infinite loop.. Kill the vdsm task. Line 231: while zerofds: Line 232: fdevents = poller.poll(ZERO_TIMEOUT) # [(fd, event)] Please make your suggestion shorter. Something that avoids you get confused. Line 233: toDelete = [] Line 234: for fd, event in fdevents: Line 235: proc, vol = zerofds[fd] Line 236: if not proc.wait(0):
Line 232: fdevents = poller.poll(ZERO_TIMEOUT) # [(fd, event)] Line 233: toDelete = [] Line 234: for fd, event in fdevents: Line 235: proc, vol = zerofds[fd] Line 236: if not proc.wait(0): Sadly seen in the wild. And the code manages it. And the whole operation succeed. Is more robust this way. Line 237: continue Line 238: else: Line 239: poller.unregister(fd) Line 240: zerofds.pop(fd)
Line 236: if not proc.wait(0): Line 237: continue Line 238: else: Line 239: poller.unregister(fd) Line 240: zerofds.pop(fd) Wrong. Line 241: if proc.returncode != 0: Line 242: log.error("zeroing %s/%s failed. Zero and remove this " Line 243: "volume manually. rc=%s %s", Line 244: sdUUID, vol, proc.rc, proc.stderr)
Line 253: # TODO: Add the list of removed fail volumes to the exception. Line 254: log.error("Remove failed for zeroed volumes: %s", e) Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) We want to know if all the volumes were deleted. The log will be improved. An else clause is very confusing for you too. Line 258: return Line 259: Line 260: Line 261: class VGTagMetadataRW(object):
.................................................... File vdsm/storage/fileSD.py Line 61: Line 62: return True Line 63: Line 64: Line 65: def getDomPath(sdUUID): sdc is one of the main reasons of the scale issues. We are not reading redundant metadata and this is trying to be a function instead to build another redundant instance. Line 66: # /rhev/data-center/mnt/export-path/sdUUID/dom_md/metadata Line 67: # /rhev/data-center/mnt/blockSD/sdUUID/dom_md/metadata 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 75: else: Line 76: return domPaths[0] Line 77: Line 78: Line 79: def getImagePath(sdUUID, imgUUID): Obliged by the actual layout and constraints. This is a function of the domain, unintiuitive? Line 80: return os.path.join(getDomPath(sdUUID), 'images', imgUUID) Line 81: Line 82: Line 83: def getDomUuidFromMetafilePath(metafile):
Line 316: imgList.append(os.path.basename(i)) Line 317: return imgList Line 318: Line 319: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 320: currImgDir = getImagePath(sdUUID, imgUUID) We want to avoid the domain instances. The domain path will be removed in a future patch. Line 321: dirName, baseName = self.oop.os.path.split(currImgDir) Line 322: toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName) Line 323: try: Line 324: self.oop.os.rename(currImgDir, toDelDir)
Line 317: return imgList Line 318: Line 319: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 320: currImgDir = getImagePath(sdUUID, imgUUID) Line 321: dirName, baseName = self.oop.os.path.split(currImgDir) You are right. Line 322: toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName) Line 323: try: Line 324: self.oop.os.rename(currImgDir, toDelDir) Line 325: except OSError as e:
Line 330: try: Line 331: self.oop.os.remove(volPath) Line 332: self.oop.os.remove(volPath + '.meta') Line 333: self.oop.os.remove(volPath + '.lease') Line 334: except OSError as e: Is not "any" is the "volume" (vol + md) that failed to be removed. str(e) is more compact but... Line 335: self.log.error("vol: %s can't be removed.", volPath) Line 336: try: Line 337: self.oop.os.rmdir(toDelDir) Line 338: except OSError as e:
.................................................... 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, volsByImg.keys()) Line 1281: else: Line 1282: dom.deleteImage(sdUUID, imgUUID, volsByImg) See next patch. 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/lvm.py Line 1175: def lvPath(vgName, lvName): Line 1176: return os.path.join("/dev", vgName, lvName) Line 1177: Line 1178: Line 1179: def lvDmDev(vgName, lvName): devicemapper returns /dev/dm-X. Need to build the full path /dev/mapper/vg-lv and later discard the dirname part. This is for lvs only, function of vgName and lvName and simpler. Line 1180: """Return the LV dm device. Line 1181: Line 1182: returns: dm-X Line 1183: If the LV is inactive there is no dm device
.................................................... File vdsm/storage/sd.py Line 140: LEASE_BLOCKS = 2048 Line 141: Line 142: UNICODE_MINIMAL_VERSION = 3 Line 143: Line 144: storage_repository = config.get('irs', 'repository') Actually this config is accessed in a lot of places. Doing it in hsm implies that a parameter will percolate all the levels. This is really a constant. The config should be split-ted but as you say, no in this patch. Line 145: mountBasePath = os.path.join(storage_repository, DOMAIN_MNT_POINT) Line 146: Line 147: Line 148: 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: I would prefer that you didn't submit this
(3 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): What do you mean by using "start"? Line 181: """Fill a block volume. Line 182: Line 183: This function requires an active LV. Line 184: """
Line 253: # TODO: Add the list of removed fail volumes to the exception. Line 254: log.error("Remove failed for zeroed volumes: %s", e) Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Why not log "toDelete" instead "of "volUUIDs" which contain whole volumes? Line 258: return Line 259: Line 260: Line 261: class VGTagMetadataRW(object):
.................................................... File vdsm/storage/fileSD.py Line 63: Line 64: Line 65: def getDomPath(sdUUID): Line 66: # /rhev/data-center/mnt/export-path/sdUUID/dom_md/metadata Line 67: # /rhev/data-center/mnt/blockSD/sdUUID/dom_md/metadata I think the comment above is misleading. The domPath returned by this function should be:
/rhev/data-center/mnt/export-path/sdUUID /rhev/data-center/mnt/blockSD/sdUUID 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) == 0:
-- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: (5 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 221: zerofds = {} Line 222: poller = select.poll() Line 223: for volUUID in volUUIDs: Line 224: proc = _zeroVolume(sdUUID, volUUID) Line 225: fd = proc.stdout.fileno() I see in http://docs.python.org/2/library/select.html#select.poll.register that file objects are valid, too, as args, but I'm not sure of the importance of this here. Line 226: zerofds[fd] = ProcVol(proc, volUUID) Line 227: poller.register(fd, select.EPOLLHUP) Line 228: Line 229: # Wait until all the asyncs procs return
Line 236: if not proc.wait(0): Line 237: continue Line 238: else: Line 239: poller.unregister(fd) Line 240: zerofds.pop(fd) Saggi, we needs to clear up zerofds here, or else we get into an infinite loop. Line 241: if proc.returncode != 0: Line 242: log.error("zeroing %s/%s failed. Zero and remove this " Line 243: "volume manually. rc=%s %s", Line 244: sdUUID, vol, proc.rc, proc.stderr)
Line 253: # TODO: Add the list of removed fail volumes to the exception. Line 254: log.error("Remove failed for zeroed volumes: %s", e) Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) toDelete is reset on every while-loop iteration, and thus quite meaningless here. Line 258: return Line 259: Line 260: Line 261: class VGTagMetadataRW(object):
.................................................... File vdsm/storage/hsm.py Line 1268: allVols = dom.getAllVolumes() Line 1269: volsByImg = sd.getVolsOfImage(allVols, imgUUID) Line 1270: # Do not validate if forced. Line 1271: if not misc.parseBool(force) and not dom.isBackup() \ Line 1272: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): comment added instead. Line 1273: msg = "Cannot delete shared image %s. volImgs: %s" \ Line 1274: % (imgUUID, volsByImg) Line 1275: raise se.CannotDeleteSharedVolume(msg) Line 1276:
.................................................... File vdsm/storage/sd.py Line 140: LEASE_BLOCKS = 2048 Line 141: Line 142: UNICODE_MINIMAL_VERSION = 3 Line 143: Line 144: storage_repository = config.get('irs', 'repository') actually, this nasty config.get('irs', 'repository') is performed in this module's import already. Line 145: mountBasePath = os.path.join(storage_repository, DOMAIN_MNT_POINT) Line 146: Line 147: Line 148: 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: No score
(2 inline comments)
.................................................... Commit Message Line 6: Line 7: BZ#836161 - Rewrite of deleteImage(). Line 8: Line 9: Volume operations should be done at the SD level to avoid Line 10: retrieving static data multiple times from disk. I get no privilege to access the information from bz#836161. Can you introduce more about what the static data are here? Why was the data retrieved multiple times from disk when volume operations were done at the pool level? Line 11: Added lvm.lvPhysDev() returning the dm-X for active LVs. Line 12: Use this to get active LV size without issue a lvm command. Line 13: Line 14: Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695
.................................................... File vdsm/storage/blockSD.py Line 253: # TODO: Add the list of removed fail volumes to the exception. Line 254: log.error("Remove failed for zeroed volumes: %s", e) Line 255: Line 256: Line 257: log.debug("VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) We can move this log into the while-loop. Line 258: return Line 259: Line 260: Line 261: class VGTagMetadataRW(object):
-- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 913: if i.startswith(blockVolume.TAG_PREFIX_IMAGE) ] Line 914: return images Line 915: Line 916: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 917: return deleteVolumes(sdUUID, volsImgs) deleteImage() are implemented in both BlockStorageDomain and FileStorageDomain classes. I think a blank deleteImage() should be added into StorageDomain class for readability. Line 918: Line 919: def zeroImage(self, sdUUID, imgUUID, volsImgs): Line 920: return zeroImgVolumes(sdUUID, imgUUID, volsImgs) Line 921:
-- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 10: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) Line 196: return p Line 197: Line 198: Line 199: def zeroImgVolumes(sdUUID, imgUUID, volUUIDs): You need to accept a stop condition in case we need to abort the operation (previously vars.task.aborting was passed). If stop condition is true then we need to try and kill the process. Line 200: ProcVol = namedtuple("ProcVol", "proc, vol") Line 201: # Put a sensible value for dd zeroing a 128 M or 1 G chunk and lvremove Line 202: # spent time. Line 203: ZEROING_TIMEOUT = 60000 # [miliseconds]
-- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 9: (16 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 173: for k, v in res.iteritems()) Line 174: Line 175: Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) Still a volume level method even though there is a class called domain. Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): Line 181: """Fill a block volume.
Line 185: dm = lvm.lvDmDev(sdUUID, volUUID) Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB - Megabyte shouldn't be in constants. It shouldn't even be a constant IMO because it's not a value that fluctuates. Your actual constant is ZERO_OP_CHUNK_SIZE or something along the same vain.
- Also this should be a parameter to the method Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID),
Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS rounding errors Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count)
Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) - Don't use tuples when passing arguments to execCmd Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False)
Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", - DD invocation should be somewhere else - don't concatenate tuples. Have it be a list and use list.extend() Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) Line 196: return p
Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), - you mangling the path even though there is a method for that in lvm.py Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) Line 196: return p Line 197:
Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count) - You are not using direct IO Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) Line 196: return p Line 197: Line 198:
Line 200: ProcVol = namedtuple("ProcVol", "proc, vol") Line 201: # Put a sensible value for dd zeroing a 128 M or 1 G chunk and lvremove Line 202: # spent time. Line 203: ZEROING_TIMEOUT = 60000 # [miliseconds] Line 204: log.debug("sd: %s, LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Log errors that don't say anything are frowned upon. Just printing the args everytime is useless. Line 205: # Prepare for zeroing Line 206: try: Line 207: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), Line 208: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID)))
Line 203: ZEROING_TIMEOUT = 60000 # [miliseconds] Line 204: log.debug("sd: %s, LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 205: # Prepare for zeroing Line 206: try: Line 207: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), If you fail you need to deactivate. Line 208: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 209: except se.StorageException as e: Line 210: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 211: volUUIDs)
Line 222: zerofds = {} Line 223: poller = select.poll() Line 224: for volUUID in volUUIDs: Line 225: proc = _zeroVolume(sdUUID, volUUID) Line 226: fd = proc.stdout.fileno() Still not fixed Line 227: zerofds[fd] = ProcVol(proc, volUUID) Line 228: poller.register(fd, select.EPOLLHUP) Line 229: Line 230: # Wait until all the asyncs procs return
Line 256: sdUUID, toDelete, exc_info=True) Line 257: Line 258: Line 259: log.debug("finished with VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 260: return Still here, still meaningless Line 261: Line 262: Line 263: class VGTagMetadataRW(object): Line 264: log = logging.getLogger("storage.Metadata.VGTagMetadataRW")
.................................................... File vdsm/storage/fileSD.py Line 60: Line 61: return True Line 62: Line 63: Line 64: def getDomPath(sdUUID): This is still here. Fix in SDC or don't fix at all. Line 65: pattern = os.path.join(sd.mountBasePath, '*', sdUUID) Line 66: # Warning! You need a global proc pool big as the number of NFS domains. Line 67: domPaths = getProcPool().glob.glob(pattern) Line 68: if len(domPaths) == 0:
Line 72: else: Line 73: return domPaths[0] Line 74: Line 75: Line 76: def getImagePath(sdUUID, imgUUID): Why is this not a function of the domain object Line 77: return os.path.join(getDomPath(sdUUID), 'images', imgUUID) Line 78: Line 79: Line 80: def getDomUuidFromMetafilePath(metafile):
.................................................... File vdsm/storage/hsm.py Line 1268: allVols = dom.getAllVolumes() Line 1269: volsByImg = sd.getVolsOfImage(allVols, imgUUID) Line 1270: # on data domains, images should not be deleted if they are templates Line 1271: # being used by other images. Line 1272: if not misc.parseBool(force) and not dom.isBackup() \ Still not moved to the top of the method Line 1273: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): Line 1274: raise se.CannotDeleteSharedVolume("Cannot delete shared image" Line 1275: "%s. volImgs: %s" % (imgUUID, volsByImg)) Line 1276:
Line 1269: volsByImg = sd.getVolsOfImage(allVols, imgUUID) Line 1270: # on data domains, images should not be deleted if they are templates Line 1271: # being used by other images. Line 1272: if not misc.parseBool(force) and not dom.isBackup() \ Line 1273: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): Comments are not a substitute for proper coding but I'll let that slide if danken pushes that in Line 1274: raise se.CannotDeleteSharedVolume("Cannot delete shared image" Line 1275: "%s. volImgs: %s" % (imgUUID, volsByImg)) Line 1276: Line 1277: # zeroImage will delete zeroed volumes at the end.
.................................................... File vdsm/storage/lvm.py Line 1175: def lvPath(vgName, lvName): Line 1176: return os.path.join("/dev", vgName, lvName) Line 1177: Line 1178: Line 1179: def lvDmDev(vgName, lvName): Use proper device mapper operations Line 1180: """Return the LV dm device. Line 1181: Line 1182: returns: dm-X Line 1183: If the LV is inactive there is no dm device
-- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 11: I would prefer that you didn't submit this
I commented on patchset 9 but things got pushed while I was reviewing
-- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Eduardo has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 9: (13 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 173: for k, v in res.iteritems()) Line 174: Line 175: Line 176: def deleteVolumes(sdUUID, vols): Line 177: lvm.removeLVs(sdUUID, vols) No. Deletion of volumes is a SD function since there is no metadata (nor added value) in our Volume class. Line 178: Line 179: Line 180: def _zeroVolume(sdUUID, volUUID): Line 181: """Fill a block volume.
Line 185: dm = lvm.lvDmDev(sdUUID, volUUID) Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Discuss it with Danken. Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID),
Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Wrong. There are no block devices that are not 512 multiples. And in any case is preferable not overflow zeros. Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count)
Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Why? Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False)
Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Wrong. The use of tuples is safer in this case. Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) Line 196: return p
Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), You are right. Line 194: "bs=%s" % BS, "count=%s" % count) Line 195: p = misc.execCmd(cmd, sudo=False, sync=False) Line 196: return p Line 197:
Line 200: ProcVol = namedtuple("ProcVol", "proc, vol") Line 201: # Put a sensible value for dd zeroing a 128 M or 1 G chunk and lvremove Line 202: # spent time. Line 203: ZEROING_TIMEOUT = 60000 # [miliseconds] Line 204: log.debug("sd: %s, LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Discuss it with Danken. Line 205: # Prepare for zeroing Line 206: try: Line 207: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), Line 208: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID)))
Line 203: ZEROING_TIMEOUT = 60000 # [miliseconds] Line 204: log.debug("sd: %s, LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 205: # Prepare for zeroing Line 206: try: Line 207: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), No. We should to understand the status. Deactivate can fail. And possibly the operation will be retried shortly. Line 208: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 209: except se.StorageException as e: Line 210: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, Line 211: volUUIDs)
Line 222: zerofds = {} Line 223: poller = select.poll() Line 224: for volUUID in volUUIDs: Line 225: proc = _zeroVolume(sdUUID, volUUID) Line 226: fd = proc.stdout.fileno() Nothing to fix. Read Danken comment. Line 227: zerofds[fd] = ProcVol(proc, volUUID) Line 228: poller.register(fd, select.EPOLLHUP) Line 229: Line 230: # Wait until all the asyncs procs return
Line 256: sdUUID, toDelete, exc_info=True) Line 257: Line 258: Line 259: log.debug("finished with VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 260: return This is here as a sentinel that the while was exited normally. Line 261: Line 262: Line 263: class VGTagMetadataRW(object): Line 264: log = logging.getLogger("storage.Metadata.VGTagMetadataRW")
.................................................... File vdsm/storage/fileSD.py Line 60: Line 61: return True Line 62: Line 63: Line 64: def getDomPath(sdUUID): SDC will be removed. The whole concept is wrong. Line 65: pattern = os.path.join(sd.mountBasePath, '*', sdUUID) Line 66: # Warning! You need a global proc pool big as the number of NFS domains. Line 67: domPaths = getProcPool().glob.glob(pattern) Line 68: if len(domPaths) == 0:
Line 72: else: Line 73: return domPaths[0] Line 74: Line 75: Line 76: def getImagePath(sdUUID, imgUUID): Because it is a function. Have no dependencies on a blockSD instance. Function sets are modules, not classes. Line 77: return os.path.join(getDomPath(sdUUID), 'images', imgUUID) Line 78: Line 79: Line 80: def getDomUuidFromMetafilePath(metafile):
.................................................... File vdsm/storage/hsm.py Line 1269: volsByImg = sd.getVolsOfImage(allVols, imgUUID) Line 1270: # on data domains, images should not be deleted if they are templates Line 1271: # being used by other images. Line 1272: if not misc.parseBool(force) and not dom.isBackup() \ Line 1273: and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): Again, Danken does not agree with you. Line 1274: raise se.CannotDeleteSharedVolume("Cannot delete shared image" Line 1275: "%s. volImgs: %s" % (imgUUID, volsByImg)) Line 1276: Line 1277: # zeroImage will delete zeroed volumes at the end.
-- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Eduardo has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 9: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 256: sdUUID, toDelete, exc_info=True) Line 257: Line 258: Line 259: log.debug("finished with VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 260: return Also remember that the function name is in the log. (A.B) Line 261: Line 262: Line 263: class VGTagMetadataRW(object): Line 264: log = logging.getLogger("storage.Metadata.VGTagMetadataRW")
-- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: (1 inline comment)
.................................................... Commit Message Line 6: Line 7: BZ#836161 - Rewrite of deleteImage(). Line 8: Line 9: Volume operations should be done at the SD level to avoid Line 10: retrieving static data multiple times from disk. Can you check again to see if you can access the bug? It looks public to me and I see no reason for it not to be available. Line 11: Added lvm.lvPhysDev() returning the dm-X for active LVs. Line 12: Use this to get active LV size without issue a lvm command. Line 13: Line 14: Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695
-- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 913: if i.startswith(blockVolume.TAG_PREFIX_IMAGE) ] Line 914: return images Line 915: Line 916: def deleteImage(self, sdUUID, imgUUID, volsImgs): Line 917: return deleteVolumes(sdUUID, volsImgs) Eduardo is an adamant warrior against virtual functions in Python. No chance he'd agree to that... Line 918: Line 919: def zeroImage(self, sdUUID, imgUUID, volsImgs): Line 920: return zeroImgVolumes(sdUUID, imgUUID, volsImgs) Line 921:
-- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 9: (4 inline comments)
.................................................... File vdsm/storage/blockSD.py Line 185: dm = lvm.lvDmDev(sdUUID, volUUID) Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB no idea why I'm a side to this. Line 190: count = size / BS Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID),
Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS but here BS is much much bigger than 512. The tail of the files would not be erased. Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count)
Line 256: sdUUID, toDelete, exc_info=True) Line 257: Line 258: Line 259: log.debug("finished with VG:%s LVs: %s, img: %s", sdUUID, volUUIDs, imgUUID) Line 260: return I think that Saggi frowns upon the meaningless "return" in the end. Line 261: Line 262: Line 263: class VGTagMetadataRW(object): Line 264: log = logging.getLogger("storage.Metadata.VGTagMetadataRW")
.................................................... File vdsm/storage/fileSD.py Line 72: else: Line 73: return domPaths[0] Line 74: Line 75: Line 76: def getImagePath(sdUUID, imgUUID): I do not see any evil in classmethods.. Line 77: return os.path.join(getDomPath(sdUUID), 'images', imgUUID) Line 78: Line 79: Line 80: def getDomUuidFromMetafilePath(metafile):
-- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12: I would prefer that you didn't submit this
we should discuss the trailing partial block which this implementation does not override. How about another dd with a skip and a small bs?
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12:
Dan, I am not familiar with partial blocks, care to elaborate? All LVs vdsm creates are in multiples of 128MB Default LVM extent size is 4MB. In order to reach 16TB LVs we'll have to double the extent size and we'll probably just make it 1GB in size (then reach a max size of 64TB per LV. All of these align perfectly to 1MB.
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 9: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 186: size = multipath.getDeviceSize(dm) # Bytes Line 187: # TODO: Change for zero 128 M chuncks and log. Line 188: # 128 M is the vdsm extent size default Line 189: BS = constants.MEGAB # 1024 ** 2 = 1 MiB Line 190: count = size / BS correcting myself: BS is 1MiB, but it is still an integer factor of our chunk size (128MiB) so this division would always be exact. Line 191: cmd = tuple(constants.CMD_LOWPRIO) Line 192: cmd += (constants.EXT_DD, "iflag=%s" % misc.DIRECTFLAG, "if=/dev/zero", Line 193: "of=%s" % os.path.join("/dev", sdUUID, volUUID), Line 194: "bs=%s" % BS, "count=%s" % count)
-- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12: Looks good to me, but someone else must approve
heh, my message crossed with that of Ayal.
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 206: try: Line 207: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), Line 208: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 209: except se.StorageException as e: Line 210: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, you say pre-zeroing failed but you go on as if it didn't? Line 211: volUUIDs) Line 212: # Following call to changelv is separate since setting rw permission on an Line 213: # LV fails if the LV is already set to the same value, hence we would not Line 214: # be able to differentiate between a real failure of deltag/addtag and one
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12: Looks good to me, approved
One comment inline can be fixed in subsequent patch.
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Eduardo has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12: (1 inline comment)
.................................................... File vdsm/storage/blockSD.py Line 206: try: Line 207: lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), Line 208: ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) Line 209: except se.StorageException as e: Line 210: log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, If the activation failed the zeroing will fail shortly anyway. If the retagging failed, anyway is worth to zeroing the volumes. In the worst case a totally or partial blanked image will remain. Anyway the user wanted to removed it anyway. Line 211: volUUIDs) Line 212: # Following call to changelv is separate since setting rw permission on an Line 213: # LV fails if the LV is already set to the same value, hence we would not Line 214: # be able to differentiate between a real failure of deltag/addtag and one
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Shu Ming has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 8: (1 inline comment)
.................................................... Commit Message Line 6: Line 7: BZ#836161 - Rewrite of deleteImage(). Line 8: Line 9: Volume operations should be done at the SD level to avoid Line 10: retrieving static data multiple times from disk. It is Okay for me now. Line 11: Added lvm.lvPhysDev() returning the dm-X for active LVs. Line 12: Use this to get active LV size without issue a lvm command. Line 13: Line 14: Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695
-- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Haim Ateya has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
Patch Set 12: Verified
-- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: BZ#836161 - Rewrite of deleteImage(). ......................................................................
BZ#836161 - Rewrite of deleteImage().
Volume operations should be done at the SD level to avoid retrieving static data multiple times from disk. Added lvm.lvDmDev() returning the dm-X for active LVs. Use this to get active LV size without issue a lvm command.
Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 Signed-off-by: Eduardo ewarszaw@redhat.com --- M vdsm/storage/blockSD.py M vdsm/storage/fileSD.py M vdsm/storage/fileVolume.py M vdsm/storage/hsm.py M vdsm/storage/image.py M vdsm/storage/lvm.py M vdsm/storage/sd.py 7 files changed, 175 insertions(+), 21 deletions(-)
Approvals: Ayal Baron: Looks good to me, approved Haim Ateya: Verified Dan Kenigsberg: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/8506 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Haim Ateya hateya@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
vdsm-patches@lists.fedorahosted.org