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 <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
