Change in vdsm[master]: BZ#836161 - Rewrite of deleteImage().
ewarszaw at redhat.com
ewarszaw at redhat.com
Mon Nov 5 08:33:48 UTC 2012
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 at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Haim Ateya <hateya at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
More information about the vdsm-patches
mailing list