[upload] Update upload subsystem for the new Red Hat upload API - #4448
[upload] Update upload subsystem for the new Red Hat upload API#4448jcastill wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jcastill
added
Kind/RedHat
3c16dac to
24290ed
Compare
|
Some samples of the PR execution. Please excuse the wall of text, but I think it's important to show how the upload will look like, specially since the tests we can run are only in an stage environment and we have very limited options to test this and get stage accounts:
|
| self.ui_log.error( | ||
| f"Presigned URL count mismatch: expected {total_chunks}, " | ||
| f"got {len(presigned_parts)}") | ||
| self._abort_upload(self._build_abort_data()) |
There was a problem hiding this comment.
You always call self._abort_upload(self._build_abort_data()) with the same argument. To pass less data, does it make sense to have the _abort_upload method without an argument, and have there directly
json=self._build_abort_data(),
instead of current
json=abort_data,
? (well, also debug message at the beginning would have to be updated accordingly).
Self devil's advocate: this approach is coherent with complete_upload method where the info about upload is specific in each case. So wither implementation (current or my proposal) are similarly "valid", in my eyes.
There was a problem hiding this comment.
This is a good point. I can change it now or post-release in September, to make sure I can test the change in prod. Which approach do you prefer?
| """Complete an upload session | ||
|
|
||
| :param complete_data: Information about the upload session | ||
| :returns: Response object onsuccess, None on failure |
There was a problem hiding this comment.
nitpick: s/onsuccess/on success/.
There was a problem hiding this comment.
... sorry. Will change this
| return None | ||
|
|
||
| def _get_upload_session_details(self, archive, archive_size, total_chunks): | ||
| """Get the details of the upload session lke attachmentId, |
| # Lets reset session details before each attempt so that stale or | ||
| # expired presigned URLs from a previous call are not reused | ||
| # if this request fails and upload_archive retries. | ||
| self._upload_session_details = {} |
There was a problem hiding this comment.
great explanation, thanks. I was about to ask why we cant "cache" the info.
There was a problem hiding this comment.
Luckily we can test in stage and I could catch this sooner
| upload_url = ( | ||
| f"{self.RH_API_HOST}" | ||
| f"{self.RH_HYDRA_ATTACHMENTS_PATH}/upload") |
There was a problem hiding this comment.
worth be coherent with other requests.* invocations and have base = .. and upload_url = ..?
(maybe we can save the base into the class instance at init, as self.base, to prevent code duplication? it has pros and cons, though..)
There was a problem hiding this comment.
what cons do you see? I only see pros tbh
There was a problem hiding this comment.
Hmm.. right no cons seen here, so I am in favour of the self.base change :)
| Number of threads to use for multipart uploads. When an archive exceeds a | ||
| certain size, defined in the targets, it is uploaded in parallel chunks | ||
| using this many threads. Default is 4. | ||
| .TP |
There was a problem hiding this comment.
Worth explaining here the:
- boundaries (1-17)
- effect (higher value means faster upload, but consumes more bandwidth and more memory (128MB per thread - but dont we copy the data in memory 2 times? isnt it 256ish MB? I would need to experiment but dont have means for it..)
(then we can drop the mention of memory footprint from cmdline help, maybe?)
There was a problem hiding this comment.
Ack, good point. I can add a vague note about the memory usage, and can try to create a simple python script to test memory usage. But I want to make sure that the users know the trade offs
There was a problem hiding this comment.
what about something like this?
.B \--upload-threads THREADS
Number of threads to use for multipart uploads (1\-16, default: 4).
When an archive exceeds a certain size, defined in the targets, it is
uploaded in parallel chunks using this many threads. Higher values
speed up the upload but consume more bandwidth and memory, as each
thread holds a 128 MiB chunk in memory.
There was a problem hiding this comment.
I'm personally of the opinion this shouldn't be user-controllable. On the security side of things there's definitely room for abuse here (though, admittedly, that's a very small concern here today given the infrastructure RH in particular would have in place, more of a concern for if/when multipart gets exposed to more generic targets), but also I don't think users really gain a net-benefit from having this control.
Uploads perceived as "slow" likely wouldn't be realistically helped all that much by more threads, as they'd be more directly limited/influenced by the end user's bandwidth.
| and use it for uploads. | ||
|
|
||
| .TP | ||
| .B \--upload-threads THREADS |
| ) | ||
| response = self.simple_http_upload(archive=files, verify=verify) | ||
| if response is None: | ||
| raise Exception( |
There was a problem hiding this comment.
Can't we raise some less generic exception class? Or pass the response details in the exception text?
| headers={'Content-Type': | ||
| 'application/octet-stream'}, | ||
| verify=verify, | ||
| timeout=TIMEOUT_DEFAULT) |
There was a problem hiding this comment.
Uploading a 4.9GB file can timeout on slow connections (5 GB / 300s = 17 MB/s minimum). Worth using similar formula like on another place (max(TIMEOUT_DEFAULT, archive_size // (512 * 1024)))?
There was a problem hiding this comment.
ok. I'll reword the TODO as well because I think it's worth keeping an eye on the timeout here.
A quick note, I asked if we can use multipart for less than 5Gb or reduce the limit size and was told that no, so we are stuck here with a big request
| The complete/abort lifecycle only applies to multipart | ||
| uploads for files exceeding _max_size_request. | ||
|
|
||
| :param archive: Dict containing the file tuple under the 'file' key |
There was a problem hiding this comment.
Why dict and not the value itself (or rather it's [1] file_obj directly)?
There was a problem hiding this comment.
it's a leftover of the old way. I'll rework this
| def multipart_upload(self, archive): | ||
| """Manage the multipart upload for files exceeding _max_size_request. | ||
|
|
||
| :param archive: Dict containing the file tuple under the 'file' key |
There was a problem hiding this comment.
Why dict and not the value itself (or rather it's [1] file_obj directly)?
| self._upload_thread_count = thread_count | ||
|
|
||
| # Calculate number of chunks for the multipart upload | ||
| chunk_size = 128 * 1024 * 1024 # 128 MiB |
There was a problem hiding this comment.
Just a thought: worth moving this constatnt close to _max_size_request declaration?
There was a problem hiding this comment.
ok, will make it more readable.
| # Calculate number of chunks for the multipart upload | ||
| chunk_size = 128 * 1024 * 1024 # 128 MiB | ||
| archive_size = os.path.getsize(file_path) | ||
| total_chunks = (archive_size + chunk_size - 1) // chunk_size |
There was a problem hiding this comment.
The + chunk_size - 1 part means you round-divide to up, not down (like // does), right? Maybe worth commenting / explaining it?
There was a problem hiding this comment.
The idea of rounding up is to make sure we cover partial chunks, but yes I can add a note explaining this
| f"Unexpected error while uploading chunk: {e}") | ||
| failed_chunks.append(chunk_info) | ||
|
|
||
| if expiration and datetime.now(timezone.utc) >= expiration: |
There was a problem hiding this comment.
There is a race condition:
- some chunk gets uploaded just before expiration, still on time,
- we get successful result,
- then the expiration time happens
- then we evaluate this check and claim the upload failed
(if some chunk was still pending, that is correct behaviour. if it was the latest chunk we wrongly claim it failed)
Not sure how better test this, though.
There was a problem hiding this comment.
Not sure how to test this either, it's quite a corner case, but not the kind of bug we'd like to wait and see because we are talking about very big file attachments, so I'll see if I can add a check and make it more robust
|
I am mostly OK with the PR, thanks for the lengthy work. Many comments I raised are rather subjective opinion or "what about" ideas of improvement - I dont insist on most of them. I feel there are a few places here and there, where the code might get improved to be more robust, but I dont want to be too paranoic and polute the PR by my fears. Rather the daily use will reveal them, if there is any real wekaness. |
I welcome the paranoia because that keeps me on my toes, and well, that's what reviews are for. My only concern is that I want something that works when the release day comes, and then after a couple of weeks of testing and verifying that works we can fix some of your concerns. Does that make sense? |
24290ed to
bbd05a4
Compare
The Red Hat Customer Portal is migrating to a new upload API, so the upload target needs to be updated to use the new endpoints and flow. Archives up to 5 GB are uploaded in a single HTTP PUT request, while archives exceeding that threshold are split into 128 MiB chunks and uploaded in parallel. As a consequence of the new upload limits, we don't perform fallback to SFTP depending on upload size, only when the upload fails or the user selects it explicitly. The multipart flow handles session initialization, per-chunk retry, session expiration checks, and automatic abort on failure. A new --upload-threads option (1-16, default 4) controls the number of parallel upload threads per chunk used. Related: RHEL-182746 Signed-off-by: Jose Castillo <jcastillo@redhat.com>
bbd05a4 to
9803b00
Compare
pmoravec
left a comment
pmoravec
left a comment
There was a problem hiding this comment.
LGTM. We might rename the get session details method and raise less generic exception than Exception, but that is minor stuff.
TurboTurtle
left a comment
TurboTurtle
left a comment
There was a problem hiding this comment.
One comment on the new option.
| Number of threads to use for multipart uploads. When an archive exceeds a | ||
| certain size, defined in the targets, it is uploaded in parallel chunks | ||
| using this many threads. Default is 4. | ||
| .TP |
There was a problem hiding this comment.
I'm personally of the opinion this shouldn't be user-controllable. On the security side of things there's definitely room for abuse here (though, admittedly, that's a very small concern here today given the infrastructure RH in particular would have in place, more of a concern for if/when multipart gets exposed to more generic targets), but also I don't think users really gain a net-benefit from having this control.
Uploads perceived as "slow" likely wouldn't be realistically helped all that much by more threads, as they'd be more directly limited/influenced by the end user's bandwidth.
Im dont have strong preference in having this option-able. I think the option can help in some specific scenarios (big bandwidth and impatient customer => increase the threads, or low bandwidth => decrease them), but I agree the option would be rather misused than properly used. So no prob making the number as hard coded. Is that the only feedback / is the PR good otherwise? (we would like to get it merged soon, cc @TurboTurtle and/or @bmr-cymru or @arif-ali ). |
4 participants
The Red Hat Customer Portal is migrating to a new upload API, so the upload target needs to be updated to use the new endpoints and flow.
Archives up to 5 GB are uploaded in a single HTTP PUT request, while archives exceeding that threshold are split into 128 MiB chunks and uploaded in parallel. As a consequence of the new upload limits, we don't perform fallback to SFTP depending on upload size, only when the upload fails or the user selects it explicitly.
The multipart flow handles session initialization, per-chunk retry, session expiration checks, and automatic abort on failure. A new --upload-threads option (1-16, default 4) controls the number of parallel upload threads per chunk used.
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines