-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with any of these solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...