Eduardo has posted comments on this change.
Change subject: One shot teardown.
......................................................................
Patch Set 8: (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):
We should avoid issuing getAllVols (i.e. lvs) in a low level, or multiple times.
Probably the cmd was already executed before in the flow.
Will be better without image and SDCache.
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()
deactivateVolumes is no sense for anything but VG based.
Sadly this (file) function is actually abused and not fixed in this patch.
We need the image volumes list and getAllVolumes is the same operation.
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 agree. This race was considered and is by comments 1-4 the template deactivation will
fail.
The whole fix for BZ#811880 here is violating the SPM uniqness principle.
Moving this part elsewhere will remove the need for activate/deactivate here solving this
race.
Changing a qcow header when the VM is running seems not to be the right thing to do.
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 "
We already seen that failing to deactivate an LV can lead to DC. Really we don't
expect to the headers to be wrong and in any case this log will printed once.
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",
In the original.
Really may be critical and for sure not okay.
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 <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Haim Ateya <hateya(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>