-
Notifications
You must be signed in to change notification settings - Fork 543
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
[redhat|policy] Check for archive size before upload #3534
[redhat|policy] Check for archive size before upload #3534
Conversation
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. |
I tried to add the check as early as possible, before uploading, but making sure it was a RH only check, so I chose to use the upload_archive() in redhat.py. Another option was to add it to the 'if' just below where it is, returning a boolean, but I thought that could add more complexity to that if. |
@@ -232,6 +232,9 @@ class RHELPolicy(RedHatPolicy): | |||
_upload_url = RH_SFTP_HOST | |||
_upload_method = 'post' | |||
_device_token = None | |||
# Max size for an http single request is 1Gb | |||
_max_size_request = 1073741824 |
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 the byte-precise granted size Red Hat Portal allows, right? (rather, one byte less due to size >= self._max_size_request
)
I just want to prevent sporadic cases when a border-line-sized sosreport passes the check but fails to be uploaded - rather go the other way, failover to SFTP even for sizes slightly below the limit.
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.
Yes, that's right. I checked with a bit more than 1Gb but can check the exact size as well in case we need to change the value.
sos/policies/distros/redhat.py
Outdated
# There's really no need to transform the size to Gb, | ||
# so we don't need to call any size converter implemented | ||
# in tools.py | ||
if len(str(size)) >= 10 and (size >= self._max_size_request): |
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.
What?
The raw size comparison is the only thing needed.
sos/policies/distros/redhat.py
Outdated
_("Size of archive is bigger than Red Hat Customer Portal " | ||
f"limit for uploads of {self._max_size_human_readable} via " | ||
"sos http upload. \n") | ||
) |
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.
It seems strange to stick 1Gb
in a variable for it only to be used once in an f-string, rather than just making the warning message contain that static value.
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.
Sure. I'll change that.
sos/policies/distros/redhat.py
Outdated
def upload_archive(self, archive): | ||
"""Override the base upload_archive to provide for automatic failover | ||
from RHCP failures to the public RH dropbox | ||
""" | ||
try: | ||
self.check_file_too_big(archive) |
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 should be gated by checking that we are in fact trying to upload to the RH customer portal - the default is for the policy to set the target, but it's entirely possible for it to not be RH_API_HOST
.
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.
Ack, but it will be good to know about the scenarios where the target is not RH_API_HOST when going through sos/policies/distros/redhat.py
a912e55
to
75e7dba
Compare
I think I've addressed all the notes, but apologies if I forgot anything! This is how the output looks right now:
We present GiB in "Size" and I print G in the warning. It may be worth changing the value I print so it's consistent. |
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 looks mostly good to me now, just would like to have the upload URL check moved to be the gating logic on the size check as noted (so that we don't perform the size check if a user has specified a different --upload-url
.
sos/policies/distros/redhat.py
Outdated
# so we don't need to call any size converter implemented | ||
# in tools.py | ||
if (size >= self._max_size_request) and\ | ||
self.get_upload_url().startswith(RH_API_HOST): |
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.
The check against the upload URL should be done prior to the call to check_file_too_big()
, otherwise we are doing the size comparison needlessly.
The Red Hat Customer Portal has a max limit for single http requests of 1Gb. From sos side, we never checked this and so customers had to wait for the whole upload to be attempted before receiving an error from the portal. This patch attempts to stop the upload before it starts, informing customers of such limit, and switching to secure FTP where there's no limit. Related: RHEL-22732 Signed-off-by: Jose Castillo <jcastillo@redhat.com>
75e7dba
to
d324f10
Compare
I changed this a bit from the previous iteration. I considered two options. First one - have the check for the RH URL and the file size in the same if:
Second option - modify the function to return upload URL, and assign it to self.upload_url after gatekeeping, like this:
I've chosen the second one to do the size check after the URL check, and not at the same time. And in both cases I considered, I removed the check for user and password because after the change to token auth these we always None. |
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.
LGTM
_("Size of archive is bigger than Red Hat Customer Portal " | ||
"limit for uploads of " | ||
f"{convert_bytes(self._max_size_request)} " | ||
" via sos http upload. \n") | ||
) |
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.
Is it worth mentioning here "File will be uploaded to SFTP server instead"?
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.
ACK with current change, optionally worth to extend the warning message.
It happens "naturally" in the code flow. This is the result:
That's why I chose not to mention it again. |
Ah right, I forgot the context / the subsequent message. Then the current log is sufficient, I would say. I think no bold text is needed here, it is just a warning message, not even an error :) |
The Red Hat Customer Portal has a max limit for
single http requests of 1Gb. From sos side, we never checked this and so customers had to wait for the whole upload to be attempted before receiving an error from the portal.
This patch attempts to stop the upload before it starts, informing customers of such limit, and switching to secure FTP where there's no limit.
Related: RHEL-22732
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines