fix rest api aiohttp timeout

[soamicharan]

Implement timeout behavior same as official python Kubernetes client: https://github.com/kubernetes-client/python/blob/f414832bb05b946eb1758df12f08806b44dd315e/kubernetes/client/rest.py#L146

Following issues will be resolved by this fix:

• timeout_seconds seems to be ineffective #308
• Issue during deployment run failure - 'Could not determine exit code for X' PrefectHQ/prefect#15259
• K8s worker fails monitoring flow and sets it to crashed. PrefectHQ/prefect#14954

[tomplus]

Thanks for your PR. Could you explain how this change fixes issues you mentioned?

[soamicharan]

Thanks for your PR. Could you explain how this change fixes issues you mentioned?

The code from official kubernetes client

        timeout = None
        if _request_timeout:
            if isinstance(_request_timeout, (int, ) if six.PY3 else (int, long)):  # noqa: E501,F821
                timeout = urllib3.Timeout(total=_request_timeout)
            elif (isinstance(_request_timeout, tuple) and
                  len(_request_timeout) == 2):
                timeout = urllib3.Timeout(
                    connect=_request_timeout[0], read=_request_timeout[1])

So _request_timeout parameter has default value None and according to official Kubernetes Client SDK, if _request_timeout is None then REST API Client (urlliib3 in official Kubernetes Client), timeout parameter value is passed as None (so client request will not be timeout)
But in current implementation in kubernetes_asyncio, the default behavior (when _request_timeout is set to None) always timeout in 5 minutes because _request_timeout parameter None value is not correctly passed to aiohttp request.

Additionally, according to the _request_parameter doc::

:param _request_timeout: timeout setting for this request. If one
                                number provided, it will be total request
                                timeout. It can also be a pair (tuple) of
                                (connection, read) timeouts.

The if _request_timeout is tuple that case is not handled.

PrefectHQ/prefect#14954
PrefectHQ/prefect#15259

In both issues, where prefect library migrated from official Kubernetes Client SDK to kubernetes_asyncio, where one of use case is to stream kubernetes job events which are long running pods (more than 5 mins), event stream (from kubernetes_asyncio.watch.Watch()) constantly throwing asyncio.TimeoutError in every 5 mins, even setting _request_timeout parameter to None expecting that it should behave same as official kubernetes client SDK but its not.

So my fix which replicates the timeout behaviour same as offiicial kubernetes client should fix the issue. This will be helpful to migrate from official kubernetes client to kubernetes_asyncio.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.35%. Comparing base (089f487) to head (9adb4b5).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   27.31%   27.35%   +0.03%     
==========================================
  Files         794      795       +1     
  Lines       96301    96326      +25     
==========================================
+ Hits        26305    26347      +42     
+ Misses      69996    69979      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tomplus]

Thanks for your explanation. It makes sense. Unfortunately this is a breaking change, so it'll be released with the next "big" release - 1.32.x (~ December 2024)

[tomplus]

Actually, aiohttp has a default timeout too - 5mins: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L219
It's strange. Could you confirm that this change solves the issues?

[soamicharan]

Actually, aiohttp has a default timeout too - 5mins: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L219 It's strange. Could you confirm that this change solves the issues?

Yes I confirmed that changes does solve the issue as in case of _request_timeout parameter value is None, I explicitly setting aiohttp timeout parameter to ClientTimeout() object which has all values None which overrides the default value of aiohttp timeout.

And I also added test case for this changes.

Thanks.

[NicholasFiorentini]

Is this PR still in progress or it is abandoned?

[soamicharan]

I added the requested changes @tomplus @NicholasFiorentini please review.

[tomplus]

LGTM, thank you.

[NicholasFiorentini]

Do we know when a new release with this fix will be available?

[tomplus]

I'm going to release this change with the next "big" release - 1.32.x (11 December 2024, https://www.kubernetes.dev/resources/release/).

[tomplus]

@NicholasFiorentini It has been deployed, version 1.32.0 contains this fix.

[NicholasFiorentini]

Thank you @tomplus !