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 <[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
