Hello Ayal Baron, Igor Lvovsky,

I'd like you to do a code review. Please visit

http://gerrit.ovirt.org/9208

to review the following change.

Change subject: BZ#836161 - Rewrite of deleteImage() complement. ......................................................................

BZ#836161 - Rewrite of deleteImage() complement.

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: I6c8c9baecee1ad7aa7789026e533aa254a738097 Complements: I304ff5cd70186ffc9789cd1ac9337efa6c5ff695 Signed-off-by: Eduardo ewarszaw@redhat.com Reviewed-on: https://gerrit.eng.lab.tlv.redhat.com/3140 Reviewed-by: Ayal Baron abaron@redhat.com Tested-by: Igor Lvovsky ilvovsky@redhat.com --- M vdsm/storage/blockSD.py M vdsm/storage/fileSD.py M vdsm/storage/hsm.py 3 files changed, 63 insertions(+), 11 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/08/9208/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py index 26d4380..506059c 100644 --- a/vdsm/storage/blockSD.py +++ b/vdsm/storage/blockSD.py @@ -207,8 +207,10 @@ lvm.changelv(sdUUID, volUUIDs, (("-a", "y"), ("--deltag", imgUUID), ("--addtag", sd.REMOVED_IMAGE_PREFIX + imgUUID))) except se.StorageException as e: - log.debug("SD %s, Image %s pre zeroing ops failed", sdUUID, imgUUID, - volUUIDs) + log.error("Can't activate or change LV tags in SD %s. " + "failing Image %s pre zeroing operation for vols: %s. %s", + sdUUID, imgUUID, volUUIDs, e) + raise # 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 @@ -915,11 +917,39 @@ if i.startswith(blockVolume.TAG_PREFIX_IMAGE) ] return images

+ def rmDCVolLinks(self, imgPath, volsImgs): + for vol in volsImgs: + lPath = os.path.join(imgPath, vol) + removedPaths = [] + try: + os.unlink(lPath) + except OSError as e: + self.log.warning("Can't unlink %s. %s", lPath, e) + else: + removedPaths.append(lPath) + self.log.debug("removed: %s", removedPaths) + return tuple(removedPaths) + + def rmDCImgDir(self, imgUUID, volsImgs): + imgPath = os.path.join(self.domaindir, sd.DOMAIN_IMAGES, imgUUID) + self.rmDCVolLinks(imgPath, volsImgs) + try: + os.rmdir(imgPath) + except OSError: + self.log.warning("Can't rmdir %s. %s", imgPath, exc_info = True) + else: + self.log.debug("removed image dir: %s", imgPath) + return imgPath + def deleteImage(self, sdUUID, imgUUID, volsImgs): - return deleteVolumes(sdUUID, volsImgs) + toDel = tuple(vName for vName, v in volsImgs.iteritems() + if v.imgs[0] == imgUUID) + deleteVolumes(sdUUID, toDel) + self.rmDCImgDir(imgUUID, volsImgs)

def zeroImage(self, sdUUID, imgUUID, volsImgs): - return zeroImgVolumes(sdUUID, imgUUID, volsImgs) + zeroImgVolumes(sdUUID, imgUUID, volsImgs) + self.rmDCImgDir(imgUUID, volsImgs)

def getAllVolumes(self): """ diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index 90ffecd..c4415da 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -496,9 +496,8 @@ remove the remnants of the removed images (they could be left sometimes (on NFS mostly) due to lazy file removal """ - removedPattern = os.path.join( - self.domaindir, sd.DOMAIN_IMAGES, - sd.REMOVED_IMAGE_PREFIX + '*') + removedPattern = os.path.join(self.domaindir, sd.DOMAIN_IMAGES, + sd.REMOVED_IMAGE_PREFIX + '*') removedImages = self.oop.glob.glob(removedPattern) self.log.debug("Removing remnants of deleted images %s" % removedImages) diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py index 4d696bf..e712102 100644 --- a/vdsm/storage/hsm.py +++ b/vdsm/storage/hsm.py @@ -1308,6 +1308,8 @@ def deleteImage(self, sdUUID, spUUID, imgUUID, postZero=False, force=False): """ Delete Image folder with all volumes + + force parameter is deprecated and not evaluated. """ #vars.task.setDefaultException(se.ChangeMeError("%s" % args)) self.getPool(spUUID) #Validates that the pool is connected. WHY? @@ -1317,15 +1319,31 @@ vars.task.getSharedLock(STORAGE, sdUUID) allVols = dom.getAllVolumes() volsByImg = sd.getVolsOfImage(allVols, imgUUID) + if not volsByImg: + self.log.error("Empty or not found image %s in SD %s. %s", + imgUUID, sdUUID, allVols) + raise se.ImageDoesNotExistInSD(imgUUID, sdUUID) + # on data domains, images should not be deleted if they are templates # being used by other images. - if not misc.parseBool(force) and not dom.isBackup() \ - and not all(len(v.imgs) == 1 for v in volsByImg.itervalues()): - raise se.CannotDeleteSharedVolume("Cannot delete shared image" - "%s. volImgs: %s" % (imgUUID, volsByImg)) + isTemplateWithChildren = False + for v in volsByImg.itervalues(): + if len(v.imgs) > 1 and v.imgs[0] == imgUUID: + isTemplateWithChildren = True + break + + if not isTemplateWithChildren: + needFake = False + elif dom.isBackup(): + needFake = True + else: + raise se.CannotDeleteSharedVolume("Cannot delete shared image %s. " + "volImgs: %s" % (imgUUID, volsByImg))

# zeroImage will delete zeroed volumes at the end. if misc.parseBool(postZero): + # postZero implies block domain. Backup domains are always NFS + # hence no need to create fake template if postZero is true. self._spmSchedule(spUUID, "zeroImage_%s" % imgUUID, dom.zeroImage, sdUUID, imgUUID, volsByImg.keys()) else: @@ -1337,6 +1355,11 @@ # intended to quickly fix the integration issue with rhev-m. In 2.3 # we should use the new resource system to synchronize the process # an eliminate all race conditions + if needFake: + img = image.Image(os.path.join(self.storage_repository, spUUID)) + tName = volsByImg.iterkeys()[0] + tParams = dom.produceVolume(imgUUID, tName).getVolumeParams() + img.createFakeTemplate(sdUUID=sdUUID, volParams=tParams) self._spmSchedule(spUUID, "deleteImage_%s" % imgUUID, lambda : True)

-- To view, visit http://gerrit.ovirt.org/9208 To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange Gerrit-Change-Id: I6c8c9baecee1ad7aa7789026e533aa254a738097 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com