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