Ayal Baron has posted comments on this change.
Change subject: One shot teardown.
......................................................................
Patch Set 8: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File vdsm/storage/blockSD.py
Line 1020: Activate all the volumes listed in volUUIDs
Line 1021: """
Line 1022: lvm.activateLVs(self.sdUUID, volUUIDs)
Line 1023:
Line 1024: def deactivateVolumes(self, imgUUID, allVols):
this method should not get allVols, only imgUUID and then get allVols itself.
Line 1025: """
Line 1026: Deactivate all the volumes belonging to the image.
Line 1027:
Line 1028: imgUUID: the image to be deactivated.
....................................................
File vdsm/storage/hsm.py
Line 3191: """
Line 3192: vars.task.getSharedLock(STORAGE, sdUUID)
Line 3193:
Line 3194: dom = sdCache.produce(sdUUID)
Line 3195: allVols = dom.getAllVolumes()
getAllVolumes here is redundant for file domains and should be done inside
deactivateVolumes
Line 3196: # Filter volumes related to this image
Line 3197: dom.deactivateVolumes(imgUUID, allVols)
Line 3198:
Line 3199: @public
....................................................
File vdsm/storage/imageRepository/formatConverter.py
Line 293: except se.CannotDeactivateLogicalVolume:
Line 294: log.error("Unable to teardown the image %s, this
error is "
Line 295: "not critical since the volume might be
in use",
Line 296: imgUUID, exc_info=True)
Line 297: # Deactivate the template
I believe this entire section (deactivate the template) is a bug.
img.teardown did not teardown the template, it skips it when creating the chain
in getChain:
205 # Build up the sorted parent -> child chain
206 while not srcVol.isShared():
^^^^^^^^^^^^^^^^^^^^^^^^
207 chain.insert(0, srcVol)
208
209 if srcVol.getParent() == volume.BLANK_UUID:
210 break
211
212 srcVol = srcVol.getParentVolume()
213
214 self.log.info("sdUUID=%s imgUUID=%s chain=%s ", sdUUID, imgUUID,
chain)
215 return chain
Even in teardown itself there is a comment:
"
255 # Do not deactivate the template yet (might be in use by an
other vm)
256 # TODO: reference counting to deactivate when unused"
This part will introduce a regression where preparing a disk for one VM based
on the same template as this will cause a race where you can deactivate the
template right after it was activated by the prepare and cause that VM to fail
running (we had this bug in the past...)
Line 298: tImgUUID = allImages[imgUUID]
Line 299: try:
Line 300: domain.deactivateVolumes(tImgUUID, allVolumes)
Line 301: except se.CannotDeactivateLogicalVolume:
Line 298: tImgUUID = allImages[imgUUID]
Line 299: try:
Line 300: domain.deactivateVolumes(tImgUUID, allVolumes)
Line 301: except se.CannotDeactivateLogicalVolume:
Line 302: log.error("Unable to teardown template %s, this
error is "
this should be log.warning
Line 303: "not critical since the volume might be
in use",
Line 304: tImgUUID, exc_info=True)
Line 305:
Line 306: log.debug("Finalizing the storage domain upgrade from version
%s to "
Line 299: try:
Line 300: domain.deactivateVolumes(tImgUUID, allVolumes)
Line 301: except se.CannotDeactivateLogicalVolume:
Line 302: log.error("Unable to teardown template %s, this
error is "
Line 303: "not critical since the volume might be
in use",
s/this error.*/this is probably okay and is due to it being in use/
Line 304: tImgUUID, exc_info=True)
Line 305:
Line 306: log.debug("Finalizing the storage domain upgrade from version
%s to "
Line 307: "version %s for domain %s", currentVersion,
targetVersion,
--
To view, visit http://gerrit.ovirt.org/4234
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6a4e99cf3a657affb4f5e96aa1ac1978a297da
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: Elad Ben Aharon <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Yeela Kaplan <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
