Controller delete should raise exceptions instead of safe_delete

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

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

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()".

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.

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?

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?

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

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 :)

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.

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.