Fix division-by-zero on import #4792
Conversation
pulpcore/app/tasks/export.py
Outdated
| @@ -453,7 +453,7 @@ def pulp_export(exporter_pk, params): | |||
| if the_export.validated_chunk_size: | |||
| chunk_size = the_export.validated_chunk_size | |||
| else: | |||
| chunk_size = 0 | |||
| chunk_size = os.path.getsize(tarfile_fp) | |||
There was a problem hiding this comment.
This is on the export side. Do we want to account for this on the import side too, so we can handle existing exports?
There was a problem hiding this comment.
Existing imports aren't a huge concern, but I'm definitely not against defense in depth. But which approach to use?
We could set it null and have the import side figure it out. Or we could have the import side not use ChunkedFile when there's no chunks, or 1 chunk. Or we could have the ChunkedFile ignore the chunking logic in some circumstances.
Separately, we could also make chunk_size required if you make a chunked export, or we could set a default value. Currently if there is no chunk size provided, there will just be one chunk.
There was a problem hiding this comment.
I'd be fine with any of these solutions.
There was a problem hiding this comment.
I'd be fine with any of these solutions.
My $0.02 would be to do this same thing on the import-side - if chunked_size==0 set it to filesize. That addresses the issue, and lets the rest of the codeflow work without needing special-casing.
|
After some discussion with the Katello folks, the most convenient thing to do at the moment is to merge this without tests and follow up with those tests in a separate PR. So that's what I'll do. |
| "meta": { | ||
| "chunk_size": chunk_size, | ||
| "file": os.path.basename(tarfile_fp), | ||
| "global_hash": tarfile_hash, |
There was a problem hiding this comment.
global_hash and file are no longer used in the import path anywhere
| "checksum_type": checksum_type, | ||
| }, | ||
| "files": {}, | ||
| } | ||
|
|
||
| if the_export.validated_chunk_size: | ||
| table_of_contents["meta"]["chunk_size"] = the_export.validated_chunk_size |
There was a problem hiding this comment.
Set chunk_size only if it was provided
And clean up vestigial code closes pulp#4777
Backport to 3.39: 💚 backport PR created✅ Backport PR branch: Backported as #4863 🤖 @patchback |
Backport to 3.43: 💚 backport PR created✅ Backport PR branch: Backported as #4864 🤖 @patchback |
closes #4777
TODO: still working on the tests, it's a bit difficult to piece together how the fixtures interact...