Fix the 'max_pool_size' parameter passing for Adapters

[milanbalazs]

The max_pool_size was typed max_pools_size in the APIClient class and it caused issue.

Test code:

import podman
import requests
client = podman.from_env()

Output:

>>> python3 test.py

Traceback (most recent call last):
  File "/local/user/podman_test/tools/test.py", line 11, in <module>
    client = podman.from_env()
  File "/local/user/podman_test/venv/lib/python3.9/site-packages/podman/client.py", line 131, in from_env
    return PodmanClient(
  File "/local/user/podman_test/venv/lib/python3.9/site-packages/podman/client.py", line 77, in __init__
    self.api = APIClient(**api_kwargs)
  File "/local/user/podman_test/venv/lib/python3.9/site-packages/podman/api/client.py", line 136, in __init__
    self.mount("http://", HTTPAdapter(**adapter_kwargs))
TypeError: __init__() got an unexpected keyword argument 'max_pool_size'

If I set the max_pool_size parameter in the from_env method (client = podman.from_env(max_pool_size=requests.adapters.DEFAULT_POOLSIZE)), the output is (the same):

Traceback (most recent call last):
  File "/local/user/podman_test/tools/test.py", line 12, in <module>
    client = podman.from_env(max_pool_size=requests.adapters.DEFAULT_POOLSIZE)
  File "/local/user/podman_test/venv/lib/python3.9/site-packages/podman/client.py", line 131, in from_env
    return PodmanClient(
  File "/local/user/podman_test/venv/lib/python3.9/site-packages/podman/client.py", line 77, in __init__
    self.api = APIClient(**api_kwargs)
  File "/local/user/podman_test/venv/lib/python3.9/site-packages/podman/api/client.py", line 136, in __init__
    self.mount("http://", HTTPAdapter(**adapter_kwargs))
TypeError: __init__() got an unexpected keyword argument 'max_pool_size'

If I use max_pools_size instead of max_pool_size, the output is:

Traceback (most recent call last):
  File "/local/user/podman_test/test.py", line 12, in <module>
    client = podman.from_env(max_pools_size=requests.adapters.DEFAULT_POOLSIZE)
TypeError: from_env() got an unexpected keyword argument 'max_pools_size'

Based on the above analysis, it's clearly visible the parameter passing is not correct...

Furthermore the implementation and the docstring are not matched (pool -> pools):

I have check the complete source code and I haven't found reference for max_pools_size parameter so I think my change is compatible with the source code.

Note:

• If a user uses the APIClient directly and the max_pools_size parameter is passed, then that implementation won't be working after my change because the parameter is renamed. But it can be made for backward compatible is it's preferred.

The tested system:

• Python 3.9.17
• RHEL 8.7 OS
• podman 4.8.2

[milanbalazs]

In my opinion, further improvement would be nice in the Adapter creation section. The UDSAdapter and SSHAdapter classes can handle the additional warts, like:

class UDSAdapter(HTTPAdapter):
    """Specialization of requests transport adapter for UNIX domain sockets."""

    def __init__(
        self,
        uds: str,
        pool_connections=DEFAULT_POOLSIZE,
        pool_maxsize=DEFAULT_POOLSIZE,
        max_retries=DEFAULT_RETRIES,
        pool_block=DEFAULT_POOLBLOCK,
        **kwargs,  # <-- It can handle the "unexpected" kwargs
    ):

But the HTTPAdapter doesn't handle, it means only the defined keyword parameters can be passed to this class:

    def __init__(
        self,
        pool_connections=DEFAULT_POOLSIZE,
        pool_maxsize=DEFAULT_POOLSIZE,
        max_retries=DEFAULT_RETRIES,
        pool_block=DEFAULT_POOLBLOCK,
    ):  # There is no "**kwargs", so it can handle only the defined keyword parameters.

In the current implementations in the APIClient class, the passed arguments are passed towards to the Adapters. It means the max_pool_size or num_pools and the HTTPAdapter could say TypeError: HTTPAdapter.__init__() got an unexpected keyword argument 'max_pool_size'. That part could be improved to create a much precise kwargs dict for Adapters.

The mainly problematic part.

        adapter_kwargs = kwargs.copy()
        if num_pools is not None:
            adapter_kwargs["pool_connections"] = num_pools
        if max_pool_size is not None:
            adapter_kwargs["pool_maxsize"] = max_pool_size
        if timeout is not None:
            adapter_kwargs["timeout"] = timeout

        if self.base_url.scheme == "http+unix":
            self.mount("http://", UDSAdapter(self.base_url.geturl(), **adapter_kwargs))
            self.mount("https://", UDSAdapter(self.base_url.geturl(), **adapter_kwargs))

        elif self.base_url.scheme == "http+ssh":
            self.mount("http://", SSHAdapter(self.base_url.geturl(), **adapter_kwargs))
            self.mount("https://", SSHAdapter(self.base_url.geturl(), **adapter_kwargs))

        elif self.base_url.scheme == "http":
            self.mount("http://", HTTPAdapter(**adapter_kwargs))
            self.mount("https://", HTTPAdapter(**adapter_kwargs))
        else:
            assert False, "APIClient.supported_schemes changed without adding a branch here."

I can implement it in another PR (Or extending this one), if you want.

[jwhonce]

@milanbalazs max_pool_size should have been the parameter name. I agree with the addition of kwargs to HTTPAdapter.

[umohnani8]

Thanks for the PR @milanbalazs! Can you please make this backward compatible so we don't break any users.

[milanbalazs]

I have just added a new commit which contains the backward compatible solution (And providing DeprecationWarning). Furthermore, I have fixed the parameter passing for HTTPAdapter class.

My minimal test about the parameter usage:

>>> import podman

>>> podman.client.APIClient(base_url="tcp://asdfadf", max_pool_size=8)
<podman.api.client.APIClient object at 0x102a80d90>

>>> podman.client.APIClient(base_url="tcp://asdfadf", max_pools_size=8)
/Users/user/podman-py/podman/api/client.py:135: DeprecationWarning: 'max_pools_size' parameter is deprecated! Please use 'max_pool_size' parameter.
  warnings.warn("'max_pools_size' parameter is deprecated! "
<podman.api.client.APIClient object at 0x102e02890>

>>> podman.client.APIClient(base_url="tcp://asdfadf", max_pools_size=8, max_pool_size=9)
/Users/user/podman-py/podman/api/client.py:135: DeprecationWarning: 'max_pools_size' parameter is deprecated! Please use 'max_pool_size' parameter.
  warnings.warn("'max_pools_size' parameter is deprecated! "
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/user/podman-py/podman/api/client.py", line 138, in __init__
    raise ValueError("Both of 'max_pools_size' and 'max_pool_size' parameters are set. "
ValueError: Both of 'max_pools_size' and 'max_pool_size' parameters are set. Please use only the 'max_pool_size', 'max_pools_size' is deprecated!

[umohnani8]

Thank you!
/approve
LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: milanbalazs, umohnani8

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [umohnani8]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[umohnani8]

@milanbalazs I think validate is failing because the subject of the commit message is too long, it should be less than 72 characters. Can you please fix that and push.

[milanbalazs]

Thanks! I have just made shorter the problematic commit massage.

[umohnani8]

@rhatdan @jwhonce PTAL

[rhatdan]

/lgtm
Thanks @milanbalazs

[milanbalazs]

Thanks to merge my PR!

When do you plan to create a new release which contains my fix (PyPi package)?

[umohnani8]

@milanbalazs I created a new 4.9 release with your changes included in it.