Eduardo has posted comments on this change.
Change subject: BZ#836161 - Rewrite of deleteImage().
......................................................................
Patch Set 8: (16 inline comments)
By danken request.
....................................................
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)
Volumes are entities that belong to the domain.
What is "public" here? This is an internal interface.
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):
Then don't even start.
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),
The image is locked.
We are removing the image (all the associates volumes) then if "one else" uses
it, you have a problem that for sure should not be resolved here.
The only reason for activate these volumes is because we need to zero them
before deleting. What is the point of deactivating volumes that will be removed
immediately? (*). Waste more lvm operations?
(*) Be quiet, we use -f.
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"))
lvm.setrwLV operates in a unique LV and we want to change a bunch at once.
If it fails it checks if the volume is already writable, with another lvm
operation and reload contention.
Here the leaf is probably already writable (and will fail) when the internal
volumes not, and we want make all of them writable at once without extra
operations. Therefore setrwLV is not pluralized.
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 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)]
Please make your suggestion shorter. Something that avoids you get confused.
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):
Sadly seen in the wild. And the code manages it. And the whole operation
succeed. Is more robust this way.
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)
Wrong.
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 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)
We want to know if all the volumes were deleted. The log will be improved.
An else clause is very confusing for you too.
Line 258: return
Line 259:
Line 260:
Line 261: class VGTagMetadataRW(object):
....................................................
File vdsm/storage/fileSD.py
Line 61:
Line 62: return True
Line 63:
Line 64:
Line 65: def getDomPath(sdUUID):
sdc is one of the main reasons of the scale issues.
We are not reading redundant metadata and this is trying to be a function
instead to build another redundant instance.
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):
Obliged by the actual layout and constraints.
This is a function of the domain, unintiuitive?
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)
We want to avoid the domain instances. The domain path will be removed in a
future patch.
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)
You are right.
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:
Is not "any" is the "volume" (vol + md) that failed to be removed.
str(e) is more compact but...
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 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)
See next patch.
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 returns /dev/dm-X.
Need to build the full path /dev/mapper/vg-lv and later discard the dirname
part.
This is for lvs only, function of vgName and lvName and simpler.
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')
Actually this config is accessed in a lot of places.
Doing it in hsm implies that a parameter will percolate all the levels.
This is really a constant.
The config should be split-ted but as you say, no in this patch.
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
