Change in vdsm[master]: vdsm: added functionality to teardownImage when disk has bee...

nsoffer at redhat.com nsoffer at redhat.com
Sun Mar 1 07:32:29 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: vdsm: added functionality to teardownImage when disk has been deleted
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/38241/1//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: added code to teardownImage in hsm.py so that teardownImage reports
Line 10: "Volume does not exist" for a previously deleted disk, where it
Line 11: previously simply reported "OK" - teardownImage now checks to see
Line 12: if volume exists before attempting to delete it
This change vdsm api, and can break backward compatibility. Is this behavior documented in the schema and missing from the implementation?

We should be very careful with changing the behavior of vdsm. There may be users counting on teardownImage() not failing and safe to use in cleanup context.

What is need for this change? what is the real world problem that this solve?
Line 13: 
Line 14: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1184718
Line 15: Change-Id: Ia929dfdc78ccaa736033e41a77bce861d5a27769


https://gerrit.ovirt.org/#/c/38241/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3270
Line 3271
Line 3272
Line 3273
Line 3274
If we should fail - and I suspect we should not - the failure should be in dom.deactivateImage() or in some lower layer.


Line 3270:         """
Line 3271:         vars.task.getSharedLock(STORAGE, sdUUID)
Line 3272: 
Line 3273:         dom = sdCache.produce(sdUUID)
Line 3274:         allVols = dom.getAllVolumes()
This is expensive operation, listing all volumes in this domain, can take multiple seconds on systems with lot of volumes.
Line 3275:         # Filter volumes related to this image
Line 3276:         imgVolumes = sd.getVolsOfImage(allVols, imgUUID).keys()
Line 3277: 
Line 3278:         if volUUID not in imgVolumes:


Line 3275:         # Filter volumes related to this image
Line 3276:         imgVolumes = sd.getVolsOfImage(allVols, imgUUID).keys()
Line 3277: 
Line 3278:         if volUUID not in imgVolumes:
Line 3279:             raise se.VolumeDoesNotExist(volUUID)
Failing here is wrong, we may leave behind stale symbolic links to the missing volume.
Line 3280: 
Line 3281:         dom.deactivateImage(imgUUID)
Line 3282: 
Line 3283:     @public


-- 
To view, visit https://gerrit.ovirt.org/38241
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia929dfdc78ccaa736033e41a77bce861d5a27769
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Candace Sheremeta <cshereme at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list