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

Candace Sheremeta has uploaded a new change for review.

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

vdsm: added functionality to teardownImage when disk has been deleted

added code to teardownImage in hsm.py so that teardownImage reports
"Volume does not exist" for a previously deleted disk, where it
previously simply reported "OK" - teardownImage now checks to see
if volume exists before attempting to delete it

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1184718
Change-Id: Ia929dfdc78ccaa736033e41a77bce861d5a27769
Signed-off-by: Candace Sheremeta 
---
M vdsm/storage/hsm.py
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/41/38241/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 8c75277..45e6634 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -3271,6 +3271,13 @@
         vars.task.getSharedLock(STORAGE, sdUUID)

dom = sdCache.produce(sdUUID)
+        allVols = dom.getAllVolumes()
+        # Filter volumes related to this image
+        imgVolumes = sd.getVolsOfImage(allVols, imgUUID).keys()
+
+        if volUUID not in imgVolumes:
+            raise se.VolumeDoesNotExist(volUUID)
+
         dom.deactivateImage(imgUUID)

@public

[e-mail] has posted comments on this change.

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

Patch Set 1:

* Update tracker::#1184718::OK
* Check Bug-Url::OK
* Check Public Bug::#1184718::OK, public bug
* Check Product::#1184718::OK, Correct product Red Hat Enterprise Virtualization Manager
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

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

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16148/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/15348/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16318/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created_staging/1124/ : FAILURE

[Nir Soffer]

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

Candace Sheremeta has posted comments on this change.

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

Patch Set 1: Code-Review-1

We need more information from the reporter.

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

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

Patch Set 1:

Following the conversation in bugzilla, seems like we aren't going to "fix" the issue.

Please abandon this patch.

[Allon Mureinik]

Allon Mureinik has abandoned this change.

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

Abandoned

Abandoned, as per conversation on bugzilla.

[e-mail] has posted comments on this change.

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

Patch Set 1:

* Update tracker::#1184718::OK