Introducing PulpTemporaryFile #793

fao89 · 2020-07-17T22:01:53Z

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

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

pulpbot · 2020-07-17T22:13:30Z

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

pulpcore/app/models/content.py

daviddavis · 2020-07-20T17:16:10Z

pulpcore/app/models/content.py

+            expected_digests = {"sha256": hashers["sha256"].hexdigest()}
+
+        if Artifact.objects.filter(**expected_digests).count():
+            raise ValidationError(_("Artifact already exists."))


I think that temporary files can be created even if there are Artifacts that already exist.

I did this because of this test:
https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/tests/functional/api/collection/v2/test_upload.py#L51-L52
I can update the test

No, I think the test makes sense but that we should probably leave it up to plugins to check whether an Artifact already exists before creating a PulpTemporaryFile. The thought being that some plugins might want to still create a PulpTemporaryFile in some cases even if the Artifact exists. But maybe this is not a valid use case.

cc @bmbouter

This still needs to be addressed. @bmbouter any thoughts?

Yes I expect a new PulpTemporaryFile would still be created without error even there is a corresponding Artifact that already exists also.

so what should I do with this test?
https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/tests/functional/api/collection/v2/test_upload.py#L51-L52

I think that test should receive an error on the task later when it realizes the Artifact sha256 already exists. So it would monitor the task and see "oh the task failed w/ a new error message". I think it will fail with a duplicate key on the sha256 when the Artifact.save() is called.

pulpcore/app/models/storage.py

pulpcore/app/models/content.py

CHANGES/6749.feature

daviddavis · 2020-07-20T17:31:41Z

Looks good. Don't forget to add some plugin writer docs.

pulpcore/app/models/content.py

bmbouter · 2020-07-20T20:07:11Z

pulpcore/app/models/content.py

+        self.file.delete(save=False)
+
+    @staticmethod
+    def init_and_validate(file, expected_digests=None, expected_size=None):


I didn't imagine we would have this method at all. To me, plugin writers will save a file, use the file, delete the file. If a user case comes up later we could add something then. @fao89 and @daviddavis what do you both think?

That's fine but I think the import code is using this to check the checksum digest. We'll have to move that code into pulp_ansible.

I kept it because it validates the checksum, and I like to raise failures the earliest possible

OK that's understandable. The internals need to change then though because it shouldn't be using Artifact in any places.

pulpcore/app/models/content.py

bmbouter · 2020-07-20T20:21:37Z

pulpcore/app/models/content.py

+
+        return PulpTemporaryFile(file=file)
+
+    def to_artifact(self):


Do we need this method? Maybe we do I'm not sure. I was thinking users would pass their file handle directly in as PulpTemporaryFile(file=my_file).save(). What would happen if we deleted this, and init_and_validate and then called it with PulpTemporaryFile(file=my_file).save()?

It seems pretty handy to have a method that converts a PulpTemporaryFile into an Artifact.

init_and_validate is validating checkums and size, so it would fail at some point when turning PulpTemporaryFile into Artifact, I'm trying to raise the failures earlier.
to_artifact is just a convenient method for not forgetting to call delete when you move PulpTemporaryFile to Artifact.

Yup I didn't understand it; I thought it was part of the internals. I see how it's used in the pulp_ansible PR now, and in seeing the usage I agree.

Suppose it could be a @classmethod on Artifact, a from_pulp_temporary_file(temp_file). Though the code is about equal parts Artifact and PulpTemporaryFile, so doesn't really matter.

@alikins I was thinking the same thing.

@fao89 am I reading correctly that the Artifact.init_and_validate now accepts a PulpTemporaryFile also? Should we just use that and remove this one? What do you think should be done?

Artifact is not accepting PulpTemporaryFile yet, this is alikins suggestion - I can implement it

Oh I thought it did from reading this docstring https://github.com/pulp/pulpcore/pull/793/files but I see now in the code it doesn't. +1 to having it take a PulpTemporaryFile also just to make the existing interface even more usable.

bmbouter · 2020-07-20T20:30:01Z

Can a new docs section be added indicating to plugin writers that this is available for passing file data to one or more tasks with some examples? I think it would go in a new subsection here. I think that would be named Making Temporary Files Available to Tasks.

pulpcore/app/models/storage.py

mdellweg · 2020-07-21T14:08:43Z

Is this temporary file using the django storage framework (and i think it should, because there are usecases without any other shared storage.)?

pulpcore/app/models/content.py

docs/plugins/plugin-writer/concepts/index.rst

mdellweg

I left a typo, a question and some suggestions.
But generally i really like this!

docs/glossary.rst

mdellweg · 2020-07-22T09:00:37Z

docs/plugins/plugin-writer/concepts/index.rst

+
+.. code-block:: python
+
+    # Example 1 - Saving a temporary file:


should we suggest that this is usually in the viewset?

I don't know how can I express it, any suggestion?

Maybe like

# Example: # (in the view) # variant 1 - Saving a temporary file: ... # variant 2 - --"-- with additional validation: ... # (in the task) # Using the temporary file to create an artifact: ...

docs/plugins/plugin-writer/concepts/index.rst

pulpcore/app/models/content.py

mdellweg · 2020-07-22T09:14:09Z

pulpcore/app/models/content.py

+                while True:
+                    chunk = f.read(1048576)  # 1 megabyte
+                    if not chunk:
+                        break


Suggested change

while True:

chunk = f.read(1048576) # 1 megabyte

if not chunk:

break

for chunk in iter(lambda: f.read(1048576), b""):

I think the lambda is less readable

mdellweg · 2020-07-22T09:17:06Z

pulpcore/app/models/content.py

