Controller delete should raise exceptions instead of safe_delete

[Amala Basha]

The safe_delete_from_backend in glance/store/__init__.py does not raise a "Not Found" after it gets a 404 and simply sends a warning and continues. As a result the end response received for the request is deceiving. The chief reason being that the controller delete also calls a safe_delete and simply logs off errors instead of doing a wild card raise

[Amala Basha (amalabasha)]

On further note, it does not seem to be raising any exception. It just logs warnings!

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/54245

[Zhi Yan Liu (lzy-dev)]

TBH Amala, I'm very interested in what's client command and code execution path you met in this "issue" context. Actually this function is a "safe" call as its named, that means call-side code should not meet any exception even store.delete() raised. If call-side code need catch that, it should call "store.delete_from_backend()".

[Amala Basha (amalabasha)]

There are plenty of situations, for instance an image to be deleted is not in the store. If this simply catches the "Not Found" without raising it, the calling request continues execution as if nothing went wrong. This does happen a lot during upload_data_to_store when a safe_kill is initiated and the image does not seem to exist!! Couple of other scenarios too.

[Zhi Yan Liu (lzy-dev)]

So Amala the root problem to me for this defect is that why those code does calling on "store.safe_delete_from_backend" but "store.delete_from_backend()"? IMO we need find out those code which expect catch exception when it require remove image bits from backend storage, and change them to using "delete_from_backend()" but "safe_delete_from_backend()". Any thoughts?

[Amala Basha (amalabasha)]

Your concern is perfectly valid. From the code, this safe_delete_from_backend seems to be called in upload_utils while an update_to_store happens and a mismatch or something requires a deletion. This seems to be fine, but safe_delete is also called during a normal controller delete as well. In such a scenario, I dont really think swallowing an exception is a good idea. Either we should change this bit of code to call delete_from_backend or just raise the appropriate exception here. Thoughts?

[Zhi Yan Liu (lzy-dev)]

@Amala Basha, I prefer go former way, thanks.

[Alex Meade (alex-meade)]

I think safe_delete_from_backend is meant to catch all the exceptions and not raise them. I think that is what 'safe' means in this context, it's kind of like a best effort cleanup if something else went wrong.

Is the real bug here that delete in the Controller should not be calling 'safe_delete_from_backend' and instead needs to call a delete that could raise a 404? What exact case does this matter? An image may not have data in the store but still have an image entity that is deleted in glance. Just trying to understand :)

[Amala Basha (amalabasha)]

Hi Alex,

Yes the real issue here is that the controller delete shouldn't ideally be doing a safe_delete as the exceptions it raises is expected to be bubbled up. If you look at the patch, we've been trying to separate the actual delete from backend from the safe_delete in a way that ensures that whatever exception is caught doesn't get lost.

 The exact scenario that I'm talking about here is where the store actually catches a 404 during a controller delete but since the exception isn't raised, the execution continues as if nothing went wrong and the end response received is a 200 OK which is deceiving.

[Ian Cordasco (icordasc)]

It seems the controller has changed only slightly since Amala's proposal (https://github.com/openstack/glance/blob/9622189e728468e0cd8b358d1ebcb417f16d449f/glance/api/v1/images.py#L1076). It's unclear to me if this is still potentially an issue without trying to reproduce this and seeing if it persists.