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