Do not leave 0-byte multipart files in Cloud Storage #602
Conversation
|
When I tried reproducing this, I would start |
That's a good question. I'm not sure off-hand. This Pull Request addresess the issue where multi-part uploads are initiated but never marked as completed. In Noobaa/RHOCS, this will be shown as "incomplete" uploads. If Quay is creating those objects in the If the files you're talking about are finalized, then that requires a different mechanism to go through and "garbage collect" the uploads folder. I'm not sure if anything like that exists in Quay. I would consider that a separate issue, if not. |
kurtismullins
changed the title
| @@ -396,6 +396,13 @@ def initiate_chunked_upload(self): | |||
| def stream_upload_chunk(self, uuid, offset, length, in_fp, storage_metadata, content_type=None): | |||
| self._initialize_cloud_conn() | |||
|
|
|||
| # Do not create zero-byte objects. | |||
| # TODO: length=-1 appears to be a special case and used in tests. Why? Document it. | |||
There was a problem hiding this comment.
Length -1 means to write the whole in_fp stream, vs just length bytes if set.
There was a problem hiding this comment.
Good to know. Thank you!
| @@ -408,6 +415,12 @@ def stream_upload_chunk(self, uuid, offset, length, in_fp, storage_metadata, con | |||
| if bytes_written > 0: | |||
| new_metadata[_CHUNKS_KEY].append(_PartUploadMetadata(chunk_path, offset, bytes_written)) | |||
|
|
|||
| # Do not leave zero-byte chunks in Object Storage | |||
| # TODO: Determine a clean way to eliminate calling this function when it's not needed. | |||
| else: | |||
There was a problem hiding this comment.
Woudn't the above check for zero-length prevent these zero bytes objects to be written in the first place. i.e Don't need to remove them anymore.
There was a problem hiding this comment.
I don't believe so. In the case where it's explicitly defined as 0 bytes when this file is called, it is short-circuited.
In other cases, I believe that the -1 you explained earlier leads to the creation and writing of a 0 byte file. I'm not exactly sure if that's how they get created, but that initial short-circuit did not fix it for all cases during my personal testing.
| upload_id, 0, -1, BytesIO(data), metadata | ||
| ) | ||
| assert len(data) == bytes_written | ||
| assert mock_remove.call_count == 1 |
There was a problem hiding this comment.
Likewise, shouldn't need to remove anymore, since it's not being written anymore?
|
@kurtismullins Is this just an issue with Nooba. What is causing the zero-bytes to be written in the first place? |
@kleesc IIRC, Ivan has witnessed this on AWS S3. I can also see it in a GCP Object Storage bucket I'm using for a long-running Quay deployment. re: Why is this happening? The chunks are created optimistically even if they don't contain data. I believe that question (and solution) will require a larger body of work (refactor) to solve correctly. This PR is primarily a "hack" work-around until a real solution can be prioritized and implemented. |
|
Resolved with #705 |
Issue: https://issues.redhat.com/browse/PROJQUAY-1123
Changelog: fix: Do not leave orphaned 0-byte multi-part uploads in Cloud Storage
Docs: n/a
Testing:
Details: