Add max_pool_size parameter to api/client.py

[tcaiazzi]

As requested in #2477, it is now possible to choose the number of connections for each urllib3 pool.

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "master" git@github.com:tcaiazzi/docker-py.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354325856
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[feliperuhland]

Although I agree with the change, it shouldn't break backward compatibility.

It would be nice to have some tests to make sure that the APIClient argument is changing the urllib pool size, also.

edit: Thanks for your time to make this PR. 👍

[feliperuhland]

This argument is duplicated.

[feliperuhland]

That change will break backward compatibility.

[feliperuhland]

That change also breaks backward compatibility.

[feliperuhland]

That breaks backward compatibility also.

[Skazza94]

Hi @feliperuhland, I've fixed what you reported in the previous comment. My colleague @tcaiazzi erroneously renamed a parameter in *ConnectionPool classes. However, we don't know how to automatically test if the pool size changes accordingly (we tested it manually on our computers).

If you have any ideas, report them here and we'll be happy to build the tests.

[feliperuhland]

Hi @Skazzino.

Thank you for the changes.

About the tests, urllib3.connectionpool.HTTPConnectionPool can be mocked and the test can check if APIClient argument changed HTTPConnectionPool the pool size. That will be useful to guarantee that any future change will break this behavior.

Also, the commit must be signed #2480 (comment)

[Skazza94]

Hi @feliperuhland,
sorry if I bother you again but for me it's the first time that I write a test for an API like this... So I'm quite newbie on it. I don't know if the test that you require should be an unit test or an integration test.

I think that this one is really similar to what we want to test (since I can create an APIClient instance with a desired pool size). In other case, I don't have any idea of how mock the urllib3 and inject it into the APIClient.

Thanks for your patience.

[feliperuhland]

Hi @Skazzino

IMHO, those tests should be a unit test, like this one.

If you need help, let me know.

[Skazza94]

Hi @feliperuhland,
I've managed to create 2 unit tests for the max pool size. However, I wasn't able to mock the urllib3.connectionpool.HTTPConnectionPool because each Adapter internally creates those objects when required.

I was able to read the actual value from each urllib3.connectionpool.HTTPConnectionPool through the Adapter and I asserted if it is equal to the desired pool size.

EDIT: We need to run client.ping method to force the creation of a Connection Pool. However, some CI tests failed because there's no Docker daemon active to run ping command. At this point, is it useful to mock the urllib3.connectionpool.HTTPConnectionPool without a "real" Docker client?

Thanks.

[feliperuhland]

Hi @Skazzino

My mock idea was not to need a docker daemon. Something like:

@mock.patch("docker.transport.unixconn.UnixHTTPConnectionPool")              
def test_default_pool_size(self, mock_obj):                                                                 
    client = docker.DockerClient()                                                                          
    client.ping()                                                                                           
    base_url = "{base_url}/v{version}/_ping".format(base_url=client.api.base_url, version=client.api._version)
    timeout = 60                                                                                            
    docker_sock = "/var/run/docker.sock"                                                                    
    mock_obj.assert_called_once_with(base_url, docker_sock, timeout, maxsize=10)

EDIT: the mock class should be one of the urllib3.connectionpool.HTTPConnectionPool implementation, I was wrong before.

[Skazza94]

Hi @feliperuhland,
thanks for the help! I was heading in the same direction yesterday (after pushing the last commit), but I found some problems:

1. Without a Docker Daemon running, this test still fails with requests.exceptions.ConnectionError: ('Connection aborted.', ConnectionRefusedError(111, 'Connection refused')) because requests catches the Exception before opening a pool.
2. From the stack trace of AppVeyor, it uses NPipe socket (so probably Windows): docker\transport\npipesocket.py:59: in connect. So if I patch the UnixHTTPConnectionPool this test won't work.
3. Even resolving the point 1 and 2, another issue remains. The "mocked response" returned from the UnixHTTPConnectionPool raises this exception since cannot be compared:

    if 400 <= self.status_code < 500:
TypeError: '<=' not supported between instances of 'int' and 'MagicMock'

Hope we can find a way to make it work! :)

[Skazza94]

Hi @feliperuhland,
any news? Sorry if I bother you but this PR is really important to me and my project.

Thanks.

[feliperuhland]

Hi @Skazzino, I'll take a look today. Sorry about the delay.

[feliperuhland]

Hi @Skazzino

1. If the transport class is mocked, the docker daemon will not start. That way, the test will run fast and can check if the class was called correctly.

2. To create tests for Linux and windows, it should be used pytest.mark.skipif, like here, then you can specify the transport class.

3. The idea to mock the transport class is to be sure that the variable used in DockerClient changes the transport class behavior. In other words, the primary purpose is to be sure that the transport class is called with the correct values.

Sorry again for the delay.

I hope that helps and if you need anything, let me know.

[Skazza94]

Hi @feliperuhland,

1. If the transport class is mocked, the docker daemon will not start. That way, the test will run fast and can check if the class was called correctly.

Ok.