+    file = fields.ArtifactFileField(null=False, upload_to=storage_path, max_length=255)
+
+    @staticmethod
+    def init_and_validate(file, expected_digests=None, expected_size=None, validate_artifact=False):


I am wondering whether this need to be it's own function, or if we can fold these optional validations in the __init__ method.

my idea was to mimic Artifact method

What i see is that if both of the expected_* parameters are omitted from the call this function is really the same as just instanciating a PulpTemporaryFile the old fashioned way.
Also if there is no expected digests, we are consuming reading the whole file for no additional benefit. Can you add a if expected_digests: there?

bmbouter · 2020-07-22T14:12:00Z

docs/glossary.rst

@@ -7,6 +7,10 @@ Glossary
        A file. They usually belong to a :term:`content unit<Content>` but may be used
        elsewhere (e.g. for PublishedArtifacts).

+    :class:`~pulpcore.app.models.PulpTemporaryFile`


I actually think this glossary is for end-user facing terms and I don't believe they would know about this. So my suggestion is to remove this from the glossary.

bmbouter

This looks good to me. Thank you @fao89 !

bmbouter · 2020-07-22T19:14:16Z

pulpcore/app/models/content.py

+        """
+        return storage.get_temp_file_path(self.pulp_id)
+
+    file = fields.ArtifactFileField(null=False, upload_to=storage_path, max_length=255)


This being ArtifactFileField instead of FileField will cause this extra functionality to run. Is that important or is FileField right?

From the docstring I guess we don't need ArtifactFileField

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

fao89 mentioned this pull request Jul 17, 2020

Stop using artifacts when importing collections pulp/pulp_ansible#335

Merged

fao89 force-pushed the 6749 branch from 9cf05f4 to da75ab9 Compare July 17, 2020 22:12

fao89 force-pushed the 6749 branch 2 times, most recently from b84c53f to d6de173 Compare July 18, 2020 04:35

daviddavis reviewed Jul 20, 2020

View reviewed changes

pulpcore/app/models/content.py Show resolved Hide resolved

fao89 commented Jul 20, 2020

View reviewed changes

pulpcore/app/models/content.py Outdated Show resolved Hide resolved

daviddavis reviewed Jul 20, 2020

View reviewed changes

pulpcore/app/models/storage.py Outdated Show resolved Hide resolved

daviddavis reviewed Jul 20, 2020

View reviewed changes

pulpcore/app/models/content.py Outdated Show resolved Hide resolved

daviddavis reviewed Jul 20, 2020

View reviewed changes

CHANGES/6749.feature Outdated Show resolved Hide resolved

bmbouter reviewed Jul 20, 2020

View reviewed changes

pulpcore/app/models/content.py Outdated Show resolved Hide resolved

bmbouter reviewed Jul 20, 2020

View reviewed changes

pulpcore/app/models/content.py Outdated Show resolved Hide resolved

bmbouter reviewed Jul 20, 2020

View reviewed changes

pulpcore/app/models/storage.py Outdated Show resolved Hide resolved

fao89 force-pushed the 6749 branch 3 times, most recently from 72a3631 to 3935b7b Compare July 21, 2020 14:04

alikins reviewed Jul 21, 2020

View reviewed changes

pulpcore/app/models/content.py Outdated Show resolved Hide resolved

fao89 force-pushed the 6749 branch from 3935b7b to cef789a Compare July 21, 2020 18:09

daviddavis reviewed Jul 21, 2020

View reviewed changes

docs/plugins/plugin-writer/concepts/index.rst Outdated Show resolved Hide resolved

daviddavis reviewed Jul 21, 2020

View reviewed changes

docs/plugins/plugin-writer/concepts/index.rst Outdated Show resolved Hide resolved

daviddavis reviewed Jul 21, 2020

View reviewed changes

docs/plugins/plugin-writer/concepts/index.rst Show resolved Hide resolved

fao89 force-pushed the 6749 branch 3 times, most recently from 032702e to 4608b52 Compare July 21, 2020 19:56

fao89 force-pushed the 6749 branch from 4608b52 to 2696690 Compare July 21, 2020 21:58

fao89 marked this pull request as ready for review July 21, 2020 21:58

fao89 force-pushed the 6749 branch from 2696690 to 05f4268 Compare July 21, 2020 22:01

mdellweg approved these changes Jul 22, 2020

View reviewed changes

fao89 force-pushed the 6749 branch 2 times, most recently from 71ebfb1 to a1dad08 Compare July 22, 2020 13:14

bmbouter reviewed Jul 22, 2020

View reviewed changes

fao89 force-pushed the 6749 branch from a1dad08 to 60f0b3f Compare July 22, 2020 15:11

bmbouter approved these changes Jul 22, 2020

View reviewed changes

fao89 force-pushed the 6749 branch from 60f0b3f to 5f93e61 Compare July 22, 2020 17:15

daviddavis approved these changes Jul 22, 2020

View reviewed changes

bmbouter reviewed Jul 22, 2020

View reviewed changes

Introducing PulpTemporaryFile

6b3be35

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

fao89 force-pushed the 6749 branch from 5f93e61 to 6b3be35 Compare July 22, 2020 19:57

daviddavis approved these changes Jul 22, 2020

View reviewed changes

bmbouter merged commit cbbe573 into pulp:master Jul 22, 2020

lubosmj mentioned this pull request Aug 13, 2020

Delete associated files from the storage #844

Merged


		.. code-block:: python

		# Example 1 - Saving a temporary file:

Introducing PulpTemporaryFile #793

Introducing PulpTemporaryFile #793

Conversation

fao89 commented Jul 17, 2020

pulpbot commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddavis commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Jul 20, 2020

mdellweg commented Jul 21, 2020

mdellweg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment