Add pre delete hook to domain for orphans #3801
Conversation
mdellweg
force-pushed
the
orphan_delete_domain
branch
2 times, most recently
from
2263b98
to
6b308e0
Compare
mdellweg
force-pushed
the
orphan_delete_domain
branch
from
6b308e0
to
b52aebe
Compare
| @@ -49,6 +49,15 @@ def get_storage(self): | |||
| def prevent_default_deletion(self): | |||
| raise models.ProtectedError("Default domain can not be updated/deleted.", [self]) | |||
|
|
|||
| @hook(BEFORE_DELETE, when="name", is_not="default") | |||
| def _cleanup_orphans_pre_delete(self): | |||
There was a problem hiding this comment.
This delete is expected to happen inside of a task, not the API, correct?
There was a problem hiding this comment.
Absolutely. You are supposed to hold the exclusive domain lock for it.
| self.content_set.filter(version_memberships__isnull=True).delete() | ||
| for artifact in self.artifact_set.all().iterator(): | ||
| # Delete on by one to properly cleanup the storage. | ||
| artifact.delete() |
There was a problem hiding this comment.
It would be nice if we could submit these deletions to a threadpool, or use async, or anything which would speed up the process relative to a sequential synchronous delete.
But that's a suggestion for another time.
|
I'm not so sure, should this be a bugfix? |
|
It could make sense as a bugfix - maybe go ahead and do this if you think we may need to backport it? |
|
Let's not do it for the sake of backporting... You can still issue an orphan cleanup prior to deleting the domain. So there's a rather easy workaround. |
| def _cleanup_orphans_pre_delete(self): | ||
| if self.content_set.exclude(version_memberships__isnull=True).exists(): | ||
| raise models.ProtectedError("There is active content in the domain.") | ||
| self.content_set.filter(version_memberships__isnull=True).delete() |
There was a problem hiding this comment.
i might be throwing a wrench in here.. but the criteria for the orphaned content/artifacts consists not only in the membership but also in the protection time https://github.com/pulp/pulpcore/blob/main/pulpcore/app/models/content.py#L176
There was a problem hiding this comment.
I know, but at this point, you intend to delete the domain (now) which would fail if anything useful (like repositories) is left. And in that condition no artifact will be deleted either. I believe it is safe to assume that at this point we do not need to wait for any expiry anymore.
There was a problem hiding this comment.
I understand what you're saying and it probably does sound logical and desirable, it's just here we're changing the concept of what an 'orphan' means. I know that right now i am being overly picky, however, same logic could be applicable to 'i want to delete the content (now) but.. oh wait..it has protection time'.
As long as it was on purpose, it's ok just leaving an explicit note on the code would have been helpful.
There was a problem hiding this comment.
I agree, i should have added a comment. It was too obvious to me. To be fair, the protection time is not on the content, but you can provide the protection time with the delete orphan call. So you can delete your content "now" if you intend to.
fixes #3800