Delete associated files from the storage #844
Conversation
|
Attached issue: https://pulp.plan.io/issues/7316 |
lubosmj
force-pushed
the
handle-files-removal-7316
branch
from
a8b5f89
to
a11116a
Compare
pulpcore/app/models/content.py
Outdated
| @@ -299,6 +301,14 @@ def storage_path(self, name): | |||
|
|
|||
| file = models.FileField(null=False, upload_to=storage_path, max_length=255) | |||
|
|
|||
| @hook(BEFORE_DELETE) | |||
There was a problem hiding this comment.
It needs to be after delete. I'm sorry. You will end in an inconsistent state in case of a rolled_back transaction.
what about:
def delete(self):
with transaction.atomic() as ta:
ta.on_commit(lambda: delete_from_storage(self.file.name))
super().delete()
There was a problem hiding this comment.
I'm wondering django didn't come up with this.
There was a problem hiding this comment.
This is what django-cleanup does.
There was a problem hiding this comment.
The problem is that when the model is being removed because of ondelete.CASCADE, then the method model.delete() is not invoked: https://docs.djangoproject.com/en/3.0/ref/models/fields/#django.db.models.CASCADE.
There was a problem hiding this comment.
Seems like django-cleanup resolves all the mentioned issues.
lubosmj
force-pushed
the
handle-files-removal-7316
branch
from
a11116a
to
ed87b6d
Compare
lubosmj
changed the title
lubosmj
force-pushed
the
handle-files-removal-7316
branch
3 times, most recently
from
107c69b
to
acf7266
Compare
lubosmj
force-pushed
the
handle-files-removal-7316
branch
from
acf7266
to
6b70938
Compare
| @@ -4,6 +4,7 @@ aiofiles | |||
| backoff | |||
| Django~=2.2.14 # LTS version, switch only if we have a compelling reason to | |||
| django-currentuser~=0.5.1 | |||
| django-cleanup~=5.0.0 | |||
There was a problem hiding this comment.
Does this work with cascaded delete?
+1 for not reinventing it
There was a problem hiding this comment.
Yes, it works in both cases.
There was a problem hiding this comment.
A drawback (or perhaps benefit) of django-cleanup is devs/plugin writers must specify a decorator if they do not want their FileField files deleted. I'm conflicted about this approach because while I don't like the implicit nature of it, I do find it surprising that django doesn't delete files when models get deleted.
CC @pulp/core for more visibility.
There was a problem hiding this comment.
If we do use django-cleanup we can remove these:
https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/upload.py#L49-L58
https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/content.py#L98-L107
There was a problem hiding this comment.
And given @lubosmj 's insight it will also work properly for cascaded deletes.
pulpcore/app/settings.py
Outdated
| @@ -65,6 +65,8 @@ | |||
| "rest_framework", | |||
| # pulp core app | |||
| "pulpcore.app", | |||
| # the cleanup config has to be placed last | |||
| 'django_cleanup.apps.CleanupConfig', | |||
pulpcore/app/models/content.py
Outdated
| a temporary file. It also accepts `class:django.core.files.File`. | ||
| file (models.FileField): The stored file. This field accepts `class:django.core.files.File`. | ||
| The field can be set using an absolute path to a temporary file. Note that the content | ||
| of the temporary file will not be automatically copied to the storage backend. |
There was a problem hiding this comment.
How is it transferred then?
Maybe:
Note that the content of the temporary file will only copied to the storage backend once save() is called. It will be immutable afterwards.
There was a problem hiding this comment.
It is not copied. It is not transferred at all.
In [1]: import tempfile
In [2]: with tempfile.NamedTemporaryFile("ab") as tf:
...: temp_file = PulpTemporaryFile(file=tf.name)
...: temp_file.save()
...:
In [3]: PulpTemporaryFile.objects.all()
Out[3]: <QuerySet [<PulpTemporaryFile: pk=db5b57ba-1f65-4f0c-85b5-6d01c84b7d77>]>
In [4]: PulpTemporaryFile.objects.all()[0].file
Out[4]: <FieldFile: /tmp/tmp7ttftw60>
In [5]: PulpTemporaryFile.objects.all()[0].file.read()
OSError: File does not exist: tmp/tmp7ttftw60
There was a problem hiding this comment.
I'm confused. Isn't this how it's supposed to work in the first place? Also should the file not be in the django-storage location after saving?
There was a problem hiding this comment.
Also i get:
SuspiciousFileOperation: The joined path (/tmp/tmpxko75ex9) is located outside of the base path component (/var/lib/pulp)
There was a problem hiding this comment.
Oh wow, so this code does nothing?
There was a problem hiding this comment.
If we use ArtifactFileField then it should move it into place. I'll open an issue for this.
There was a problem hiding this comment.
We decided to have FileField instead: #793 (comment)
There was a problem hiding this comment.
So the example we provide does not even work?
How is this PulpTemporaryFile supposed to be used?
There was a problem hiding this comment.
I believe this works if you pass in a File object (or rather a PulpTemporaryUploadedFile). What doesn't work is if you pass in a file path to a file on disk. We need to fix the latter.
CHANGES/plugin_api/7316.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Added the django-cleanup handlers for removing temporary files from the storage | |||
There was a problem hiding this comment.
I would probably make this a .removal or .feature. Or better yet, also add a .feature saying that files for FileFields will automatically be cleaned up using django-cleanup.
|
Also, I think we can remove these methods with this change: https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/upload.py#L49-L58 https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/content.py#L98-L107 |
lubosmj
force-pushed
the
handle-files-removal-7316
branch
2 times, most recently
from
03bf4f7
to
62433a1
Compare
closes #7316
lubosmj
force-pushed
the
handle-files-removal-7316
branch
from
62433a1
to
95a60c2
Compare
closes #7316