2. To create tests for Linux and windows, it should be used `pytest.mark.skipif`, like [here](https://github.com/docker/docker-py/blob/cb19bf117d5088904f15fd7f0cf21213c7141555/tests/unit/utils_build_test.py#L232), then you can specify the transport class.

Okay, I'll add them to the tests (accordingly to what I'm mocking)

3. The idea to mock the transport class is to be sure that the variable used in DockerClient changes the transport class behavior. In other words, the primary purpose is to be sure that the transport class is called with the correct values.

Yes, I agree with you. But if I run the unit tests (like the example you wrote before), I get the following stack trace, since the mocked response isn't an integer and I cannot access to the response from the Client:

Traceback (most recent call last):
  File "/usr/lib/python3.7/unittest/mock.py", line 1204, in patched
    return func(*args, **keywargs)
  File "/home/skazza/Scrivania/git/docker-py/tests/unit/client_test.py", line 85, in test_default_pool_size
    client.info()
  File "/usr/local/lib/python3.7/dist-packages/docker-4.1.0.dev0-py3.7.egg/docker/client.py", line 187, in info
    return self.api.info(*args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/docker-4.1.0.dev0-py3.7.egg/docker/api/daemon.py", line 95, in info
    return self._result(self._get(self._url("/info")), True)
  File "/usr/local/lib/python3.7/dist-packages/docker-4.1.0.dev0-py3.7.egg/docker/api/client.py", line 271, in _result
    self._raise_for_status(response)
  File "/usr/local/lib/python3.7/dist-packages/docker-4.1.0.dev0-py3.7.egg/docker/api/client.py", line 265, in _raise_for_status
    response.raise_for_status()
  File "/home/skazza/.local/lib/python3.7/site-packages/requests/models.py", line 933, in raise_for_status
    if 400 <= self.status_code < 500:
TypeError: '<=' not supported between instances of 'int' and 'MagicMock'

[feliperuhland]

Hi @Skazzino

Is this branch up to date?

[Skazza94]

Hi @feliperuhland,
and thanks for the quick reply! I've pushed the new tests right now.

[feliperuhland]

Hi @Skazzino

The first thing I noticed was the need to mock response.status when the ping method is called. It's necessary to add the return value behavior:

mock_obj.return_value.urlopen.return_value.status = 200

Another thing that I saw was the assert_called_once_with call that needs to be the same as the original one, so it should be without docker_sock and timeout parameters name.

 mock_obj.assert_called_once_with(base_url, "/var/run/docker.sock", 60, maxsize=DEFAULT_MAX_POOL_SIZE)

I hope that could help.

[Skazza94]

Hi @feliperuhland,
new unit tests passed but AppVeyor failed on insalling a library (I don't think this is a problem related to the edits that we made). Any ideas?

Thanks.

[feliperuhland]

Hi @Skazzino

It seems related to dependencies installation:

ERROR: Package 'setuptools' requires a different Python: 2.7.17 not in '>=3.5'

[feliperuhland]

Also, it's essential to rebase from master to be sure that everything is up to date.

:)

[Skazza94]

Hi @feliperuhland,
I've merged the fork with the master. However the failure persists... I haven't touched the requirements.txt (nor the test-requirements.txt).

It seems to be a Python2.7 related issue. How we should proceed?

EDIT: From Jenkins another error occured in tests/integration/api_container_test.py, in test_attach_no_stream, with following assertion error:

    assert output == 'hello\n'.encode(encoding='ascii')
E   AssertionError: assert '' == 'hello\n'
E     + hello

Still not related with my code. Tried to run the same test on my host and it works. What's going on?

Thanks.

[Skazza94]

Hi @feliperuhland,
after 17 days I'm still waiting a response... Thanks.

[tcaiazzi]

Hi @feliperuhland,
I'm still waiting a response!
Thanks!

[feliperuhland]

Hi @Skazzino

Cloud you update de branch again, please?

I push into another branch, and all the tests passed.

Thanks

Edit: #2516

[Skazza94]

Hi @feliperuhland,
all tests passed, finally!

Hope everything is okay now, tell me if you need something else. Thanks for all the help in these months.

[feliperuhland]

Hi @Skazzino
You have to sign the commits. #2480 (comment)

[Skazza94]

Hi @feliperuhland,
is this command still valid? git rebase -i HEAD~842354325856 or should i change the HEAD stuff?

[feliperuhland]

Hi @Skazzino

You can use git rebase -i HEAD~30, (the number represents how many commits do you want to rebase) and edit commits one by one.

If you are not confortable with git command line, you can create another branch to execute the commands.

[Skazza94]

Hi @feliperuhland,
I've signed the commit.

[feliperuhland]

The bot should approve the signature check

[Skazza94]

The bot should approve the signature check

Hi @feliperuhland,
nothing seems to happen. What should i do?

[feliperuhland]

Hi @Skazzino

My suggestion is to clean the past commits and sign those that are going to stay.
If you need help with that, let me know.

[Skazza94]

Hi,
some months passed since last reply on this PR. Is it ok or should we do something else?