Make retry handle http status codes

[Germandrummer92]

Retry Raw Http Requests

Description

We are currently facing some storage driver exceptions when uploading many small files. Stacktrace (parts removed):

self._driver.upload_object_via_stream( File "/usr/local/lib/python3.8/site-packages/libcloud/storage/drivers/s3.py", line 754, in upload_object_via_stream return self._put_object(container=container, object_name=object_name, File "/usr/local/lib/python3.8/site-packages/libcloud/storage/drivers/s3.py", line 900, in _put_object result_dict = self._upload_object( File "/usr/local/lib/python3.8/site-packages/libcloud/storage/base.py", line 837, in _upload_object response.parse_error() File "/usr/local/lib/python3.8/site-packages/libcloud/storage/drivers/s3.py", line 147, in parse_error raise LibcloudError('Unknown error. Status code: %d' % (self.status), libcloud.common.types.LibcloudError: <LibcloudError in <class 'libcloud.storage.drivers.s3.S3StorageDriver'> 'Unknown error. Status code: 429'>

This change also enables retrying the raw requests. Together with #1588 (for which we have a workaround), this would solve our issue completely and we could keep using libcloud.

I only added a call to retrying the raw requests.

Status

done, ready for review

Checklist (tick everything that applies)

• Code linting (required, can be done after the PR checks)
• Documentation
• Tests
• ICLA (required for bigger changes)

[Germandrummer92]

After further looking into the code, it appears the retry does not work for the status exceptions as requests does not automatically raise them. instead we should wrap the retry handler around the "raise_for_status" as well. For now I will make this a draft only and see if we can fix it correclty

[Germandrummer92]

I am not super happy with the current solution if someone has a better idea let me know. Otherwise this would at least give the ability to handle these status code exceptions. I also think the parsing of the 429 into the RateLimitedError is currently not done correctly/not working, but with #1588 we could work around it.

Maybe we need a predicate for the exceptions instead of only the types configurable though @RunOrVeith.

[Kami]

Thanks for the contribution.

Since it's a larger one, can you please look into filling an ICLA - https://www.apache.org/licenses/icla.pdf.

I plan to review it once #1588 is merged.

[Kami]

@Germandrummer92

Maybe we need a predicate for the exceptions instead of only the types configurable though @RunOrVeith.

Yeah, having another more generic approach with a callable to which response / exception is passed or similar, would probably be more flexible than just having a fixed list of exception classes (and class with a callable which checks the exception type would just be a subclass of this parent class).

[Germandrummer92]

I signed the icla.

Overall i think it should work now as before, but the part in base.py with ignoring the HTTPException isn't super nice. Instead the better solution would probably be to have the retry wrapper wrapped around more code so that the actual exception by the rawResponseCls is then thrown and can be handled by the retry handler. I would only tackle that after #1588 is merged tough.

[Kami]

#1588 has been merged so now you can hopefully sync up your changes with latest trunk without too many issues.

[Kami]

@Germandrummer92 Do you have any idea when you will have a chance to work on this PR?

Ideally we would include those changes in the next release, but if it will take a while I will just do new release this week and we can include those changes in the next one.

[Germandrummer92]

@Kami Will try and get it done in the next few days. But also fine with putting it in the next release.

[Germandrummer92]

@RunOrVeith @Kami

I think like this it's a much smaller and cleaner change and shouldn't break anything while still retrying raw requests and correctly mapping the exceptions before they are checked in the retry class.

re: the change in test_ovh: this test also fails for me on the current trunk code? maybe ovh changed their response code recently here? (obviously parsing the exception message is maybe not the best idea...)

[Kami]

LGTM.

And if I understand the code correctly - only change that this PR now includes is retrying raw requests, right?

[Germandrummer92]

Yes, but there is another important change:

Before the retry was only wrapping the raw requests request which actually doesn't raise any exception, since we don't explicitly call raise_for_status, instead now it also wraps the creation of the responseCls (line 677 in common/base.py), in which the actual exception is raised (line 166 in common/base.py).

So I actually think the retry wasn't very helpful before since the http exceptions weren't thrown.

I will take a look at the failing tests now, they worked for me locally

[Kami]

@Germandrummer92 Thanks for clarifying. And I already have a fix locally and will merge it shortly :)

[Kami]

I made a couple of small changes (07062df, 2c3e380) and merged it into trunk. Thanks!