Upload chunks as separate files #914

lubosmj · 2020-09-16T17:49:46Z

closes #4498

pulpbot · 2020-09-16T17:55:16Z

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

daviddavis · 2020-09-16T21:11:46Z

I think the main problem with this solution is that it's tied to S3 and we support a variety of other storage backends besides S3 (e.g. Azure). I think the option of where to store upload chunks should be to either use the default storage backend or to override that and use the filesystem.

daviddavis · 2020-09-16T21:31:21Z

pulpcore/app/models/upload.py


+    Because of that, we are not able to dynamically change the storage used for chunked uploads
+    without applying additional migrations after performing relevant changes to the settings file.
+    Support for a callable storage is new in Django 3.1.


With this limitation in django 2, it's creating a rather complex solution to have a separate storage setting for chunks. I'm inclined to ditch the setting altogether (until django 3.1) and just store chunks in the default storage.

ipanova · 2020-09-17T15:58:35Z

I think the main problem with this solution is that it's tied to S3 and we support a variety of other storage backends besides S3 (e.g. Azure). I think the option of where to store upload chunks should be to either use the default storage backend or to override that and use the filesystem.

I agree that we should not be tied to S3 only but use the default storage backend. I am not so sure about idea of using the local fs because this will be an issue for, for example when having 100 docker puh operations simultaneously whereas layers are quire big and it might happen that local fs won't be enough

daviddavis · 2020-09-17T16:15:57Z

I agree that we should not be tied to S3 only but use the default storage backend. I am not so sure about idea of using the local fs because this will be an issue for, for example when having 100 docker puh operations simultaneously whereas layers are quire big and it might happen that local fs won't be enough

I agree but if we give users the choice of where to store their upload chunks, I don't think we should prevent them from using a filesystem.

mdellweg · 2020-09-17T16:30:08Z

I thought it would just use the temporary file storage.

daviddavis · 2020-09-17T16:41:53Z

The problem with just using temporary file storage is that it's horribly inefficient in some cases. It requires that Pulp downloads the chunks, assemble the files, and then re-uploads the chunks back into S3. This is after having already having uploaded the chunks into S3 in the first place. And it also presents the same problem @ipanova outlined in that users could potentially run out of filesystem space.

daviddavis · 2020-09-17T17:22:40Z

