automation(a)ovirt.org 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']) -- 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(a)redhat.com&gt; Gerrit-Reviewer: automation(a)ovirt.org Gerrit-HasComments: No

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(a)redhat.com&gt; Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt; Gerrit-Reviewer: automation(a)ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes

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. -- 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(a)redhat.com&gt; Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt; Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com&gt; Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt; Gerrit-Reviewer: automation(a)ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No

automation(a)ovirt.org 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 -- 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(a)redhat.com&gt; Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt; Gerrit-Reviewer: Candace Sheremeta <cshereme(a)redhat.com&gt; Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt; Gerrit-Reviewer: automation(a)ovirt.org Gerrit-HasComments: No