Fixes deletion of UploadChunk when Upload deletes

[bmbouter]

According to the Django docs, the on_delete=models.CASCADE option does
not call Model.delete(), but it does call the Model.post_delete() signal
handler.

This replaces the UploadChunk.delete() with an
UploadChunk.post_delete() signal handler which correctly handles
the UploadChunk.file file deletion.

closes #7316

[bmbouter]

When running the reproducer without this patch I see leftover files. When running it with this patch I do not.

[daviddavis]

A few questions:

• Is a test possible?
• Does UploadChunk still need to extend HandleTempFilesMixin?
• Should we apply this pattern to other HandleTempFilesMixin models?
• Is using django-lifecycle not an option?

[pulpbot]

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

[bmbouter]

A few questions:

* Is a test possible?

I was thinking about a test, but what seemed problematic is I can't predict the pk the file will get and there isn't a way to do it from the API. I could have the tools check /var/lib/pulp/media/upload/ before and after, or use the total count, but that won't work if we ever parallelize the tests or have other upload tests that run before this one. This is why I skipped it, but what do you think I should do.

[bmbouter]

A few questions:

* Is using django-lifecycle not an option?

I tried this actually and it is not. I tried the AFTER_DELETE hook, and the issue is that it also tries to watch the model for the delete() call. Only Django's signals will work because the ORM only fires it's signals, not django-lifecycle's.

[daviddavis]

That makes sense and I agree. I don't think a test is easy/possible.

[bmbouter]

A few questions:

* Does UploadChunk still need to extend HandleTempFilesMixin?

We can remove it, and I'm pushing a version now that will show the tests also passing. UploadChunk doesn't benefit from the HandleTempFilesMixin.delete() with the new post_delete handler handling the file deletion. In my testing just now with the reproducer script, it also doesn't benefit from HandleTempFilesMixin.save() because the file is already closed already. I checked using self.file.closed which was True.

[bmbouter]

A few questions:
* Should we apply this pattern to other HandleTempFilesMixin models?

I think we should not for a few reasons. I did an audit and UploadChunk is the only model that both has a FileField or ArtifactFileField and has a ForeignKey with a cascade delete. That means in all the other cases the Model.delete() will be called. The signal approach removes the delete from the model which I think is a downside to using it.

What I would like to do though is clean up in a variety of ways. I think we could do away with a lot of our code and have the same functionality. Should I file an issue for this?

[daviddavis]

I think we should not for a few reasons. I did an audit and UploadChunk is the only model that both has a FileField or ArtifactFileField and has a ForeignKey with a cascade delete. That means in all the other cases the Model.delete() will be called. The signal approach removes the delete from the model which I think is a downside to using it.

👍

What I would like to do though is clean up in a variety of ways. I think we could do away with a lot of our code and have the same functionality. Should I file an issue for this?

I'm not sure I follow.

[dralley]

What I would like to do though is clean up in a variety of ways. I think we could do away with a lot of our code and have the same functionality. Should I file an issue for this?

+1

[bmbouter]

In terms of cleanup I mean that I don't think we need our custom storage backend, and I also don't think we need TemporaryDownloadedFile. Also maybe we could do without the mixin in so many places...