I agree though that using temporary storage only for now makes sense (or at least until we're on django 3.1 and can offer some configuration options). I would document the issues I describe above though so users are aware.

lubosmj · 2020-09-22T08:22:58Z

@daviddavis, speaking of the option for temporary storage (i.e. FileSystemStorage), users would have to deal with migrations again, as I mentioned in the documentation.

The only thing I can actually do, is to note that In Pulp, we do support only default_storage and FileSystemStorage for storing chunked uploads; other options are not guaranteed to work properly. Yet, I am not sure if this is you what you meant by your last comment. And of course, I will remove the wrapper around the S3 storage.

mdellweg · 2020-09-22T09:19:21Z

Sorry to say it like that, but having the user create site-local migrations is not an option at all.
We struggle a lot not to generate or cherry-pick migrations on release branches, already.

daviddavis · 2020-09-22T11:55:26Z

@lubosmj my last comment is that I think we should ditch the storage setting altogether for now and just use default_storage for chunk uploads.

@mdellweg I'm not sure what you mean by users creating "site-local migrations"

mdellweg · 2020-09-22T12:07:13Z

@daviddavis referring to this: https://github.com/pulp/pulpcore/pull/914/files#diff-f47f8ca652dac550c8bc9d449ca0d253R165

daviddavis · 2020-09-29T12:27:34Z

What happens to users that have upload chunks in their database and then they upgrade?

docs/workflows/upload-publish.rst

lubosmj · 2020-09-29T12:59:34Z

What happens to users that have upload chunks in their database and then they upgrade?

They could probably lose track of uploads since the file field is deleted by the migration. Will users perform chunked uploading during the upgrade by any chance? I thought that once an upload is finished, all users create an artifact from it; then, there should not be any uploads left behind.

I may modify the migration to move all uploads to the directory used for chunked uploads. Then, uploaded files will be referenced by UploadChunk models instead of Upload models.

daviddavis · 2020-09-29T14:25:55Z

@lubosmj I brought this topic up at the pulpcore team meeting. The consensus was to add a release note saying that users should delete all uploads before upgrading. So I think we want a XXXX.removal saying that upload chunks are being moved from the filesystem to the default storage the user has configured and users need to delete any chunked uploads before upgrading.

mdellweg · 2020-09-30T08:33:51Z

Honestly i understood that we expect the administrator to clean the diskspace after the migration.
But at least we need a note that pending uploads will not survive the upgrade.

pulpcore/app/models/upload.py

pulpcore/app/models/storage.py

daviddavis · 2020-09-30T11:33:35Z

Overall, looks good to me. Great job on this. 👍

@mdellweg or @ipanova could you also take a look?

ipanova

looks good to me, minor comments that should not be considered as a blocker.

ipanova · 2020-09-30T14:17:08Z

pulpcore/app/models/upload.py

+        upload_chunk = UploadChunk(
+            upload=self, offset=offset, size=len(chunk), sha256=current_sha256
+        )
+        upload_chunk.file.save("", ContentFile(chunk_read))


why do you have empty string here?

as discussed, we need to add name

I used the same technique like we use in PulpTemporaryFile. A chunk's filename does not depend on the provided filename, but rather on pulp_id which is known when saving the chunk.

pulpcore/app/models/upload.py

as disccused per irc more changes are needed to be added first

daviddavis · 2020-10-12T16:56:22Z

CHANGES/4498.removal

@@ -0,0 +1,2 @@
+The local file system directory used for uploaded chunks was changed and moved to the default
+storage instead. Users are encouraged to remove all uploaded files before applying this change.


small change:

s/remove all uploaded/remove all uncommitted uploaded/

daviddavis · 2020-10-12T16:59:03Z

I thought we talked about using pulp_id or uuid to store the chunks in storage and not sha256?

daviddavis · 2020-10-12T17:11:56Z

Oh, I see now you are using pulp_id_str to put it in a nested dir. I think this is probably overkill. I don't think that there will be more than a 100 chunks at any one time.

pulpcore/app/models/upload.py

ipanova · 2020-10-14T14:14:45Z

pulpcore/app/models/upload.py

+        return storage.get_upload_chunk_file_path(self.pulp_id)
+
+    file = fields.FileField(null=False, upload_to=storage_path, max_length=255)
+    sha256 = models.CharField(max_length=64, null=True)


what is the purpose of this field if it is not being used when UploadChunk is created? please whether remove it completely or use it during the creation of upload chunk.

closes #4498

lubosmj · 2020-10-16T11:42:51Z

@daviddavis , after the latest changes, nested directories are not created. However, chunks' file names are still equal to pulp_id.

daviddavis · 2020-10-16T13:14:32Z

Cool, I would probably get rid of the get_upload_chunk_file_path function too but if you think it's useful to have, then that's fine.

daviddavis

Added one optional comment. Looks good enough to merge to me.

Thanks @lubosmj!

lubosmj · 2020-10-16T19:31:55Z

Yes, I think that it is useful to have that function!

lubosmj mentioned this pull request Sep 16, 2020

Use PulpTemporaryFile to store uploaded chunks pulp/pulp_container#133

Closed

lubosmj force-pushed the not-only-s3-chunked-uploads-4498 branch from a004394 to d3bbe34 Compare September 16, 2020 18:27

daviddavis reviewed Sep 16, 2020

View reviewed changes

lubosmj force-pushed the not-only-s3-chunked-uploads-4498 branch 3 times, most recently from 22e1a8d to 45ea0ca Compare September 23, 2020 15:44

lubosmj requested a review from daviddavis September 23, 2020 18:20

daviddavis reviewed Sep 29, 2020

View reviewed changes

docs/workflows/upload-publish.rst Show resolved Hide resolved

lubosmj force-pushed the not-only-s3-chunked-uploads-4498 branch from 45ea0ca to 19a81e3 Compare September 29, 2020 20:09

lubosmj requested a review from daviddavis September 29, 2020 20:10

daviddavis reviewed Sep 30, 2020

View reviewed changes

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

daviddavis reviewed Sep 30, 2020

View reviewed changes

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

ipanova previously approved these changes Sep 30, 2020

View reviewed changes

ipanova reviewed Sep 30, 2020

View reviewed changes

mdellweg reviewed Sep 30, 2020

View reviewed changes

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

ipanova self-requested a review September 30, 2020 21:01

lubosmj force-pushed the not-only-s3-chunked-uploads-4498 branch from 19a81e3 to 643b75f Compare October 9, 2020 13:07

lubosmj requested a review from daviddavis October 9, 2020 13:42

daviddavis reviewed Oct 12, 2020

View reviewed changes

ipanova reviewed Oct 14, 2020

View reviewed changes

Upload chunks as separate files

0642584

closes #4498

lubosmj force-pushed the not-only-s3-chunked-uploads-4498 branch from 643b75f to 0642584 Compare October 16, 2020 11:10

lubosmj requested review from daviddavis and ipanova October 16, 2020 11:39

daviddavis approved these changes Oct 16, 2020

View reviewed changes

ipanova approved these changes Oct 19, 2020

View reviewed changes

dkliban merged commit e46c3ff into pulp:master Oct 20, 2020

lubosmj mentioned this pull request Oct 23, 2020

Upload chunks as separate files #981

Merged

Upload chunks as separate files #914

Upload chunks as separate files #914

lubosmj commented Sep 16, 2020

pulpbot commented Sep 16, 2020

daviddavis commented Sep 16, 2020

daviddavis Sep 16, 2020 •

edited

ipanova commented Sep 17, 2020 •

edited

daviddavis commented Sep 17, 2020

mdellweg commented Sep 17, 2020

daviddavis commented Sep 17, 2020

daviddavis commented Sep 17, 2020

lubosmj commented Sep 22, 2020

mdellweg commented Sep 22, 2020

daviddavis commented Sep 22, 2020

mdellweg commented Sep 22, 2020

daviddavis commented Sep 29, 2020

lubosmj commented Sep 29, 2020 •

edited

daviddavis commented Sep 29, 2020

mdellweg commented Sep 30, 2020

daviddavis commented Sep 30, 2020 •

edited

ipanova left a comment

ipanova Sep 30, 2020

ipanova Sep 30, 2020

lubosmj Oct 9, 2020

daviddavis Oct 12, 2020

daviddavis commented Oct 12, 2020

daviddavis commented Oct 12, 2020

ipanova Oct 14, 2020

lubosmj commented Oct 16, 2020 •

edited

daviddavis commented Oct 16, 2020

daviddavis left a comment

lubosmj commented Oct 16, 2020

		@@ -0,0 +1,2 @@
		The local file system directory used for uploaded chunks was changed and moved to the default
		storage instead. Users are encouraged to remove all uploaded files before applying this change.

Upload chunks as separate files #914

Upload chunks as separate files #914

Conversation

lubosmj commented Sep 16, 2020

pulpbot commented Sep 16, 2020

daviddavis commented Sep 16, 2020

daviddavis Sep 16, 2020 • edited

Choose a reason for hiding this comment

ipanova commented Sep 17, 2020 • edited

daviddavis commented Sep 17, 2020

mdellweg commented Sep 17, 2020

daviddavis commented Sep 17, 2020

daviddavis commented Sep 17, 2020

lubosmj commented Sep 22, 2020

mdellweg commented Sep 22, 2020

daviddavis commented Sep 22, 2020

mdellweg commented Sep 22, 2020

daviddavis commented Sep 29, 2020

lubosmj commented Sep 29, 2020 • edited

daviddavis commented Sep 29, 2020

mdellweg commented Sep 30, 2020

daviddavis commented Sep 30, 2020 • edited

ipanova left a comment

Choose a reason for hiding this comment

ipanova Sep 30, 2020

Choose a reason for hiding this comment

ipanova Sep 30, 2020

Choose a reason for hiding this comment

lubosmj Oct 9, 2020

Choose a reason for hiding this comment

daviddavis Oct 12, 2020

Choose a reason for hiding this comment

daviddavis commented Oct 12, 2020

daviddavis commented Oct 12, 2020

ipanova Oct 14, 2020

Choose a reason for hiding this comment

lubosmj commented Oct 16, 2020 • edited

daviddavis commented Oct 16, 2020

daviddavis left a comment

Choose a reason for hiding this comment

lubosmj commented Oct 16, 2020

daviddavis Sep 16, 2020 •

edited

ipanova commented Sep 17, 2020 •

edited

lubosmj commented Sep 29, 2020 •

edited

daviddavis commented Sep 30, 2020 •

edited

lubosmj commented Oct 16, 2020 •

edited