Retry on 503 #5938
Retry on 503 #5938
Conversation
TheRealFalcon
force-pushed
the
retry-on-503
branch
from
eb9ebae to
583e263
Compare
| @@ -33,6 +43,7 @@ | |||
| LOG = logging.getLogger(__name__) | |||
|
|
|||
| REDACTED = "REDACTED" | |||
| ExceptionCallback = Optional[Callable[["UrlError"], bool]] | |||
There was a problem hiding this comment.
My goal here is to unify the exception_cb interface used in readurl and wait_for_url as they are currently different.
None of the pre-existing callbacks used the "request_args" argument, and it can also be retrieved from an HTTPError if needed in the future, and calculated it would be harder in wait_for_url(), so I removed it from the callback interface and updated the call points accordingly.
| return to_wait | ||
|
|
||
|
|
||
| def _handle_error( |
There was a problem hiding this comment.
I'm hoping this wouldn't be hard to extend to also modify future request arguments if ever needed in the future...but...YAGNI
| except exceptions.SSLError as e: | ||
| # ssl exceptions are not going to get fixed by waiting a | ||
| # few seconds | ||
| raise UrlError(e, url=url) from e | ||
| except exceptions.HTTPError as e: |
There was a problem hiding this comment.
The isinstance() check is silly now that we can use multiple except arms
| @@ -639,61 +724,60 @@ def dual_stack( | |||
| return (returned_address, return_result) | |||
|
|
|||
|
|
|||
| class HandledResponse(NamedTuple): | |||
There was a problem hiding this comment.
I don't love this honestly, but it provides a more structured way of return stuff from the nested functions in wait_for_url()
There was a problem hiding this comment.
Thanks @TheRealFalcon, this looks really close. I left a couple of comments inline.
| @@ -653,19 +655,23 @@ def _imds_exception_cb(self, msg, exception=None): | |||
| temporarily unroutable or unavailable will still retry due to the | |||
| callsite wait_for_url. | |||
| """ | |||
| if isinstance(exception, uhelp.UrlError): | |||
| if exception.code: | |||
There was a problem hiding this comment.
Nice cleanup, but this is still redundant. We could eliminate this branch and indent its contents for the same outcome.
There was a problem hiding this comment.
elif exception.code >= 400: will result in a TypeError, and ways of working around that seems less elegant than this.
cloudinit/url_helper.py
Outdated
| try: | ||
| to_wait = float( | ||
| time.mktime( | ||
| time.strptime(retry_after, "%a, %d %b %Y %H:%M:%S %Z") |
There was a problem hiding this comment.
eek, parsing!
rfc9110 describes three formats
The preferred format is a fixed-length and single-zone subset of the date and time specification used by the Internet Message Format [RFC5322].
HTTP-date = IMF-fixdate / obs-date
An example of the preferred format is
Sun, 06 Nov 1994 08:49:37 GMT ; IMF-fixdate
Examples of the two obsolete formats are
Sunday, 06-Nov-94 08:49:37 GMT ; obsolete RFC 850 format
Sun Nov 6 08:49:37 1994 ; ANSI C's asctime() formatA recipient that parses a timestamp value in an HTTP field MUST accept all three HTTP-date formats.
This implementation fails for an RFC 850 date and ANSI C's asctime() format as well.
Maybe we could just use email.utils.parsedate(), which appears to support all three? It is documented to support RFC 2822, which was obsoleted by RFC 5322, and the source code documents its support for RFC 850.
There was a problem hiding this comment.
Thanks!!! I didn't realize the standard library had that (even though it's already used in the same file 😂 ), so I agree, unnecessary manual parsing is bad!
If the server is busy, no need to fail. Add type hints to adjacent code paths and necessary refactors. Fixes GH-5577 --------- Co-authored-by: Brett Holman <brett.holman@canonical.com>
If the server is busy, no need to fail. Add type hints to adjacent code paths and necessary refactors. Fixes canonicalGH-5577 --------- Co-authored-by: Brett Holman <brett.holman@canonical.com>
Follow-up to #5578
Proposed Commit Message
Additional Context
Test Steps
Merge type