Pulp

I updated the description per your comment. I also added you to the 'Contributor' group which will allow you to update issues in the future. If there are others that also want to edit issues in pulp.plan.io please let me know what their usernames are.

The description of the responsibilities of _release_resource is spot on. I also want more reliability to ensure tasks are not left running and ReservedResource records are left in DB indefinitely. Let's talk about how to accomplish that.

Adding acks_late by itself won't really help the failure scenario you describe because if an exception is raised the task is still acknowledged [0]. We could add acks_late, make it idempotent, and have a Python failure cause a Celery retry [1]. From my reading of the retry feature it will dispatch a new task with the same task id to the same queue which will cause it to be processed out of order. The out of order processing is safe, but it will still have the task in the running state and the ReservedResource record will exist for longer than it should. It would probably be a good improvement though.

I want to put some more detail on what happens when the "mongo query fails". It will raise an Exception that will be a descendant of the PyMongo exception AutoReconnect. There is a Pulp configuration option called unsafe_autoretry which wraps all mongo operations in a try-except block and catches any AutoReconnect and retries the operation. If enabled this would cause the scenario of a mongo failure in _release_resource to wait and retry until it resolved. This would effectively prevent the failure scenario described. It's an option. The feature is called "unsafe retry" for a reason because there are some potentially negative side effects. I wanted to highlight how the failure scenario is related to that failure handling feature of Pulp.

One last thing to note is that whenever the worker who has the "ghost task" (the one that didn't get cleaned up due to the failure in _release_resource()) will be marked as cancelled upon worker restart. So it's an issue, but it wouldn't be a permanent data quality issue because at some point the worker will restart. This is a related FYI. I still think making an improvement here would be good.

Given all ^ do you think a change should be made to _release_resource to use retry if a Python failure occurs, set acks_late, and ensure the task is idempotent?

[0]: http://docs.celeryproject.org/en/latest/faq.html#faq-acks-late-vs-retry
[1]: http://docs.celeryproject.org/en/latest/userguide/tasks.html#retrying

I want more reliability here, but what about this type of failure scenario. The failures to guard against here are Python exceptions, probably raised due to some database availability issue. If we add acks_late and retry=True and make it idempotent couldn't this happen?

0. The _release_resource task is running
1. The database becomes unavailable such that when you query it you get a Python exception (subclass or pymongo.exceptions.AutoReconnect).
2. The _release_resource task fails and is set back to the queue because of retry=True
3. Go back to step 0

^ will cause an infinite loop until an administrator intervenes. It may be better for the task to just fail, or the user can use the Pulp feature unsafe_retry=True. What do you think about this case?

Thanks for noting the severity also. I'm going to drop it to low because it seems we both see it that way.

To share some of the thinking I had while developing this, I used acks_late on _queue_reserved_task() to guard against the resource manager dying (like power loss or OOM) while processing a task. I can't guard against that type of failure with _release_resource task because of Issue #489.

It's nice to talk to someone who has looked at the work I've put together so closely.