Remove subrepos during distribution tree removal.

[goosemania]

Cleanup orphaned subrepos in a migration.

closes #7440
https://pulp.plan.io/issues/7440

[pulpbot]

Attached issue: https://pulp.plan.io/issues/7440

[goosemania]

• I can't override the Model.delete method because it's not called for bulk or queryset removals.
• To use pre-delete, I had to remove on_delete=PROTECT for repositories, otherwise I can't remove them before DistributionTree itself. It's bad, PROTECT there is not to remove subrepos without a DistributionTree.
• To use post_delete, I'd need to remove on_delete=CASCADE for variants and addons, otherwise, they are already removed and I don't have information which subrepos to remove.
• Alternatively, to use post_delete, I could lookup all the subrepos without addon/variant reference. But it would be searching all the rpm repos through backward reference for every distribution tree removal.

[bmbouter]

Maybe consider using django-lifecycle hook. As of 3.6, all models can use this https://rsinger86.github.io/django-lifecycle/

[goosemania]

I'll read more about it. Does it solve the problem or order and cascade/protect rules?

[bmbouter]

It does not, it's mainly a syntactical sugar

[goosemania]

Addressed in the next push

[goosemania]

I'm taking that back :)

with this patch:

diff --git a/pulp_rpm/app/models/distribution.py b/pulp_rpm/app/models/distribution.py
index 988ef37d..14be224c 100644
--- a/pulp_rpm/app/models/distribution.py
+++ b/pulp_rpm/app/models/distribution.py
@@ -3,8 +3,11 @@ from logging import getLogger
 
 from django.db import models
 from django.db import transaction
-from django.db.models.signals import post_delete
-from django.dispatch import receiver
+
+from django_lifecycle import (
+    AFTER_DELETE,
+    hook,
+)
 
 from pulpcore.plugin.models import (
     Artifact,
@@ -91,6 +94,24 @@ class DistributionTree(Content):
     discnum = models.IntegerField(null=True)
     totaldiscs = models.IntegerField(null=True)
 
+    @hook(AFTER_DELETE)
+    def cleanup_subrepos(self):
+        """
+        Remove subrepos when a DistributionTree is being removed.
+
+        """
+        with transaction.atomic():
+            for variant in self.variants.all():
+                subrepo = variant.repository
+                variant.delete()
+                if subrepo:
+                    subrepo.delete()
+            for addon in self.addons.all():
+                subrepo = addon.repository
+                addon.delete()
+                if subrepo:
+                    subrepo.delete()
+
     def repositories(self):
         """
         Return the subrepos in this DistributionTree.
@@ -331,22 +352,3 @@ class Variant(BaseModel):
             "packages",
             "distribution_tree",
         )
-
-
-@receiver(post_delete, sender=DistributionTree)
-def cleanup_subrepos(sender, instance, **kwargs):
-    """
-    Remove subrepos when a DistributionTree is being removed.
-
-    """
-    with transaction.atomic():
-        for variant in instance.variants.all():
-            subrepo = variant.repository
-            variant.delete()
-            if subrepo:
-                subrepo.delete()
-        for addon in instance.addons.all():
-            subrepo = addon.repository
-            addon.delete()
-            if subrepo:
-                subrepo.delete()

I'm getting django.db.utils.IntegrityError: update or delete on table "rpm_distributiontree" violates foreign key constraint "rpm_addon_distribution_tree_id_91a3fbf6_fk_rpm_distr" on table "rpm_addon"

[goosemania]

I'm leaning towards keeping the signal. Django lifecycle doesn't seem to be just a syntactic sugar, it seem not to use signals at all https://github.com/rsinger86/django-lifecycle/blob/master/django_lifecycle/mixins.py#L138-L141

[goosemania]

An alternative approach from #django channel which @bmbouter shared is to override Model.delete() and Queryset.delete().

[dralley]

Needs a changelog entry, but LGTM pending CI results.

[goosemania]

I tried the Model.delete() and QuerySet.delete() approach and I learned that when Django performs CASCADE delete the .delete() methods are not called. https://code.djangoproject.com/ticket/29398

So it seems like signals is the way.

I improved the original approach I believe. I brought back the CASCADE for variants and addons and have simpler subrepo removal code.

Let me know what you all think.

[dralley]

Seems reasonable

[goosemania]

I'm merging to unblock the release.
If anyone has any reservations for the chosen approach, we can refactor any time later.